[PATCH 0/3] MOCS versioning

[PATCH 0/3] MOCS versioning

[Ben Widawsky]

Copying the kernel commit message:

Starting with GEN9, Memory Object Control State (MOCS) becomes an index
into a table as opposed to the direct programming within the command.
The table has 62 usable entries (ie 6 bits can represent all settings),
and each buffer type may use one of these 62 entries to describe
cacheability type, and age (and some other less useful fields).

Because we hadn't dealt with MOCS settings like this, we didn't think
ahead too well and have ended up with a mess for GEN9 (and soon GEN10)
platform. The plan for for future platforms is that the ideal MOCS
settings will be determined, defined, and written in the public PRMs.
After this point, the i915.ko will absorb these settings and sometime
afterwards flip the alpha switch. All driver releases without the final
MOCS table must be considered alpha. Here on, userspace can assume the
MOCS table is definitively done. There will be some reserved entries for
'oh shit' scenarios. This avoids versioning the MOCS table which leaves
somewhat of a mess in userspace trying to handle arbitrarily many MOCS
versions.

But we do have a mess on GEN9. In the beginning, the MOCS table entries
were pre-populated by the hardware based on estimations made prior to
tapeout and we could just use that. Subsequently much performance tuning
was done to determine optimal settings that the i915 driver should load
on top of the hardware defaults. That was posted last as v6 of the
original per-engine MOCS settings:
https://patchwork.freedesktop.org/patch/53237/. Since the MOCS table is
not context saved/restored, it isn't feasible to let userspace upload
its own MOCS table. After a good amount of debate, it was decided that
we'd utilize only the minimal set of entires in mesa anyway, and so we
took only those entries for our MOCS entries.

Now we've come to the realization that indeed there are other MOCS
entries which are more optimal for various buffer types and workloads.
The problem is that the meaning of the indices is ABI (we assume index 0
is the uncached entry, and that there are only 3 entries total).

What this patch [simply] aims to do is expose a parameter to inform
userspace which "version" of the table was loaded by i915. Upon
sufficient data, new entries can be added, and the version can be
bumped. For example, from my original mesa mocs branch:

commit c9b0481bce24af032386701de0266eb5bc24e988
Author: Ben Widawsky <benjamin.widawsky@intel.com>
Date:   Fri Apr 8 10:21:16 2016 -0700

    i965: Use PTE mocs

    Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>

diff --git a/src/mesa/drivers/dri/i965/brw_mocs.c b/src/mesa/drivers/dri/i965/brw_mocs.c
index 5df154eb86..b7bfdab671 100644
--- a/src/mesa/drivers/dri/i965/brw_mocs.c
+++ b/src/mesa/drivers/dri/i965/brw_mocs.c
@@ -14,6 +14,9 @@
 /* Skylake: MOCS is now an index into an array of 62 different caching
  * configurations programmed by the kernel.
  */
+
+/* TC=PTE, LeCC=PTE, LRUM=3, L3CC=WB */
+#define SKL_MOCS_PTE_PTE (3 << 1)
 /* TC=LLC/eLLC, LeCC=WB, LRUM=3, L3CC=WB */
 #define SKL_MOCS_WB  (2 << 1)
 /* TC=LLC/eLLC, LeCC=PTE, LRUM=3, L3CC=WB */
@@ -26,6 +29,9 @@ brw_mocs_get_control_state(const struct brw_context *brw,
    switch (brw->gen) {
    default:
    case 9:
+      if (brw->intelScreen->mocs_version > 1)
+         return SKL_MOCS_PTE_PTE;
+
       return type == INTEL_MOCS_PTE ? SKL_MOCS_PTE : SKL_MOCS_WB;
    case 8:
       return type == INTEL_MOCS_PTE ? BDW_MOCS_PTE : BDW_MOCS_WB;

tl;dr: A versioned MOCS table will allow userspace to be aware of new
and potentially interesting cacheability settings. Next GEN platforms
will not be considered production worthy until the MOCS table is
finalized.

Ben Widawsky (1):
  drm/i915: Version the MOCS settings

 drivers/gpu/drm/i915/i915_drv.c |  3 +++
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_pci.c | 13 +++++++++----
 include/uapi/drm/i915_drm.h     |  8 ++++++++
 4 files changed, 22 insertions(+), 4 deletions(-)

Ben Widawsky (2):
  intel: Merge latest i915 uapi
  intel: Make driver aware of MOCS table version

 src/intel/drm/i915_drm.h                  |  8 ++++++++
 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 ++
 6 files changed, 44 insertions(+), 1 deletion(-)

-- 
2.13.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/1] drm/i915: Version the MOCS settings

[Ben Widawsky]

From: Ben Widawsky <benjamin.widawsky@intel.com>

Starting with GEN9, Memory Object Control State (MOCS) becomes an index
into a table as opposed to the direct programming within the command.
The table has 62 usable entries (ie 6 bits can represent all settings),
and each buffer type may use one of these 62 entries to describe
cacheability type, and age (and some other less useful fields).

Because we hadn't dealt with MOCS settings like this, we didn't think
ahead too well and have ended up with a mess for GEN9 (and soon GEN10)
platform. The plan for for future platforms is that the ideal MOCS
settings will be determined, defined, and written in the public PRMs.
After this point, the i915.ko will absorb these settings and sometime
afterwards flip the alpha switch. All driver releases without the final
MOCS table must be considered alpha. Here on, userspace can assume the
MOCS table is definitively done. There will be some reserved entries for
'oh shit' scenarios. This avoids versioning the MOCS table which leaves
somewhat of a mess in userspace trying to handle arbitrarily many MOCS
versions.

But we do have a mess on GEN9. In the beginning, the MOCS table entries
were pre-populated by the hardware based on estimations made prior to
tapeout and we could just use that. Subsequently much performance tuning
was done to determine optimal settings that the i915 driver should load
on top of the hardware defaults. That was posted last as v6 of the
original per-engine MOCS settings:
https://patchwork.freedesktop.org/patch/53237/. Since the MOCS table is
not context saved/restored, it isn't feasible to let userspace upload
its own MOCS table. After a good amount of debate, it was decided that
we'd utilize only the minimal set of entires in mesa anyway, and so we
took only those entries for our MOCS entries.

Now we've come to the realization that indeed there are other MOCS
entries which are more optimal for various buffer types and workloads.
The problem is that the meaning of the indices is ABI (we assume index 0
is the uncached entry, and that there are only 3 entries total).

What this patch [simply] aims to do is expose a parameter to inform
userspace which "version" of the table was loaded by i915. Upon
sufficient data, new entries can be added, and the version can be
bumped. For example, from my original mesa mocs branch:

commit c9b0481bce24af032386701de0266eb5bc24e988
Author: Ben Widawsky <benjamin.widawsky@intel.com>
Date:   Fri Apr 8 10:21:16 2016 -0700

    i965: Use PTE mocs

    Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>

diff --git a/src/mesa/drivers/dri/i965/brw_mocs.c b/src/mesa/drivers/dri/i965/brw_mocs.c
index 5df154eb86..b7bfdab671 100644
--- a/src/mesa/drivers/dri/i965/brw_mocs.c
+++ b/src/mesa/drivers/dri/i965/brw_mocs.c
@@ -14,6 +14,9 @@
 /* Skylake: MOCS is now an index into an array of 62 different caching
  * configurations programmed by the kernel.
  */
+
+/* TC=PTE, LeCC=PTE, LRUM=3, L3CC=WB */
+#define SKL_MOCS_PTE_PTE (3 << 1)
 /* TC=LLC/eLLC, LeCC=WB, LRUM=3, L3CC=WB */
 #define SKL_MOCS_WB  (2 << 1)
 /* TC=LLC/eLLC, LeCC=PTE, LRUM=3, L3CC=WB */
@@ -26,6 +29,9 @@ brw_mocs_get_control_state(const struct brw_context *brw,
    switch (brw->gen) {
    default:
    case 9:
+      if (brw->intelScreen->mocs_version > 1)
+         return SKL_MOCS_PTE_PTE;
+
       return type == INTEL_MOCS_PTE ? SKL_MOCS_PTE : SKL_MOCS_WB;
    case 8:
       return type == INTEL_MOCS_PTE ? BDW_MOCS_PTE : BDW_MOCS_WB;

tl;dr: A versioned MOCS table will allow userspace to be aware of new
and potentially interesting cacheability settings. Next GEN platforms
will not be considered production worthy until the MOCS table is
finalized.

v2: Update 1.5 year old patch. Add comments. Update commit message.

Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c |  3 +++
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_pci.c | 13 +++++++++----
 include/uapi/drm/i915_drm.h     |  8 ++++++++
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9167a73f3c69..26c27b6ae814 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -401,6 +401,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		if (!value)
 			return -ENODEV;
 		break;
+	case I915_PARAM_MOCS_TABLE_VERSION:
+		value = INTEL_INFO(dev_priv)->mocs_version;
+		break;
 	default:
 		DRM_DEBUG("Unknown parameter %d\n", param->param);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index effbe4f72a64..9b30f6e6ef9b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -859,6 +859,8 @@ struct intel_device_info {
 		u16 degamma_lut_size;
 		u16 gamma_lut_size;
 	} color;
+
+	u8 mocs_version;
 };
 
 struct intel_display_error_state;
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 04aaf553e3fa..9697eb91d972 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -356,7 +356,8 @@ static const struct intel_device_info intel_cherryview_info = {
 	.platform = INTEL_SKYLAKE, \
 	.has_csr = 1, \
 	.has_guc = 1, \
-	.ddb_size = 896
+	.ddb_size = 896, \
+	.mocs_version = MOCS_TABLE_VERSION
 
 static const struct intel_device_info intel_skylake_info = {
 	SKL_PLATFORM,
@@ -390,6 +391,7 @@ static const struct intel_device_info intel_skylake_gt3_info = {
 	.has_full_ppgtt = 1, \
 	.has_full_48bit_ppgtt = 1, \
 	.has_reset_engine = 1, \
+	.mocs_version = MOCS_TABLE_VERSION, \
 	GEN_DEFAULT_PIPEOFFSETS, \
 	IVB_CURSOR_OFFSETS, \
 	BDW_COLORS
@@ -413,7 +415,8 @@ static const struct intel_device_info intel_geminilake_info = {
 	.platform = INTEL_KABYLAKE, \
 	.has_csr = 1, \
 	.has_guc = 1, \
-	.ddb_size = 896
+	.ddb_size = 896, \
+	.mocs_version = MOCS_TABLE_VERSION
 
 static const struct intel_device_info intel_kabylake_info = {
 	KBL_PLATFORM,
@@ -431,7 +434,8 @@ static const struct intel_device_info intel_kabylake_gt3_info = {
 	.platform = INTEL_COFFEELAKE, \
 	.has_csr = 1, \
 	.has_guc = 1, \
-	.ddb_size = 896
+	.ddb_size = 896, \
+	.mocs_version = MOCS_TABLE_VERSION
 
 static const struct intel_device_info intel_coffeelake_info = {
 	CFL_PLATFORM,
@@ -448,7 +452,8 @@ static const struct intel_device_info intel_cannonlake_info = {
 	.platform = INTEL_CANNONLAKE,
 	.gen = 10,
 	.ddb_size = 1024,
-	.has_csr = 1,
+	.has_csr = 1, \
+	.mocs_version = MOCS_TABLE_VERSION
 };
 
 /*
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7ccbd6a2bbe0..2e370c40e48d 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -431,6 +431,14 @@ typedef struct drm_i915_irq_wait {
  */
 #define I915_PARAM_HAS_EXEC_BATCH_FIRST	 48
 
+/* What version of the MOCS table we have. For GEN9 GPUs, the PRM defined
+ * non-optimal settings for the MOCS table. As a result, we were required to use a
+ * small subset, and later add new settings. This param allows userspace to
+ * determine which settings are there.
+ */
+#define MOCS_TABLE_VERSION		  1 /* Build time MOCS table version */
+#define I915_PARAM_MOCS_TABLE_VERSION	 49
+
 typedef struct drm_i915_getparam {
 	__s32 param;
 	/*
-- 
2.13.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] intel: Merge latest i915 uapi

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 src/intel/drm/i915_drm.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/intel/drm/i915_drm.h b/src/intel/drm/i915_drm.h
index c26bf7c125..69e38ce89f 100644
--- a/src/intel/drm/i915_drm.h
+++ b/src/intel/drm/i915_drm.h
@@ -431,6 +431,14 @@ typedef struct drm_i915_irq_wait {
  */
 #define I915_PARAM_HAS_EXEC_BATCH_FIRST	 48
 
+/* What version of the MOCS table we have. For GEN9 GPUs, the PRM defined
+ * non-optimal settings for the MOCS table. As a result, we were required to use a
+ * small subset, and later add new settings. This param allows userspace to
+ * determine which settings are there.
+ */
+#define MOCS_TABLE_VERSION               1 /* Build time MOCS table version */
+#define I915_PARAM_MOCS_TABLE_VERSION	 49
+
 typedef struct drm_i915_getparam {
 	__s32 param;
 	/*
-- 
2.13.2

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ben Widawsky]

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");
+      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);
-- 
2.13.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Version the MOCS settings

[Chris Wilson]

Quoting Ben Widawsky (2017-07-07 00:27:01)
>  drivers/gpu/drm/i915/i915_drv.c |  3 +++
>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>  drivers/gpu/drm/i915/i915_pci.c | 13 +++++++++----
>  include/uapi/drm/i915_drm.h     |  8 ++++++++
>  4 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9167a73f3c69..26c27b6ae814 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -401,6 +401,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>                 if (!value)
>                         return -ENODEV;
>                 break;
> +       case I915_PARAM_MOCS_TABLE_VERSION:
> +               value = INTEL_INFO(dev_priv)->mocs_version;

If we use intel_mocs_get_table_version() we can put this magic number
in intel_mocs.c next to the tables, where we can keep its history and
hopefully be able to remember to update it.

> +/* What version of the MOCS table we have. For GEN9 GPUs, the PRM defined
> + * non-optimal settings for the MOCS table. As a result, we were required to use a
> + * small subset, and later add new settings. This param allows userspace to
> + * determine which settings are there.
> + */
> +#define MOCS_TABLE_VERSION               1 /* Build time MOCS table version */

How are you planing to share this? When we update we bump this number,
and then mesa copies it across and uses it after verifying it as 0,1 on
an old kernel.

I don't think you want to expose the updated constant here, but symbolic
names for each version? (What would be the point?)

Next question, why a version number and not just the number of entries
defined? Each index is defined by ABI once assigned, so the number of
entries still operates as a version number and allows easy checking.

	if (advanced_cacheing_idx < kernel_max_mocs)
		return advanced_cacheing_idx;
	if (default_cacheing_idx < kernel_max_mocs)
		return default_cacheing_idx;

	return follow_pte_idx;

give or take the smarts to choose the preferred indices for any
particular scenario.

In the future, if we finally get user defined mocs, the table_size will
then give the start of the user modifiable indices (presming they want
to keep the predefined entries for compatibility?))
-Chris
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/1] drm/i915: Version the MOCS settings

[Emil Velikov]

On 7 July 2017 at 11:34, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Ben Widawsky (2017-07-07 00:27:01)
>>  drivers/gpu/drm/i915/i915_drv.c |  3 +++
>>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>>  drivers/gpu/drm/i915/i915_pci.c | 13 +++++++++----
>>  include/uapi/drm/i915_drm.h     |  8 ++++++++
>>  4 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 9167a73f3c69..26c27b6ae814 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -401,6 +401,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>>                 if (!value)
>>                         return -ENODEV;
>>                 break;
>> +       case I915_PARAM_MOCS_TABLE_VERSION:
>> +               value = INTEL_INFO(dev_priv)->mocs_version;
>
> If we use intel_mocs_get_table_version() we can put this magic number
> in intel_mocs.c next to the tables, where we can keep its history and
> hopefully be able to remember to update it.
>
>> +/* What version of the MOCS table we have. For GEN9 GPUs, the PRM defined
>> + * non-optimal settings for the MOCS table. As a result, we were required to use a
>> + * small subset, and later add new settings. This param allows userspace to
>> + * determine which settings are there.
>> + */
>> +#define MOCS_TABLE_VERSION               1 /* Build time MOCS table version */
>
> How are you planing to share this? When we update we bump this number,
> and then mesa copies it across and uses it after verifying it as 0,1 on
> an old kernel.
>
> I don't think you want to expose the updated constant here, but symbolic
> names for each version? (What would be the point?)
>
FWIW I have to agree with Chris here - having the value is of limited
use. Furthermore it mostly confuses people when writing the user space
parts.

For example:
Mesa implements v1 and uses the define. Kernel headers get updated to
v2 and Mesa supporting v1 gets rebuild against them.
Mesa stores/treats as the MOCS version has "v2" when the actual
hardware/kernel supports "v1".

The expected issues vary depending on the implementation, but I
suspect it won't be fun :-)

IMHO it's better if user space is explicit on the versions it supports
and kernel should avoid exposing such defines unless really needed.

HTH
Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Mesa-dev] [PATCH 1/1] drm/i915: Version the MOCS settings

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 3095 bytes --]

On Fri, Jul 7, 2017 at 3:34 AM, Chris Wilson <chris@chris-wilson.co.uk>
wrote:

> Quoting Ben Widawsky (2017-07-07 00:27:01)
> >  drivers/gpu/drm/i915/i915_drv.c |  3 +++
> >  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>   drivers/gpu/drm/i915/i915_pci.c | 13 +++++++++----
> >  include/uapi/drm/i915_drm.h     |  8 ++++++++
> >  4 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> > index 9167a73f3c69..26c27b6ae814 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -401,6 +401,9 @@ static int i915_getparam(struct drm_device *dev,
> void *data,
> >                 if (!value)
> >                         return -ENODEV;
> >                 break;
> > +       case I915_PARAM_MOCS_TABLE_VERSION:
> > +               value = INTEL_INFO(dev_priv)->mocs_version;
>
> If we use intel_mocs_get_table_version() we can put this magic number
> in intel_mocs.c next to the tables, where we can keep its history and
> hopefully be able to remember to update it.
>
> > +/* What version of the MOCS table we have. For GEN9 GPUs, the PRM
> defined
> > + * non-optimal settings for the MOCS table. As a result, we were
> required to use a
> > + * small subset, and later add new settings. This param allows
> userspace to
> > + * determine which settings are there.
> > + */
> > +#define MOCS_TABLE_VERSION               1 /* Build time MOCS table
> version */
>
> How are you planing to share this? When we update we bump this number,
> and then mesa copies it across and uses it after verifying it as 0,1 on
> an old kernel.
>

Agreed.  I don't see how having a #define for compile-time mocs version is
useful.  The compile-time version doesn't really matter and we wouldn't
want to use that in i965/anv anyway (more on that in the other patch).


> I don't think you want to expose the updated constant here, but symbolic
> names for each version? (What would be the point?)
>
> Next question, why a version number and not just the number of entries
> defined? Each index is defined by ABI once assigned, so the number of
> entries still operates as a version number and allows easy checking.
>
>         if (advanced_cacheing_idx < kernel_max_mocs)
>                 return advanced_cacheing_idx;
>         if (default_cacheing_idx < kernel_max_mocs)
>                 return default_cacheing_idx;
>
>         return follow_pte_idx;
>
> give or take the smarts to choose the preferred indices for any
> particular scenario.
>

I'll have to think about it a bit more but this sounds like a fairly good
idea.  I see two major benefits:

 1. The kernel can return ARRAY_SIZE(mocs_table_for_your_gen) and we will
never forget to update it.
 2. It makes the "does this MOCS value exist" check much easier.  I imagine
future userspace code which chooses mocs values having some sort of "try
and fall back" approach to making MOCS choices and this would be convenient.

That said, having it be a version may have it's advantages, I just don't
know what they are yet.

--Jason

[-- Attachment #1.2: Type: text/html, Size: 4067 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 5210 bytes --]

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
>

[-- Attachment #1.2: Type: text/html, Size: 7424 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/1] drm/i915: Version the MOCS settings

[Ben Widawsky]

On 17-07-07 11:34:48, Chris Wilson wrote:
>Quoting Ben Widawsky (2017-07-07 00:27:01)
>>  drivers/gpu/drm/i915/i915_drv.c |  3 +++
>>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>>  drivers/gpu/drm/i915/i915_pci.c | 13 +++++++++----
>>  include/uapi/drm/i915_drm.h     |  8 ++++++++
>>  4 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 9167a73f3c69..26c27b6ae814 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -401,6 +401,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>>                 if (!value)
>>                         return -ENODEV;
>>                 break;
>> +       case I915_PARAM_MOCS_TABLE_VERSION:
>> +               value = INTEL_INFO(dev_priv)->mocs_version;
>
>If we use intel_mocs_get_table_version() we can put this magic number
>in intel_mocs.c next to the tables, where we can keep its history and
>hopefully be able to remember to update it.
>

Yeah, that seems like an improvement to me as well.

>> +/* What version of the MOCS table we have. For GEN9 GPUs, the PRM defined
>> + * non-optimal settings for the MOCS table. As a result, we were required to use a
>> + * small subset, and later add new settings. This param allows userspace to
>> + * determine which settings are there.
>> + */
>> +#define MOCS_TABLE_VERSION               1 /* Build time MOCS table version */
>
>How are you planing to share this? When we update we bump this number,
>and then mesa copies it across and uses it after verifying it as 0,1 on
>an old kernel.
>
>I don't think you want to expose the updated constant here, but symbolic
>names for each version? (What would be the point?)
>

At least one thing wrong here is we would need per GEN constants, which is maybe
what you meant and I misunderstood. Assuming you had per GEN constants, which I
don't like, I believe everything works out fine. So, I'll remove this compile
time MOCS versioning.

>Next question, why a version number and not just the number of entries
>defined? Each index is defined by ABI once assigned, so the number of
>entries still operates as a version number and allows easy checking.
>
>	if (advanced_cacheing_idx < kernel_max_mocs)
>		return advanced_cacheing_idx;
>	if (default_cacheing_idx < kernel_max_mocs)
>		return default_cacheing_idx;
>
>	return follow_pte_idx;
>
>give or take the smarts to choose the preferred indices for any
>particular scenario.
>
>In the future, if we finally get user defined mocs, the table_size will
>then give the start of the user modifiable indices (presming they want
>to keep the predefined entries for compatibility?))
>-Chris

Yes, I considered this as well. I see no difference really as to one versus the
other. In fact, if you're to support multiple table versions, I think it's
actually easier with a pure version:

switch (kernel_mocs_version) {
case 3:
	return new_best_cacheing_index;
case 2:
	return old_best_cacheing_index;
case 1:
	return naive_best_index;
}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Mesa-dev] [PATCH 1/1] drm/i915: Version the MOCS settings

[Ben Widawsky]

On 17-07-07 09:23:26, Jason Ekstrand wrote:
>On Fri, Jul 7, 2017 at 3:34 AM, Chris Wilson <chris@chris-wilson.co.uk>
>wrote:
>
>> Quoting Ben Widawsky (2017-07-07 00:27:01)
>> >  drivers/gpu/drm/i915/i915_drv.c |  3 +++
>> >  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>>   drivers/gpu/drm/i915/i915_pci.c | 13 +++++++++----
>> >  include/uapi/drm/i915_drm.h     |  8 ++++++++
>> >  4 files changed, 22 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> > index 9167a73f3c69..26c27b6ae814 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > @@ -401,6 +401,9 @@ static int i915_getparam(struct drm_device *dev,
>> void *data,
>> >                 if (!value)
>> >                         return -ENODEV;
>> >                 break;
>> > +       case I915_PARAM_MOCS_TABLE_VERSION:
>> > +               value = INTEL_INFO(dev_priv)->mocs_version;
>>
>> If we use intel_mocs_get_table_version() we can put this magic number
>> in intel_mocs.c next to the tables, where we can keep its history and
>> hopefully be able to remember to update it.
>>
>> > +/* What version of the MOCS table we have. For GEN9 GPUs, the PRM
>> defined
>> > + * non-optimal settings for the MOCS table. As a result, we were
>> required to use a
>> > + * small subset, and later add new settings. This param allows
>> userspace to
>> > + * determine which settings are there.
>> > + */
>> > +#define MOCS_TABLE_VERSION               1 /* Build time MOCS table
>> version */
>>
>> How are you planing to share this? When we update we bump this number,
>> and then mesa copies it across and uses it after verifying it as 0,1 on
>> an old kernel.
>>
>
>Agreed.  I don't see how having a #define for compile-time mocs version is
>useful.  The compile-time version doesn't really matter and we wouldn't
>want to use that in i965/anv anyway (more on that in the other patch).
>
>

I think we're all agreed here.

>> I don't think you want to expose the updated constant here, but symbolic
>> names for each version? (What would be the point?)
>>
>> Next question, why a version number and not just the number of entries
>> defined? Each index is defined by ABI once assigned, so the number of
>> entries still operates as a version number and allows easy checking.
>>
>>         if (advanced_cacheing_idx < kernel_max_mocs)
>>                 return advanced_cacheing_idx;
>>         if (default_cacheing_idx < kernel_max_mocs)
>>                 return default_cacheing_idx;
>>
>>         return follow_pte_idx;
>>
>> give or take the smarts to choose the preferred indices for any
>> particular scenario.
>>
>
>I'll have to think about it a bit more but this sounds like a fairly good
>idea.  I see two major benefits:
>
> 1. The kernel can return ARRAY_SIZE(mocs_table_for_your_gen) and we will
>never forget to update it.
> 2. It makes the "does this MOCS value exist" check much easier.  I imagine
>future userspace code which chooses mocs values having some sort of "try
>and fall back" approach to making MOCS choices and this would be convenient.
>
>That said, having it be a version may have it's advantages, I just don't
>know what they are yet.
>
>--Jason

Please direct comments to my response to Chris if you have more. To me it's 6
one way, half dozen the other - and I believe version has a more direct meaning
(and the ability to potentially, albeit a terrible idea, rewrite entries).

If people are going to block a review based on this, I will change it, but I'd
rather not.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/1] drm/i915: Version the MOCS settings

[Chris Wilson]

Quoting Ben Widawsky (2017-07-07 19:42:25)
> On 17-07-07 11:34:48, Chris Wilson wrote:
> >Quoting Ben Widawsky (2017-07-07 00:27:01)
> >>  drivers/gpu/drm/i915/i915_drv.c |  3 +++
> >>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
> >>  drivers/gpu/drm/i915/i915_pci.c | 13 +++++++++----
> >>  include/uapi/drm/i915_drm.h     |  8 ++++++++
> >>  4 files changed, 22 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index 9167a73f3c69..26c27b6ae814 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -401,6 +401,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
> >>                 if (!value)
> >>                         return -ENODEV;
> >>                 break;
> >> +       case I915_PARAM_MOCS_TABLE_VERSION:
> >> +               value = INTEL_INFO(dev_priv)->mocs_version;
> >
> >If we use intel_mocs_get_table_version() we can put this magic number
> >in intel_mocs.c next to the tables, where we can keep its history and
> >hopefully be able to remember to update it.
> >
> 
> Yeah, that seems like an improvement to me as well.
> 
> >> +/* What version of the MOCS table we have. For GEN9 GPUs, the PRM defined
> >> + * non-optimal settings for the MOCS table. As a result, we were required to use a
> >> + * small subset, and later add new settings. This param allows userspace to
> >> + * determine which settings are there.
> >> + */
> >> +#define MOCS_TABLE_VERSION               1 /* Build time MOCS table version */
> >
> >How are you planing to share this? When we update we bump this number,
> >and then mesa copies it across and uses it after verifying it as 0,1 on
> >an old kernel.
> >
> >I don't think you want to expose the updated constant here, but symbolic
> >names for each version? (What would be the point?)
> >
> 
> At least one thing wrong here is we would need per GEN constants, which is maybe
> what you meant and I misunderstood. Assuming you had per GEN constants, which I
> don't like, I believe everything works out fine. So, I'll remove this compile
> time MOCS versioning.

I figured you were going towards per-gen versioning, which is kind of
why I liked the idea of table size -- but that only makes sense if
somehow the index has the same meaning across gen (which it won't).

> >Next question, why a version number and not just the number of entries
> >defined? Each index is defined by ABI once assigned, so the number of
> >entries still operates as a version number and allows easy checking.
> >
> >       if (advanced_cacheing_idx < kernel_max_mocs)
> >               return advanced_cacheing_idx;
> >       if (default_cacheing_idx < kernel_max_mocs)
> >               return default_cacheing_idx;
> >
> >       return follow_pte_idx;
> >
> >give or take the smarts to choose the preferred indices for any
> >particular scenario.
> >
> >In the future, if we finally get user defined mocs, the table_size will
> >then give the start of the user modifiable indices (presming they want
> >to keep the predefined entries for compatibility?))
> >-Chris
> 
> Yes, I considered this as well. I see no difference really as to one versus the
> other. In fact, if you're to support multiple table versions, I think it's
> actually easier with a pure version:
> 
> switch (kernel_mocs_version) {
> case 3:
>         return new_best_cacheing_index;
> case 2:
>         return old_best_cacheing_index;
> case 1:
>         return naive_best_index;
> }

Indeed 6 of one, half a dozen of the other. Whichever you pick, 3 years
down the line you wish you picked the other. The big advantage of using
an absolute version is that you can just stuff these into tables. Ok, I
like that more, a version parameter (that may be per-gen) worksforme.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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 <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.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx