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

["Lis, Tomasz" <tomasz.lis@intel.com>]

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