All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] drm/i915: Set all undefined MOCS entries to follow PTE
@ 2017-05-04 15:02 ` David Weinehall
  0 siblings, 0 replies; 11+ messages in thread
From: David Weinehall @ 2017-05-04 15:02 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx, stable

On Thu, May 04, 2017 at 10:51:29AM +0100, Chris Wilson wrote:
> A good default for garbage entries from the user is to follow the
> default setting of the object (i.e. the PTE). Currently they use the
> uncached entry, and now the only way to accidentally hit uncached
> performance is via explicit use of the uncached MOCS or setting the
> object to uncached. Note that these entries are currently undefined in
> the ABI and we reserve the right to change them. We originally chose
> uncached to eliminate any problem with reducing the caching level in
> future, but the object is a much better definition of the minimum
> caching level.
> 
> Fixes: 3bbaba0ceaa2 ("drm/i915: Added Programming of the MOCS")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: stable@vger.kernel.org

LGTM, and passes our nightly msdk test case.

Tested-by: David Weinehall <david.weinehall@linux.intel.com>
Reviewed-by: David Weinehall <david.weinehall@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_mocs.c | 39 +++++++++++++++------------------------
>  1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 92e461c68385..e7a7781ca457 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 implicitly initialized to follow the 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 @@ int intel_mocs_init_engine(struct intel_engine_cs *engine)
>  			   table.table[index].control_value);
>  
>  	/*
> -	 * Ok, now set the unused entries to uncached. These entries
> +	 * Ok, now set the unused entries to follow the PTE. 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.
>  	 */
>  	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);
>  
>  	return 0;
>  }
> @@ -295,16 +289,13 @@ static int emit_mocs_control_table(struct drm_i915_gem_request *req,
>  	}
>  
>  	/*
> -	 * Ok, now set the unused entries to uncached. These entries
> +	 * Ok, now set the unused entries to follow the PTE. 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.
>  	 */
>  	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;
> @@ -355,18 +346,17 @@ static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
>  	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 rest of the table to follow the PTE.
> +	 * Leave the last pair as they are reserved by the hardware.
>  	 */
>  	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;
> @@ -402,17 +392,18 @@ void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv)
>  
>  	/* 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 follow the PTE.
> +	 * Leave the last pair as they are reserved by the hardware.
>  	 */
>  	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.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH] drm/i915: Set all undefined MOCS entries to follow PTE
@ 2017-05-04  9:51 Chris Wilson
  0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2017-05-04  9:51 UTC (permalink / raw)
  To: intel-gfx
  Cc: Chris Wilson, David Weinehall, Arkadiusz Hiler, Tvrtko Ursulin,
	stable

A good default for garbage entries from the user is to follow the
default setting of the object (i.e. the PTE). Currently they use the
uncached entry, and now the only way to accidentally hit uncached
performance is via explicit use of the uncached MOCS or setting the
object to uncached. Note that these entries are currently undefined in
the ABI and we reserve the right to change them. We originally chose
uncached to eliminate any problem with reducing the caching level in
future, but the object is a much better definition of the minimum
caching level.

Fixes: 3bbaba0ceaa2 ("drm/i915: Added Programming of the MOCS")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_mocs.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index 92e461c68385..e7a7781ca457 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 implicitly initialized to follow the 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 @@ int intel_mocs_init_engine(struct intel_engine_cs *engine)
 			   table.table[index].control_value);
 
 	/*
-	 * Ok, now set the unused entries to uncached. These entries
+	 * Ok, now set the unused entries to follow the PTE. 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.
 	 */
 	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);
 
 	return 0;
 }
@@ -295,16 +289,13 @@ static int emit_mocs_control_table(struct drm_i915_gem_request *req,
 	}
 
 	/*
-	 * Ok, now set the unused entries to uncached. These entries
+	 * Ok, now set the unused entries to follow the PTE. 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.
 	 */
 	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;
@@ -355,18 +346,17 @@ static int emit_mocs_l3cc_table(struct drm_i915_gem_request *req,
 	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 rest of the table to follow the PTE.
+	 * Leave the last pair as they are reserved by the hardware.
 	 */
 	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;
@@ -402,17 +392,18 @@ void intel_mocs_init_l3cc_table(struct drm_i915_private *dev_priv)
 
 	/* 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 follow the PTE.
+	 * Leave the last pair as they are reserved by the hardware.
 	 */
 	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.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-06-29  8:35 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-04 15:02 [PATCH] drm/i915: Set all undefined MOCS entries to follow PTE David Weinehall
2017-05-04 15:02 ` David Weinehall
2017-05-04 17:56 ` [Intel-gfx] " Francisco Jerez
2017-05-04 20:03   ` Chris Wilson
2017-05-04 20:59     ` Francisco Jerez
2017-06-28 10:19       ` Chris Wilson
2017-06-28 21:10         ` Francisco Jerez
2017-06-28 21:10           ` [Intel-gfx] " Francisco Jerez
2017-06-29  8:35         ` Daniel Vetter
2017-06-29  8:35           ` [Intel-gfx] " Daniel Vetter
  -- strict thread matches above, loose matches on Subject: below --
2017-05-04  9:51 Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.