diff mbox series

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

Message ID 20191112092854.869-26-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [01/27] drm/i915: Flush context free work on cleanup | expand

Commit Message

Chris Wilson Nov. 12, 2019, 9:28 a.m. UTC 
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(-)

Comments

Mika Kuoppala Nov. 12, 2019, 5:02 p.m. UTC | #1 
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
Chris Wilson Nov. 12, 2019, 6:12 p.m. UTC | #2 
Quoting Mika Kuoppala (2019-11-12 17:02:18)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > +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,

A bit of a quandary as

@@ -361,12 +361,12 @@ static void __init_mocs_table(struct intel_uncore *uncore,
 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,
+               [RCS0]  = i915_mmio_reg_offset(GEN9_GFX_MOCS(0)),
+               [VCS0]  = i915_mmio_reg_offset(GEN9_MFX0_MOCS(0)),
+               [VCS1]  = i915_mmio_reg_offset(GEN9_BLT_MOCS(0)),
+               [VECS0] = i915_mmio_reg_offset(GEN9_VEBOX_MOCS(0)),
+               [BCS0]  = i915_mmio_reg_offset(GEN9_MFX1_MOCS(0)),
+               [VCS2]  = i915_mmio_reg_offset(GEN11_MFX2_MOCS(0)),
        };

        GEM_BUG_ON(engine->id >= ARRAY_SIZE(offset));

does not compile as they do not evaluate to a constant.

The alternative is to have the raw offsets for the indexed macros to
build off.
-Chris
diff mbox series

Patch

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++)
+
+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,
+	};
+
+	GEM_BUG_ON(engine->id > ARRAY_SIZE(offset));
+	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);
 }