Re: [PATCH 26/27] drm/i915/gt: Refactor mocs loops into single control macro

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> We repeatedly (and more so in future) use the same looping construct
> over the mocs definition table to setup the register state. Refactor the
> loop construct into a reusable macro.
>
> add/remove: 2/1 grow/shrink: 1/2 up/down: 113/-330 (-217)
> Function                                     old     new   delta
> intel_mocs_init_engine.cold                    -      71     +71
> offset                                         -      28     +28
> __func__                                   17273   17287     +14
> intel_mocs_init                              143     113     -30
> mocs_register.isra                            91       -     -91
> intel_mocs_init_engine                       503     294    -209
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_mocs.c | 128 ++++++++++-----------------
>  1 file changed, 47 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
> index e6f3f36a3988..63d0fdf67215 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> @@ -329,27 +329,6 @@ static bool get_mocs_settings(const struct drm_i915_private *i915,
>  	return true;
>  }
>  
> -static i915_reg_t mocs_register(const struct intel_engine_cs *engine, int index)
> -{
> -	switch (engine->id) {
> -	case RCS0:
> -		return GEN9_GFX_MOCS(index);
> -	case VCS0:
> -		return GEN9_MFX0_MOCS(index);
> -	case BCS0:
> -		return GEN9_BLT_MOCS(index);
> -	case VECS0:
> -		return GEN9_VEBOX_MOCS(index);
> -	case VCS1:
> -		return GEN9_MFX1_MOCS(index);
> -	case VCS2:
> -		return GEN11_MFX2_MOCS(index);
> -	default:
> -		MISSING_CASE(engine->id);
> -		return INVALID_MMIO_REG;
> -	}
> -}
> -
>  /*
>   * Get control_value from MOCS entry taking into account when it's not used:
>   * I915_MOCS_PTE's value is returned in this case.
> @@ -357,29 +336,47 @@ static i915_reg_t mocs_register(const struct intel_engine_cs *engine, int index)
>  static u32 get_entry_control(const struct drm_i915_mocs_table *table,
>  			     unsigned int index)
>  {
> -	if (table->table[index].used)
> +	if (index < table->size && table->table[index].used)
>  		return table->table[index].control_value;
>  
>  	return table->table[I915_MOCS_PTE].control_value;
>  }
>  
> -static void init_mocs_table(struct intel_engine_cs *engine,
> -			    const struct drm_i915_mocs_table *table)
> +#define for_each_mocs(mocs, t, i) \
> +	for (i = 0; \
> +	     i < t->n_entries ? (mocs = get_entry_control(t, i)), 1 : 0;\
> +	     i++)

checkpatch might complain about some param reusage.

> +
> +static void __init_mocs_table(struct intel_uncore *uncore,
> +			      const struct drm_i915_mocs_table *table,
> +			      u32 addr)
>  {
> -	struct intel_uncore *uncore = engine->uncore;
> -	u32 unused_value = table->table[I915_MOCS_PTE].control_value;
>  	unsigned int i;
> +	u32 mocs;
> +
> +	for_each_mocs(mocs, table, i)
> +		intel_uncore_write_fw(uncore, _MMIO(addr + i * 4), mocs);
> +}
>  
> -	for (i = 0; i < table->size; i++)
> -		intel_uncore_write_fw(uncore,
> -				      mocs_register(engine, i),
> -				      get_entry_control(table, i));
> +static u32 mocs_register(const struct intel_engine_cs *engine)
> +{
> +	static const u32 offset[] = {
> +		[RCS0]  = 0x0c800,
> +		[VCS0]  = 0x0c900,
> +		[VCS1]  = 0x0ca00,
> +		[VECS0] = 0x0cb00,
> +		[BCS0]  = 0x0cc00,
> +		[VCS2]  = 0x10000,

Someone might complain about the lack of defines. Remove
the unused defines or change them to work without index.

> +	};
> +
> +	GEM_BUG_ON(engine->id > ARRAY_SIZE(offset));

engine->id >= ARRAY_SIZE(offset);

Also the comments throughout the file need massaging
after this change.

These 3 mocs patches looks good otherwise.

Please update resend as a separate series.

-Mika

> +	return offset[engine->id];
> +}
>  
> -	/* All remaining entries are unused */
> -	for (; i < table->n_entries; i++)
> -		intel_uncore_write_fw(uncore,
> -				      mocs_register(engine, i),
> -				      unused_value);
> +static void init_mocs_table(struct intel_engine_cs *engine,
> +			    const struct drm_i915_mocs_table *table)
> +{
> +	__init_mocs_table(engine->uncore, table, mocs_register(engine));
>  }
>  
>  /*
> @@ -389,7 +386,7 @@ static void init_mocs_table(struct intel_engine_cs *engine,
>  static u16 get_entry_l3cc(const struct drm_i915_mocs_table *table,
>  			  unsigned int index)
>  {
> -	if (table->table[index].used)
> +	if (index < table->size && table->table[index].used)
>  		return table->table[index].l3cc_value;
>  
>  	return table->table[I915_MOCS_PTE].l3cc_value;
> @@ -400,37 +397,23 @@ static inline u32 l3cc_combine(u16 low, u16 high)
>  	return low | (u32)high << 16;
>  }
>  
> +#define for_each_l3cc(l3cc, t, i) \
> +	for (i = 0; \
> +	     i < (t->n_entries + 1) / 2 ? \
> +	     (l3cc = l3cc_combine(get_entry_l3cc(t, 2 * i), \
> +				  get_entry_l3cc(t, 2 * i + 1))), 1 : \
> +	     0; \
> +	     i++)
> +
>  static void init_l3cc_table(struct intel_engine_cs *engine,
>  			    const struct drm_i915_mocs_table *table)
>  {
>  	struct intel_uncore *uncore = engine->uncore;
> -	u16 unused_value = table->table[I915_MOCS_PTE].l3cc_value;
>  	unsigned int i;
> +	u32 l3cc;
>  
> -	for (i = 0; i < table->size / 2; i++) {
> -		u16 low = get_entry_l3cc(table, 2 * i);
> -		u16 high = get_entry_l3cc(table, 2 * i + 1);
> -
> -		intel_uncore_write(uncore,
> -				   GEN9_LNCFCMOCS(i),
> -				   l3cc_combine(low, high));
> -	}
> -
> -	/* Odd table size - 1 left over */
> -	if (table->size & 1) {
> -		u16 low = get_entry_l3cc(table, 2 * i);
> -
> -		intel_uncore_write(uncore,
> -				   GEN9_LNCFCMOCS(i),
> -				   l3cc_combine(low, unused_value));
> -		i++;
> -	}
> -
> -	/* All remaining entries are also unused */
> -	for (; i < table->n_entries / 2; i++)
> -		intel_uncore_write(uncore,
> -				   GEN9_LNCFCMOCS(i),
> -				   l3cc_combine(unused_value, unused_value));
> +	for_each_l3cc(l3cc, table, i)
> +		intel_uncore_write_fw(uncore, GEN9_LNCFCMOCS(i), l3cc);
>  }
>  
>  void intel_mocs_init_engine(struct intel_engine_cs *engine)
> @@ -451,11 +434,9 @@ void intel_mocs_init_engine(struct intel_engine_cs *engine)
>  		init_l3cc_table(engine, &table);
>  }
>  
> -static void intel_mocs_init_global(struct intel_gt *gt)
> +static void init_global_mocs(struct intel_gt *gt)
>  {
> -	struct intel_uncore *uncore = gt->uncore;
>  	struct drm_i915_mocs_table table;
> -	unsigned int index;
>  
>  	/*
>  	 * LLC and eDRAM control values are not applicable to dgfx
> @@ -463,29 +444,14 @@ static void intel_mocs_init_global(struct intel_gt *gt)
>  	if (IS_DGFX(gt->i915))
>  		return;
>  
> -	GEM_BUG_ON(!HAS_GLOBAL_MOCS_REGISTERS(gt->i915));
> -
>  	if (!get_mocs_settings(gt->i915, &table))
>  		return;
>  
> -	for (index = 0; index < table.size; index++)
> -		intel_uncore_write(uncore,
> -				   GEN12_GLOBAL_MOCS(index),
> -				   table.table[index].control_value);
> -
> -	/*
> -	 * Ok, now set the unused entries to the invalid entry (index 0). These
> -	 * entries are officially undefined and no contract for the contents and
> -	 * settings is given for these entries.
> -	 */
> -	for (; index < table.n_entries; index++)
> -		intel_uncore_write(uncore,
> -				   GEN12_GLOBAL_MOCS(index),
> -				   table.table[I915_MOCS_PTE].control_value);
> +	__init_mocs_table(gt->uncore, &table, 0x4000);
>  }
>  
>  void intel_mocs_init(struct intel_gt *gt)
>  {
>  	if (HAS_GLOBAL_MOCS_REGISTERS(gt->i915))
> -		intel_mocs_init_global(gt);
> +		init_global_mocs(gt);
>  }
> -- 
> 2.24.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx