[3/3] intel: Make driver aware of MOCS table version

Submitted by Ben Widawsky on July 6, 2017, 11:27 p.m.

Details

Message ID 20170706232703.14229-4-ben@bwidawsk.net
State New
Headers show
Series "MOCS versioning" ( rev: 2 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ben Widawsky July 6, 2017, 11:27 p.m.
We don't yet have optimal MOCS settings, but we have enough to know how
to at least determine when we might have non-optimal settings within our
driver.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 src/intel/vulkan/anv_device.c             | 12 ++++++++++++
 src/intel/vulkan/anv_private.h            |  2 ++
 src/mesa/drivers/dri/i915/intel_context.c |  7 ++++++-
 src/mesa/drivers/dri/i965/intel_screen.c  | 14 ++++++++++++++
 src/mesa/drivers/dri/i965/intel_screen.h  |  2 ++
 5 files changed, 36 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 3dc55dbb8d..8e180dbf18 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -368,6 +368,18 @@  anv_physical_device_init(struct anv_physical_device *device,
          device->info.max_cs_threads = max_cs_threads;
    }
 
+   if (device->info.gen >= 9) {
+      device->mocs_version = anv_gem_get_param(fd,
+                                               I915_PARAM_MOCS_TABLE_VERSION);
+      switch (device->mocs_version) {
+      default:
+         anv_perf_warn("Kernel exposes newer MOCS table\n");
+      case 1:
+      case 0:
+         device->mocs_version = MOCS_TABLE_VERSION;
+      }
+   }
+
    brw_process_intel_debug_variable();
 
    device->compiler = brw_compiler_create(NULL, &device->info);
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 573778dad5..b8241a9b22 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -684,6 +684,8 @@  struct anv_physical_device {
     uint32_t                                    eu_total;
     uint32_t                                    subslice_total;
 
+    uint8_t                                     mocs_version;
+
     struct {
       uint32_t                                  type_count;
       struct anv_memory_type                    types[VK_MAX_MEMORY_TYPES];
diff --git a/src/mesa/drivers/dri/i915/intel_context.c b/src/mesa/drivers/dri/i915/intel_context.c
index e0766a0e3f..9169ea650e 100644
--- a/src/mesa/drivers/dri/i915/intel_context.c
+++ b/src/mesa/drivers/dri/i915/intel_context.c
@@ -521,8 +521,13 @@  intelInitContext(struct intel_context *intel,
    INTEL_DEBUG = parse_debug_string(getenv("INTEL_DEBUG"), debug_control);
    if (INTEL_DEBUG & DEBUG_BUFMGR)
       dri_bufmgr_set_debug(intel->bufmgr, true);
-   if (INTEL_DEBUG & DEBUG_PERF)
+   if (INTEL_DEBUG & DEBUG_PERF) {
       intel->perf_debug = true;
+      if (screen->mocs_version > MOCS_TABLE_VERSION) {
+         fprintf(stderr, "Kernel exposes newer MOCS table\n");
+         screen->mocs_version = MOCS_TABLE_VERSION;
+      }
+   }
 
    if (INTEL_DEBUG & DEBUG_AUB)
       drm_intel_bufmgr_gem_set_aub_dump(intel->bufmgr, true);
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
index c75f2125d4..c53f133d49 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -2301,6 +2301,20 @@  __DRIconfig **intelInitScreen2(__DRIscreen *dri_screen)
          (ret != -1 || errno != EINVAL);
    }
 
+   if (devinfo->gen >= 9) {
+      screen->mocs_version = intel_get_integer(screen,
+                                               I915_PARAM_MOCS_TABLE_VERSION);
+      switch (screen->mocs_version) {
+      case 1:
+      case 0:
+         screen->mocs_version = MOCS_TABLE_VERSION;
+         break;
+      default:
+         /* We want to perf debug, but we can't yet */
+         break;
+      }
+   }
+
    dri_screen->extensions = !screen->has_context_reset_notification
       ? screenExtensions : intelRobustScreenExtensions;
 
diff --git a/src/mesa/drivers/dri/i965/intel_screen.h b/src/mesa/drivers/dri/i965/intel_screen.h
index f78b3e8f74..eb801f8155 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.h
+++ b/src/mesa/drivers/dri/i965/intel_screen.h
@@ -112,6 +112,8 @@  struct intel_screen
    bool mesa_format_supports_texture[MESA_FORMAT_COUNT];
    bool mesa_format_supports_render[MESA_FORMAT_COUNT];
    enum isl_format mesa_to_isl_render_format[MESA_FORMAT_COUNT];
+
+   unsigned mocs_version;
 };
 
 extern void intelDestroyContext(__DRIcontext * driContextPriv);

Comments

On Thu, Jul 6, 2017 at 4:27 PM, Ben Widawsky <ben@bwidawsk.net> wrote:

> We don't yet have optimal MOCS settings, but we have enough to know how
> to at least determine when we might have non-optimal settings within our
> driver.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  src/intel/vulkan/anv_device.c             | 12 ++++++++++++
>  src/intel/vulkan/anv_private.h            |  2 ++
>  src/mesa/drivers/dri/i915/intel_context.c |  7 ++++++-
>  src/mesa/drivers/dri/i965/intel_screen.c  | 14 ++++++++++++++
>  src/mesa/drivers/dri/i965/intel_screen.h  |  2 ++
>  5 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 3dc55dbb8d..8e180dbf18 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -368,6 +368,18 @@ anv_physical_device_init(struct anv_physical_device
> *device,
>           device->info.max_cs_threads = max_cs_threads;
>     }
>
> +   if (device->info.gen >= 9) {
> +      device->mocs_version = anv_gem_get_param(fd,
> +
>  I915_PARAM_MOCS_TABLE_VERSION);
> +      switch (device->mocs_version) {
> +      default:
> +         anv_perf_warn("Kernel exposes newer MOCS table\n");
>

A perf_warn here seems reasonable though it makes more sense to me to make
it

if (device->mocs_version > ANV_MAX_KNOWN_MOCS_VERSION)
   anv_perf_warn("...");


> +      case 1:
> +      case 0:
> +         device->mocs_version = MOCS_TABLE_VERSION;
>

Why are we stomping device->mocs_version to MOCS_TABLE_VERSION?  Are you
just trying to avoid the version 0?  If so, why not just have

/* If the MOCS_TABLE_VERSION query fails, assume version 1 */
if (device->mocs_version == 0)
   device->mocs_version = 1;

I don't think we want to have it dependent on a #define in an external
header file.  What if someone updates it for i965 and doesn't update anv or
vice-versa?


> +      }
> +   }
> +
>     brw_process_intel_debug_variable();
>
>     device->compiler = brw_compiler_create(NULL, &device->info);
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> index 573778dad5..b8241a9b22 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -684,6 +684,8 @@ struct anv_physical_device {
>      uint32_t                                    eu_total;
>      uint32_t                                    subslice_total;
>
> +    uint8_t                                     mocs_version;
> +
>      struct {
>        uint32_t                                  type_count;
>        struct anv_memory_type
> types[VK_MAX_MEMORY_TYPES];
> diff --git a/src/mesa/drivers/dri/i915/intel_context.c
> b/src/mesa/drivers/dri/i915/intel_context.c
> index e0766a0e3f..9169ea650e 100644
> --- a/src/mesa/drivers/dri/i915/intel_context.c
> +++ b/src/mesa/drivers/dri/i915/intel_context.c
> @@ -521,8 +521,13 @@ intelInitContext(struct intel_context *intel,
>     INTEL_DEBUG = parse_debug_string(getenv("INTEL_DEBUG"),
> debug_control);
>     if (INTEL_DEBUG & DEBUG_BUFMGR)
>        dri_bufmgr_set_debug(intel->bufmgr, true);
> -   if (INTEL_DEBUG & DEBUG_PERF)
> +   if (INTEL_DEBUG & DEBUG_PERF) {
>        intel->perf_debug = true;
> +      if (screen->mocs_version > MOCS_TABLE_VERSION) {
> +         fprintf(stderr, "Kernel exposes newer MOCS table\n");
> +         screen->mocs_version = MOCS_TABLE_VERSION;
> +      }
> +   }
>
>     if (INTEL_DEBUG & DEBUG_AUB)
>        drm_intel_bufmgr_gem_set_aub_dump(intel->bufmgr, true);
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
> b/src/mesa/drivers/dri/i965/intel_screen.c
> index c75f2125d4..c53f133d49 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -2301,6 +2301,20 @@ __DRIconfig **intelInitScreen2(__DRIscreen
> *dri_screen)
>           (ret != -1 || errno != EINVAL);
>     }
>
> +   if (devinfo->gen >= 9) {
> +      screen->mocs_version = intel_get_integer(screen,
> +
>  I915_PARAM_MOCS_TABLE_VERSION);
> +      switch (screen->mocs_version) {
> +      case 1:
> +      case 0:
> +         screen->mocs_version = MOCS_TABLE_VERSION;
>

Same comments apply here.


> +         break;
> +      default:
> +         /* We want to perf debug, but we can't yet */
> +         break;
> +      }
> +   }
> +
>     dri_screen->extensions = !screen->has_context_reset_notification
>        ? screenExtensions : intelRobustScreenExtensions;
>
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.h
> b/src/mesa/drivers/dri/i965/intel_screen.h
> index f78b3e8f74..eb801f8155 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.h
> +++ b/src/mesa/drivers/dri/i965/intel_screen.h
> @@ -112,6 +112,8 @@ struct intel_screen
>     bool mesa_format_supports_texture[MESA_FORMAT_COUNT];
>     bool mesa_format_supports_render[MESA_FORMAT_COUNT];
>     enum isl_format mesa_to_isl_render_format[MESA_FORMAT_COUNT];
> +
> +   unsigned mocs_version;
>  };
>
>  extern void intelDestroyContext(__DRIcontext * driContextPriv);
> --
> 2.13.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
On 17-07-07 09:28:08, Jason Ekstrand wrote:
>On Thu, Jul 6, 2017 at 4:27 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>
>> We don't yet have optimal MOCS settings, but we have enough to know how
>> to at least determine when we might have non-optimal settings within our
>> driver.
>>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> ---
>>  src/intel/vulkan/anv_device.c             | 12 ++++++++++++
>>  src/intel/vulkan/anv_private.h            |  2 ++
>>  src/mesa/drivers/dri/i915/intel_context.c |  7 ++++++-
>>  src/mesa/drivers/dri/i965/intel_screen.c  | 14 ++++++++++++++
>>  src/mesa/drivers/dri/i965/intel_screen.h  |  2 ++
>>  5 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
>> index 3dc55dbb8d..8e180dbf18 100644
>> --- a/src/intel/vulkan/anv_device.c
>> +++ b/src/intel/vulkan/anv_device.c
>> @@ -368,6 +368,18 @@ anv_physical_device_init(struct anv_physical_device
>> *device,
>>           device->info.max_cs_threads = max_cs_threads;
>>     }
>>
>> +   if (device->info.gen >= 9) {
>> +      device->mocs_version = anv_gem_get_param(fd,
>> +
>>  I915_PARAM_MOCS_TABLE_VERSION);
>> +      switch (device->mocs_version) {
>> +      default:
>> +         anv_perf_warn("Kernel exposes newer MOCS table\n");
>>
>
>A perf_warn here seems reasonable though it makes more sense to me to make
>it
>
>if (device->mocs_version > ANV_MAX_KNOWN_MOCS_VERSION)
>   anv_perf_warn("...");
>
>

One thing to keep in mind: the max MOCS version can vary by platform (hopefully
it doesn't).

>> +      case 1:
>> +      case 0:
>> +         device->mocs_version = MOCS_TABLE_VERSION;
>>
>
>Why are we stomping device->mocs_version to MOCS_TABLE_VERSION?  Are you
>just trying to avoid the version 0?  If so, why not just have
>
>/* If the MOCS_TABLE_VERSION query fails, assume version 1 */
>if (device->mocs_version == 0)
>   device->mocs_version = 1;
>

I think the switch looks better, especially as the versions increase.

>I don't think we want to have it dependent on a #define in an external
>header file.  What if someone updates it for i965 and doesn't update anv or
>vice-versa?
>
>

Yeah, I am removing that external define as mentioned in the other thread. I
think it was a bad idea that I jammed in at the last minute.

>> +      }
>> +   }
>> +
>>     brw_process_intel_debug_variable();
>>
>>     device->compiler = brw_compiler_create(NULL, &device->info);
>> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
>> private.h
>> index 573778dad5..b8241a9b22 100644
>> --- a/src/intel/vulkan/anv_private.h
>> +++ b/src/intel/vulkan/anv_private.h
>> @@ -684,6 +684,8 @@ struct anv_physical_device {
>>      uint32_t                                    eu_total;
>>      uint32_t                                    subslice_total;
>>
>> +    uint8_t                                     mocs_version;
>> +
>>      struct {
>>        uint32_t                                  type_count;
>>        struct anv_memory_type
>> types[VK_MAX_MEMORY_TYPES];
>> diff --git a/src/mesa/drivers/dri/i915/intel_context.c
>> b/src/mesa/drivers/dri/i915/intel_context.c
>> index e0766a0e3f..9169ea650e 100644
>> --- a/src/mesa/drivers/dri/i915/intel_context.c
>> +++ b/src/mesa/drivers/dri/i915/intel_context.c
>> @@ -521,8 +521,13 @@ intelInitContext(struct intel_context *intel,
>>     INTEL_DEBUG = parse_debug_string(getenv("INTEL_DEBUG"),
>> debug_control);
>>     if (INTEL_DEBUG & DEBUG_BUFMGR)
>>        dri_bufmgr_set_debug(intel->bufmgr, true);
>> -   if (INTEL_DEBUG & DEBUG_PERF)
>> +   if (INTEL_DEBUG & DEBUG_PERF) {
>>        intel->perf_debug = true;
>> +      if (screen->mocs_version > MOCS_TABLE_VERSION) {
>> +         fprintf(stderr, "Kernel exposes newer MOCS table\n");
>> +         screen->mocs_version = MOCS_TABLE_VERSION;
>> +      }
>> +   }
>>
>>     if (INTEL_DEBUG & DEBUG_AUB)
>>        drm_intel_bufmgr_gem_set_aub_dump(intel->bufmgr, true);
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
>> b/src/mesa/drivers/dri/i965/intel_screen.c
>> index c75f2125d4..c53f133d49 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>> @@ -2301,6 +2301,20 @@ __DRIconfig **intelInitScreen2(__DRIscreen
>> *dri_screen)
>>           (ret != -1 || errno != EINVAL);
>>     }
>>
>> +   if (devinfo->gen >= 9) {
>> +      screen->mocs_version = intel_get_integer(screen,
>> +
>>  I915_PARAM_MOCS_TABLE_VERSION);
>> +      switch (screen->mocs_version) {
>> +      case 1:
>> +      case 0:
>> +         screen->mocs_version = MOCS_TABLE_VERSION;
>>
>
>Same comments apply here.
>
>
>> +         break;
>> +      default:
>> +         /* We want to perf debug, but we can't yet */
>> +         break;
>> +      }
>> +   }
>> +
>>     dri_screen->extensions = !screen->has_context_reset_notification
>>        ? screenExtensions : intelRobustScreenExtensions;
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.h
>> b/src/mesa/drivers/dri/i965/intel_screen.h
>> index f78b3e8f74..eb801f8155 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.h
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.h
>> @@ -112,6 +112,8 @@ struct intel_screen
>>     bool mesa_format_supports_texture[MESA_FORMAT_COUNT];
>>     bool mesa_format_supports_render[MESA_FORMAT_COUNT];
>>     enum isl_format mesa_to_isl_render_format[MESA_FORMAT_COUNT];
>> +
>> +   unsigned mocs_version;
>>  };
>>
>>  extern void intelDestroyContext(__DRIcontext * driContextPriv);

If you feel strongly about moving to if/else, I can do it. Let me know.