diff mbox series

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

Message ID 20190122051227.8329-2-lucas.demarchi@intel.com (mailing list archive)
State New, archived
Headers show
Series Define MOCS table for Icelake | expand

Commit Message

Lucas De Marchi Jan. 22, 2019, 5:12 a.m. UTC 
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(-)

Comments

Chris Wilson Jan. 22, 2019, 2:28 p.m. UTC | #1 
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
Lis, Tomasz Jan. 23, 2019, 6:33 p.m. UTC | #2 
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));
>   }
>   
>   /**
Lucas De Marchi Jan. 23, 2019, 6:39 p.m. UTC | #3 
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));
>>  }
>>  /**
>
diff mbox series

Patch

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));
 }
 
 /**