Re: [Mesa-dev] [Intel-gfx] [PATCH 3/3] intel: Make driver aware of MOCS table version

[Ben Widawsky]

On 17-07-07 09:28:08, Jason Ekstrand wrote:

On Thu, Jul 6, 2017 at 4:27 PM, Ben Widawsky  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 
---
 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, >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_teu_total;
 uint32_tsubslice_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 

Re: [Mesa-dev] [Intel-gfx] [PATCH 3/3] intel: Make driver aware of MOCS table version

[Jason Ekstrand]

On Thu, Jul 6, 2017 at 4:27 PM, Ben Widawsky  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 
> ---
>  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, >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_teu_total;
>  uint32_tsubslice_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