[Intel-xe] [PATCH 0/6] Assorted MOCS updates

[Intel-xe] [PATCH 0/6] Assorted MOCS updates

[Matt Roper]

The Xe MOCS code is heavily modelled after the equivalent i915 code, but
there are a few items that are not appropriate for the Xe driver.
Philippe's patch to add the MTL-specific table is also included.

As a follow-up, I'd like to rework Xe's MOCS to just use the xe_reg_sr
framework.  But that's a bit more invasive, so I'll save that for a
future series.


Matt Roper (5):
  drm/xe/mocs: Drop unwanted TGL table
  drm/xe/mocs: Add missing RKL handling
  drm/xe/mocs: Drop xe_mocs_info_index
  drm/xe/mocs: Drop duplicate assignment of uc_index
  drm/xe/mocs: LNCF MOCS settings only need to be restored on pre-Xe_HP

Philippe Lecluse (1):
  drm/xe/mocs: add MTL mocs

 drivers/gpu/drm/xe/xe_execlist.c   |   2 +-
 drivers/gpu/drm/xe/xe_guc_ads.c    |  10 +-
 drivers/gpu/drm/xe/xe_guc_submit.c |   1 -
 drivers/gpu/drm/xe/xe_mocs.c       | 160 ++++++++++++-----------------
 drivers/gpu/drm/xe/xe_mocs.h       |   1 -
 5 files changed, 76 insertions(+), 98 deletions(-)

-- 
2.39.1

[Intel-xe] [PATCH 1/6] drm/xe/mocs: Drop unwanted TGL table

[Matt Roper]

TGL/RKL/ADLS/ADLP are all supposed to use the same MOCS table, with
values defined in the bspec.  Any entries listed in the bspec as
reserved/error/undefined should always be initialized to the most cached
and least coherent setting possible so that any userspace accidentally
referencing those undefined entries will only experience an increase in
coherency if spec updates down the road start defining real values.

The TGL and gen12 tables that exist in the driver today are identical
except that the TGL includes one additional (incorrect) setting for PAT
index 1.  This is a holdover from i915 where the platform was enabled
with an incorrect setting and by the time we noticed, it was too late to
fix the table without breaking ABI compatibility (and on TGL we did
indeed have some buggy userspace that was referencing the 'reserved'
entry 1).  Since the Xe driver starts fresh with a clean slate on ABI,
there's no need to repeat the mistakes of i915 here.

Bspec: 45101
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/xe/xe_mocs.c | 46 ------------------------------------
 1 file changed, 46 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
index 86b966fffbe5..09adb69d1545 100644
--- a/drivers/gpu/drm/xe/xe_mocs.c
+++ b/drivers/gpu/drm/xe/xe_mocs.c
@@ -247,47 +247,6 @@ struct xe_mocs_info {
 		LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
 		L3_1_UC)
 
-static const struct xe_mocs_entry tgl_mocs_desc[] = {
-	/*
-	 * NOTE:
-	 * Reserved and unspecified MOCS indices have been set to (L3 + LCC).
-	 * These reserved entries should never be used, they may be changed
-	 * to low performant variants with better coherency in the future if
-	 * more entries are needed. We are programming index XE_MOCS_PTE(1)
-	 * only, __init_mocs_table() take care to program unused index with
-	 * this entry.
-	 */
-	MOCS_ENTRY(XE_MOCS_PTE,
-		   LE_0_PAGETABLE | LE_TC_0_PAGETABLE,
-		   L3_1_UC),
-	GEN11_MOCS_ENTRIES,
-
-	/* Implicitly enable L1 - HDC:L1 + L3 + LLC */
-	MOCS_ENTRY(48,
-		   LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
-		   L3_3_WB),
-	/* Implicitly enable L1 - HDC:L1 + L3 */
-	MOCS_ENTRY(49,
-		   LE_1_UC | LE_TC_1_LLC,
-		   L3_3_WB),
-	/* Implicitly enable L1 - HDC:L1 + LLC */
-	MOCS_ENTRY(50,
-		   LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
-		   L3_1_UC),
-	/* Implicitly enable L1 - HDC:L1 */
-	MOCS_ENTRY(51,
-		   LE_1_UC | LE_TC_1_LLC,
-		   L3_1_UC),
-	/* HW Special Case (CCS) */
-	MOCS_ENTRY(60,
-		   LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
-		   L3_1_UC),
-	/* HW Special Case (Displayable) */
-	MOCS_ENTRY(61,
-		   LE_1_UC | LE_TC_1_LLC,
-		   L3_3_WB),
-};
-
 static const struct xe_mocs_entry dg1_mocs_desc[] = {
 	/* UC */
 	MOCS_ENTRY(1, 0, L3_1_UC),
@@ -422,11 +381,6 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
 		info->unused_entries_index = 5;
 		break;
 	case XE_TIGERLAKE:
-		info->size  = ARRAY_SIZE(tgl_mocs_desc);
-		info->table = tgl_mocs_desc;
-		info->n_entries = GEN9_NUM_MOCS_ENTRIES;
-		info->uc_index = 3;
-		break;
 	case XE_ALDERLAKE_S:
 	case XE_ALDERLAKE_P:
 		info->size  = ARRAY_SIZE(gen12_mocs_desc);
-- 
2.39.1

[Intel-xe] [PATCH 2/6] drm/xe/mocs: Add missing RKL handling

[Matt Roper]

RKL should use the same "gen12" MOCS handling as TGL/ADL-S/ADL-P.

Bspec: 45101
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/xe/xe_mocs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
index 09adb69d1545..ec89ff3ac29b 100644
--- a/drivers/gpu/drm/xe/xe_mocs.c
+++ b/drivers/gpu/drm/xe/xe_mocs.c
@@ -381,6 +381,7 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
 		info->unused_entries_index = 5;
 		break;
 	case XE_TIGERLAKE:
+	case XE_ROCKETLAKE:
 	case XE_ALDERLAKE_S:
 	case XE_ALDERLAKE_P:
 		info->size  = ARRAY_SIZE(gen12_mocs_desc);
-- 
2.39.1

[Intel-xe] [PATCH 3/6] drm/xe/mocs: Drop xe_mocs_info_index

[Matt Roper]

The values in the xe_mocs_info_index enum only match old pre-gen12
hardware not supported by the Xe driver.

The only usage of this enum was to set a default value for
info->unused_entries_index, but this is unnecessary since every platform
in the subsequent switch statement sets a proper platform-specific value
(and the XE_MOCS_PTE default doesn't even make sense since the hardware
dropped the "use PAT settings" capability in gen12).

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/xe/xe_mocs.c | 30 ++----------------------------
 1 file changed, 2 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
index ec89ff3ac29b..583e198af88d 100644
--- a/drivers/gpu/drm/xe/xe_mocs.c
+++ b/drivers/gpu/drm/xe/xe_mocs.c
@@ -23,30 +23,6 @@ static inline void mocs_dbg(const struct drm_device *dev,
 { /* noop */ }
 #endif
 
-/*
- * MOCS indexes used for GPU surfaces, defining the cacheability of the
- * surface data and the coherency for this data wrt. CPU vs. GPU accesses.
- */
-enum xe_mocs_info_index {
-	/*
-	 * Not cached anywhere, coherency between CPU and GPU accesses is
-	 * guaranteed.
-	 */
-	XE_MOCS_UNCACHED,
-	/*
-	 * Cacheability and coherency controlled by the kernel automatically
-	 * based on the xxxx  IOCTL setting and the current
-	 * usage of the surface (used for display scanout or not).
-	 */
-	XE_MOCS_PTE,
-	/*
-	 * Cached in all GPU caches available on the platform.
-	 * Coherency between CPU and GPU accesses to the surface is not
-	 * guaranteed without extra synchronization.
-	 */
-	XE_MOCS_CACHED,
-};
-
 enum {
 	HAS_GLOBAL_MOCS = BIT(0),
 	HAS_RENDER_L3CC = BIT(1),
@@ -341,7 +317,6 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
 
 	memset(info, 0, sizeof(struct xe_mocs_info));
 
-	info->unused_entries_index = XE_MOCS_PTE;
 	switch (xe->info.platform) {
 	case XE_PVC:
 		info->size = ARRAY_SIZE(pvc_mocs_desc);
@@ -406,9 +381,8 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
 }
 
 /*
- * Get control_value from MOCS entry taking into account when it's not used
- * then if unused_entries_index is non-zero then its value will be returned
- * otherwise XE_MOCS_PTE's value is returned in this case.
+ * Get control_value from MOCS entry.  If the table entry is not defined, the
+ * settings from unused_entries_index will be returned.
  */
 static u32 get_entry_control(const struct xe_mocs_info *info,
 			     unsigned int index)
-- 
2.39.1

[Intel-xe] [PATCH 4/6] drm/xe/mocs: Drop duplicate assignment of uc_index

[Matt Roper]

The DG1 branch needlessly assigns uc_index twice.  Drop the second
instance.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/xe/xe_mocs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
index 583e198af88d..696001f60259 100644
--- a/drivers/gpu/drm/xe/xe_mocs.c
+++ b/drivers/gpu/drm/xe/xe_mocs.c
@@ -352,7 +352,6 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
 		info->table = dg1_mocs_desc;
 		info->uc_index = 1;
 		info->n_entries = GEN9_NUM_MOCS_ENTRIES;
-		info->uc_index = 1;
 		info->unused_entries_index = 5;
 		break;
 	case XE_TIGERLAKE:
-- 
2.39.1

[Intel-xe] [PATCH 5/6] drm/xe/mocs: add MTL mocs

[Matt Roper]

From: Philippe Lecluse <philippe.lecluse@intel.com>

It was incorrectly using dg2_mocs for now.

v2 (MattR):
 - Use REG_GENMASK/REG_FIELD_PREP for bitfields
 - Add bspec references

Bspec: 45101, 45410, 63882
Signed-off-by: Philippe Lecluse <philippe.lecluse@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/xe/xe_mocs.c | 69 +++++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
index 696001f60259..3b48934d99d4 100644
--- a/drivers/gpu/drm/xe/xe_mocs.c
+++ b/drivers/gpu/drm/xe/xe_mocs.c
@@ -62,6 +62,10 @@ struct xe_mocs_info {
 #define L3_GLBGO(value)		((value) << 6)
 #define L3_LKUP(value)		((value) << 7)
 
+/* Defines for the tables (GLOB_MOCS_0 - GLOB_MOCS_16) */
+#define _L4_CACHEABILITY	REG_GENMASK(3, 2)
+#define IG_PAT			REG_BIT(8)
+
 /* Helper defines */
 #define GEN9_NUM_MOCS_ENTRIES	64  /* 63-64 are reserved, but configured. */
 #define PVC_NUM_MOCS_ENTRIES	3
@@ -89,6 +93,12 @@ struct xe_mocs_info {
 #define L3_2_RESERVED		_L3_CACHEABILITY(2)
 #define L3_3_WB			_L3_CACHEABILITY(3)
 
+/* L4 caching options */
+#define L4_0_WB                 REG_FIELD_PREP(_L4_CACHEABILITY, 0)
+#define L4_1_WT                 REG_FIELD_PREP(_L4_CACHEABILITY, 1)
+#define L4_2_RESERVED           REG_FIELD_PREP(_L4_CACHEABILITY, 2)
+#define L4_3_UC                 REG_FIELD_PREP(_L4_CACHEABILITY, 3)
+
 #define MOCS_ENTRY(__idx, __control_value, __l3cc_value) \
 	[__idx] = { \
 		.control_value = __control_value, \
@@ -310,6 +320,57 @@ static const struct xe_mocs_entry pvc_mocs_desc[] = {
 	MOCS_ENTRY(2, 0, L3_3_WB),
 };
 
+static const struct xe_mocs_entry mtl_mocs_desc[] = {
+	/* Error - Reserved for Non-Use */
+	MOCS_ENTRY(0,
+		   0,
+		   L3_LKUP(1) | L3_3_WB),
+	/* Cached - L3 + L4 */
+	MOCS_ENTRY(1,
+		   IG_PAT,
+		   L3_LKUP(1) | L3_3_WB),
+	/* L4 - GO:L3 */
+	MOCS_ENTRY(2,
+		   IG_PAT,
+		   L3_LKUP(1) | L3_1_UC),
+	/* Uncached - GO:L3 */
+	MOCS_ENTRY(3,
+		   IG_PAT | L4_3_UC,
+		   L3_LKUP(1) | L3_1_UC),
+	/* L4 - GO:Mem */
+	MOCS_ENTRY(4,
+		   IG_PAT,
+		   L3_LKUP(1) | L3_GLBGO(1) | L3_1_UC),
+	/* Uncached - GO:Mem */
+	MOCS_ENTRY(5,
+		   IG_PAT | L4_3_UC,
+		   L3_LKUP(1) | L3_GLBGO(1) | L3_1_UC),
+	/* L4 - L3:NoLKUP; GO:L3 */
+	MOCS_ENTRY(6,
+		   IG_PAT,
+		   L3_1_UC),
+	/* Uncached - L3:NoLKUP; GO:L3 */
+	MOCS_ENTRY(7,
+		   IG_PAT | L4_3_UC,
+		   L3_1_UC),
+	/* L4 - L3:NoLKUP; GO:Mem */
+	MOCS_ENTRY(8,
+		   IG_PAT,
+		   L3_GLBGO(1) | L3_1_UC),
+	/* Uncached - L3:NoLKUP; GO:Mem */
+	MOCS_ENTRY(9,
+		   IG_PAT | L4_3_UC,
+		   L3_GLBGO(1) | L3_1_UC),
+	/* Display - L3; L4:WT */
+	MOCS_ENTRY(14,
+		   IG_PAT | L4_1_WT,
+		   L3_LKUP(1) | L3_3_WB),
+	/* CCS - Non-Displayable */
+	MOCS_ENTRY(15,
+		   IG_PAT,
+		   L3_GLBGO(1) | L3_1_UC),
+};
+
 static unsigned int get_mocs_settings(struct xe_device *xe,
 				      struct xe_mocs_info *info)
 {
@@ -327,11 +388,11 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
 		info->unused_entries_index = 2;
 		break;
 	case XE_METEORLAKE:
-		info->size = ARRAY_SIZE(dg2_mocs_desc);
-		info->table = dg2_mocs_desc;
+		info->size = ARRAY_SIZE(mtl_mocs_desc);
+		info->table = mtl_mocs_desc;
 		info->n_entries = MTL_NUM_MOCS_ENTRIES;
-		info->uc_index = 1;
-		info->unused_entries_index = 3;
+		info->uc_index = 9;
+		info->unused_entries_index = 1;
 		break;
 	case XE_DG2:
 		if (xe->info.subplatform == XE_SUBPLATFORM_DG2_G10 &&
-- 
2.39.1

[Intel-xe] [PATCH 6/6] drm/xe/mocs: LNCF MOCS settings only need to be restored on pre-Xe_HP

[Matt Roper]

Reprogramming the LNCF MOCS registers on render domain reset is not
intended to be regular driver programming, but rather the implementation
of a specific workaround (Wa_1607983814).  This workaround no longer
applies on Xe_HP any beyond, so we can expect that these registers, like
the rest of the LNCF/LBCF registers, will maintain their values through
all engine resets.  We should only add these registers to the GuC's
save/restore list on platforms that need the workaround.

Furthermore, xe_mocs_init_engine() appears to be another attempt to
satisfy this same workaround.  This is unnecessary on the Xe driver
since even on platforms where the workaround is necessary, all
single-engine resets are initiated by the GuC and thus the GuC will take
care of saving/restoring these registers.  The only host-initiated
resets we have in Xe are full GT resets which will already
(re)initialize these registers as part of the regular xe_mocs_init()
flow.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/xe/xe_execlist.c   |  2 +-
 drivers/gpu/drm/xe/xe_guc_ads.c    | 10 +++++++---
 drivers/gpu/drm/xe/xe_guc_submit.c |  1 -
 drivers/gpu/drm/xe/xe_mocs.c       | 13 -------------
 drivers/gpu/drm/xe/xe_mocs.h       |  1 -
 5 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c
index d555d77cbf49..fd0ebfe7cae3 100644
--- a/drivers/gpu/drm/xe/xe_execlist.c
+++ b/drivers/gpu/drm/xe/xe_execlist.c
@@ -462,7 +462,7 @@ static void execlist_engine_suspend_wait(struct xe_engine *e)
 
 static void execlist_engine_resume(struct xe_engine *e)
 {
-	xe_mocs_init_engine(e);
+	/* NIY */
 }
 
 static const struct xe_engine_ops execlist_engine_ops = {
diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
index 0c08cecaca40..a233023a6616 100644
--- a/drivers/gpu/drm/xe/xe_guc_ads.c
+++ b/drivers/gpu/drm/xe/xe_guc_ads.c
@@ -430,6 +430,7 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
 					  struct iosys_map *regset_map,
 					  struct xe_hw_engine *hwe)
 {
+	struct xe_device *xe = ads_to_xe(ads);
 	struct xe_hw_engine *hwe_rcs_reset_domain =
 		xe_gt_any_hw_engine_by_reset_domain(hwe->gt, XE_ENGINE_CLASS_RENDER);
 	struct xe_reg_sr_entry *entry;
@@ -464,9 +465,12 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
 					  e->reg, e->flags, count++);
 	}
 
-	for (i = 0; i < LNCFCMOCS_REG_COUNT; i++) {
-		guc_mmio_regset_write_one(ads, regset_map,
-					  GEN9_LNCFCMOCS(i).reg, 0, count++);
+	/* Wa_1607983814 */
+	if (GRAPHICS_VER(xe) == 12 && GRAPHICS_VERx100(xe) < 1250) {
+		for (i = 0; i < LNCFCMOCS_REG_COUNT; i++) {
+			guc_mmio_regset_write_one(ads, regset_map,
+						  GEN9_LNCFCMOCS(i).reg, 0, count++);
+		}
 	}
 
 	XE_BUG_ON(ads->regset_size < (count * sizeof(struct guc_mmio_reg)));
diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index a54f7f82d04d..3766b77a0d90 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -1267,7 +1267,6 @@ static void guc_engine_resume(struct xe_engine *e)
 
 	XE_BUG_ON(e->guc->suspend_pending);
 
-	xe_mocs_init_engine(e);
 	guc_engine_add_msg(e, msg, RESUME);
 }
 
diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
index 3b48934d99d4..7e495d699295 100644
--- a/drivers/gpu/drm/xe/xe_mocs.c
+++ b/drivers/gpu/drm/xe/xe_mocs.c
@@ -507,19 +507,6 @@ static void init_l3cc_table(struct xe_gt *gt,
 	}
 }
 
-void xe_mocs_init_engine(const struct xe_engine *engine)
-{
-	struct xe_mocs_info table;
-	unsigned int flags;
-
-	flags = get_mocs_settings(engine->gt->xe, &table);
-	if (!flags)
-		return;
-
-	if (flags & HAS_RENDER_L3CC && engine->class == XE_ENGINE_CLASS_RENDER)
-		init_l3cc_table(engine->gt, &table);
-}
-
 void xe_mocs_init(struct xe_gt *gt)
 {
 	struct xe_mocs_info table;
diff --git a/drivers/gpu/drm/xe/xe_mocs.h b/drivers/gpu/drm/xe/xe_mocs.h
index aba1abe216ab..63500a1d6660 100644
--- a/drivers/gpu/drm/xe/xe_mocs.h
+++ b/drivers/gpu/drm/xe/xe_mocs.h
@@ -11,7 +11,6 @@
 struct xe_engine;
 struct xe_gt;
 
-void xe_mocs_init_engine(const struct xe_engine *engine);
 void xe_mocs_init(struct xe_gt *gt);
 
 /**
-- 
2.39.1

Re: [Intel-xe] [PATCH 1/6] drm/xe/mocs: Drop unwanted TGL table

[Lucas De Marchi]

On Thu, Feb 16, 2023 at 03:17:19PM -0800, Matt Roper wrote:
>TGL/RKL/ADLS/ADLP are all supposed to use the same MOCS table, with
>values defined in the bspec.  Any entries listed in the bspec as
>reserved/error/undefined should always be initialized to the most cached
>and least coherent setting possible so that any userspace accidentally
>referencing those undefined entries will only experience an increase in
>coherency if spec updates down the road start defining real values.
>
>The TGL and gen12 tables that exist in the driver today are identical
>except that the TGL includes one additional (incorrect) setting for PAT
>index 1.  This is a holdover from i915 where the platform was enabled
>with an incorrect setting and by the time we noticed, it was too late to
>fix the table without breaking ABI compatibility (and on TGL we did
>indeed have some buggy userspace that was referencing the 'reserved'
>entry 1).  Since the Xe driver starts fresh with a clean slate on ABI,
>there's no need to repeat the mistakes of i915 here.
>
>Bspec: 45101
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> drivers/gpu/drm/xe/xe_mocs.c | 46 ------------------------------------
> 1 file changed, 46 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
>index 86b966fffbe5..09adb69d1545 100644
>--- a/drivers/gpu/drm/xe/xe_mocs.c
>+++ b/drivers/gpu/drm/xe/xe_mocs.c
>@@ -247,47 +247,6 @@ struct xe_mocs_info {
> 		LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
> 		L3_1_UC)
>
>-static const struct xe_mocs_entry tgl_mocs_desc[] = {
>-	/*
>-	 * NOTE:
>-	 * Reserved and unspecified MOCS indices have been set to (L3 + LCC).
>-	 * These reserved entries should never be used, they may be changed
>-	 * to low performant variants with better coherency in the future if
>-	 * more entries are needed. We are programming index XE_MOCS_PTE(1)
>-	 * only, __init_mocs_table() take care to program unused index with
>-	 * this entry.
>-	 */
>-	MOCS_ENTRY(XE_MOCS_PTE,
>-		   LE_0_PAGETABLE | LE_TC_0_PAGETABLE,
>-		   L3_1_UC),
>-	GEN11_MOCS_ENTRIES,
>-
>-	/* Implicitly enable L1 - HDC:L1 + L3 + LLC */
>-	MOCS_ENTRY(48,
>-		   LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
>-		   L3_3_WB),
>-	/* Implicitly enable L1 - HDC:L1 + L3 */
>-	MOCS_ENTRY(49,
>-		   LE_1_UC | LE_TC_1_LLC,
>-		   L3_3_WB),
>-	/* Implicitly enable L1 - HDC:L1 + LLC */
>-	MOCS_ENTRY(50,
>-		   LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
>-		   L3_1_UC),
>-	/* Implicitly enable L1 - HDC:L1 */
>-	MOCS_ENTRY(51,
>-		   LE_1_UC | LE_TC_1_LLC,
>-		   L3_1_UC),
>-	/* HW Special Case (CCS) */
>-	MOCS_ENTRY(60,
>-		   LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
>-		   L3_1_UC),
>-	/* HW Special Case (Displayable) */
>-	MOCS_ENTRY(61,
>-		   LE_1_UC | LE_TC_1_LLC,
>-		   L3_3_WB),
>-};
>-
> static const struct xe_mocs_entry dg1_mocs_desc[] = {
> 	/* UC */
> 	MOCS_ENTRY(1, 0, L3_1_UC),
>@@ -422,11 +381,6 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
> 		info->unused_entries_index = 5;
> 		break;
> 	case XE_TIGERLAKE:
>-		info->size  = ARRAY_SIZE(tgl_mocs_desc);
>-		info->table = tgl_mocs_desc;
>-		info->n_entries = GEN9_NUM_MOCS_ENTRIES;
>-		info->uc_index = 3;

there is another difference here: it's not only the index 1 that is
different, but any uninitialized entry.

that unused entries would be programmed to XE_MOCS_PTE (1):
LE_0_PAGETABLE | LE_TC_0_PAGETABLE | L3_1_UC. After this patch
they are programmed to
to index 3: LE_1_UC | LE_TC_1_LLC | L3_1_UC

 From the commit message I was understanding you said it was only 1 entry
with wrong value, which as per above is not the case. Maybe reword the
commit message a little bit?

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Lucas De Marchi

>-		break;
> 	case XE_ALDERLAKE_S:
> 	case XE_ALDERLAKE_P:
> 		info->size  = ARRAY_SIZE(gen12_mocs_desc);
>-- 
>2.39.1
>

Re: [Intel-xe] [PATCH 2/6] drm/xe/mocs: Add missing RKL handling

[Lucas De Marchi]

On Thu, Feb 16, 2023 at 03:17:20PM -0800, Matt Roper wrote:
>RKL should use the same "gen12" MOCS handling as TGL/ADL-S/ADL-P.
>
>Bspec: 45101
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>


Lucas De Marchi

>---
> drivers/gpu/drm/xe/xe_mocs.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
>index 09adb69d1545..ec89ff3ac29b 100644
>--- a/drivers/gpu/drm/xe/xe_mocs.c
>+++ b/drivers/gpu/drm/xe/xe_mocs.c
>@@ -381,6 +381,7 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
> 		info->unused_entries_index = 5;
> 		break;
> 	case XE_TIGERLAKE:
>+	case XE_ROCKETLAKE:
> 	case XE_ALDERLAKE_S:
> 	case XE_ALDERLAKE_P:
> 		info->size  = ARRAY_SIZE(gen12_mocs_desc);
>-- 
>2.39.1
>

Re: [Intel-xe] [PATCH 3/6] drm/xe/mocs: Drop xe_mocs_info_index

[Lucas De Marchi]

On Thu, Feb 16, 2023 at 03:17:21PM -0800, Matt Roper wrote:
>The values in the xe_mocs_info_index enum only match old pre-gen12
>hardware not supported by the Xe driver.
>
>The only usage of this enum was to set a default value for
>info->unused_entries_index, but this is unnecessary since every platform
>in the subsequent switch statement sets a proper platform-specific value
>(and the XE_MOCS_PTE default doesn't even make sense since the hardware
>dropped the "use PAT settings" capability in gen12).
>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> drivers/gpu/drm/xe/xe_mocs.c | 30 ++----------------------------
> 1 file changed, 2 insertions(+), 28 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
>index ec89ff3ac29b..583e198af88d 100644
>--- a/drivers/gpu/drm/xe/xe_mocs.c
>+++ b/drivers/gpu/drm/xe/xe_mocs.c
>@@ -23,30 +23,6 @@ static inline void mocs_dbg(const struct drm_device *dev,
> { /* noop */ }
> #endif
>
>-/*
>- * MOCS indexes used for GPU surfaces, defining the cacheability of the
>- * surface data and the coherency for this data wrt. CPU vs. GPU accesses.
>- */
>-enum xe_mocs_info_index {
>-	/*
>-	 * Not cached anywhere, coherency between CPU and GPU accesses is
>-	 * guaranteed.
>-	 */
>-	XE_MOCS_UNCACHED,
>-	/*
>-	 * Cacheability and coherency controlled by the kernel automatically
>-	 * based on the xxxx  IOCTL setting and the current
>-	 * usage of the surface (used for display scanout or not).
>-	 */
>-	XE_MOCS_PTE,
>-	/*
>-	 * Cached in all GPU caches available on the platform.
>-	 * Coherency between CPU and GPU accesses to the surface is not
>-	 * guaranteed without extra synchronization.
>-	 */
>-	XE_MOCS_CACHED,
>-};
>-
> enum {
> 	HAS_GLOBAL_MOCS = BIT(0),
> 	HAS_RENDER_L3CC = BIT(1),
>@@ -341,7 +317,6 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
>
> 	memset(info, 0, sizeof(struct xe_mocs_info));
>
>-	info->unused_entries_index = XE_MOCS_PTE;

since leaving this as 0 should never be valid, maybe add a warning after
the switch for the cases someoneone forgot to set it appropriately?

with that,

	Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi


> 	switch (xe->info.platform) {
> 	case XE_PVC:
> 		info->size = ARRAY_SIZE(pvc_mocs_desc);
>@@ -406,9 +381,8 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
> }
>
> /*
>- * Get control_value from MOCS entry taking into account when it's not used
>- * then if unused_entries_index is non-zero then its value will be returned
>- * otherwise XE_MOCS_PTE's value is returned in this case.
>+ * Get control_value from MOCS entry.  If the table entry is not defined, the
>+ * settings from unused_entries_index will be returned.
>  */
> static u32 get_entry_control(const struct xe_mocs_info *info,
> 			     unsigned int index)
>-- 
>2.39.1
>

Re: [Intel-xe] [PATCH 4/6] drm/xe/mocs: Drop duplicate assignment of uc_index

[Lucas De Marchi]

On Thu, Feb 16, 2023 at 03:17:22PM -0800, Matt Roper wrote:
>The DG1 branch needlessly assigns uc_index twice.  Drop the second
>instance.
>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>


Lucas De Marchi

>---
> drivers/gpu/drm/xe/xe_mocs.c | 1 -
> 1 file changed, 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
>index 583e198af88d..696001f60259 100644
>--- a/drivers/gpu/drm/xe/xe_mocs.c
>+++ b/drivers/gpu/drm/xe/xe_mocs.c
>@@ -352,7 +352,6 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
> 		info->table = dg1_mocs_desc;
> 		info->uc_index = 1;
> 		info->n_entries = GEN9_NUM_MOCS_ENTRIES;
>-		info->uc_index = 1;
> 		info->unused_entries_index = 5;
> 		break;
> 	case XE_TIGERLAKE:
>-- 
>2.39.1
>

Re: [Intel-xe] [PATCH 5/6] drm/xe/mocs: add MTL mocs

[Lucas De Marchi]

On Thu, Feb 16, 2023 at 03:17:23PM -0800, Matt Roper wrote:
>From: Philippe Lecluse <philippe.lecluse@intel.com>
>
>It was incorrectly using dg2_mocs for now.
>
>v2 (MattR):
> - Use REG_GENMASK/REG_FIELD_PREP for bitfields
> - Add bspec references
>
>Bspec: 45101, 45410, 63882
>Signed-off-by: Philippe Lecluse <philippe.lecluse@intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>


Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>


Lucas De Marchi

>---
> drivers/gpu/drm/xe/xe_mocs.c | 69 +++++++++++++++++++++++++++++++++---
> 1 file changed, 65 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
>index 696001f60259..3b48934d99d4 100644
>--- a/drivers/gpu/drm/xe/xe_mocs.c
>+++ b/drivers/gpu/drm/xe/xe_mocs.c
>@@ -62,6 +62,10 @@ struct xe_mocs_info {
> #define L3_GLBGO(value)		((value) << 6)
> #define L3_LKUP(value)		((value) << 7)
>
>+/* Defines for the tables (GLOB_MOCS_0 - GLOB_MOCS_16) */
>+#define _L4_CACHEABILITY	REG_GENMASK(3, 2)
>+#define IG_PAT			REG_BIT(8)
>+
> /* Helper defines */
> #define GEN9_NUM_MOCS_ENTRIES	64  /* 63-64 are reserved, but configured. */
> #define PVC_NUM_MOCS_ENTRIES	3
>@@ -89,6 +93,12 @@ struct xe_mocs_info {
> #define L3_2_RESERVED		_L3_CACHEABILITY(2)
> #define L3_3_WB			_L3_CACHEABILITY(3)
>
>+/* L4 caching options */
>+#define L4_0_WB                 REG_FIELD_PREP(_L4_CACHEABILITY, 0)
>+#define L4_1_WT                 REG_FIELD_PREP(_L4_CACHEABILITY, 1)
>+#define L4_2_RESERVED           REG_FIELD_PREP(_L4_CACHEABILITY, 2)
>+#define L4_3_UC                 REG_FIELD_PREP(_L4_CACHEABILITY, 3)
>+
> #define MOCS_ENTRY(__idx, __control_value, __l3cc_value) \
> 	[__idx] = { \
> 		.control_value = __control_value, \
>@@ -310,6 +320,57 @@ static const struct xe_mocs_entry pvc_mocs_desc[] = {
> 	MOCS_ENTRY(2, 0, L3_3_WB),
> };
>
>+static const struct xe_mocs_entry mtl_mocs_desc[] = {
>+	/* Error - Reserved for Non-Use */
>+	MOCS_ENTRY(0,
>+		   0,
>+		   L3_LKUP(1) | L3_3_WB),
>+	/* Cached - L3 + L4 */
>+	MOCS_ENTRY(1,
>+		   IG_PAT,
>+		   L3_LKUP(1) | L3_3_WB),
>+	/* L4 - GO:L3 */
>+	MOCS_ENTRY(2,
>+		   IG_PAT,
>+		   L3_LKUP(1) | L3_1_UC),
>+	/* Uncached - GO:L3 */
>+	MOCS_ENTRY(3,
>+		   IG_PAT | L4_3_UC,
>+		   L3_LKUP(1) | L3_1_UC),
>+	/* L4 - GO:Mem */
>+	MOCS_ENTRY(4,
>+		   IG_PAT,
>+		   L3_LKUP(1) | L3_GLBGO(1) | L3_1_UC),
>+	/* Uncached - GO:Mem */
>+	MOCS_ENTRY(5,
>+		   IG_PAT | L4_3_UC,
>+		   L3_LKUP(1) | L3_GLBGO(1) | L3_1_UC),
>+	/* L4 - L3:NoLKUP; GO:L3 */
>+	MOCS_ENTRY(6,
>+		   IG_PAT,
>+		   L3_1_UC),
>+	/* Uncached - L3:NoLKUP; GO:L3 */
>+	MOCS_ENTRY(7,
>+		   IG_PAT | L4_3_UC,
>+		   L3_1_UC),
>+	/* L4 - L3:NoLKUP; GO:Mem */
>+	MOCS_ENTRY(8,
>+		   IG_PAT,
>+		   L3_GLBGO(1) | L3_1_UC),
>+	/* Uncached - L3:NoLKUP; GO:Mem */
>+	MOCS_ENTRY(9,
>+		   IG_PAT | L4_3_UC,
>+		   L3_GLBGO(1) | L3_1_UC),
>+	/* Display - L3; L4:WT */
>+	MOCS_ENTRY(14,
>+		   IG_PAT | L4_1_WT,
>+		   L3_LKUP(1) | L3_3_WB),
>+	/* CCS - Non-Displayable */
>+	MOCS_ENTRY(15,
>+		   IG_PAT,
>+		   L3_GLBGO(1) | L3_1_UC),
>+};
>+
> static unsigned int get_mocs_settings(struct xe_device *xe,
> 				      struct xe_mocs_info *info)
> {
>@@ -327,11 +388,11 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
> 		info->unused_entries_index = 2;
> 		break;
> 	case XE_METEORLAKE:
>-		info->size = ARRAY_SIZE(dg2_mocs_desc);
>-		info->table = dg2_mocs_desc;
>+		info->size = ARRAY_SIZE(mtl_mocs_desc);
>+		info->table = mtl_mocs_desc;
> 		info->n_entries = MTL_NUM_MOCS_ENTRIES;
>-		info->uc_index = 1;
>-		info->unused_entries_index = 3;
>+		info->uc_index = 9;
>+		info->unused_entries_index = 1;
> 		break;
> 	case XE_DG2:
> 		if (xe->info.subplatform == XE_SUBPLATFORM_DG2_G10 &&
>-- 
>2.39.1
>

Re: [Intel-xe] [PATCH 6/6] drm/xe/mocs: LNCF MOCS settings only need to be restored on pre-Xe_HP

[Lucas De Marchi]

On Thu, Feb 16, 2023 at 03:17:24PM -0800, Matt Roper wrote:
>Reprogramming the LNCF MOCS registers on render domain reset is not
>intended to be regular driver programming, but rather the implementation
>of a specific workaround (Wa_1607983814).  This workaround no longer
>applies on Xe_HP any beyond, so we can expect that these registers, like
>the rest of the LNCF/LBCF registers, will maintain their values through
>all engine resets.  We should only add these registers to the GuC's
>save/restore list on platforms that need the workaround.
>
>Furthermore, xe_mocs_init_engine() appears to be another attempt to
>satisfy this same workaround.  This is unnecessary on the Xe driver
>since even on platforms where the workaround is necessary, all
>single-engine resets are initiated by the GuC and thus the GuC will take
>care of saving/restoring these registers.  The only host-initiated
>resets we have in Xe are full GT resets which will already
>(re)initialize these registers as part of the regular xe_mocs_init()
>flow.
>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> drivers/gpu/drm/xe/xe_execlist.c   |  2 +-
> drivers/gpu/drm/xe/xe_guc_ads.c    | 10 +++++++---
> drivers/gpu/drm/xe/xe_guc_submit.c |  1 -
> drivers/gpu/drm/xe/xe_mocs.c       | 13 -------------
> drivers/gpu/drm/xe/xe_mocs.h       |  1 -
> 5 files changed, 8 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c
>index d555d77cbf49..fd0ebfe7cae3 100644
>--- a/drivers/gpu/drm/xe/xe_execlist.c
>+++ b/drivers/gpu/drm/xe/xe_execlist.c
>@@ -462,7 +462,7 @@ static void execlist_engine_suspend_wait(struct xe_engine *e)
>
> static void execlist_engine_resume(struct xe_engine *e)
> {
>-	xe_mocs_init_engine(e);
>+	/* NIY */

what does NIY mean?  maybe "nop" is more common? And... what about
the execlist backend staying without this? Yep, execlist right now is
not very functioal, but should we be intentionally breaking it?

> }
>
> static const struct xe_engine_ops execlist_engine_ops = {
>diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
>index 0c08cecaca40..a233023a6616 100644
>--- a/drivers/gpu/drm/xe/xe_guc_ads.c
>+++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>@@ -430,6 +430,7 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
> 					  struct iosys_map *regset_map,
> 					  struct xe_hw_engine *hwe)
> {
>+	struct xe_device *xe = ads_to_xe(ads);
> 	struct xe_hw_engine *hwe_rcs_reset_domain =
> 		xe_gt_any_hw_engine_by_reset_domain(hwe->gt, XE_ENGINE_CLASS_RENDER);
> 	struct xe_reg_sr_entry *entry;
>@@ -464,9 +465,12 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
> 					  e->reg, e->flags, count++);
> 	}
>
>-	for (i = 0; i < LNCFCMOCS_REG_COUNT; i++) {
>-		guc_mmio_regset_write_one(ads, regset_map,
>-					  GEN9_LNCFCMOCS(i).reg, 0, count++);
>+	/* Wa_1607983814 */
>+	if (GRAPHICS_VER(xe) == 12 && GRAPHICS_VERx100(xe) < 1250) {
>+		for (i = 0; i < LNCFCMOCS_REG_COUNT; i++) {
>+			guc_mmio_regset_write_one(ads, regset_map,
>+						  GEN9_LNCFCMOCS(i).reg, 0, count++);

calculate_regset_size() unconditionally accounts for
LNCFCMOCS_REG_COUNT. Although this is just a "max", maybe we could
remove it from there by moving the if condition to a function
bool needs_wa_1607983814() { ... }

Another idea would be maybe to extend xe_rtp to allow a FUNC() not only
in the match but also in the action. We'd also need to extend it to
allow that function to apply the actions. Humn... if we need it more
than in just one place we can do that in future. For now this does the
job.


>+		}
> 	}
>
> 	XE_BUG_ON(ads->regset_size < (count * sizeof(struct guc_mmio_reg)));
>diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>index a54f7f82d04d..3766b77a0d90 100644
>--- a/drivers/gpu/drm/xe/xe_guc_submit.c
>+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>@@ -1267,7 +1267,6 @@ static void guc_engine_resume(struct xe_engine *e)
>
> 	XE_BUG_ON(e->guc->suspend_pending);
>
>-	xe_mocs_init_engine(e);
> 	guc_engine_add_msg(e, msg, RESUME);
> }
>
>diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
>index 3b48934d99d4..7e495d699295 100644
>--- a/drivers/gpu/drm/xe/xe_mocs.c
>+++ b/drivers/gpu/drm/xe/xe_mocs.c
>@@ -507,19 +507,6 @@ static void init_l3cc_table(struct xe_gt *gt,
> 	}
> }
>
>-void xe_mocs_init_engine(const struct xe_engine *engine)
>-{
>-	struct xe_mocs_info table;
>-	unsigned int flags;
>-
>-	flags = get_mocs_settings(engine->gt->xe, &table);
>-	if (!flags)
>-		return;
>-
>-	if (flags & HAS_RENDER_L3CC && engine->class == XE_ENGINE_CLASS_RENDER)

do we have any other plans for HAS_RENDER_L3CC?  It seems it's not used.
For any error handling part we could check size, table or
unused_entries_index


Lucas De Marchi

>-		init_l3cc_table(engine->gt, &table);
>-}
>-
> void xe_mocs_init(struct xe_gt *gt)
> {
> 	struct xe_mocs_info table;
>diff --git a/drivers/gpu/drm/xe/xe_mocs.h b/drivers/gpu/drm/xe/xe_mocs.h
>index aba1abe216ab..63500a1d6660 100644
>--- a/drivers/gpu/drm/xe/xe_mocs.h
>+++ b/drivers/gpu/drm/xe/xe_mocs.h
>@@ -11,7 +11,6 @@
> struct xe_engine;
> struct xe_gt;
>
>-void xe_mocs_init_engine(const struct xe_engine *engine);
> void xe_mocs_init(struct xe_gt *gt);
>
> /**
>-- 
>2.39.1
>

Re: [Intel-xe] [PATCH 6/6] drm/xe/mocs: LNCF MOCS settings only need to be restored on pre-Xe_HP

[Matt Roper]

On Wed, Feb 22, 2023 at 04:11:19PM -0800, Lucas De Marchi wrote:
> On Thu, Feb 16, 2023 at 03:17:24PM -0800, Matt Roper wrote:
> > Reprogramming the LNCF MOCS registers on render domain reset is not
> > intended to be regular driver programming, but rather the implementation
> > of a specific workaround (Wa_1607983814).  This workaround no longer
> > applies on Xe_HP any beyond, so we can expect that these registers, like
> > the rest of the LNCF/LBCF registers, will maintain their values through
> > all engine resets.  We should only add these registers to the GuC's
> > save/restore list on platforms that need the workaround.
> > 
> > Furthermore, xe_mocs_init_engine() appears to be another attempt to
> > satisfy this same workaround.  This is unnecessary on the Xe driver
> > since even on platforms where the workaround is necessary, all
> > single-engine resets are initiated by the GuC and thus the GuC will take
> > care of saving/restoring these registers.  The only host-initiated
> > resets we have in Xe are full GT resets which will already
> > (re)initialize these registers as part of the regular xe_mocs_init()
> > flow.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_execlist.c   |  2 +-
> > drivers/gpu/drm/xe/xe_guc_ads.c    | 10 +++++++---
> > drivers/gpu/drm/xe/xe_guc_submit.c |  1 -
> > drivers/gpu/drm/xe/xe_mocs.c       | 13 -------------
> > drivers/gpu/drm/xe/xe_mocs.h       |  1 -
> > 5 files changed, 8 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c
> > index d555d77cbf49..fd0ebfe7cae3 100644
> > --- a/drivers/gpu/drm/xe/xe_execlist.c
> > +++ b/drivers/gpu/drm/xe/xe_execlist.c
> > @@ -462,7 +462,7 @@ static void execlist_engine_suspend_wait(struct xe_engine *e)
> > 
> > static void execlist_engine_resume(struct xe_engine *e)
> > {
> > -	xe_mocs_init_engine(e);
> > +	/* NIY */
> 
> what does NIY mean?  maybe "nop" is more common? And... what about
> the execlist backend staying without this? Yep, execlist right now is
> not very functioal, but should we be intentionally breaking it?

I assume "NIY" means "not in yet."  That comment is what was used for
several other unimplemented stubs in this same file, so I just did the
same here for consistency.

Removing this shouldn't have any detrimental impact on the current
execlist implementation.  The programming here is only necessary to
restore after a single-engine reset, but it's not possible to ever have
an engine reset today because that was never implemented in Xe (and as
far as I know, never will be).

> 
> > }
> > 
> > static const struct xe_engine_ops execlist_engine_ops = {
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
> > index 0c08cecaca40..a233023a6616 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ads.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> > @@ -430,6 +430,7 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
> > 					  struct iosys_map *regset_map,
> > 					  struct xe_hw_engine *hwe)
> > {
> > +	struct xe_device *xe = ads_to_xe(ads);
> > 	struct xe_hw_engine *hwe_rcs_reset_domain =
> > 		xe_gt_any_hw_engine_by_reset_domain(hwe->gt, XE_ENGINE_CLASS_RENDER);
> > 	struct xe_reg_sr_entry *entry;
> > @@ -464,9 +465,12 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
> > 					  e->reg, e->flags, count++);
> > 	}
> > 
> > -	for (i = 0; i < LNCFCMOCS_REG_COUNT; i++) {
> > -		guc_mmio_regset_write_one(ads, regset_map,
> > -					  GEN9_LNCFCMOCS(i).reg, 0, count++);
> > +	/* Wa_1607983814 */
> > +	if (GRAPHICS_VER(xe) == 12 && GRAPHICS_VERx100(xe) < 1250) {
> > +		for (i = 0; i < LNCFCMOCS_REG_COUNT; i++) {
> > +			guc_mmio_regset_write_one(ads, regset_map,
> > +						  GEN9_LNCFCMOCS(i).reg, 0, count++);
> 
> calculate_regset_size() unconditionally accounts for
> LNCFCMOCS_REG_COUNT. Although this is just a "max", maybe we could
> remove it from there by moving the if condition to a function
> bool needs_wa_1607983814() { ... }

Yeah, good point; I'll update that.


Matt

> 
> Another idea would be maybe to extend xe_rtp to allow a FUNC() not only
> in the match but also in the action. We'd also need to extend it to
> allow that function to apply the actions. Humn... if we need it more
> than in just one place we can do that in future. For now this does the
> job.
> 
> 
> > +		}
> > 	}
> > 
> > 	XE_BUG_ON(ads->regset_size < (count * sizeof(struct guc_mmio_reg)));
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index a54f7f82d04d..3766b77a0d90 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -1267,7 +1267,6 @@ static void guc_engine_resume(struct xe_engine *e)
> > 
> > 	XE_BUG_ON(e->guc->suspend_pending);
> > 
> > -	xe_mocs_init_engine(e);
> > 	guc_engine_add_msg(e, msg, RESUME);
> > }
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
> > index 3b48934d99d4..7e495d699295 100644
> > --- a/drivers/gpu/drm/xe/xe_mocs.c
> > +++ b/drivers/gpu/drm/xe/xe_mocs.c
> > @@ -507,19 +507,6 @@ static void init_l3cc_table(struct xe_gt *gt,
> > 	}
> > }
> > 
> > -void xe_mocs_init_engine(const struct xe_engine *engine)
> > -{
> > -	struct xe_mocs_info table;
> > -	unsigned int flags;
> > -
> > -	flags = get_mocs_settings(engine->gt->xe, &table);
> > -	if (!flags)
> > -		return;
> > -
> > -	if (flags & HAS_RENDER_L3CC && engine->class == XE_ENGINE_CLASS_RENDER)
> 
> do we have any other plans for HAS_RENDER_L3CC?  It seems it's not used.
> For any error handling part we could check size, table or
> unused_entries_index
> 
> 
> Lucas De Marchi
> 
> > -		init_l3cc_table(engine->gt, &table);
> > -}
> > -
> > void xe_mocs_init(struct xe_gt *gt)
> > {
> > 	struct xe_mocs_info table;
> > diff --git a/drivers/gpu/drm/xe/xe_mocs.h b/drivers/gpu/drm/xe/xe_mocs.h
> > index aba1abe216ab..63500a1d6660 100644
> > --- a/drivers/gpu/drm/xe/xe_mocs.h
> > +++ b/drivers/gpu/drm/xe/xe_mocs.h
> > @@ -11,7 +11,6 @@
> > struct xe_engine;
> > struct xe_gt;
> > 
> > -void xe_mocs_init_engine(const struct xe_engine *engine);
> > void xe_mocs_init(struct xe_gt *gt);
> > 
> > /**
> > -- 
> > 2.39.1
> > 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation

Re: [Intel-xe] [PATCH 6/6] drm/xe/mocs: LNCF MOCS settings only need to be restored on pre-Xe_HP

[Lucas De Marchi]

On Thu, Feb 23, 2023 at 09:37:15AM -0800, Matt Roper wrote:
>On Wed, Feb 22, 2023 at 04:11:19PM -0800, Lucas De Marchi wrote:
>> On Thu, Feb 16, 2023 at 03:17:24PM -0800, Matt Roper wrote:
>> > Reprogramming the LNCF MOCS registers on render domain reset is not
>> > intended to be regular driver programming, but rather the implementation
>> > of a specific workaround (Wa_1607983814).  This workaround no longer
>> > applies on Xe_HP any beyond, so we can expect that these registers, like
>> > the rest of the LNCF/LBCF registers, will maintain their values through
>> > all engine resets.  We should only add these registers to the GuC's
>> > save/restore list on platforms that need the workaround.
>> >
>> > Furthermore, xe_mocs_init_engine() appears to be another attempt to
>> > satisfy this same workaround.  This is unnecessary on the Xe driver
>> > since even on platforms where the workaround is necessary, all
>> > single-engine resets are initiated by the GuC and thus the GuC will take
>> > care of saving/restoring these registers.  The only host-initiated
>> > resets we have in Xe are full GT resets which will already
>> > (re)initialize these registers as part of the regular xe_mocs_init()
>> > flow.
>> >
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> > ---
>> > drivers/gpu/drm/xe/xe_execlist.c   |  2 +-
>> > drivers/gpu/drm/xe/xe_guc_ads.c    | 10 +++++++---
>> > drivers/gpu/drm/xe/xe_guc_submit.c |  1 -
>> > drivers/gpu/drm/xe/xe_mocs.c       | 13 -------------
>> > drivers/gpu/drm/xe/xe_mocs.h       |  1 -
>> > 5 files changed, 8 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_execlist.c b/drivers/gpu/drm/xe/xe_execlist.c
>> > index d555d77cbf49..fd0ebfe7cae3 100644
>> > --- a/drivers/gpu/drm/xe/xe_execlist.c
>> > +++ b/drivers/gpu/drm/xe/xe_execlist.c
>> > @@ -462,7 +462,7 @@ static void execlist_engine_suspend_wait(struct xe_engine *e)
>> >
>> > static void execlist_engine_resume(struct xe_engine *e)
>> > {
>> > -	xe_mocs_init_engine(e);
>> > +	/* NIY */
>>
>> what does NIY mean?  maybe "nop" is more common? And... what about
>> the execlist backend staying without this? Yep, execlist right now is
>> not very functioal, but should we be intentionally breaking it?
>
>I assume "NIY" means "not in yet."  That comment is what was used for
>several other unimplemented stubs in this same file, so I just did the
>same here for consistency.

oh... I think this is "not implemented yet". Ok, so it's not a nop, it's
more a "TODO" if this is ever really done.

>
>Removing this shouldn't have any detrimental impact on the current
>execlist implementation.  The programming here is only necessary to
>restore after a single-engine reset, but it's not possible to ever have
>an engine reset today because that was never implemented in Xe (and as
>far as I know, never will be).

ok

thanks
Lucas De Marchi

>
>>
>> > }
>> >
>> > static const struct xe_engine_ops execlist_engine_ops = {
>> > diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
>> > index 0c08cecaca40..a233023a6616 100644
>> > --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>> > +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>> > @@ -430,6 +430,7 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
>> > 					  struct iosys_map *regset_map,
>> > 					  struct xe_hw_engine *hwe)
>> > {
>> > +	struct xe_device *xe = ads_to_xe(ads);
>> > 	struct xe_hw_engine *hwe_rcs_reset_domain =
>> > 		xe_gt_any_hw_engine_by_reset_domain(hwe->gt, XE_ENGINE_CLASS_RENDER);
>> > 	struct xe_reg_sr_entry *entry;
>> > @@ -464,9 +465,12 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
>> > 					  e->reg, e->flags, count++);
>> > 	}
>> >
>> > -	for (i = 0; i < LNCFCMOCS_REG_COUNT; i++) {
>> > -		guc_mmio_regset_write_one(ads, regset_map,
>> > -					  GEN9_LNCFCMOCS(i).reg, 0, count++);
>> > +	/* Wa_1607983814 */
>> > +	if (GRAPHICS_VER(xe) == 12 && GRAPHICS_VERx100(xe) < 1250) {
>> > +		for (i = 0; i < LNCFCMOCS_REG_COUNT; i++) {
>> > +			guc_mmio_regset_write_one(ads, regset_map,
>> > +						  GEN9_LNCFCMOCS(i).reg, 0, count++);
>>
>> calculate_regset_size() unconditionally accounts for
>> LNCFCMOCS_REG_COUNT. Although this is just a "max", maybe we could
>> remove it from there by moving the if condition to a function
>> bool needs_wa_1607983814() { ... }
>
>Yeah, good point; I'll update that.
>
>
>Matt
>
>>
>> Another idea would be maybe to extend xe_rtp to allow a FUNC() not only
>> in the match but also in the action. We'd also need to extend it to
>> allow that function to apply the actions. Humn... if we need it more
>> than in just one place we can do that in future. For now this does the
>> job.
>>
>>
>> > +		}
>> > 	}
>> >
>> > 	XE_BUG_ON(ads->regset_size < (count * sizeof(struct guc_mmio_reg)));
>> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>> > index a54f7f82d04d..3766b77a0d90 100644
>> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> > @@ -1267,7 +1267,6 @@ static void guc_engine_resume(struct xe_engine *e)
>> >
>> > 	XE_BUG_ON(e->guc->suspend_pending);
>> >
>> > -	xe_mocs_init_engine(e);
>> > 	guc_engine_add_msg(e, msg, RESUME);
>> > }
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
>> > index 3b48934d99d4..7e495d699295 100644
>> > --- a/drivers/gpu/drm/xe/xe_mocs.c
>> > +++ b/drivers/gpu/drm/xe/xe_mocs.c
>> > @@ -507,19 +507,6 @@ static void init_l3cc_table(struct xe_gt *gt,
>> > 	}
>> > }
>> >
>> > -void xe_mocs_init_engine(const struct xe_engine *engine)
>> > -{
>> > -	struct xe_mocs_info table;
>> > -	unsigned int flags;
>> > -
>> > -	flags = get_mocs_settings(engine->gt->xe, &table);
>> > -	if (!flags)
>> > -		return;
>> > -
>> > -	if (flags & HAS_RENDER_L3CC && engine->class == XE_ENGINE_CLASS_RENDER)
>>
>> do we have any other plans for HAS_RENDER_L3CC?  It seems it's not used.
>> For any error handling part we could check size, table or
>> unused_entries_index
>>
>>
>> Lucas De Marchi
>>
>> > -		init_l3cc_table(engine->gt, &table);
>> > -}
>> > -
>> > void xe_mocs_init(struct xe_gt *gt)
>> > {
>> > 	struct xe_mocs_info table;
>> > diff --git a/drivers/gpu/drm/xe/xe_mocs.h b/drivers/gpu/drm/xe/xe_mocs.h
>> > index aba1abe216ab..63500a1d6660 100644
>> > --- a/drivers/gpu/drm/xe/xe_mocs.h
>> > +++ b/drivers/gpu/drm/xe/xe_mocs.h
>> > @@ -11,7 +11,6 @@
>> > struct xe_engine;
>> > struct xe_gt;
>> >
>> > -void xe_mocs_init_engine(const struct xe_engine *engine);
>> > void xe_mocs_init(struct xe_gt *gt);
>> >
>> > /**
>> > --
>> > 2.39.1
>> >
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation

Re: [Intel-xe] [PATCH 1/6] drm/xe/mocs: Drop unwanted TGL table

[Balasubramani Vivekanandan]

On 16.02.2023 15:17, Matt Roper wrote:
> TGL/RKL/ADLS/ADLP are all supposed to use the same MOCS table, with
> values defined in the bspec.  Any entries listed in the bspec as
> reserved/error/undefined should always be initialized to the most cached
> and least coherent setting possible so that any userspace accidentally
> referencing those undefined entries will only experience an increase in
> coherency if spec updates down the road start defining real values.
> 
> The TGL and gen12 tables that exist in the driver today are identical
> except that the TGL includes one additional (incorrect) setting for PAT
> index 1.  This is a holdover from i915 where the platform was enabled
> with an incorrect setting and by the time we noticed, it was too late to
> fix the table without breaking ABI compatibility (and on TGL we did
> indeed have some buggy userspace that was referencing the 'reserved'
> entry 1).  Since the Xe driver starts fresh with a clean slate on ABI,
> there's no need to repeat the mistakes of i915 here.
> 
> Bspec: 45101
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>

Regards,
Bala

> ---
>  drivers/gpu/drm/xe/xe_mocs.c | 46 ------------------------------------
>  1 file changed, 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
> index 86b966fffbe5..09adb69d1545 100644
> --- a/drivers/gpu/drm/xe/xe_mocs.c
> +++ b/drivers/gpu/drm/xe/xe_mocs.c
> @@ -247,47 +247,6 @@ struct xe_mocs_info {
>  		LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
>  		L3_1_UC)
>  
> -static const struct xe_mocs_entry tgl_mocs_desc[] = {
> -	/*
> -	 * NOTE:
> -	 * Reserved and unspecified MOCS indices have been set to (L3 + LCC).
> -	 * These reserved entries should never be used, they may be changed
> -	 * to low performant variants with better coherency in the future if
> -	 * more entries are needed. We are programming index XE_MOCS_PTE(1)
> -	 * only, __init_mocs_table() take care to program unused index with
> -	 * this entry.
> -	 */
> -	MOCS_ENTRY(XE_MOCS_PTE,
> -		   LE_0_PAGETABLE | LE_TC_0_PAGETABLE,
> -		   L3_1_UC),
> -	GEN11_MOCS_ENTRIES,
> -
> -	/* Implicitly enable L1 - HDC:L1 + L3 + LLC */
> -	MOCS_ENTRY(48,
> -		   LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
> -		   L3_3_WB),
> -	/* Implicitly enable L1 - HDC:L1 + L3 */
> -	MOCS_ENTRY(49,
> -		   LE_1_UC | LE_TC_1_LLC,
> -		   L3_3_WB),
> -	/* Implicitly enable L1 - HDC:L1 + LLC */
> -	MOCS_ENTRY(50,
> -		   LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
> -		   L3_1_UC),
> -	/* Implicitly enable L1 - HDC:L1 */
> -	MOCS_ENTRY(51,
> -		   LE_1_UC | LE_TC_1_LLC,
> -		   L3_1_UC),
> -	/* HW Special Case (CCS) */
> -	MOCS_ENTRY(60,
> -		   LE_3_WB | LE_TC_1_LLC | LE_LRUM(3),
> -		   L3_1_UC),
> -	/* HW Special Case (Displayable) */
> -	MOCS_ENTRY(61,
> -		   LE_1_UC | LE_TC_1_LLC,
> -		   L3_3_WB),
> -};
> -
>  static const struct xe_mocs_entry dg1_mocs_desc[] = {
>  	/* UC */
>  	MOCS_ENTRY(1, 0, L3_1_UC),
> @@ -422,11 +381,6 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
>  		info->unused_entries_index = 5;
>  		break;
>  	case XE_TIGERLAKE:
> -		info->size  = ARRAY_SIZE(tgl_mocs_desc);
> -		info->table = tgl_mocs_desc;
> -		info->n_entries = GEN9_NUM_MOCS_ENTRIES;
> -		info->uc_index = 3;
> -		break;
>  	case XE_ALDERLAKE_S:
>  	case XE_ALDERLAKE_P:
>  		info->size  = ARRAY_SIZE(gen12_mocs_desc);
> -- 
> 2.39.1
> 

Re: [Intel-xe] [PATCH 3/6] drm/xe/mocs: Drop xe_mocs_info_index

[Balasubramani Vivekanandan]

On 16.02.2023 15:17, Matt Roper wrote:
> The values in the xe_mocs_info_index enum only match old pre-gen12
> hardware not supported by the Xe driver.
> 
> The only usage of this enum was to set a default value for
> info->unused_entries_index, but this is unnecessary since every platform
> in the subsequent switch statement sets a proper platform-specific value
> (and the XE_MOCS_PTE default doesn't even make sense since the hardware
> dropped the "use PAT settings" capability in gen12).
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_mocs.c | 30 ++----------------------------
>  1 file changed, 2 insertions(+), 28 deletions(-)

Reviewed-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>

> 
> diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
> index ec89ff3ac29b..583e198af88d 100644
> --- a/drivers/gpu/drm/xe/xe_mocs.c
> +++ b/drivers/gpu/drm/xe/xe_mocs.c
> @@ -23,30 +23,6 @@ static inline void mocs_dbg(const struct drm_device *dev,
>  { /* noop */ }
>  #endif
>  
> -/*
> - * MOCS indexes used for GPU surfaces, defining the cacheability of the
> - * surface data and the coherency for this data wrt. CPU vs. GPU accesses.
> - */
> -enum xe_mocs_info_index {
> -	/*
> -	 * Not cached anywhere, coherency between CPU and GPU accesses is
> -	 * guaranteed.
> -	 */
> -	XE_MOCS_UNCACHED,
> -	/*
> -	 * Cacheability and coherency controlled by the kernel automatically
> -	 * based on the xxxx  IOCTL setting and the current
> -	 * usage of the surface (used for display scanout or not).
> -	 */
> -	XE_MOCS_PTE,
> -	/*
> -	 * Cached in all GPU caches available on the platform.
> -	 * Coherency between CPU and GPU accesses to the surface is not
> -	 * guaranteed without extra synchronization.
> -	 */
> -	XE_MOCS_CACHED,
> -};
> -
>  enum {
>  	HAS_GLOBAL_MOCS = BIT(0),
>  	HAS_RENDER_L3CC = BIT(1),
> @@ -341,7 +317,6 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
>  
>  	memset(info, 0, sizeof(struct xe_mocs_info));
>  
> -	info->unused_entries_index = XE_MOCS_PTE;
>  	switch (xe->info.platform) {
>  	case XE_PVC:
>  		info->size = ARRAY_SIZE(pvc_mocs_desc);
> @@ -406,9 +381,8 @@ static unsigned int get_mocs_settings(struct xe_device *xe,
>  }
>  
>  /*
> - * Get control_value from MOCS entry taking into account when it's not used
> - * then if unused_entries_index is non-zero then its value will be returned
> - * otherwise XE_MOCS_PTE's value is returned in this case.
> + * Get control_value from MOCS entry.  If the table entry is not defined, the
> + * settings from unused_entries_index will be returned.
>   */
>  static u32 get_entry_control(const struct xe_mocs_info *info,
>  			     unsigned int index)
> -- 
> 2.39.1
>