[PATCH v8 0/7] Define MOCS table for Icelake

[PATCH v8 0/7] Define MOCS table for Icelake

[Lucas De Marchi]

This reworks v7 of the series
(https://patchwork.freedesktop.org/series/54070/) to handle the comments
there and some more of my own.

Changes:
  - Initialize undefined entries to PTE rather than uncached (suggested
    by Chris)
  - Add a new macro to define MOCS entries: this allows to make the
    table more readable and to implicitly mark entries that are defined
  - Minor improvements to "drm/i915: cache number of MOCS entries"
    (suggested by Tvrtko)
  - Reorder patches to avoid unneeded changes (suggested by Tvrtko)

Lucas De Marchi (5):
  drm/i915: initialize unused MOCS entries to PTE
  drm/i915: Simplify MOCS table definition
  drm/i915: use a macro to define MOCS entries
  drm/i915: keep track of used entries in MOCS table
  drm/i915: cache number of MOCS entries

Tomasz Lis (2):
  drm/i915/skl: Rework MOCS tables to keep common part in a define
  drm/i915/icl: Define MOCS table for Icelake

 drivers/gpu/drm/i915/intel_mocs.c | 356 ++++++++++++++++++------------
 1 file changed, 211 insertions(+), 145 deletions(-)

-- 
2.20.0

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

[PATCH v8 1/7] drm/i915: initialize unused MOCS entries to PTE

[Lucas De Marchi]

Instead of initializing them to uncached, let's set them to PTE for
kernel tracking. While at it do some minor adjustments to comments and
coding style.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_mocs.c | 56 +++++++++++++------------------
 1 file changed, 23 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index e976c5ce5479..0d6b94a239d6 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -85,10 +85,7 @@ struct drm_i915_mocs_table {
  *
  * Entries not part of the following tables are undefined as far as
  * userspace is concerned and shouldn't be relied upon.  For the time
- * being they will be implicitly initialized to the strictest caching
- * configuration (uncached) to guarantee forwards compatibility with
- * userspace programs written against more recent kernels providing
- * additional MOCS entries.
+ * being they will be initialized to PTE.
  *
  * NOTE: These tables MUST start with being uncached and the length
  *       MUST be less than 63 as the last two registers are reserved
@@ -249,16 +246,13 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine)
 			   table.table[index].control_value);
 
 	/*
-	 * Ok, now set the unused entries to uncached. These entries
-	 * are officially undefined and no contract for the contents
-	 * and settings is given for these entries.
-	 *
-	 * Entry 0 in the table is uncached - so we are just writing
-	 * that value to all the used entries.
+	 * Now set the unused entries to PTE. These entries are officially
+	 * undefined and no contract for the contents and settings is given
+	 * for these entries.
 	 */
 	for (; index < GEN9_NUM_MOCS_ENTRIES; index++)
 		I915_WRITE(mocs_register(engine->id, index),
-			   table.table[0].control_value);
+			   table.table[I915_MOCS_PTE].control_value);
 }
 
 /**
@@ -293,16 +287,13 @@ static int emit_mocs_control_table(struct i915_request *rq,
 	}
 
 	/*
-	 * Ok, now set the unused entries to uncached. These entries
-	 * are officially undefined and no contract for the contents
-	 * and settings is given for these entries.
-	 *
-	 * Entry 0 in the table is uncached - so we are just writing
-	 * that value to all the used entries.
+	 * Now set the unused entries to PTE. These entries are officially
+	 * undefined and no contract for the contents and settings is given
+	 * for these entries.
 	 */
 	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
 		*cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
-		*cs++ = table->table[0].control_value;
+		*cs++ = table->table[I915_MOCS_PTE].control_value;
 	}
 
 	*cs++ = MI_NOOP;
@@ -345,7 +336,7 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
 
 	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2);
 
-	for (i = 0; i < table->size/2; i++) {
+	for (i = 0; i < table->size / 2; i++) {
 		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
 		*cs++ = l3cc_combine(table, 2 * i, 2 * i + 1);
 	}
@@ -353,18 +344,18 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
 	if (table->size & 0x01) {
 		/* Odd table size - 1 left over */
 		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
-		*cs++ = l3cc_combine(table, 2 * i, 0);
+		*cs++ = l3cc_combine(table, 2 * i, I915_MOCS_PTE);
 		i++;
 	}
 
 	/*
-	 * Now set the rest of the table to uncached - use entry 0 as
-	 * this will be uncached. Leave the last pair uninitialised as
-	 * they are reserved by the hardware.
+	 * Now set the unused entries to PTE. These entries are officially
+	 * undefined and no contract for the contents and settings is given
+	 * for these entries.
 	 */
 	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
 		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
-		*cs++ = l3cc_combine(table, 0, 0);
+		*cs++ = l3cc_combine(table, I915_MOCS_PTE, I915_MOCS_PTE);
 	}
 
 	*cs++ = MI_NOOP;
@@ -395,22 +386,21 @@ void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv)
 	if (!get_mocs_settings(dev_priv, &table))
 		return;
 
-	for (i = 0; i < table.size/2; i++)
-		I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 2*i, 2*i+1));
+	for (i = 0; i < table.size / 2; i++)
+		I915_WRITE(GEN9_LNCFCMOCS(i),
+			   l3cc_combine(&table, 2 * i, 2 * i + 1));
 
 	/* Odd table size - 1 left over */
 	if (table.size & 0x01) {
-		I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 2*i, 0));
+		I915_WRITE(GEN9_LNCFCMOCS(i),
+			   l3cc_combine(&table, 2 * i, I915_MOCS_PTE));
 		i++;
 	}
 
-	/*
-	 * Now set the rest of the table to uncached - use entry 0 as
-	 * this will be uncached. Leave the last pair as initialised as
-	 * they are reserved by the hardware.
-	 */
+	/* Now set the rest of the table to PTE */
 	for (; i < (GEN9_NUM_MOCS_ENTRIES / 2); i++)
-		I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 0, 0));
+		I915_WRITE(GEN9_LNCFCMOCS(i),
+			   l3cc_combine(&table, I915_MOCS_PTE, I915_MOCS_PTE));
 }
 
 /**
-- 
2.20.0

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

[PATCH v8 2/7] drm/i915: Simplify MOCS table definition

[Lucas De Marchi]

Make the defines for LE and L3 caching options to contain the shifts and
remove the zeros from the tables as shifting zeros always result in
zero.

Starting from Ice Lake the MOCS table is defined in the spec and
contains all entries. So to simplify checking the table with the values
set in code, the value is now part of the macro name. This allows to
still give the most used option and sensible name, but also to easily
cross check the table from the spec for gen >= 11.

By removing the zeros we avoid maintaining a huge table since the one
from spec contains many more entries. The new table for Ice Lake will
be added by other patches, this only reformats the table.

While at it also fix the indentation.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_mocs.c | 80 +++++++++++--------------------
 1 file changed, 29 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index 0d6b94a239d6..4ea80bb7dcc8 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -36,8 +36,8 @@ struct drm_i915_mocs_table {
 };
 
 /* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
-#define LE_CACHEABILITY(value)	((value) << 0)
-#define LE_TGT_CACHE(value)	((value) << 2)
+#define _LE_CACHEABILITY(value)	((value) << 0)
+#define _LE_TGT_CACHE(value)	((value) << 2)
 #define LE_LRUM(value)		((value) << 4)
 #define LE_AOM(value)		((value) << 6)
 #define LE_RSC(value)		((value) << 7)
@@ -48,28 +48,28 @@ struct drm_i915_mocs_table {
 /* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
 #define L3_ESC(value)		((value) << 0)
 #define L3_SCC(value)		((value) << 1)
-#define L3_CACHEABILITY(value)	((value) << 4)
+#define _L3_CACHEABILITY(value)	((value) << 4)
 
 /* Helper defines */
 #define GEN9_NUM_MOCS_ENTRIES	62  /* 62 out of 64 - 63 & 64 are reserved. */
 
 /* (e)LLC caching options */
-#define LE_PAGETABLE		0
-#define LE_UC			1
-#define LE_WT			2
-#define LE_WB			3
-
-/* L3 caching options */
-#define L3_DIRECT		0
-#define L3_UC			1
-#define L3_RESERVED		2
-#define L3_WB			3
+#define LE_0_PAGETABLE		_LE_CACHEABILITY(0)
+#define LE_1_UC			_LE_CACHEABILITY(1)
+#define LE_2_WT			_LE_CACHEABILITY(2)
+#define LE_3_WB			_LE_CACHEABILITY(3)
 
 /* Target cache */
-#define LE_TC_PAGETABLE		0
-#define LE_TC_LLC		1
-#define LE_TC_LLC_ELLC		2
-#define LE_TC_LLC_ELLC_ALT	3
+#define LE_TC_0_PAGETABLE	_LE_TGT_CACHE(0)
+#define LE_TC_1_LLC		_LE_TGT_CACHE(1)
+#define LE_TC_2_LLC_ELLC	_LE_TGT_CACHE(2)
+#define LE_TC_3_LLC_ELLC_ALT	_LE_TGT_CACHE(3)
+
+/* L3 caching options */
+#define L3_0_DIRECT		_L3_CACHEABILITY(0)
+#define L3_1_UC			_L3_CACHEABILITY(1)
+#define L3_2_RESERVED		_L3_CACHEABILITY(2)
+#define L3_3_WB			_L3_CACHEABILITY(3)
 
 /*
  * MOCS tables
@@ -96,31 +96,21 @@ struct drm_i915_mocs_table {
 static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
 	[I915_MOCS_UNCACHED] = {
 	  /* 0x00000009 */
-	  .control_value = LE_CACHEABILITY(LE_UC) |
-			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
-			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
-			   LE_PFM(0) | LE_SCF(0),
-
+	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC,
 	  /* 0x0010 */
-	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
+	  .l3cc_value =    L3_1_UC,
 	},
 	[I915_MOCS_PTE] = {
 	  /* 0x00000038 */
-	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
-			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
-			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
-			   LE_PFM(0) | LE_SCF(0),
+	  .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3),
 	  /* 0x0030 */
-	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
+	  .l3cc_value =    L3_3_WB,
 	},
 	[I915_MOCS_CACHED] = {
 	  /* 0x0000003b */
-	  .control_value = LE_CACHEABILITY(LE_WB) |
-			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
-			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
-			   LE_PFM(0) | LE_SCF(0),
+	  .control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
 	  /* 0x0030 */
-	  .l3cc_value =   L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
+	  .l3cc_value =   L3_3_WB,
 	},
 };
 
@@ -128,33 +118,21 @@ static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
 static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
 	[I915_MOCS_UNCACHED] = {
 	  /* 0x00000009 */
-	  .control_value = LE_CACHEABILITY(LE_UC) |
-			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
-			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
-			   LE_PFM(0) | LE_SCF(0),
-
+	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC,
 	  /* 0x0010 */
-	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
+	  .l3cc_value = L3_1_UC,
 	},
 	[I915_MOCS_PTE] = {
 	  /* 0x00000038 */
-	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
-			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
-			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
-			   LE_PFM(0) | LE_SCF(0),
-
+	  .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3),
 	  /* 0x0030 */
-	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
+	  .l3cc_value = L3_3_WB,
 	},
 	[I915_MOCS_CACHED] = {
 	  /* 0x00000039 */
-	  .control_value = LE_CACHEABILITY(LE_UC) |
-			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
-			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
-			   LE_PFM(0) | LE_SCF(0),
-
+	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
 	  /* 0x0030 */
-	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
+	  .l3cc_value = L3_3_WB,
 	},
 };
 
-- 
2.20.0

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

[PATCH v8 3/7] drm/i915/skl: Rework MOCS tables to keep common part in a define

[Lucas De Marchi]

From: Tomasz Lis <tomasz.lis@intel.com>

The MOCS tables are going to be very similar across platforms.

To reduce the amount of copied code, this patch rips the common part and
puts it into a definition valid for all gen9 platforms.

v2: Made defines for or-ing flags. Renamed macros from MOCS_TABLE
    to MOCS_ENTRIES. (Joonas)
v3 (Lucas):
  - Fix indentation
  - Rebase on rework done by additional patch
  - Remove define for or-ing flags as it made the table more complex by
    requiring zeroed values to be passed
  - Do not embed comma in the macro, so to treat that just as another
    item and please source code formatting tools

Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
Suggested-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_mocs.c | 57 ++++++++++++++-----------------
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index 4ea80bb7dcc8..c7a2a8d81d90 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -93,46 +93,39 @@ struct drm_i915_mocs_table {
  *       may only be updated incrementally by adding entries at the
  *       end.
  */
+
+#define GEN9_MOCS_ENTRIES \
+	[I915_MOCS_UNCACHED] = { \
+		/* 0x00000009 */ \
+		.control_value = LE_1_UC | LE_TC_2_LLC_ELLC, \
+		/* 0x0010 */ \
+		.l3cc_value = L3_1_UC, \
+	}, \
+	[I915_MOCS_PTE] = { \
+		/* 0x00000038 */ \
+		.control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \
+		/* 0x0030 */ \
+		.l3cc_value = L3_3_WB, \
+	}
+
 static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
-	[I915_MOCS_UNCACHED] = {
-	  /* 0x00000009 */
-	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC,
-	  /* 0x0010 */
-	  .l3cc_value =    L3_1_UC,
-	},
-	[I915_MOCS_PTE] = {
-	  /* 0x00000038 */
-	  .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3),
-	  /* 0x0030 */
-	  .l3cc_value =    L3_3_WB,
-	},
+	GEN9_MOCS_ENTRIES,
 	[I915_MOCS_CACHED] = {
-	  /* 0x0000003b */
-	  .control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
-	  /* 0x0030 */
-	  .l3cc_value =   L3_3_WB,
+		/* 0x0000003b */
+		.control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
+		/* 0x0030 */
+		.l3cc_value =   L3_3_WB,
 	},
 };
 
 /* NOTE: the LE_TGT_CACHE is not used on Broxton */
 static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
-	[I915_MOCS_UNCACHED] = {
-	  /* 0x00000009 */
-	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC,
-	  /* 0x0010 */
-	  .l3cc_value = L3_1_UC,
-	},
-	[I915_MOCS_PTE] = {
-	  /* 0x00000038 */
-	  .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3),
-	  /* 0x0030 */
-	  .l3cc_value = L3_3_WB,
-	},
+	GEN9_MOCS_ENTRIES,
 	[I915_MOCS_CACHED] = {
-	  /* 0x00000039 */
-	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
-	  /* 0x0030 */
-	  .l3cc_value = L3_3_WB,
+		/* 0x00000039 */
+		.control_value = LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
+		/* 0x0030 */
+		.l3cc_value = L3_3_WB,
 	},
 };
 
-- 
2.20.0

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

[PATCH v8 4/7] drm/i915: use a macro to define MOCS entries

[Lucas De Marchi]

Let's use a macro to make tables smaller and at the same time allow us
to add fields that apply to all entries in future.

For the sake of readability, I'm calling an exception on 80 chars limit.
Lines are aligned for easy comparison of the entry values.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_mocs.c | 39 +++++++++++--------------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index c7a2a8d81d90..faae2eefc5cc 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -71,6 +71,12 @@ struct drm_i915_mocs_table {
 #define L3_2_RESERVED		_L3_CACHEABILITY(2)
 #define L3_3_WB			_L3_CACHEABILITY(3)
 
+#define MOCS_ENTRY(__idx, __control_value, __l3cc_value) \
+	[__idx] = { \
+		.control_value = __control_value, \
+		.l3cc_value = __l3cc_value, \
+	}
+
 /*
  * MOCS tables
  *
@@ -93,40 +99,23 @@ struct drm_i915_mocs_table {
  *       may only be updated incrementally by adding entries at the
  *       end.
  */
-
 #define GEN9_MOCS_ENTRIES \
-	[I915_MOCS_UNCACHED] = { \
-		/* 0x00000009 */ \
-		.control_value = LE_1_UC | LE_TC_2_LLC_ELLC, \
-		/* 0x0010 */ \
-		.l3cc_value = L3_1_UC, \
-	}, \
-	[I915_MOCS_PTE] = { \
-		/* 0x00000038 */ \
-		.control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \
-		/* 0x0030 */ \
-		.l3cc_value = L3_3_WB, \
-	}
+	MOCS_ENTRY(I915_MOCS_UNCACHED,	LE_1_UC | LE_TC_2_LLC_ELLC, \
+					L3_1_UC), \
+	MOCS_ENTRY(I915_MOCS_PTE,	LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \
+					L3_3_WB) \
 
 static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
 	GEN9_MOCS_ENTRIES,
-	[I915_MOCS_CACHED] = {
-		/* 0x0000003b */
-		.control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
-		/* 0x0030 */
-		.l3cc_value =   L3_3_WB,
-	},
+	MOCS_ENTRY(I915_MOCS_CACHED,	LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
+					L3_3_WB)
 };
 
 /* NOTE: the LE_TGT_CACHE is not used on Broxton */
 static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
 	GEN9_MOCS_ENTRIES,
-	[I915_MOCS_CACHED] = {
-		/* 0x00000039 */
-		.control_value = LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
-		/* 0x0030 */
-		.l3cc_value = L3_3_WB,
-	},
+	MOCS_ENTRY(I915_MOCS_CACHED,	LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
+					L3_3_WB)
 };
 
 /**
-- 
2.20.0

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

[PATCH v8 5/7] drm/i915: keep track of used entries in MOCS table

[Lucas De Marchi]

Instead of considering we have defined entries for any index in the
table, let's keep track of the ones we explicitly defined. This will
allow Gen 11 to have it's new table defined in which we have holes of
undefined entries.

Repeated comments about the meaning of undefined entries were removed
since they are overly verbose and copy-pasted in several functions: now
the definition is in the top only.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_mocs.c | 88 ++++++++++++++++++++-----------
 1 file changed, 57 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index faae2eefc5cc..af2ae2f396ae 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -28,6 +28,7 @@
 struct drm_i915_mocs_entry {
 	u32 control_value;
 	u16 l3cc_value;
+	u16 used;
 };
 
 struct drm_i915_mocs_table {
@@ -75,6 +76,7 @@ struct drm_i915_mocs_table {
 	[__idx] = { \
 		.control_value = __control_value, \
 		.l3cc_value = __l3cc_value, \
+		.used = 1, \
 	}
 
 /*
@@ -195,24 +197,26 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine)
 	struct drm_i915_private *dev_priv = engine->i915;
 	struct drm_i915_mocs_table table;
 	unsigned int index;
+	u32 unused_value;
 
 	if (!get_mocs_settings(dev_priv, &table))
 		return;
 
 	GEM_BUG_ON(table.size > GEN9_NUM_MOCS_ENTRIES);
 
-	for (index = 0; index < table.size; index++)
-		I915_WRITE(mocs_register(engine->id, index),
-			   table.table[index].control_value);
+	/* Set unused values to PTE */
+	unused_value = table.table[I915_MOCS_PTE].control_value;
 
-	/*
-	 * Now set the unused entries to PTE. These entries are officially
-	 * undefined and no contract for the contents and settings is given
-	 * for these entries.
-	 */
+	for (index = 0; index < table.size; index++) {
+		u32 value = table.table[index].used ?
+			table.table[index].control_value : unused_value;
+
+		I915_WRITE(mocs_register(engine->id, index), value);
+	}
+
+	/* All remaining entries are also unused */
 	for (; index < GEN9_NUM_MOCS_ENTRIES; index++)
-		I915_WRITE(mocs_register(engine->id, index),
-			   table.table[I915_MOCS_PTE].control_value);
+		I915_WRITE(mocs_register(engine->id, index), unused_value);
 }
 
 /**
@@ -230,11 +234,15 @@ static int emit_mocs_control_table(struct i915_request *rq,
 {
 	enum intel_engine_id engine = rq->engine->id;
 	unsigned int index;
+	u32 unused_value;
 	u32 *cs;
 
 	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
 		return -ENODEV;
 
+	/* Set unused values to PTE */
+	unused_value = table->table[I915_MOCS_PTE].control_value;
+
 	cs = intel_ring_begin(rq, 2 + 2 * GEN9_NUM_MOCS_ENTRIES);
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
@@ -242,18 +250,17 @@ static int emit_mocs_control_table(struct i915_request *rq,
 	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES);
 
 	for (index = 0; index < table->size; index++) {
+		u32 value = table->table[index].used ?
+			table->table[index].control_value : unused_value;
+
 		*cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
-		*cs++ = table->table[index].control_value;
+		*cs++ = value;
 	}
 
-	/*
-	 * Now set the unused entries to PTE. These entries are officially
-	 * undefined and no contract for the contents and settings is given
-	 * for these entries.
-	 */
+	/* All remaining entries are also unused */
 	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
 		*cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
-		*cs++ = table->table[I915_MOCS_PTE].control_value;
+		*cs++ = unused_value;
 	}
 
 	*cs++ = MI_NOOP;
@@ -284,12 +291,15 @@ static inline u32 l3cc_combine(const struct drm_i915_mocs_table *table,
 static int emit_mocs_l3cc_table(struct i915_request *rq,
 				const struct drm_i915_mocs_table *table)
 {
-	unsigned int i;
+	unsigned int i, unused_index;
 	u32 *cs;
 
 	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
 		return -ENODEV;
 
+	/* Set unused values to PTE */
+	unused_index = I915_MOCS_PTE;
+
 	cs = intel_ring_begin(rq, 2 + GEN9_NUM_MOCS_ENTRIES);
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
@@ -297,25 +307,29 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
 	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2);
 
 	for (i = 0; i < table->size / 2; i++) {
+		u16 low = table->table[2 * i].used ?
+			2 * i : unused_index;
+		u16 high = table->table[2 * i + 1].used ?
+			2 * i + 1 : unused_index;
+
 		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
-		*cs++ = l3cc_combine(table, 2 * i, 2 * i + 1);
+		*cs++ = l3cc_combine(table, low, high);
 	}
 
 	if (table->size & 0x01) {
+		u16 low = table->table[2 * i].used ?
+			2 * i : unused_index;
+
 		/* Odd table size - 1 left over */
 		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
-		*cs++ = l3cc_combine(table, 2 * i, I915_MOCS_PTE);
+		*cs++ = l3cc_combine(table, low, unused_index);
 		i++;
 	}
 
-	/*
-	 * Now set the unused entries to PTE. These entries are officially
-	 * undefined and no contract for the contents and settings is given
-	 * for these entries.
-	 */
+	/* All remaining entries are also unused */
 	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
 		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
-		*cs++ = l3cc_combine(table, I915_MOCS_PTE, I915_MOCS_PTE);
+		*cs++ = l3cc_combine(table, unused_index, unused_index);
 	}
 
 	*cs++ = MI_NOOP;
@@ -341,26 +355,38 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
 void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv)
 {
 	struct drm_i915_mocs_table table;
-	unsigned int i;
+	unsigned int i, unused_index;
 
 	if (!get_mocs_settings(dev_priv, &table))
 		return;
 
-	for (i = 0; i < table.size / 2; i++)
+	/* Set unused values to PTE */
+	unused_index = I915_MOCS_PTE;
+
+	for (i = 0; i < table.size / 2; i++) {
+		u16 low = table.table[2 * i].used ?
+			2 * i : unused_index;
+		u16 high = table.table[2 * i + 1].used ?
+			2 * i + 1 : unused_index;
+
 		I915_WRITE(GEN9_LNCFCMOCS(i),
-			   l3cc_combine(&table, 2 * i, 2 * i + 1));
+			   l3cc_combine(&table, low, high));
+	}
 
 	/* Odd table size - 1 left over */
 	if (table.size & 0x01) {
+		u16 low = table.table[2 * i].used ?
+			2 * i : unused_index;
+
 		I915_WRITE(GEN9_LNCFCMOCS(i),
-			   l3cc_combine(&table, 2 * i, I915_MOCS_PTE));
+			   l3cc_combine(&table, low, unused_index));
 		i++;
 	}
 
 	/* Now set the rest of the table to PTE */
 	for (; i < (GEN9_NUM_MOCS_ENTRIES / 2); i++)
 		I915_WRITE(GEN9_LNCFCMOCS(i),
-			   l3cc_combine(&table, I915_MOCS_PTE, I915_MOCS_PTE));
+			   l3cc_combine(&table, unused_index, unused_index));
 }
 
 /**
-- 
2.20.0

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

[PATCH v8 6/7] drm/i915: cache number of MOCS entries

[Lucas De Marchi]

Instead of checking the gen number every time we need to know the max
number of entries, just save it into the table struct so we don't need
extra branches throughout the code. This will be useful for Ice Lake
that has 64 rather than 62 defined entries. Ice Lake changes will be
added in a follow up.

v2: make size and n_entries `unsigned int` and introduce changes as a
    pre-work for the Ice Lake changes (Tvrtko)

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_mocs.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index af2ae2f396ae..716f3f6f2966 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -32,7 +32,8 @@ struct drm_i915_mocs_entry {
 };
 
 struct drm_i915_mocs_table {
-	u32 size;
+	unsigned int size;
+	unsigned int n_entries;
 	const struct drm_i915_mocs_entry *table;
 };
 
@@ -140,10 +141,12 @@ static bool get_mocs_settings(struct drm_i915_private *dev_priv,
 	if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv) ||
 	    IS_ICELAKE(dev_priv)) {
 		table->size  = ARRAY_SIZE(skylake_mocs_table);
+		table->n_entries = GEN9_NUM_MOCS_ENTRIES;
 		table->table = skylake_mocs_table;
 		result = true;
 	} else if (IS_GEN9_LP(dev_priv)) {
 		table->size  = ARRAY_SIZE(broxton_mocs_table);
+		table->n_entries = GEN9_NUM_MOCS_ENTRIES;
 		table->table = broxton_mocs_table;
 		result = true;
 	} else {
@@ -202,8 +205,6 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine)
 	if (!get_mocs_settings(dev_priv, &table))
 		return;
 
-	GEM_BUG_ON(table.size > GEN9_NUM_MOCS_ENTRIES);
-
 	/* Set unused values to PTE */
 	unused_value = table.table[I915_MOCS_PTE].control_value;
 
@@ -215,7 +216,7 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine)
 	}
 
 	/* All remaining entries are also unused */
-	for (; index < GEN9_NUM_MOCS_ENTRIES; index++)
+	for (; index < table.n_entries; index++)
 		I915_WRITE(mocs_register(engine->id, index), unused_value);
 }
 
@@ -237,17 +238,17 @@ static int emit_mocs_control_table(struct i915_request *rq,
 	u32 unused_value;
 	u32 *cs;
 
-	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
+	if (GEM_WARN_ON(table->size > table->n_entries))
 		return -ENODEV;
 
 	/* Set unused values to PTE */
 	unused_value = table->table[I915_MOCS_PTE].control_value;
 
-	cs = intel_ring_begin(rq, 2 + 2 * GEN9_NUM_MOCS_ENTRIES);
+	cs = intel_ring_begin(rq, 2 + 2 * table->n_entries);
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
 
-	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES);
+	*cs++ = MI_LOAD_REGISTER_IMM(table->n_entries);
 
 	for (index = 0; index < table->size; index++) {
 		u32 value = table->table[index].used ?
@@ -258,7 +259,7 @@ static int emit_mocs_control_table(struct i915_request *rq,
 	}
 
 	/* All remaining entries are also unused */
-	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
+	for (; index < table->n_entries; index++) {
 		*cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
 		*cs++ = unused_value;
 	}
@@ -294,17 +295,17 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
 	unsigned int i, unused_index;
 	u32 *cs;
 
-	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
+	if (GEM_WARN_ON(table->size > table->n_entries))
 		return -ENODEV;
 
 	/* Set unused values to PTE */
 	unused_index = I915_MOCS_PTE;
 
-	cs = intel_ring_begin(rq, 2 + GEN9_NUM_MOCS_ENTRIES);
+	cs = intel_ring_begin(rq, 2 + table->n_entries);
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
 
-	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2);
+	*cs++ = MI_LOAD_REGISTER_IMM(table->n_entries / 2);
 
 	for (i = 0; i < table->size / 2; i++) {
 		u16 low = table->table[2 * i].used ?
@@ -327,7 +328,7 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
 	}
 
 	/* All remaining entries are also unused */
-	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
+	for (; i < table->n_entries / 2; i++) {
 		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
 		*cs++ = l3cc_combine(table, unused_index, unused_index);
 	}
@@ -384,7 +385,7 @@ void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv)
 	}
 
 	/* Now set the rest of the table to PTE */
-	for (; i < (GEN9_NUM_MOCS_ENTRIES / 2); i++)
+	for (; i < table.n_entries / 2; i++)
 		I915_WRITE(GEN9_LNCFCMOCS(i),
 			   l3cc_combine(&table, unused_index, unused_index));
 }
-- 
2.20.0

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

[PATCH v8 7/7] drm/i915/icl: Define MOCS table for Icelake

[Lucas De Marchi]

From: Tomasz Lis <tomasz.lis@intel.com>

The table has been unified across OSes to minimize virtualization overhead.

The MOCS table is now published as part of bspec, and versioned. Entries
are supposed to never be modified, but new ones can be added. Adding
entries increases table version. The patch includes version 1 entries.

Meaning of each entry is now explained in bspec, and user mode clients
are expected to know what each entry means. The 3 entries used for previous
platforms are still compatible with their legacy definitions, but that is
not guaranteed to be true for future platforms.

v2: Fixed SCC values, improved commit comment (Daniele)
v3: Improved MOCS table comment (Daniele)
v4: Moved new entries below gen9 ones. Put common entries into
    definition to be used in multiple arrays. (Lucas)
v5: Made defines for or-ing flags. Renamed macros from MOCS_TABLE
    to MOCS_ENTRIES. Switched LE_CoS to upper case. (Joonas)
v6: Removed definitions of reserved entries. (Michal)
    Increased limit of entries sent to the hardware on gen11+.
v7: Simplify table as done for previou gens (Lucas)
v8: Rebase on cached number of entries per-platform and use new
    MOCS_ENTRY() macro (Lucas)

BSpec: 34007
BSpec: 560

Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_mocs.c | 107 +++++++++++++++++++++++++++---
 1 file changed, 98 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index 716f3f6f2966..3afd8c30cacc 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -46,6 +46,8 @@ struct drm_i915_mocs_table {
 #define LE_SCC(value)		((value) << 8)
 #define LE_PFM(value)		((value) << 11)
 #define LE_SCF(value)		((value) << 14)
+#define LE_COS(value)		((value) << 15)
+#define LE_SSE(value)		((value) << 17)
 
 /* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
 #define L3_ESC(value)		((value) << 0)
@@ -54,6 +56,7 @@ struct drm_i915_mocs_table {
 
 /* Helper defines */
 #define GEN9_NUM_MOCS_ENTRIES	62  /* 62 out of 64 - 63 & 64 are reserved. */
+#define GEN11_NUM_MOCS_ENTRIES	64  /* 63-64 are reserved, but configured. */
 
 /* (e)LLC caching options */
 #define LE_0_PAGETABLE		_LE_CACHEABILITY(0)
@@ -89,18 +92,22 @@ struct drm_i915_mocs_table {
  * LNCFCMOCS0 - LNCFCMOCS32 registers.
  *
  * These tables are intended to be kept reasonably consistent across
- * platforms. However some of the fields are not applicable to all of
- * them.
+ * HW platforms, and for ICL+, be identical across OSes. To achieve
+ * that, for Icelake and above, list of entries is published as part
+ * of bspec.
  *
  * Entries not part of the following tables are undefined as far as
  * userspace is concerned and shouldn't be relied upon.  For the time
  * being they will be initialized to PTE.
  *
- * NOTE: These tables MUST start with being uncached and the length
- *       MUST be less than 63 as the last two registers are reserved
- *       by the hardware.  These tables are part of the kernel ABI and
- *       may only be updated incrementally by adding entries at the
- *       end.
+ * The last two entries are reserved by the hardware. For ICL+ they
+ * should be initialized according to bspec and never used, for older
+ * platforms they should never be written to.
+ *
+ * NOTE: These tables are part of bspec and defined as part of hardware
+ *       interface for ICL+. For older platforms, they are part of kernel
+ *       ABI. It is expected that existing entries will remain constant
+ *       and the tables will only be updated by adding new entries.
  */
 #define GEN9_MOCS_ENTRIES \
 	MOCS_ENTRY(I915_MOCS_UNCACHED,	LE_1_UC | LE_TC_2_LLC_ELLC, \
@@ -121,6 +128,84 @@ static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
 					L3_3_WB)
 };
 
+#define GEN11_MOCS_ENTRIES \
+	/* Base - Uncached (Deprecated) */ \
+	MOCS_ENTRY(I915_MOCS_UNCACHED,	LE_1_UC | LE_TC_1_LLC, \
+					L3_1_UC), \
+	/* Base - L3 + LeCC:PAT (Deprecated) */ \
+	MOCS_ENTRY(I915_MOCS_PTE,	LE_0_PAGETABLE | LE_TC_1_LLC, \
+					L3_3_WB), \
+	/* Base - L3 + LLC */ \
+	MOCS_ENTRY(2,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
+			L3_3_WB), \
+	/* Base - Uncached */ \
+	MOCS_ENTRY(3,	LE_1_UC | LE_TC_1_LLC, \
+			L3_1_UC), \
+	/* Base - L3 */ \
+	MOCS_ENTRY(4,	LE_1_UC | LE_TC_1_LLC, \
+			L3_3_WB), \
+	/* Base - LLC */ \
+	MOCS_ENTRY(5,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
+			L3_1_UC), \
+	/* Age 0 - LLC */ \
+	MOCS_ENTRY(6,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1), \
+			L3_1_UC), \
+	/* Age 0 - L3 + LLC */ \
+	MOCS_ENTRY(7,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1), \
+			L3_3_WB), \
+	/* Age: Don't Chg. - LLC */ \
+	MOCS_ENTRY(8,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2), \
+			L3_1_UC), \
+	/* Age: Don't Chg. - L3 + LLC */ \
+	MOCS_ENTRY(9,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2), \
+			L3_3_WB), \
+	/* No AOM - LLC */ \
+	MOCS_ENTRY(10,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_AOM(1), \
+			L3_1_UC), \
+	/* No AOM - L3 + LLC */ \
+	MOCS_ENTRY(11,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_AOM(1), \
+			L3_3_WB), \
+	/* No AOM; Age 0 - LLC */ \
+	MOCS_ENTRY(12,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1) | LE_AOM(1), \
+			L3_1_UC), \
+	/* No AOM; Age 0 - L3 + LLC */ \
+	MOCS_ENTRY(13,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1) | LE_AOM(1), \
+			L3_3_WB), \
+	/* No AOM; Age:DC - LLC */ \
+	MOCS_ENTRY(14,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2) | LE_AOM(1), \
+			L3_1_UC), \
+	/* No AOM; Age:DC - L3 + LLC */ \
+	MOCS_ENTRY(15,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2) | LE_AOM(1), \
+			L3_3_WB), \
+	/* Self-Snoop - L3 + LLC */ \
+	MOCS_ENTRY(18,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SSE(3), \
+			L3_3_WB), \
+	/* Skip Caching - L3 + LLC(12.5%) */ \
+	MOCS_ENTRY(19,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SCC(7), \
+			L3_3_WB), \
+	/* Skip Caching - L3 + LLC(25%) */ \
+	MOCS_ENTRY(20,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SCC(3), \
+			L3_3_WB), \
+	/* Skip Caching - L3 + LLC(50%) */ \
+	MOCS_ENTRY(21,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SCC(1), \
+			L3_3_WB), \
+	/* Skip Caching - L3 + LLC(75%) */ \
+	MOCS_ENTRY(22,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_RSC(1) | LE_SCC(3), \
+			L3_3_WB), \
+	/* Skip Caching - L3 + LLC(87.5%) */ \
+	MOCS_ENTRY(23,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_RSC(1) | LE_SCC(7), \
+			L3_3_WB), \
+	/* HW Reserved - SW program but never use */ \
+	MOCS_ENTRY(62,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
+			L3_1_UC), \
+	/* HW Reserved - SW program but never use */ \
+	MOCS_ENTRY(63,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
+			L3_1_UC), \
+
+static const struct drm_i915_mocs_entry icelake_mocs_table[] = {
+	GEN11_MOCS_ENTRIES
+};
+
 /**
  * get_mocs_settings()
  * @dev_priv:	i915 device.
@@ -138,8 +223,12 @@ static bool get_mocs_settings(struct drm_i915_private *dev_priv,
 {
 	bool result = false;
 
-	if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv) ||
-	    IS_ICELAKE(dev_priv)) {
+	if (IS_ICELAKE(dev_priv)) {
+		table->size  = ARRAY_SIZE(icelake_mocs_table);
+		table->table = icelake_mocs_table;
+		table->n_entries = GEN11_NUM_MOCS_ENTRIES;
+		result = true;
+	} else if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv)) {
 		table->size  = ARRAY_SIZE(skylake_mocs_table);
 		table->n_entries = GEN9_NUM_MOCS_ENTRIES;
 		table->table = skylake_mocs_table;
-- 
2.20.0

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

✗ Fi.CI.CHECKPATCH: warning for Define MOCS table for Icelake (rev2)

[Patchwork]

== Series Details ==

Series: Define MOCS table for Icelake (rev2)
URL   : https://patchwork.freedesktop.org/series/54070/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
42a8e03cdfa2 drm/i915: initialize unused MOCS entries to PTE
392416ee2864 drm/i915: Simplify MOCS table definition
d732afcb4803 drm/i915/skl: Rework MOCS tables to keep common part in a define
75c2a9d1ab5d drm/i915: use a macro to define MOCS entries
-:63: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#63: FILE: drivers/gpu/drm/i915/intel_mocs.c:111:
+	MOCS_ENTRY(I915_MOCS_CACHED,	LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
+					L3_3_WB)

-:76: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#76: FILE: drivers/gpu/drm/i915/intel_mocs.c:118:
+	MOCS_ENTRY(I915_MOCS_CACHED,	LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
+					L3_3_WB)

total: 0 errors, 0 warnings, 2 checks, 60 lines checked
c0f65453147a drm/i915: keep track of used entries in MOCS table
9ace3442d5cf drm/i915: cache number of MOCS entries
d4f7316f00e2 drm/i915/icl: Define MOCS table for Icelake
-:92: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#92: FILE: drivers/gpu/drm/i915/intel_mocs.c:131:
+#define GEN11_MOCS_ENTRIES \
+	/* Base - Uncached (Deprecated) */ \
+	MOCS_ENTRY(I915_MOCS_UNCACHED,	LE_1_UC | LE_TC_1_LLC, \
+					L3_1_UC), \
+	/* Base - L3 + LeCC:PAT (Deprecated) */ \
+	MOCS_ENTRY(I915_MOCS_PTE,	LE_0_PAGETABLE | LE_TC_1_LLC, \
+					L3_3_WB), \
+	/* Base - L3 + LLC */ \
+	MOCS_ENTRY(2,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
+			L3_3_WB), \
+	/* Base - Uncached */ \
+	MOCS_ENTRY(3,	LE_1_UC | LE_TC_1_LLC, \
+			L3_1_UC), \
+	/* Base - L3 */ \
+	MOCS_ENTRY(4,	LE_1_UC | LE_TC_1_LLC, \
+			L3_3_WB), \
+	/* Base - LLC */ \
+	MOCS_ENTRY(5,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
+			L3_1_UC), \
+	/* Age 0 - LLC */ \
+	MOCS_ENTRY(6,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1), \
+			L3_1_UC), \
+	/* Age 0 - L3 + LLC */ \
+	MOCS_ENTRY(7,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1), \
+			L3_3_WB), \
+	/* Age: Don't Chg. - LLC */ \
+	MOCS_ENTRY(8,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2), \
+			L3_1_UC), \
+	/* Age: Don't Chg. - L3 + LLC */ \
+	MOCS_ENTRY(9,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2), \
+			L3_3_WB), \
+	/* No AOM - LLC */ \
+	MOCS_ENTRY(10,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_AOM(1), \
+			L3_1_UC), \
+	/* No AOM - L3 + LLC */ \
+	MOCS_ENTRY(11,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_AOM(1), \
+			L3_3_WB), \
+	/* No AOM; Age 0 - LLC */ \
+	MOCS_ENTRY(12,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1) | LE_AOM(1), \
+			L3_1_UC), \
+	/* No AOM; Age 0 - L3 + LLC */ \
+	MOCS_ENTRY(13,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1) | LE_AOM(1), \
+			L3_3_WB), \
+	/* No AOM; Age:DC - LLC */ \
+	MOCS_ENTRY(14,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2) | LE_AOM(1), \
+			L3_1_UC), \
+	/* No AOM; Age:DC - L3 + LLC */ \
+	MOCS_ENTRY(15,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2) | LE_AOM(1), \
+			L3_3_WB), \
+	/* Self-Snoop - L3 + LLC */ \
+	MOCS_ENTRY(18,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SSE(3), \
+			L3_3_WB), \
+	/* Skip Caching - L3 + LLC(12.5%) */ \
+	MOCS_ENTRY(19,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SCC(7), \
+			L3_3_WB), \
+	/* Skip Caching - L3 + LLC(25%) */ \
+	MOCS_ENTRY(20,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SCC(3), \
+			L3_3_WB), \
+	/* Skip Caching - L3 + LLC(50%) */ \
+	MOCS_ENTRY(21,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SCC(1), \
+			L3_3_WB), \
+	/* Skip Caching - L3 + LLC(75%) */ \
+	MOCS_ENTRY(22,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_RSC(1) | LE_SCC(3), \
+			L3_3_WB), \
+	/* Skip Caching - L3 + LLC(87.5%) */ \
+	MOCS_ENTRY(23,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_RSC(1) | LE_SCC(7), \
+			L3_3_WB), \
+	/* HW Reserved - SW program but never use */ \
+	MOCS_ENTRY(62,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
+			L3_1_UC), \
+	/* HW Reserved - SW program but never use */ \
+	MOCS_ENTRY(63,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
+			L3_1_UC), \
+

total: 1 errors, 0 warnings, 0 checks, 142 lines checked

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

✓ Fi.CI.BAT: success for Define MOCS table for Icelake (rev2)

[Patchwork]

== Series Details ==

Series: Define MOCS table for Icelake (rev2)
URL   : https://patchwork.freedesktop.org/series/54070/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5459 -> Patchwork_12002
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/54070/revisions/2/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12002 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  
#### Possible fixes ####

  * igt@i915_selftest@live_hangcheck:
    - fi-bwr-2160:        DMESG-FAIL [fdo#108735] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-icl-u2}:        FAIL [fdo#103167] -> PASS

  * igt@pm_rpm@module-reload:
    - {fi-icl-u2}:        DMESG-WARN [fdo#108654] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
  [fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315


Participating hosts (47 -> 41)
------------------------------

  Additional (1): fi-glk-j4005 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-icl-y fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5459 -> Patchwork_12002

  CI_DRM_5459: 0f693a275dd91391b476ada7481cf08f4fe610aa @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4780: 1c1612bdc36b44a704095e7b0ba5542818ce793f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12002: d4f7316f00e20115e8fb29d92d2d80bba89a7b45 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d4f7316f00e2 drm/i915/icl: Define MOCS table for Icelake
9ace3442d5cf drm/i915: cache number of MOCS entries
c0f65453147a drm/i915: keep track of used entries in MOCS table
75c2a9d1ab5d drm/i915: use a macro to define MOCS entries
d732afcb4803 drm/i915/skl: Rework MOCS tables to keep common part in a define
392416ee2864 drm/i915: Simplify MOCS table definition
42a8e03cdfa2 drm/i915: initialize unused MOCS entries to PTE

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12002/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Define MOCS table for Icelake (rev2)

[Patchwork]

== Series Details ==

Series: Define MOCS table for Icelake (rev2)
URL   : https://patchwork.freedesktop.org/series/54070/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5459_full -> Patchwork_12002_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_12002_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_schedule@pi-ringfull-blt:
    - shard-apl:          NOTRUN -> FAIL [fdo#103158]

  * igt@kms_content_protection@legacy:
    - shard-apl:          NOTRUN -> FAIL [fdo#108597]

  * igt@kms_cursor_crc@cursor-64x21-onscreen:
    - shard-apl:          PASS -> FAIL [fdo#103232]

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          PASS -> FAIL [fdo#105363]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-wc:
    - shard-glk:          PASS -> FAIL [fdo#103167]

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
    - shard-glk:          PASS -> FAIL [fdo#103166]
    - shard-apl:          PASS -> FAIL [fdo#103166]

  
#### Possible fixes ####

  * igt@kms_busy@extended-pageflip-hang-oldfb-render-b:
    - shard-snb:          {SKIP} [fdo#109271] / [fdo#109278] -> PASS

  * igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
    - shard-glk:          FAIL [fdo#108145] -> PASS

  * igt@kms_cursor_crc@cursor-128x42-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-gtt-untiled:
    - shard-snb:          {SKIP} [fdo#109271] -> PASS +3

  * igt@kms_rotation_crc@multiplane-rotation:
    - shard-kbl:          FAIL -> PASS

  * igt@kms_setmode@basic:
    - shard-apl:          INCOMPLETE [fdo#103927] -> PASS
    - shard-kbl:          FAIL [fdo#99912] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103158]: https://bugs.freedesktop.org/show_bug.cgi?id=103158
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108597]: https://bugs.freedesktop.org/show_bug.cgi?id=108597
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 5)
------------------------------

  Missing    (2): shard-skl shard-iclb 


Build changes
-------------

    * Linux: CI_DRM_5459 -> Patchwork_12002

  CI_DRM_5459: 0f693a275dd91391b476ada7481cf08f4fe610aa @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4780: 1c1612bdc36b44a704095e7b0ba5542818ce793f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12002: d4f7316f00e20115e8fb29d92d2d80bba89a7b45 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12002/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 1/7] drm/i915: initialize unused MOCS entries to PTE

[Chris Wilson]

Quoting Lucas De Marchi (2019-01-22 05:12:21)
> Instead of initializing them to uncached, let's set them to PTE for
> kernel tracking. While at it do some minor adjustments to comments and
> coding style.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

I'm in favour. I do not think this contributes an ABI change, as these
values are explicitly outside of the ABI. What it does mean is that the
buffer contents are consistent with our cache tracking; and for
userspace the results were always undefined. So we should at least be
able to guarantee that the data written by userspace from the CPU is
visible. After that, your caches are on your own.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 3/7] drm/i915/skl: Rework MOCS tables to keep common part in a define

[Chris Wilson]

Quoting Lucas De Marchi (2019-01-22 05:12:23)
> From: Tomasz Lis <tomasz.lis@intel.com>
> 
> The MOCS tables are going to be very similar across platforms.
> 
> To reduce the amount of copied code, this patch rips the common part and
> puts it into a definition valid for all gen9 platforms.
> 
> v2: Made defines for or-ing flags. Renamed macros from MOCS_TABLE
>     to MOCS_ENTRIES. (Joonas)
> v3 (Lucas):
>   - Fix indentation
>   - Rebase on rework done by additional patch
>   - Remove define for or-ing flags as it made the table more complex by
>     requiring zeroed values to be passed
>   - Do not embed comma in the macro, so to treat that just as another
>     item and please source code formatting tools
> 
> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
> Suggested-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_mocs.c | 57 ++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 4ea80bb7dcc8..c7a2a8d81d90 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -93,46 +93,39 @@ struct drm_i915_mocs_table {
>   *       may only be updated incrementally by adding entries at the
>   *       end.
>   */
> +
> +#define GEN9_MOCS_ENTRIES \
> +       [I915_MOCS_UNCACHED] = { \
> +               /* 0x00000009 */ \
> +               .control_value = LE_1_UC | LE_TC_2_LLC_ELLC, \
> +               /* 0x0010 */ \
> +               .l3cc_value = L3_1_UC, \
> +       }, \
> +       [I915_MOCS_PTE] = { \
> +               /* 0x00000038 */ \
> +               .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \
> +               /* 0x0030 */ \
> +               .l3cc_value = L3_3_WB, \
> +       }
> +
>  static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
> -       [I915_MOCS_UNCACHED] = {
> -         /* 0x00000009 */
> -         .control_value = LE_1_UC | LE_TC_2_LLC_ELLC,
> -         /* 0x0010 */
> -         .l3cc_value =    L3_1_UC,
> -       },
> -       [I915_MOCS_PTE] = {
> -         /* 0x00000038 */
> -         .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> -         /* 0x0030 */
> -         .l3cc_value =    L3_3_WB,
> -       },
> +       GEN9_MOCS_ENTRIES,
>         [I915_MOCS_CACHED] = {
> -         /* 0x0000003b */
> -         .control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> -         /* 0x0030 */
> -         .l3cc_value =   L3_3_WB,
> +               /* 0x0000003b */

You scared me with the indentation change at the same time.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 4/7] drm/i915: use a macro to define MOCS entries

[Chris Wilson]

Quoting Lucas De Marchi (2019-01-22 05:12:24)
> Let's use a macro to make tables smaller and at the same time allow us
> to add fields that apply to all entries in future.
> 
> For the sake of readability, I'm calling an exception on 80 chars limit.
> Lines are aligned for easy comparison of the entry values.

> +       MOCS_ENTRY(I915_MOCS_UNCACHED,  LE_1_UC | LE_TC_2_LLC_ELLC, \
> +                                       L3_1_UC), \

          MOCS_ENTRY(I915_MOCS_UNCACHED,
		     LE_1_UC | LE_TC_2_LLC_ELLC, L3_1_UC), \

is even more readable with the visual clustering of attribute flags.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 6/7] drm/i915: cache number of MOCS entries

[Chris Wilson]

Quoting Lucas De Marchi (2019-01-22 05:12:26)
> Instead of checking the gen number every time we need to know the max
> number of entries, just save it into the table struct so we don't need
> extra branches throughout the code. This will be useful for Ice Lake
> that has 64 rather than 62 defined entries. Ice Lake changes will be
> added in a follow up.
> 
> v2: make size and n_entries `unsigned int` and introduce changes as a
>     pre-work for the Ice Lake changes (Tvrtko)
> 
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 5/7] drm/i915: keep track of used entries in MOCS table

[Chris Wilson]

Quoting Lucas De Marchi (2019-01-22 05:12:25)
> Instead of considering we have defined entries for any index in the
> table, let's keep track of the ones we explicitly defined. This will
> allow Gen 11 to have it's new table defined in which we have holes of
> undefined entries.
> 
> Repeated comments about the meaning of undefined entries were removed
> since they are overly verbose and copy-pasted in several functions: now
> the definition is in the top only.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_mocs.c | 88 ++++++++++++++++++++-----------
>  1 file changed, 57 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index faae2eefc5cc..af2ae2f396ae 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -28,6 +28,7 @@
>  struct drm_i915_mocs_entry {
>         u32 control_value;
>         u16 l3cc_value;
> +       u16 used;
>  };

> +       /* Set unused values to PTE */
> +       unused_index = I915_MOCS_PTE;
> +
> +       for (i = 0; i < table.size / 2; i++) {
> +               u16 low = table.table[2 * i].used ?
> +                       2 * i : unused_index;
> +               u16 high = table.table[2 * i + 1].used ?
> +                       2 * i + 1 : unused_index;

I'm underwhelmed here.

Could we not do something like

static unsigned int
get_entry_index(struct tbl *tbl, unsigned int idx, unsigned int unused_index)
{
	return tbl->used ? idx : unused_index;
}

		u16 lo = get_entry_index(table.table, 2 * i, unused_index);
		u16 hi = get_entry_index(table.table, 2 * i + 1, unused_index);

That just fits and repeated enough to be worth a little extra effort.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 4/7] drm/i915: use a macro to define MOCS entries

[Lucas De Marchi]

On Tue, Jan 22, 2019 at 6:32 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Lucas De Marchi (2019-01-22 05:12:24)
> > Let's use a macro to make tables smaller and at the same time allow us
> > to add fields that apply to all entries in future.
> >
> > For the sake of readability, I'm calling an exception on 80 chars limit.
> > Lines are aligned for easy comparison of the entry values.
>
> > +       MOCS_ENTRY(I915_MOCS_UNCACHED,  LE_1_UC | LE_TC_2_LLC_ELLC, \
> > +                                       L3_1_UC), \
>
>           MOCS_ENTRY(I915_MOCS_UNCACHED,
>                      LE_1_UC | LE_TC_2_LLC_ELLC, L3_1_UC), \

My intention was to split the lines for each *value*, so it's easy to
see what control_value vs l3cc_value is set to
(too difficult to spot mistakes on adding a comma rather than a |).

But I'm not strongly against your version, so I'll switch to that.

thanks
Lucas De Marchi

>
> is even more readable with the visual clustering of attribute flags.
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH v8 4/7] drm/i915: use a macro to define MOCS entries

[Chris Wilson]

Quoting Lucas De Marchi (2019-01-22 21:33:25)
> On Tue, Jan 22, 2019 at 6:32 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Quoting Lucas De Marchi (2019-01-22 05:12:24)
> > > Let's use a macro to make tables smaller and at the same time allow us
> > > to add fields that apply to all entries in future.
> > >
> > > For the sake of readability, I'm calling an exception on 80 chars limit.
> > > Lines are aligned for easy comparison of the entry values.
> >
> > > +       MOCS_ENTRY(I915_MOCS_UNCACHED,  LE_1_UC | LE_TC_2_LLC_ELLC, \
> > > +                                       L3_1_UC), \
> >
> >           MOCS_ENTRY(I915_MOCS_UNCACHED,
> >                      LE_1_UC | LE_TC_2_LLC_ELLC, L3_1_UC), \
> 
> My intention was to split the lines for each *value*, so it's easy to
> see what control_value vs l3cc_value is set to
> (too difficult to spot mistakes on adding a comma rather than a |).
> 
> But I'm not strongly against your version, so I'll switch to that.

Have another new line :)

Because you are right as I confused that \ for a |.

           MOCS_ENTRY(I915_MOCS_UNCACHED, \
                      LE_1_UC | LE_TC_2_LLC_ELLC, \
		      L3_1_UC), \

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

Re: [PATCH v8 1/7] drm/i915: initialize unused MOCS entries to PTE

[Lis, Tomasz]

On 2019-01-22 06:12, Lucas De Marchi wrote:
> Instead of initializing them to uncached, let's set them to PTE for
> kernel tracking. While at it do some minor adjustments to comments and
> coding style.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>
One comment (with no expectations for change) below.
> ---
>   drivers/gpu/drm/i915/intel_mocs.c | 56 +++++++++++++------------------
>   1 file changed, 23 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index e976c5ce5479..0d6b94a239d6 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -85,10 +85,7 @@ struct drm_i915_mocs_table {
>    *
>    * Entries not part of the following tables are undefined as far as
>    * userspace is concerned and shouldn't be relied upon.  For the time
> - * being they will be implicitly initialized to the strictest caching
> - * configuration (uncached) to guarantee forwards compatibility with
> - * userspace programs written against more recent kernels providing
> - * additional MOCS entries.
> + * being they will be initialized to PTE.
>    *
>    * NOTE: These tables MUST start with being uncached and the length
>    *       MUST be less than 63 as the last two registers are reserved
> @@ -249,16 +246,13 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine)
>   			   table.table[index].control_value);
>   
>   	/*
> -	 * Ok, now set the unused entries to uncached. These entries
> -	 * are officially undefined and no contract for the contents
> -	 * and settings is given for these entries.
> -	 *
> -	 * Entry 0 in the table is uncached - so we are just writing
> -	 * that value to all the used entries.
> +	 * Now set the unused entries to PTE. These entries are officially
> +	 * undefined and no contract for the contents and settings is given
> +	 * for these entries.
>   	 */
>   	for (; index < GEN9_NUM_MOCS_ENTRIES; index++)
>   		I915_WRITE(mocs_register(engine->id, index),
> -			   table.table[0].control_value);
> +			   table.table[I915_MOCS_PTE].control_value);
>   }
>   
>   /**
> @@ -293,16 +287,13 @@ static int emit_mocs_control_table(struct i915_request *rq,
>   	}
>   
>   	/*
> -	 * Ok, now set the unused entries to uncached. These entries
> -	 * are officially undefined and no contract for the contents
> -	 * and settings is given for these entries.
> -	 *
> -	 * Entry 0 in the table is uncached - so we are just writing
> -	 * that value to all the used entries.
> +	 * Now set the unused entries to PTE. These entries are officially
> +	 * undefined and no contract for the contents and settings is given
> +	 * for these entries.
>   	 */
>   	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
>   		*cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
> -		*cs++ = table->table[0].control_value;
> +		*cs++ = table->table[I915_MOCS_PTE].control_value;
Entries from enum i915_mocs_table_index are not guaranteed to mean the 
same thing in future gens;
but for the time, that will work. And later it might still work, we 
don't know.
-Tomasz
>   	}
>   
>   	*cs++ = MI_NOOP;
> @@ -345,7 +336,7 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
>   
>   	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2);
>   
> -	for (i = 0; i < table->size/2; i++) {
> +	for (i = 0; i < table->size / 2; i++) {
>   		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
>   		*cs++ = l3cc_combine(table, 2 * i, 2 * i + 1);
>   	}
> @@ -353,18 +344,18 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
>   	if (table->size & 0x01) {
>   		/* Odd table size - 1 left over */
>   		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
> -		*cs++ = l3cc_combine(table, 2 * i, 0);
> +		*cs++ = l3cc_combine(table, 2 * i, I915_MOCS_PTE);
>   		i++;
>   	}
>   
>   	/*
> -	 * Now set the rest of the table to uncached - use entry 0 as
> -	 * this will be uncached. Leave the last pair uninitialised as
> -	 * they are reserved by the hardware.
> +	 * Now set the unused entries to PTE. These entries are officially
> +	 * undefined and no contract for the contents and settings is given
> +	 * for these entries.
>   	 */
>   	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
>   		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
> -		*cs++ = l3cc_combine(table, 0, 0);
> +		*cs++ = l3cc_combine(table, I915_MOCS_PTE, I915_MOCS_PTE);
>   	}
>   
>   	*cs++ = MI_NOOP;
> @@ -395,22 +386,21 @@ void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv)
>   	if (!get_mocs_settings(dev_priv, &table))
>   		return;
>   
> -	for (i = 0; i < table.size/2; i++)
> -		I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 2*i, 2*i+1));
> +	for (i = 0; i < table.size / 2; i++)
> +		I915_WRITE(GEN9_LNCFCMOCS(i),
> +			   l3cc_combine(&table, 2 * i, 2 * i + 1));
>   
>   	/* Odd table size - 1 left over */
>   	if (table.size & 0x01) {
> -		I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 2*i, 0));
> +		I915_WRITE(GEN9_LNCFCMOCS(i),
> +			   l3cc_combine(&table, 2 * i, I915_MOCS_PTE));
>   		i++;
>   	}
>   
> -	/*
> -	 * Now set the rest of the table to uncached - use entry 0 as
> -	 * this will be uncached. Leave the last pair as initialised as
> -	 * they are reserved by the hardware.
> -	 */
> +	/* Now set the rest of the table to PTE */
>   	for (; i < (GEN9_NUM_MOCS_ENTRIES / 2); i++)
> -		I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 0, 0));
> +		I915_WRITE(GEN9_LNCFCMOCS(i),
> +			   l3cc_combine(&table, I915_MOCS_PTE, I915_MOCS_PTE));
>   }
>   
>   /**

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

Re: [PATCH v8 2/7] drm/i915: Simplify MOCS table definition

[Lis, Tomasz]

On 2019-01-22 06:12, Lucas De Marchi wrote:
> Make the defines for LE and L3 caching options to contain the shifts and
> remove the zeros from the tables as shifting zeros always result in
> zero.
>
> Starting from Ice Lake the MOCS table is defined in the spec and
> contains all entries. So to simplify checking the table with the values
> set in code, the value is now part of the macro name. This allows to
> still give the most used option and sensible name, but also to easily
> cross check the table from the spec for gen >= 11.
>
> By removing the zeros we avoid maintaining a huge table since the one
> from spec contains many more entries. The new table for Ice Lake will
> be added by other patches, this only reformats the table.
>
> While at it also fix the indentation.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
That is much cleaner.
Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>
-Tomasz
> ---
>   drivers/gpu/drm/i915/intel_mocs.c | 80 +++++++++++--------------------
>   1 file changed, 29 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 0d6b94a239d6..4ea80bb7dcc8 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -36,8 +36,8 @@ struct drm_i915_mocs_table {
>   };
>   
>   /* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
> -#define LE_CACHEABILITY(value)	((value) << 0)
> -#define LE_TGT_CACHE(value)	((value) << 2)
> +#define _LE_CACHEABILITY(value)	((value) << 0)
> +#define _LE_TGT_CACHE(value)	((value) << 2)
>   #define LE_LRUM(value)		((value) << 4)
>   #define LE_AOM(value)		((value) << 6)
>   #define LE_RSC(value)		((value) << 7)
> @@ -48,28 +48,28 @@ struct drm_i915_mocs_table {
>   /* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
>   #define L3_ESC(value)		((value) << 0)
>   #define L3_SCC(value)		((value) << 1)
> -#define L3_CACHEABILITY(value)	((value) << 4)
> +#define _L3_CACHEABILITY(value)	((value) << 4)
>   
>   /* Helper defines */
>   #define GEN9_NUM_MOCS_ENTRIES	62  /* 62 out of 64 - 63 & 64 are reserved. */
>   
>   /* (e)LLC caching options */
> -#define LE_PAGETABLE		0
> -#define LE_UC			1
> -#define LE_WT			2
> -#define LE_WB			3
> -
> -/* L3 caching options */
> -#define L3_DIRECT		0
> -#define L3_UC			1
> -#define L3_RESERVED		2
> -#define L3_WB			3
> +#define LE_0_PAGETABLE		_LE_CACHEABILITY(0)
> +#define LE_1_UC			_LE_CACHEABILITY(1)
> +#define LE_2_WT			_LE_CACHEABILITY(2)
> +#define LE_3_WB			_LE_CACHEABILITY(3)
>   
>   /* Target cache */
> -#define LE_TC_PAGETABLE		0
> -#define LE_TC_LLC		1
> -#define LE_TC_LLC_ELLC		2
> -#define LE_TC_LLC_ELLC_ALT	3
> +#define LE_TC_0_PAGETABLE	_LE_TGT_CACHE(0)
> +#define LE_TC_1_LLC		_LE_TGT_CACHE(1)
> +#define LE_TC_2_LLC_ELLC	_LE_TGT_CACHE(2)
> +#define LE_TC_3_LLC_ELLC_ALT	_LE_TGT_CACHE(3)
> +
> +/* L3 caching options */
> +#define L3_0_DIRECT		_L3_CACHEABILITY(0)
> +#define L3_1_UC			_L3_CACHEABILITY(1)
> +#define L3_2_RESERVED		_L3_CACHEABILITY(2)
> +#define L3_3_WB			_L3_CACHEABILITY(3)
>   
>   /*
>    * MOCS tables
> @@ -96,31 +96,21 @@ struct drm_i915_mocs_table {
>   static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
>   	[I915_MOCS_UNCACHED] = {
>   	  /* 0x00000009 */
> -	  .control_value = LE_CACHEABILITY(LE_UC) |
> -			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> -			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> -			   LE_PFM(0) | LE_SCF(0),
> -
> +	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC,
>   	  /* 0x0010 */
> -	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	  .l3cc_value =    L3_1_UC,
>   	},
>   	[I915_MOCS_PTE] = {
>   	  /* 0x00000038 */
> -	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
> -			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> -			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> -			   LE_PFM(0) | LE_SCF(0),
> +	  .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3),
>   	  /* 0x0030 */
> -	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	  .l3cc_value =    L3_3_WB,
>   	},
>   	[I915_MOCS_CACHED] = {
>   	  /* 0x0000003b */
> -	  .control_value = LE_CACHEABILITY(LE_WB) |
> -			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> -			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> -			   LE_PFM(0) | LE_SCF(0),
> +	  .control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
>   	  /* 0x0030 */
> -	  .l3cc_value =   L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	  .l3cc_value =   L3_3_WB,
>   	},
>   };
>   
> @@ -128,33 +118,21 @@ static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
>   static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
>   	[I915_MOCS_UNCACHED] = {
>   	  /* 0x00000009 */
> -	  .control_value = LE_CACHEABILITY(LE_UC) |
> -			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> -			   LE_LRUM(0) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> -			   LE_PFM(0) | LE_SCF(0),
> -
> +	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC,
>   	  /* 0x0010 */
> -	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_UC),
> +	  .l3cc_value = L3_1_UC,
>   	},
>   	[I915_MOCS_PTE] = {
>   	  /* 0x00000038 */
> -	  .control_value = LE_CACHEABILITY(LE_PAGETABLE) |
> -			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> -			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> -			   LE_PFM(0) | LE_SCF(0),
> -
> +	  .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3),
>   	  /* 0x0030 */
> -	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	  .l3cc_value = L3_3_WB,
>   	},
>   	[I915_MOCS_CACHED] = {
>   	  /* 0x00000039 */
> -	  .control_value = LE_CACHEABILITY(LE_UC) |
> -			   LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> -			   LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> -			   LE_PFM(0) | LE_SCF(0),
> -
> +	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
>   	  /* 0x0030 */
> -	  .l3cc_value =    L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> +	  .l3cc_value = L3_3_WB,
>   	},
>   };
>   

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

Re: [PATCH v8 3/7] drm/i915/skl: Rework MOCS tables to keep common part in a define

[Lis, Tomasz]

On 2019-01-22 06:12, Lucas De Marchi wrote:
> From: Tomasz Lis <tomasz.lis@intel.com>
>
> The MOCS tables are going to be very similar across platforms.
>
> To reduce the amount of copied code, this patch rips the common part and
> puts it into a definition valid for all gen9 platforms.
>
> v2: Made defines for or-ing flags. Renamed macros from MOCS_TABLE
>      to MOCS_ENTRIES. (Joonas)
> v3 (Lucas):
>    - Fix indentation
>    - Rebase on rework done by additional patch
>    - Remove define for or-ing flags as it made the table more complex by
>      requiring zeroed values to be passed
>    - Do not embed comma in the macro, so to treat that just as another
>      item and please source code formatting tools
>
> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
> Suggested-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Acked-by: Tomasz Lis <tomasz.lis@intel.com>
-Tomasz
> ---
>   drivers/gpu/drm/i915/intel_mocs.c | 57 ++++++++++++++-----------------
>   1 file changed, 25 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 4ea80bb7dcc8..c7a2a8d81d90 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -93,46 +93,39 @@ struct drm_i915_mocs_table {
>    *       may only be updated incrementally by adding entries at the
>    *       end.
>    */
> +
> +#define GEN9_MOCS_ENTRIES \
> +	[I915_MOCS_UNCACHED] = { \
> +		/* 0x00000009 */ \
> +		.control_value = LE_1_UC | LE_TC_2_LLC_ELLC, \
> +		/* 0x0010 */ \
> +		.l3cc_value = L3_1_UC, \
> +	}, \
> +	[I915_MOCS_PTE] = { \
> +		/* 0x00000038 */ \
> +		.control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \
> +		/* 0x0030 */ \
> +		.l3cc_value = L3_3_WB, \
> +	}
> +
>   static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
> -	[I915_MOCS_UNCACHED] = {
> -	  /* 0x00000009 */
> -	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC,
> -	  /* 0x0010 */
> -	  .l3cc_value =    L3_1_UC,
> -	},
> -	[I915_MOCS_PTE] = {
> -	  /* 0x00000038 */
> -	  .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> -	  /* 0x0030 */
> -	  .l3cc_value =    L3_3_WB,
> -	},
> +	GEN9_MOCS_ENTRIES,
>   	[I915_MOCS_CACHED] = {
> -	  /* 0x0000003b */
> -	  .control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> -	  /* 0x0030 */
> -	  .l3cc_value =   L3_3_WB,
> +		/* 0x0000003b */
> +		.control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> +		/* 0x0030 */
> +		.l3cc_value =   L3_3_WB,
>   	},
>   };
>   
>   /* NOTE: the LE_TGT_CACHE is not used on Broxton */
>   static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
> -	[I915_MOCS_UNCACHED] = {
> -	  /* 0x00000009 */
> -	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC,
> -	  /* 0x0010 */
> -	  .l3cc_value = L3_1_UC,
> -	},
> -	[I915_MOCS_PTE] = {
> -	  /* 0x00000038 */
> -	  .control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> -	  /* 0x0030 */
> -	  .l3cc_value = L3_3_WB,
> -	},
> +	GEN9_MOCS_ENTRIES,
>   	[I915_MOCS_CACHED] = {
> -	  /* 0x00000039 */
> -	  .control_value = LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> -	  /* 0x0030 */
> -	  .l3cc_value = L3_3_WB,
> +		/* 0x00000039 */
> +		.control_value = LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> +		/* 0x0030 */
> +		.l3cc_value = L3_3_WB,
>   	},
>   };
>   

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

Re: [PATCH v8 1/7] drm/i915: initialize unused MOCS entries to PTE

[Lucas De Marchi]

On Wed, Jan 23, 2019 at 07:33:35PM +0100, Tomasz Lis wrote:
>
>
>On 2019-01-22 06:12, Lucas De Marchi wrote:
>>Instead of initializing them to uncached, let's set them to PTE for
>>kernel tracking. While at it do some minor adjustments to comments and
>>coding style.
>>
>>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>

thanks

>One comment (with no expectations for change) below.

>>+		*cs++ = table->table[I915_MOCS_PTE].control_value;
>Entries from enum i915_mocs_table_index are not guaranteed to mean the 
>same thing in future gens;
>but for the time, that will work. And later it might still work, we 
>don't know.


I thought about this, but these values are part of the kernel API
(the same thing could be said for the first entry, btw).

If/when they don't make sense anymore we would need to remap them to
entry that makes sense.

Lucas De Marchi

>-Tomasz
>>  	}
>>  	*cs++ = MI_NOOP;
>>@@ -345,7 +336,7 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
>>  	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2);
>>-	for (i = 0; i < table->size/2; i++) {
>>+	for (i = 0; i < table->size / 2; i++) {
>>  		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
>>  		*cs++ = l3cc_combine(table, 2 * i, 2 * i + 1);
>>  	}
>>@@ -353,18 +344,18 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
>>  	if (table->size & 0x01) {
>>  		/* Odd table size - 1 left over */
>>  		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
>>-		*cs++ = l3cc_combine(table, 2 * i, 0);
>>+		*cs++ = l3cc_combine(table, 2 * i, I915_MOCS_PTE);
>>  		i++;
>>  	}
>>  	/*
>>-	 * Now set the rest of the table to uncached - use entry 0 as
>>-	 * this will be uncached. Leave the last pair uninitialised as
>>-	 * they are reserved by the hardware.
>>+	 * Now set the unused entries to PTE. These entries are officially
>>+	 * undefined and no contract for the contents and settings is given
>>+	 * for these entries.
>>  	 */
>>  	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
>>  		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
>>-		*cs++ = l3cc_combine(table, 0, 0);
>>+		*cs++ = l3cc_combine(table, I915_MOCS_PTE, I915_MOCS_PTE);
>>  	}
>>  	*cs++ = MI_NOOP;
>>@@ -395,22 +386,21 @@ void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv)
>>  	if (!get_mocs_settings(dev_priv, &table))
>>  		return;
>>-	for (i = 0; i < table.size/2; i++)
>>-		I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 2*i, 2*i+1));
>>+	for (i = 0; i < table.size / 2; i++)
>>+		I915_WRITE(GEN9_LNCFCMOCS(i),
>>+			   l3cc_combine(&table, 2 * i, 2 * i + 1));
>>  	/* Odd table size - 1 left over */
>>  	if (table.size & 0x01) {
>>-		I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 2*i, 0));
>>+		I915_WRITE(GEN9_LNCFCMOCS(i),
>>+			   l3cc_combine(&table, 2 * i, I915_MOCS_PTE));
>>  		i++;
>>  	}
>>-	/*
>>-	 * Now set the rest of the table to uncached - use entry 0 as
>>-	 * this will be uncached. Leave the last pair as initialised as
>>-	 * they are reserved by the hardware.
>>-	 */
>>+	/* Now set the rest of the table to PTE */
>>  	for (; i < (GEN9_NUM_MOCS_ENTRIES / 2); i++)
>>-		I915_WRITE(GEN9_LNCFCMOCS(i), l3cc_combine(&table, 0, 0));
>>+		I915_WRITE(GEN9_LNCFCMOCS(i),
>>+			   l3cc_combine(&table, I915_MOCS_PTE, I915_MOCS_PTE));
>>  }
>>  /**
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 4/7] drm/i915: use a macro to define MOCS entries

[Lis, Tomasz]

On 2019-01-22 06:12, Lucas De Marchi wrote:
> Let's use a macro to make tables smaller and at the same time allow us
> to add fields that apply to all entries in future.
>
> For the sake of readability, I'm calling an exception on 80 chars limit.
> Lines are aligned for easy comparison of the entry values.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_mocs.c | 39 +++++++++++--------------------
>   1 file changed, 14 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index c7a2a8d81d90..faae2eefc5cc 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -71,6 +71,12 @@ struct drm_i915_mocs_table {
>   #define L3_2_RESERVED		_L3_CACHEABILITY(2)
>   #define L3_3_WB			_L3_CACHEABILITY(3)
>   
> +#define MOCS_ENTRY(__idx, __control_value, __l3cc_value) \
> +	[__idx] = { \
> +		.control_value = __control_value, \
> +		.l3cc_value = __l3cc_value, \
> +	}
> +
>   /*
>    * MOCS tables
>    *
> @@ -93,40 +99,23 @@ struct drm_i915_mocs_table {
>    *       may only be updated incrementally by adding entries at the
>    *       end.
>    */
> -
The idea behind this EOL was that the comment above relates to several 
statements below, not just the first one.
But I'm not really sure what our commenting rules are in this case - a 
comment which bundles several definitions.
-Tomasz
>   #define GEN9_MOCS_ENTRIES \
> -	[I915_MOCS_UNCACHED] = { \
> -		/* 0x00000009 */ \
> -		.control_value = LE_1_UC | LE_TC_2_LLC_ELLC, \
> -		/* 0x0010 */ \
> -		.l3cc_value = L3_1_UC, \
> -	}, \
> -	[I915_MOCS_PTE] = { \
> -		/* 0x00000038 */ \
> -		.control_value = LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \
> -		/* 0x0030 */ \
> -		.l3cc_value = L3_3_WB, \
> -	}
> +	MOCS_ENTRY(I915_MOCS_UNCACHED,	LE_1_UC | LE_TC_2_LLC_ELLC, \
> +					L3_1_UC), \
> +	MOCS_ENTRY(I915_MOCS_PTE,	LE_0_PAGETABLE | LE_TC_2_LLC_ELLC | LE_LRUM(3), \
> +					L3_3_WB) \
>   
>   static const struct drm_i915_mocs_entry skylake_mocs_table[] = {
>   	GEN9_MOCS_ENTRIES,
> -	[I915_MOCS_CACHED] = {
> -		/* 0x0000003b */
> -		.control_value = LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> -		/* 0x0030 */
> -		.l3cc_value =   L3_3_WB,
> -	},
> +	MOCS_ENTRY(I915_MOCS_CACHED,	LE_3_WB | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> +					L3_3_WB)
>   };
>   
>   /* NOTE: the LE_TGT_CACHE is not used on Broxton */
>   static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
>   	GEN9_MOCS_ENTRIES,
> -	[I915_MOCS_CACHED] = {
> -		/* 0x00000039 */
> -		.control_value = LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> -		/* 0x0030 */
> -		.l3cc_value = L3_3_WB,
> -	},
> +	MOCS_ENTRY(I915_MOCS_CACHED,	LE_1_UC | LE_TC_2_LLC_ELLC | LE_LRUM(3),
> +					L3_3_WB)
>   };
>   
>   /**

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

Re: [PATCH v8 5/7] drm/i915: keep track of used entries in MOCS table

[Lis, Tomasz]

On 2019-01-22 06:12, Lucas De Marchi wrote:
> Instead of considering we have defined entries for any index in the
> table, let's keep track of the ones we explicitly defined. This will
> allow Gen 11 to have it's new table defined in which we have holes of
> undefined entries.
>
> Repeated comments about the meaning of undefined entries were removed
> since they are overly verbose and copy-pasted in several functions: now
> the definition is in the top only.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>
-Tomasz
> ---
>   drivers/gpu/drm/i915/intel_mocs.c | 88 ++++++++++++++++++++-----------
>   1 file changed, 57 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index faae2eefc5cc..af2ae2f396ae 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -28,6 +28,7 @@
>   struct drm_i915_mocs_entry {
>   	u32 control_value;
>   	u16 l3cc_value;
> +	u16 used;
>   };
>   
>   struct drm_i915_mocs_table {
> @@ -75,6 +76,7 @@ struct drm_i915_mocs_table {
>   	[__idx] = { \
>   		.control_value = __control_value, \
>   		.l3cc_value = __l3cc_value, \
> +		.used = 1, \
>   	}
>   
>   /*
> @@ -195,24 +197,26 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine)
>   	struct drm_i915_private *dev_priv = engine->i915;
>   	struct drm_i915_mocs_table table;
>   	unsigned int index;
> +	u32 unused_value;
>   
>   	if (!get_mocs_settings(dev_priv, &table))
>   		return;
>   
>   	GEM_BUG_ON(table.size > GEN9_NUM_MOCS_ENTRIES);
>   
> -	for (index = 0; index < table.size; index++)
> -		I915_WRITE(mocs_register(engine->id, index),
> -			   table.table[index].control_value);
> +	/* Set unused values to PTE */
> +	unused_value = table.table[I915_MOCS_PTE].control_value;
>   
> -	/*
> -	 * Now set the unused entries to PTE. These entries are officially
> -	 * undefined and no contract for the contents and settings is given
> -	 * for these entries.
> -	 */
> +	for (index = 0; index < table.size; index++) {
> +		u32 value = table.table[index].used ?
> +			table.table[index].control_value : unused_value;
> +
> +		I915_WRITE(mocs_register(engine->id, index), value);
> +	}
> +
> +	/* All remaining entries are also unused */
>   	for (; index < GEN9_NUM_MOCS_ENTRIES; index++)
> -		I915_WRITE(mocs_register(engine->id, index),
> -			   table.table[I915_MOCS_PTE].control_value);
> +		I915_WRITE(mocs_register(engine->id, index), unused_value);
>   }
>   
>   /**
> @@ -230,11 +234,15 @@ static int emit_mocs_control_table(struct i915_request *rq,
>   {
>   	enum intel_engine_id engine = rq->engine->id;
>   	unsigned int index;
> +	u32 unused_value;
>   	u32 *cs;
>   
>   	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
>   		return -ENODEV;
>   
> +	/* Set unused values to PTE */
> +	unused_value = table->table[I915_MOCS_PTE].control_value;
> +
>   	cs = intel_ring_begin(rq, 2 + 2 * GEN9_NUM_MOCS_ENTRIES);
>   	if (IS_ERR(cs))
>   		return PTR_ERR(cs);
> @@ -242,18 +250,17 @@ static int emit_mocs_control_table(struct i915_request *rq,
>   	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES);
>   
>   	for (index = 0; index < table->size; index++) {
> +		u32 value = table->table[index].used ?
> +			table->table[index].control_value : unused_value;
> +
>   		*cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
> -		*cs++ = table->table[index].control_value;
> +		*cs++ = value;
>   	}
>   
> -	/*
> -	 * Now set the unused entries to PTE. These entries are officially
> -	 * undefined and no contract for the contents and settings is given
> -	 * for these entries.
> -	 */
> +	/* All remaining entries are also unused */
>   	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
>   		*cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
> -		*cs++ = table->table[I915_MOCS_PTE].control_value;
> +		*cs++ = unused_value;
>   	}
>   
>   	*cs++ = MI_NOOP;
> @@ -284,12 +291,15 @@ static inline u32 l3cc_combine(const struct drm_i915_mocs_table *table,
>   static int emit_mocs_l3cc_table(struct i915_request *rq,
>   				const struct drm_i915_mocs_table *table)
>   {
> -	unsigned int i;
> +	unsigned int i, unused_index;
>   	u32 *cs;
>   
>   	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
>   		return -ENODEV;
>   
> +	/* Set unused values to PTE */
> +	unused_index = I915_MOCS_PTE;
> +
>   	cs = intel_ring_begin(rq, 2 + GEN9_NUM_MOCS_ENTRIES);
>   	if (IS_ERR(cs))
>   		return PTR_ERR(cs);
> @@ -297,25 +307,29 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
>   	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2);
>   
>   	for (i = 0; i < table->size / 2; i++) {
> +		u16 low = table->table[2 * i].used ?
> +			2 * i : unused_index;
> +		u16 high = table->table[2 * i + 1].used ?
> +			2 * i + 1 : unused_index;
> +
>   		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
> -		*cs++ = l3cc_combine(table, 2 * i, 2 * i + 1);
> +		*cs++ = l3cc_combine(table, low, high);
>   	}
>   
>   	if (table->size & 0x01) {
> +		u16 low = table->table[2 * i].used ?
> +			2 * i : unused_index;
> +
>   		/* Odd table size - 1 left over */
>   		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
> -		*cs++ = l3cc_combine(table, 2 * i, I915_MOCS_PTE);
> +		*cs++ = l3cc_combine(table, low, unused_index);
>   		i++;
>   	}
>   
> -	/*
> -	 * Now set the unused entries to PTE. These entries are officially
> -	 * undefined and no contract for the contents and settings is given
> -	 * for these entries.
> -	 */
> +	/* All remaining entries are also unused */
>   	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
>   		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
> -		*cs++ = l3cc_combine(table, I915_MOCS_PTE, I915_MOCS_PTE);
> +		*cs++ = l3cc_combine(table, unused_index, unused_index);
>   	}
>   
>   	*cs++ = MI_NOOP;
> @@ -341,26 +355,38 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
>   void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv)
>   {
>   	struct drm_i915_mocs_table table;
> -	unsigned int i;
> +	unsigned int i, unused_index;
>   
>   	if (!get_mocs_settings(dev_priv, &table))
>   		return;
>   
> -	for (i = 0; i < table.size / 2; i++)
> +	/* Set unused values to PTE */
> +	unused_index = I915_MOCS_PTE;
> +
> +	for (i = 0; i < table.size / 2; i++) {
> +		u16 low = table.table[2 * i].used ?
> +			2 * i : unused_index;
> +		u16 high = table.table[2 * i + 1].used ?
> +			2 * i + 1 : unused_index;
> +
>   		I915_WRITE(GEN9_LNCFCMOCS(i),
> -			   l3cc_combine(&table, 2 * i, 2 * i + 1));
> +			   l3cc_combine(&table, low, high));
> +	}
>   
>   	/* Odd table size - 1 left over */
>   	if (table.size & 0x01) {
> +		u16 low = table.table[2 * i].used ?
> +			2 * i : unused_index;
> +
>   		I915_WRITE(GEN9_LNCFCMOCS(i),
> -			   l3cc_combine(&table, 2 * i, I915_MOCS_PTE));
> +			   l3cc_combine(&table, low, unused_index));
>   		i++;
>   	}
>   
>   	/* Now set the rest of the table to PTE */
>   	for (; i < (GEN9_NUM_MOCS_ENTRIES / 2); i++)
>   		I915_WRITE(GEN9_LNCFCMOCS(i),
> -			   l3cc_combine(&table, I915_MOCS_PTE, I915_MOCS_PTE));
> +			   l3cc_combine(&table, unused_index, unused_index));
>   }
>   
>   /**

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

Re: [PATCH v8 4/7] drm/i915: use a macro to define MOCS entries

[Lucas De Marchi]

On Tue, Jan 22, 2019 at 09:37:02PM +0000, Chris Wilson wrote:
>Quoting Lucas De Marchi (2019-01-22 21:33:25)
>> On Tue, Jan 22, 2019 at 6:32 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> >
>> > Quoting Lucas De Marchi (2019-01-22 05:12:24)
>> > > Let's use a macro to make tables smaller and at the same time allow us
>> > > to add fields that apply to all entries in future.
>> > >
>> > > For the sake of readability, I'm calling an exception on 80 chars limit.
>> > > Lines are aligned for easy comparison of the entry values.
>> >
>> > > +       MOCS_ENTRY(I915_MOCS_UNCACHED,  LE_1_UC | LE_TC_2_LLC_ELLC, \
>> > > +                                       L3_1_UC), \
>> >
>> >           MOCS_ENTRY(I915_MOCS_UNCACHED,
>> >                      LE_1_UC | LE_TC_2_LLC_ELLC, L3_1_UC), \
>>
>> My intention was to split the lines for each *value*, so it's easy to
>> see what control_value vs l3cc_value is set to
>> (too difficult to spot mistakes on adding a comma rather than a |).
>>
>> But I'm not strongly against your version, so I'll switch to that.
>
>Have another new line :)
>
>Because you are right as I confused that \ for a |.
>
>           MOCS_ENTRY(I915_MOCS_UNCACHED, \
>                      LE_1_UC | LE_TC_2_LLC_ELLC, \
>		      L3_1_UC), \

ok. The Ice Lake table is huge (see last patch) and this will mandate 3
lines per entry, but at least it will be clear.

Lucas De Marchi

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

Re: [PATCH v8 6/7] drm/i915: cache number of MOCS entries

[Lis, Tomasz]

On 2019-01-22 06:12, Lucas De Marchi wrote:
> Instead of checking the gen number every time we need to know the max
> number of entries, just save it into the table struct so we don't need
> extra branches throughout the code. This will be useful for Ice Lake
> that has 64 rather than 62 defined entries. Ice Lake changes will be
> added in a follow up.
>
> v2: make size and n_entries `unsigned int` and introduce changes as a
>      pre-work for the Ice Lake changes (Tvrtko)
>
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
I would name it total_entries or n_hw_entries, not n_entries; but that's 
just a name, so:
Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>
-Tomasz
> ---
>   drivers/gpu/drm/i915/intel_mocs.c | 27 ++++++++++++++-------------
>   1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index af2ae2f396ae..716f3f6f2966 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -32,7 +32,8 @@ struct drm_i915_mocs_entry {
>   };
>   
>   struct drm_i915_mocs_table {
> -	u32 size;
> +	unsigned int size;
> +	unsigned int n_entries;
>   	const struct drm_i915_mocs_entry *table;
>   };
>   
> @@ -140,10 +141,12 @@ static bool get_mocs_settings(struct drm_i915_private *dev_priv,
>   	if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv) ||
>   	    IS_ICELAKE(dev_priv)) {
>   		table->size  = ARRAY_SIZE(skylake_mocs_table);
> +		table->n_entries = GEN9_NUM_MOCS_ENTRIES;
>   		table->table = skylake_mocs_table;
>   		result = true;
>   	} else if (IS_GEN9_LP(dev_priv)) {
>   		table->size  = ARRAY_SIZE(broxton_mocs_table);
> +		table->n_entries = GEN9_NUM_MOCS_ENTRIES;
>   		table->table = broxton_mocs_table;
>   		result = true;
>   	} else {
> @@ -202,8 +205,6 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine)
>   	if (!get_mocs_settings(dev_priv, &table))
>   		return;
>   
> -	GEM_BUG_ON(table.size > GEN9_NUM_MOCS_ENTRIES);
> -
>   	/* Set unused values to PTE */
>   	unused_value = table.table[I915_MOCS_PTE].control_value;
>   
> @@ -215,7 +216,7 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine)
>   	}
>   
>   	/* All remaining entries are also unused */
> -	for (; index < GEN9_NUM_MOCS_ENTRIES; index++)
> +	for (; index < table.n_entries; index++)
>   		I915_WRITE(mocs_register(engine->id, index), unused_value);
>   }
>   
> @@ -237,17 +238,17 @@ static int emit_mocs_control_table(struct i915_request *rq,
>   	u32 unused_value;
>   	u32 *cs;
>   
> -	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
> +	if (GEM_WARN_ON(table->size > table->n_entries))
>   		return -ENODEV;
>   
>   	/* Set unused values to PTE */
>   	unused_value = table->table[I915_MOCS_PTE].control_value;
>   
> -	cs = intel_ring_begin(rq, 2 + 2 * GEN9_NUM_MOCS_ENTRIES);
> +	cs = intel_ring_begin(rq, 2 + 2 * table->n_entries);
>   	if (IS_ERR(cs))
>   		return PTR_ERR(cs);
>   
> -	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES);
> +	*cs++ = MI_LOAD_REGISTER_IMM(table->n_entries);
>   
>   	for (index = 0; index < table->size; index++) {
>   		u32 value = table->table[index].used ?
> @@ -258,7 +259,7 @@ static int emit_mocs_control_table(struct i915_request *rq,
>   	}
>   
>   	/* All remaining entries are also unused */
> -	for (; index < GEN9_NUM_MOCS_ENTRIES; index++) {
> +	for (; index < table->n_entries; index++) {
>   		*cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
>   		*cs++ = unused_value;
>   	}
> @@ -294,17 +295,17 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
>   	unsigned int i, unused_index;
>   	u32 *cs;
>   
> -	if (WARN_ON(table->size > GEN9_NUM_MOCS_ENTRIES))
> +	if (GEM_WARN_ON(table->size > table->n_entries))
>   		return -ENODEV;
>   
>   	/* Set unused values to PTE */
>   	unused_index = I915_MOCS_PTE;
>   
> -	cs = intel_ring_begin(rq, 2 + GEN9_NUM_MOCS_ENTRIES);
> +	cs = intel_ring_begin(rq, 2 + table->n_entries);
>   	if (IS_ERR(cs))
>   		return PTR_ERR(cs);
>   
> -	*cs++ = MI_LOAD_REGISTER_IMM(GEN9_NUM_MOCS_ENTRIES / 2);
> +	*cs++ = MI_LOAD_REGISTER_IMM(table->n_entries / 2);
>   
>   	for (i = 0; i < table->size / 2; i++) {
>   		u16 low = table->table[2 * i].used ?
> @@ -327,7 +328,7 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
>   	}
>   
>   	/* All remaining entries are also unused */
> -	for (; i < GEN9_NUM_MOCS_ENTRIES / 2; i++) {
> +	for (; i < table->n_entries / 2; i++) {
>   		*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(i));
>   		*cs++ = l3cc_combine(table, unused_index, unused_index);
>   	}
> @@ -384,7 +385,7 @@ void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv)
>   	}
>   
>   	/* Now set the rest of the table to PTE */
> -	for (; i < (GEN9_NUM_MOCS_ENTRIES / 2); i++)
> +	for (; i < table.n_entries / 2; i++)
>   		I915_WRITE(GEN9_LNCFCMOCS(i),
>   			   l3cc_combine(&table, unused_index, unused_index));
>   }

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

Re: [PATCH v8 7/7] drm/i915/icl: Define MOCS table for Icelake

[Lis, Tomasz]

On 2019-01-22 06:12, Lucas De Marchi wrote:
> From: Tomasz Lis <tomasz.lis@intel.com>
>
> The table has been unified across OSes to minimize virtualization overhead.
>
> The MOCS table is now published as part of bspec, and versioned. Entries
> are supposed to never be modified, but new ones can be added. Adding
> entries increases table version. The patch includes version 1 entries.
>
> Meaning of each entry is now explained in bspec, and user mode clients
> are expected to know what each entry means. The 3 entries used for previous
> platforms are still compatible with their legacy definitions, but that is
> not guaranteed to be true for future platforms.
>
> v2: Fixed SCC values, improved commit comment (Daniele)
> v3: Improved MOCS table comment (Daniele)
> v4: Moved new entries below gen9 ones. Put common entries into
>      definition to be used in multiple arrays. (Lucas)
> v5: Made defines for or-ing flags. Renamed macros from MOCS_TABLE
>      to MOCS_ENTRIES. Switched LE_CoS to upper case. (Joonas)
> v6: Removed definitions of reserved entries. (Michal)
>      Increased limit of entries sent to the hardware on gen11+.
> v7: Simplify table as done for previou gens (Lucas)
> v8: Rebase on cached number of entries per-platform and use new
>      MOCS_ENTRY() macro (Lucas)
>
> BSpec: 34007
> BSpec: 560
>
> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Acked-by: Tomasz Lis <tomasz.lis@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_mocs.c | 107 +++++++++++++++++++++++++++---
>   1 file changed, 98 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 716f3f6f2966..3afd8c30cacc 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -46,6 +46,8 @@ struct drm_i915_mocs_table {
>   #define LE_SCC(value)		((value) << 8)
>   #define LE_PFM(value)		((value) << 11)
>   #define LE_SCF(value)		((value) << 14)
> +#define LE_COS(value)		((value) << 15)
> +#define LE_SSE(value)		((value) << 17)
>   
>   /* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word */
>   #define L3_ESC(value)		((value) << 0)
> @@ -54,6 +56,7 @@ struct drm_i915_mocs_table {
>   
>   /* Helper defines */
>   #define GEN9_NUM_MOCS_ENTRIES	62  /* 62 out of 64 - 63 & 64 are reserved. */
> +#define GEN11_NUM_MOCS_ENTRIES	64  /* 63-64 are reserved, but configured. */
>   
>   /* (e)LLC caching options */
>   #define LE_0_PAGETABLE		_LE_CACHEABILITY(0)
> @@ -89,18 +92,22 @@ struct drm_i915_mocs_table {
>    * LNCFCMOCS0 - LNCFCMOCS32 registers.
>    *
>    * These tables are intended to be kept reasonably consistent across
> - * platforms. However some of the fields are not applicable to all of
> - * them.
> + * HW platforms, and for ICL+, be identical across OSes. To achieve
> + * that, for Icelake and above, list of entries is published as part
> + * of bspec.
>    *
>    * Entries not part of the following tables are undefined as far as
>    * userspace is concerned and shouldn't be relied upon.  For the time
>    * being they will be initialized to PTE.
>    *
> - * NOTE: These tables MUST start with being uncached and the length
> - *       MUST be less than 63 as the last two registers are reserved
> - *       by the hardware.  These tables are part of the kernel ABI and
> - *       may only be updated incrementally by adding entries at the
> - *       end.
> + * The last two entries are reserved by the hardware. For ICL+ they
> + * should be initialized according to bspec and never used, for older
> + * platforms they should never be written to.
> + *
> + * NOTE: These tables are part of bspec and defined as part of hardware
> + *       interface for ICL+. For older platforms, they are part of kernel
> + *       ABI. It is expected that existing entries will remain constant
> + *       and the tables will only be updated by adding new entries.
We have no guarantee that the entries will remain constant across gens.. 
so maybe:

+ *       ABI. It is expected that, for specific hardware platform, existing
+ *       entries will remain constant and the table will only be updated
+ *       by adding new entries, filling unused positions.

-Tomasz
>    */
>   #define GEN9_MOCS_ENTRIES \
>   	MOCS_ENTRY(I915_MOCS_UNCACHED,	LE_1_UC | LE_TC_2_LLC_ELLC, \
> @@ -121,6 +128,84 @@ static const struct drm_i915_mocs_entry broxton_mocs_table[] = {
>   					L3_3_WB)
>   };
>   
> +#define GEN11_MOCS_ENTRIES \
> +	/* Base - Uncached (Deprecated) */ \
> +	MOCS_ENTRY(I915_MOCS_UNCACHED,	LE_1_UC | LE_TC_1_LLC, \
> +					L3_1_UC), \
> +	/* Base - L3 + LeCC:PAT (Deprecated) */ \
> +	MOCS_ENTRY(I915_MOCS_PTE,	LE_0_PAGETABLE | LE_TC_1_LLC, \
> +					L3_3_WB), \
> +	/* Base - L3 + LLC */ \
> +	MOCS_ENTRY(2,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
> +			L3_3_WB), \
> +	/* Base - Uncached */ \
> +	MOCS_ENTRY(3,	LE_1_UC | LE_TC_1_LLC, \
> +			L3_1_UC), \
> +	/* Base - L3 */ \
> +	MOCS_ENTRY(4,	LE_1_UC | LE_TC_1_LLC, \
> +			L3_3_WB), \
> +	/* Base - LLC */ \
> +	MOCS_ENTRY(5,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
> +			L3_1_UC), \
> +	/* Age 0 - LLC */ \
> +	MOCS_ENTRY(6,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1), \
> +			L3_1_UC), \
> +	/* Age 0 - L3 + LLC */ \
> +	MOCS_ENTRY(7,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1), \
> +			L3_3_WB), \
> +	/* Age: Don't Chg. - LLC */ \
> +	MOCS_ENTRY(8,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2), \
> +			L3_1_UC), \
> +	/* Age: Don't Chg. - L3 + LLC */ \
> +	MOCS_ENTRY(9,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2), \
> +			L3_3_WB), \
> +	/* No AOM - LLC */ \
> +	MOCS_ENTRY(10,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_AOM(1), \
> +			L3_1_UC), \
> +	/* No AOM - L3 + LLC */ \
> +	MOCS_ENTRY(11,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_AOM(1), \
> +			L3_3_WB), \
> +	/* No AOM; Age 0 - LLC */ \
> +	MOCS_ENTRY(12,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1) | LE_AOM(1), \
> +			L3_1_UC), \
> +	/* No AOM; Age 0 - L3 + LLC */ \
> +	MOCS_ENTRY(13,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(1) | LE_AOM(1), \
> +			L3_3_WB), \
> +	/* No AOM; Age:DC - LLC */ \
> +	MOCS_ENTRY(14,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2) | LE_AOM(1), \
> +			L3_1_UC), \
> +	/* No AOM; Age:DC - L3 + LLC */ \
> +	MOCS_ENTRY(15,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(2) | LE_AOM(1), \
> +			L3_3_WB), \
> +	/* Self-Snoop - L3 + LLC */ \
> +	MOCS_ENTRY(18,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SSE(3), \
> +			L3_3_WB), \
> +	/* Skip Caching - L3 + LLC(12.5%) */ \
> +	MOCS_ENTRY(19,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SCC(7), \
> +			L3_3_WB), \
> +	/* Skip Caching - L3 + LLC(25%) */ \
> +	MOCS_ENTRY(20,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SCC(3), \
> +			L3_3_WB), \
> +	/* Skip Caching - L3 + LLC(50%) */ \
> +	MOCS_ENTRY(21,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_SCC(1), \
> +			L3_3_WB), \
> +	/* Skip Caching - L3 + LLC(75%) */ \
> +	MOCS_ENTRY(22,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_RSC(1) | LE_SCC(3), \
> +			L3_3_WB), \
> +	/* Skip Caching - L3 + LLC(87.5%) */ \
> +	MOCS_ENTRY(23,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3) | LE_RSC(1) | LE_SCC(7), \
> +			L3_3_WB), \
> +	/* HW Reserved - SW program but never use */ \
> +	MOCS_ENTRY(62,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
> +			L3_1_UC), \
> +	/* HW Reserved - SW program but never use */ \
> +	MOCS_ENTRY(63,	LE_3_WB | LE_TC_1_LLC | LE_LRUM(3), \
> +			L3_1_UC), \
> +
> +static const struct drm_i915_mocs_entry icelake_mocs_table[] = {
> +	GEN11_MOCS_ENTRIES
> +};
> +
>   /**
>    * get_mocs_settings()
>    * @dev_priv:	i915 device.
> @@ -138,8 +223,12 @@ static bool get_mocs_settings(struct drm_i915_private *dev_priv,
>   {
>   	bool result = false;
>   
> -	if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv) ||
> -	    IS_ICELAKE(dev_priv)) {
> +	if (IS_ICELAKE(dev_priv)) {
> +		table->size  = ARRAY_SIZE(icelake_mocs_table);
> +		table->table = icelake_mocs_table;
> +		table->n_entries = GEN11_NUM_MOCS_ENTRIES;
> +		result = true;
> +	} else if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv)) {
>   		table->size  = ARRAY_SIZE(skylake_mocs_table);
>   		table->n_entries = GEN9_NUM_MOCS_ENTRIES;
>   		table->table = skylake_mocs_table;

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

Re: [PATCH v8 5/7] drm/i915: keep track of used entries in MOCS table

[Lucas De Marchi]

On Tue, Jan 22, 2019 at 02:40:48PM +0000, Chris Wilson wrote:
>Quoting Lucas De Marchi (2019-01-22 05:12:25)
>> Instead of considering we have defined entries for any index in the
>> table, let's keep track of the ones we explicitly defined. This will
>> allow Gen 11 to have it's new table defined in which we have holes of
>> undefined entries.
>>
>> Repeated comments about the meaning of undefined entries were removed
>> since they are overly verbose and copy-pasted in several functions: now
>> the definition is in the top only.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_mocs.c | 88 ++++++++++++++++++++-----------
>>  1 file changed, 57 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
>> index faae2eefc5cc..af2ae2f396ae 100644
>> --- a/drivers/gpu/drm/i915/intel_mocs.c
>> +++ b/drivers/gpu/drm/i915/intel_mocs.c
>> @@ -28,6 +28,7 @@
>>  struct drm_i915_mocs_entry {
>>         u32 control_value;
>>         u16 l3cc_value;
>> +       u16 used;
>>  };
>
>> +       /* Set unused values to PTE */
>> +       unused_index = I915_MOCS_PTE;
>> +
>> +       for (i = 0; i < table.size / 2; i++) {
>> +               u16 low = table.table[2 * i].used ?
>> +                       2 * i : unused_index;
>> +               u16 high = table.table[2 * i + 1].used ?
>> +                       2 * i + 1 : unused_index;
>
>I'm underwhelmed here.

Indeed, I should have put more effort in making this readable :)

>
>Could we not do something like
>
>static unsigned int
>get_entry_index(struct tbl *tbl, unsigned int idx, unsigned int unused_index)
>{
>	return tbl->used ? idx : unused_index;
>}
>
>		u16 lo = get_entry_index(table.table, 2 * i, unused_index);
>		u16 hi = get_entry_index(table.table, 2 * i + 1, unused_index);
>
>That just fits and repeated enough to be worth a little extra effort.

in order to make the l3cc and control parts more similar, I did it like
this:

    static u16 get_entry_l3cc(const struct drm_i915_mocs_table *table,
    			  unsigned int index)
    {
    	if (table->table[index].used)
    		return table->table[index].l3cc_value;
    
    	return table->table[I915_MOCS_PTE].l3cc_value;
    }
    
    static u32 get_entry_control(const struct drm_i915_mocs_table *table,
    			     unsigned int index)
    {
    	if (table->table[index].used)
    		return table->table[index].control_value;
    
    	return table->table[I915_MOCS_PTE].control_value;
    }

Then on all of them use as value rather than as value in one and index
in the other (adapting l3cc_combine appropriately).

This series is conflicting now with drm-intel, so I will need to re-send
it entirely.

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