Disable automatic load CCS load balancing

The hardware should not dynamically balance the load between CCS engines. Wa_14019159160 recommends disabling it across all platforms.

Fixes d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")

Signed-off-by Andi Shyti <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Matt Roper <[email protected]>

Cc <[email protected]> # v6.2+

---
 drivers/gpu/drm/i915/gt/intel_gt_regs.h     | 1 +
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 6 ++++++
 2 files changed, 7 insertions(+)

drivers/gpu/drm/i915/gt/intel_gt_regs.h +1 -0

index 50962cfd1353..cf709f6c05ae 100644
@@ -1478,6 +1478,7 @@
 
 #define GEN12_RCU_MODE				_MMIO(0x14800)
 #define   GEN12_RCU_MODE_CCS_ENABLE		REG_BIT(0)
+#define   XEHP_RCU_MODE_FIXED_SLICE_CCS_MODE	REG_BIT(1)
 
 #define CHV_FUSE_GT				_MMIO(VLV_GUNIT_BASE + 0x2168)
 #define   CHV_FGT_DISABLE_SS0			(1 << 10)

drivers/gpu/drm/i915/gt/intel_workarounds.c +0 -0

index d67d44611c28..9126b37186fc 100644

drivers/gpu/drm/i915/gt/intel_workarounds.c +2 -0

+	 */
+	wa_masked_en(wal, GEN12_RCU_MODE, XEHP_RCU_MODE_FIXED_SLICE_CCS_MODE);
 }
 
 static void

Signature · 1 line

2.43.0

drivers/gpu/drm/i915/i915_query.c +0 -0

 static int
 query_engine_info(struct drm_i915_private *i915,
 		  struct drm_i915_query_item *query_item)

Signature · 1 line

2.43.0

Enable only one CCS engine by default with all the compute sices

allocated to it.

While generating the list of UABI engines to be exposed to the user, exclude any additional CCS engines beyond the first instance.

This change can be tested with igt i915_query.

Fixes d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")

Signed-off-by Andi Shyti <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Matt Roper <[email protected]>

Cc <[email protected]> # v6.2+

---
  drivers/gpu/drm/i915/gt/intel_engine_user.c |  9 +++++++++
  drivers/gpu/drm/i915/gt/intel_gt.c          | 11 +++++++++++
  drivers/gpu/drm/i915/gt/intel_gt_regs.h     |  2 ++
  drivers/gpu/drm/i915/i915_query.c           |  1 +
  4 files changed, 23 insertions(+)

drivers/gpu/drm/i915/gt/intel_engine_user.c +0 -0

index 833987015b8b..7041acc77810 100644
@@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
  		if (engine->uabi_class == I915_NO_UABI_CLASS)

drivers/gpu/drm/i915/gt/intel_engine_user.c +1 -0

+
  		rb_link_node(&engine->uabi_node, prev, p);
  		rb_insert_color(&engine->uabi_node, &i915->uabi_engines);
 

drivers/gpu/drm/i915/gt/intel_gt.c +11 -0

index a425db5ed3a2..e19df4ef47f6 100644
@@ -168,6 +168,14 @@ static void init_unused_rings(struct intel_gt *gt)
  	}
  }
 
+static void intel_gt_apply_ccs_mode(struct intel_gt *gt)
+{
+	if (!IS_DG2(gt->i915))
+		return;
+
+	intel_uncore_write(gt->uncore, XEHP_CCS_MODE, 0);
+}
+
  int intel_gt_init_hw(struct intel_gt *gt)
  {
  	struct drm_i915_private *i915 = gt->i915;

@@ -195,6 +203,9 @@ int intel_gt_init_hw(struct intel_gt *gt)
 
  	intel_gt_init_swizzling(gt);
 
+	/* Configure CCS mode */
+	intel_gt_apply_ccs_mode(gt);
+
  	/*
  	 * At least 830 can leave some of the unused rings
  	 * "active" (ie. head != tail) after resume which

drivers/gpu/drm/i915/gt/intel_gt_regs.h +2 -0

index cf709f6c05ae..c148113770ea 100644
@@ -1605,6 +1605,8 @@
  #define   GEN12_VOLTAGE_MASK			REG_GENMASK(10, 0)
  #define   GEN12_CAGF_MASK			REG_GENMASK(19, 11)
 
+#define XEHP_CCS_MODE                          _MMIO(0x14804)
+
  #define GEN11_GT_INTR_DW(x)			_MMIO(0x190018 + ((x) * 4))
  #define   GEN11_CSME				(31)
  #define   GEN12_HECI_2				(30)

drivers/gpu/drm/i915/i915_query.c +0 -0

index 3baa2f54a86e..d5a5143971f5 100644

Quoted 1 line · Andi Shyti

Enable only one CCS engine by default with all the compute sices

slices

Quoted 10 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +3 -0

index 833987015b8b..7041acc77810 100644
@@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
  		if (engine->uabi_class == I915_NO_UABI_CLASS)
  			continue;
+		/*
+		 * Do not list and do not count CCS engines other than the first
+		 */

drivers/gpu/drm/i915/gt/intel_engine_user.c void intel_engines_driver_register(struct drm_i915_private *i915) · line 253

Quoted 5 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +5 -0

+		if (engine->uabi_class == I915_ENGINE_CLASS_COMPUTE &&
+		    engine->uabi_instance > 0) {
+			i915->engine_uabi_class_count[engine->uabi_class]--;
+			continue;
+		}

It's a bit ugly to decrement after increment, instead of somehow restructuring the loop to satisfy both cases more elegantly.

And I wonder if
internally (in dmesg when engine name is logged) we don't end up with ccs0 ccs0 ccs0 ccs0.. for all instances.

Quoted 3 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +1 -0

+
  		rb_link_node(&engine->uabi_node, prev, p);
  		rb_insert_color(&engine->uabi_node, &i915->uabi_engines);

drivers/gpu/drm/i915/i915_query.c static int query_geometry_subslices(struct drm_i915_private *i915, · line 126

Quoted 8 lines

drivers/gpu/drm/i915/i915_query.c +1 -0

index 3baa2f54a86e..d5a5143971f5 100644
@@ -124,6 +124,7 @@ static int query_geometry_subslices(struct drm_i915_private *i915,
  	return fill_topology_info(sseu, query_item, sseu->geometry_subslice_mask);
  }
+

Zap please.

Hi Tvrtko,

Quoted 4 lines · Tvrtko Ursulin

Quoted 1 line · Andi Shyti

Enable only one CCS engine by default with all the compute sices

slices

Thanks!

Quoted 10 lines

Quoted 10 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +3 -0

index 833987015b8b..7041acc77810 100644
@@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
   		if (engine->uabi_class == I915_NO_UABI_CLASS)
   			continue;
+		/*
+		 * Do not list and do not count CCS engines other than the first
+		 */

Quoted 8 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c void intel_engines_driver_register(struct drm_i915_private *i915) · line 253

Quoted 5 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +5 -0

+		if (engine->uabi_class == I915_ENGINE_CLASS_COMPUTE &&
+		    engine->uabi_instance > 0) {
+			i915->engine_uabi_class_count[engine->uabi_class]--;
+			continue;
+		}

It's a bit ugly to decrement after increment, instead of somehow restructuring the loop to satisfy both cases more elegantly.

yes, agree, indeed I had a hard time here to accept this change myself.

But moving the check above where the counter was incremented it would have been much uglier.

This check looks ugly everywhere you place it :-)

In any case, I'm working on a patch that is splitting this function in two parts and there is some refactoring happening here (for the first initialization and the dynamic update).

Please let me know if it's OK with you or you want me to fix it in this run.

Quoted 3 lines

And I wonder if
internally (in dmesg when engine name is logged) we don't end up with ccs0 ccs0 ccs0 ccs0.. for all instances.

I don't see this. Even in sysfs we see only one ccs. Where is it?

Quoted 4 lines

Quoted 4 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +1 -0

+
   		rb_link_node(&engine->uabi_node, prev, p);
   		rb_insert_color(&engine->uabi_node, &i915->uabi_engines);

[...]

Quoted 10 lines

drivers/gpu/drm/i915/i915_query.c static int query_geometry_subslices(struct drm_i915_private *i915, · line 126

Quoted 8 lines

drivers/gpu/drm/i915/i915_query.c +1 -0

index 3baa2f54a86e..d5a5143971f5 100644
@@ -124,6 +124,7 @@ static int query_geometry_subslices(struct drm_i915_private *i915,
   	return fill_topology_info(sseu, query_item, sseu->geometry_subslice_mask);
   }
+

Zap please.

yes... yes... I noticed it after sending the patch :-)

Thanks,
Andi

Quoted 33 lines · Andi Shyti

Quoted 4 lines · Tvrtko Ursulin

Quoted 1 line · Andi Shyti

Enable only one CCS engine by default with all the compute sices

slices

Thanks!

Quoted 10 lines

Quoted 10 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +3 -0

index 833987015b8b..7041acc77810 100644
@@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
   		if (engine->uabi_class == I915_NO_UABI_CLASS)
   			continue;
+		/*
+		 * Do not list and do not count CCS engines other than the first
+		 */

Quoted 8 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c void intel_engines_driver_register(struct drm_i915_private *i915) · line 253

Quoted 5 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +5 -0

+		if (engine->uabi_class == I915_ENGINE_CLASS_COMPUTE &&
+		    engine->uabi_instance > 0) {
+			i915->engine_uabi_class_count[engine->uabi_class]--;
+			continue;
+		}

It's a bit ugly to decrement after increment, instead of somehow restructuring the loop to satisfy both cases more elegantly.

yes, agree, indeed I had a hard time here to accept this change myself.

But moving the check above where the counter was incremented it would have been much uglier.

Quoted 1 line

This check looks ugly everywhere you place it :-)

One idea would be to introduce a separate local counter array for name_instance, so not use i915->engine_uabi_class_count[]. First one increments for every engine, second only for the exposed ones. That way feels wouldn't be too ugly.

Quoted 7 lines

In any case, I'm working on a patch that is splitting this function in two parts and there is some refactoring happening here (for the first initialization and the dynamic update).

Please let me know if it's OK with you or you want me to fix it in this run.

Quoted 5 lines

Quoted 3 lines

And I wonder if
internally (in dmesg when engine name is logged) we don't end up with ccs0 ccs0 ccs0 ccs0.. for all instances.

I don't see this. Even in sysfs we see only one ccs. Where is it?

When you run this patch on something with two or more ccs-es, the "renamed ccs... to ccs.." debug logs do not all log the new name as ccs0?

Hi Tvrtko,

Quoted 34 lines · Tvrtko Ursulin

Quoted 33 lines · Andi Shyti

Quoted 4 lines · Tvrtko Ursulin

Quoted 1 line · Andi Shyti

Enable only one CCS engine by default with all the compute sices

slices

Thanks!

Quoted 10 lines

Quoted 10 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +3 -0

index 833987015b8b..7041acc77810 100644
@@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
    		if (engine->uabi_class == I915_NO_UABI_CLASS)
    			continue;
+		/*
+		 * Do not list and do not count CCS engines other than the first
+		 */

Quoted 8 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c void intel_engines_driver_register(struct drm_i915_private *i915) · line 252

Quoted 5 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +5 -0

+		if (engine->uabi_class == I915_ENGINE_CLASS_COMPUTE &&
+		    engine->uabi_instance > 0) {
+			i915->engine_uabi_class_count[engine->uabi_class]--;
+			continue;
+		}

It's a bit ugly to decrement after increment, instead of somehow restructuring the loop to satisfy both cases more elegantly.

yes, agree, indeed I had a hard time here to accept this change myself.

But moving the check above where the counter was incremented it would have been much uglier.

Quoted 6 lines

Quoted 1 line

This check looks ugly everywhere you place it :-)

One idea would be to introduce a separate local counter array for name_instance, so not use i915->engine_uabi_class_count[]. First one increments for every engine, second only for the exposed ones. That way feels wouldn't be too ugly.

Ah... you mean that whenever we change the CCS mode, we update the indexes of the exposed engines from list of the real engines. Will try.

My approach was to regenerate the list everytime the CCS mode was changed, but your suggestion looks a bit simplier.

Quoted 11 lines

Quoted 11 lines

In any case, I'm working on a patch that is splitting this function in two parts and there is some refactoring happening here (for the first initialization and the dynamic update).

Please let me know if it's OK with you or you want me to fix it in this run.

Quoted 3 lines

And I wonder if
internally (in dmesg when engine name is logged) we don't end up with ccs0 ccs0 ccs0 ccs0.. for all instances.

Quoted 4 lines

Quoted 1 line

I don't see this. Even in sysfs we see only one ccs. Where is it?

When you run this patch on something with two or more ccs-es, the "renamed ccs... to ccs.." debug logs do not all log the new name as ccs0?

it shouldn't, because the name_instance is anyway incremented normally... anyway, I will test it.

Quoted 46 lines · Andi Shyti

Hi Tvrtko,

Quoted 36 lines · Tvrtko Ursulin

Quoted 35 lines · Andi Shyti

Quoted 4 lines · Tvrtko Ursulin

Quoted 1 line · Andi Shyti

Enable only one CCS engine by default with all the compute sices

slices

Thanks!

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +2 -0

index 833987015b8b..7041acc77810 100644
@@ -243,6 +243,15 @@ void intel_engines_driver_register(struct 
 drm_i915_private *i915)
             if (engine->uabi_class == I915_NO_UABI_CLASS)
                 continue;
+        /*
+         * Do not list and do not count CCS engines other than 

the first
+         */

Quoted 8 lines

Quoted 5 lines

+        if (engine->uabi_class == I915_ENGINE_CLASS_COMPUTE && +            engine->uabi_instance > 0) {
+            i915->engine_uabi_class_count[engine->uabi_class]--; +            continue;
+        }

It's a bit ugly to decrement after increment, instead of somehow restructuring the loop to satisfy both cases more elegantly.

yes, agree, indeed I had a hard time here to accept this change myself.

But moving the check above where the counter was incremented it would have been much uglier.

Quoted 6 lines

Quoted 1 line

This check looks ugly everywhere you place it :-)

One idea would be to introduce a separate local counter array for name_instance, so not use i915->engine_uabi_class_count[]. First one increments for every engine, second only for the exposed ones. That way feels wouldn't be too ugly.

Quoted 6 lines

Ah... you mean that whenever we change the CCS mode, we update the indexes of the exposed engines from list of the real engines. Will try.

My approach was to regenerate the list everytime the CCS mode was changed, but your suggestion looks a bit simplier.

No, I meant just for this first stage of permanently single engine. For avoiding the decrement after increment. Something like this, but not compile tested even:

drivers/gpu/drm/i915/gt/intel_engine_user.c +2 -1

index 833987015b8b..4c33f30612c4 100644
@@ -203,7 +203,8 @@ static void engine_rename(struct intel_engine_cs 
 *engine, const char *name, u16
 
  void intel_engines_driver_register(struct drm_i915_private *i915)
  {
-       u16 name_instance, other_instance = 0;
+       u16 class_instance[I915_LAST_UABI_ENGINE_CLASS + 2] = { };
+       u16 uabi_class, other_instance = 0;
         struct legacy_ring ring = {};
         struct list_head *it, *next;

        struct rb_node **p, *prev;

drivers/gpu/drm/i915/gt/intel_engine_user.c +7 -8

@@ -222,15 +223,14 @@ void intel_engines_driver_register(struct 
 drm_i915_private *i915)
 
                 GEM_BUG_ON(engine->class >= ARRAY_SIZE(uabi_classes));
                 engine->uabi_class = uabi_classes[engine->class];
+
                 if (engine->uabi_class == I915_NO_UABI_CLASS) {
-                       name_instance = other_instance++;
-               } else {
-                       GEM_BUG_ON(engine->uabi_class >=
-                                  
 ARRAY_SIZE(i915->engine_uabi_class_count));
-                       name_instance =
-                               
 i915->engine_uabi_class_count[engine->uabi_class]++;
-               }
-               engine->uabi_instance = name_instance;
+                       uabi_class = I915_LAST_UABI_ENGINE_CLASS + 1;
+               else
+                       uabi_class = engine->uabi_class;
+
+               GEM_BUG_ON(uabi_class >= ARRAY_SIZE(class_instance));
+               engine->uabi_instance = class_instance[uabi_class]++;

                /*
                 * Replace the internal name with the final user and
log facing

drivers/gpu/drm/i915/gt/intel_engine_user.c +4 -0

+               GEM_BUG_ON(uabi_class >=
+                          ARRAY_SIZE(i915->engine_uabi_class_count));
+               i915->engine_uabi_class_count[uabi_class]++;
+
                 rb_link_node(&engine->uabi_node, prev, p);
                 rb_insert_color(&engine->uabi_node, &i915->uabi_engines);
 

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

In any case, I'm working on a patch that is splitting this function in two parts and there is some refactoring happening here (for the first initialization and the dynamic update).

Please let me know if it's OK with you or you want me to fix it in this run.

Quoted 3 lines

And I wonder if
internally (in dmesg when engine name is logged) we don't end up with ccs0 ccs0 ccs0 ccs0.. for all instances.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

I don't see this. Even in sysfs we see only one ccs. Where is it?

When you run this patch on something with two or more ccs-es, the "renamed ccs... to ccs.." debug logs do not all log the new name as ccs0?

it shouldn't, because the name_instance is anyway incremented normally... anyway, I will test it.

Hm maybe it needs more than two ccs engines and then it would be ccs0, ccs1, ccs2, ccs2, on a four ccs part. Or something.. It just feels the decrement of i915->engine_uabi_class_count, which engine_instance currently uses, has to mess this up somehow.

Regards,

Tvrtko

Enable only one CCS engine by default with all the compute sices allocated to it.

While generating the list of UABI engines to be exposed to the user, exclude any additional CCS engines beyond the first instance.

This change can be tested with igt i915_query.

Fixes d2eae8e98d59 ("drm/i915/dg2: Drop force_probe requirement")

Signed-off-by Andi Shyti <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Cc Matt Roper <[email protected]>

Cc <[email protected]> # v6.2+

---
 drivers/gpu/drm/i915/gt/intel_engine_user.c |  9 +++++++++
 drivers/gpu/drm/i915/gt/intel_gt.c          | 11 +++++++++++
 drivers/gpu/drm/i915/gt/intel_gt_regs.h     |  2 ++
 drivers/gpu/drm/i915/i915_query.c           |  1 +
 4 files changed, 23 insertions(+)

drivers/gpu/drm/i915/gt/intel_engine_user.c +0 -0

index 833987015b8b..7041acc77810 100644
@@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
 		if (engine->uabi_class == I915_NO_UABI_CLASS)

drivers/gpu/drm/i915/gt/intel_engine_user.c +1 -0

+
 		rb_link_node(&engine->uabi_node, prev, p);
 		rb_insert_color(&engine->uabi_node, &i915->uabi_engines);
 

drivers/gpu/drm/i915/gt/intel_gt.c +0 -0

index a425db5ed3a2..e19df4ef47f6 100644

drivers/gpu/drm/i915/gt/intel_gt.c +5 -0

+}
+
 int intel_gt_init_hw(struct intel_gt *gt)
 {
 	struct drm_i915_private *i915 = gt->i915;

@@ -195,6 +203,9 @@ int intel_gt_init_hw(struct intel_gt *gt)
 
 	intel_gt_init_swizzling(gt);
 
+	/* Configure CCS mode */
+	intel_gt_apply_ccs_mode(gt);
+
 	/*
 	 * At least 830 can leave some of the unused rings
 	 * "active" (ie. head != tail) after resume which

drivers/gpu/drm/i915/gt/intel_gt_regs.h +0 -0

index cf709f6c05ae..c148113770ea 100644

drivers/gpu/drm/i915/gt/intel_gt_regs.h +1 -0

+
 #define GEN11_GT_INTR_DW(x)			_MMIO(0x190018 + ((x) * 4))
 #define   GEN11_CSME				(31)
 #define   GEN12_HECI_2				(30)

drivers/gpu/drm/i915/i915_query.c +1 -0

index 3baa2f54a86e..d5a5143971f5 100644
@@ -124,6 +124,7 @@ static int query_geometry_subslices(struct drm_i915_private *i915,
 	return fill_topology_info(sseu, query_item, sseu->geometry_subslice_mask);
 }
 
+
 static int
 query_engine_info(struct drm_i915_private *i915,
 		  struct drm_i915_query_item *query_item)

Signature · 1 line

2.43.0

On Tue, Feb 20, 2024 at 03:35:26PM +0100, Andi Shyti wrote:

Quoted 6 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +0 -0

index 833987015b8b..7041acc77810 100644
@@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
 		if (engine->uabi_class == I915_NO_UABI_CLASS)

drivers/gpu/drm/i915/gt/intel_engine_user.c void intel_engines_driver_register(struct drm_i915_private *i915) · line 253

Quoted 10 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +8 -0

 			continue;
 
+		/*
+		 * Do not list and do not count CCS engines other than the first
+		 */
+		if (engine->uabi_class == I915_ENGINE_CLASS_COMPUTE &&
+		    engine->uabi_instance > 0) {
+			i915->engine_uabi_class_count[engine->uabi_class]--;
+			continue;
+		}

Wouldn't it be simpler to just add a workaround to the end of engine_mask_apply_compute_fuses() if we want to ensure only a single compute engine gets exposed? Then both the driver internals and uapi will agree that's there's just one CCS (and on which one there is).

If we want to do something fancy with "hotplugging" a new engine later on or whatever, that can be handled in the future series (although as noted on the previous patch, it sounds like these changes might not actually be aligned with the workaround we were trying to address).

Quoted 12 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +1 -0

+
 		rb_link_node(&engine->uabi_node, prev, p);
 		rb_insert_color(&engine->uabi_node, &i915->uabi_engines);
 

drivers/gpu/drm/i915/gt/intel_gt.c +0 -0

index a425db5ed3a2..e19df4ef47f6 100644
@@ -168,6 +168,14 @@ static void init_unused_rings(struct intel_gt *gt)
 	}
 }
 

drivers/gpu/drm/i915/gt/intel_gt.c static void init_unused_rings(struct intel_gt *gt) · line 176

Quoted 6 lines

drivers/gpu/drm/i915/gt/intel_gt.c +6 -0

+static void intel_gt_apply_ccs_mode(struct intel_gt *gt)
+{
+	if (!IS_DG2(gt->i915))
+		return;
+
+	intel_uncore_write(gt->uncore, XEHP_CCS_MODE, 0);

This doesn't look right to me. A value of 0 means every cslice gets associated with CCS0.

On a DG2-G11 platform, that will flat out break
compute since CCS0 is never present (G11 only has a single CCS and it's always the hardware's CCS1). Even on a G10 or G12 this could also break things depending on the fusing of your card if the hardware CCS0 happens to be missing.

Also, the register says that we need a field value of 0x7 for each cslice that's fused off. By passing 0, we're telling the CCS engine that it can use cslices that may not actually exist.

Quoted 2 lines

drivers/gpu/drm/i915/gt/intel_gt.c +2 -0

+}
+

Quoted 8 lines

drivers/gpu/drm/i915/gt/intel_gt_regs.h +0 -0

index cf709f6c05ae..c148113770ea 100644
@@ -1605,6 +1605,8 @@
 #define   GEN12_VOLTAGE_MASK			REG_GENMASK(10, 0)
 #define   GEN12_CAGF_MASK			REG_GENMASK(19, 11)
 

drivers/gpu/drm/i915/gt/intel_gt_regs.h line 1608

Quoted 1 line

drivers/gpu/drm/i915/gt/intel_gt_regs.h +1 -0

+#define XEHP_CCS_MODE                          _MMIO(0x14804)

Nitpick: this doesn't seem to be in the proper place and also breaks the file's convention of using tabs to move over to column 48 for the definition value.

Hi Matt,

thanks a lot for looking into this.

Quoted 1 line · Matt Roper

On Tue, Feb 20, 2024 at 03:35:26PM +0100, Andi Shyti wrote:

[...]

Quoted 22 lines

Quoted 6 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +0 -0

index 833987015b8b..7041acc77810 100644
@@ -243,6 +243,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
 		if (engine->uabi_class == I915_NO_UABI_CLASS)

drivers/gpu/drm/i915/gt/intel_engine_user.c void intel_engines_driver_register(struct drm_i915_private *i915) · line 253

Quoted 10 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +8 -0

 			continue;
 
+		/*
+		 * Do not list and do not count CCS engines other than the first
+		 */
+		if (engine->uabi_class == I915_ENGINE_CLASS_COMPUTE &&
+		    engine->uabi_instance > 0) {
+			i915->engine_uabi_class_count[engine->uabi_class]--;
+			continue;
+		}

Wouldn't it be simpler to just add a workaround to the end of engine_mask_apply_compute_fuses() if we want to ensure only a single compute engine gets exposed? Then both the driver internals and uapi will agree that's there's just one CCS (and on which one there is).

Quoted 4 lines

If we want to do something fancy with "hotplugging" a new engine later on or whatever, that can be handled in the future series (although as noted on the previous patch, it sounds like these changes might not actually be aligned with the workaround we were trying to address).

The hotplugging capability is one of the features I was looking for, actually.

I have done some more refactoring in this piece of code in upcoming patches.

Will check, though, if I can do something with compute_fuses(), even though, the other cslices are not really fused off (read below).

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/i915/gt/intel_engine_user.c +1 -0

+
 		rb_link_node(&engine->uabi_node, prev, p);
 		rb_insert_color(&engine->uabi_node, &i915->uabi_engines);
 

drivers/gpu/drm/i915/gt/intel_gt.c +0 -0

index a425db5ed3a2..e19df4ef47f6 100644
@@ -168,6 +168,14 @@ static void init_unused_rings(struct intel_gt *gt)
 	}
 }
 

Quoted 9 lines

drivers/gpu/drm/i915/gt/intel_gt.c static void init_unused_rings(struct intel_gt *gt) · line 176

Quoted 6 lines

drivers/gpu/drm/i915/gt/intel_gt.c +6 -0

+static void intel_gt_apply_ccs_mode(struct intel_gt *gt)
+{
+	if (!IS_DG2(gt->i915))
+		return;
+
+	intel_uncore_write(gt->uncore, XEHP_CCS_MODE, 0);

This doesn't look right to me. A value of 0 means every cslice gets associated with CCS0.

Yes, that's what I'm trying to do. The behavior I'm looking for is this one:

/*
...
* With 1 engine (ccs0):
* slice 0, 1, 2, 3: ccs0
*
* With 2 engines (ccs0, ccs1):
* slice 0, 2: ccs0
* slice 1, 3: ccs1
*
* With 4 engines (ccs0, ccs1, ccs2, ccs3):
* slice 0: ccs0
* slice 1: ccs1
* slice 2: ccs2
* slice 3: ccs3
...
*/

where the user can configure runtime the mode, making sure that no client is connected to i915.

But, this needs to be written

As we are now forcing mode '1', then all cslices are connected with ccs0.

Quoted 7 lines

On a DG2-G11 platform, that will flat out break
compute since CCS0 is never present (G11 only has a single CCS and it's always the hardware's CCS1). Even on a G10 or G12 this could also break things depending on the fusing of your card if the hardware CCS0 happens to be missing.

Quoted 3 lines

Also, the register says that we need a field value of 0x7 for each cslice that's fused off. By passing 0, we're telling the CCS engine that it can use cslices that may not actually exist.

does it? Or do I need to write the id (0x0-0x3) of the user engine? That's how the mode is calculated in this algorithm.

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/i915/gt/intel_gt.c +2 -0

+}
+

[...]

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/gt/intel_gt_regs.h +0 -0

index cf709f6c05ae..c148113770ea 100644
@@ -1605,6 +1605,8 @@
 #define   GEN12_VOLTAGE_MASK			REG_GENMASK(10, 0)
 #define   GEN12_CAGF_MASK			REG_GENMASK(19, 11)
 

Quoted 5 lines

drivers/gpu/drm/i915/gt/intel_gt_regs.h line 1608

Quoted 1 line

drivers/gpu/drm/i915/gt/intel_gt_regs.h +1 -0

+#define XEHP_CCS_MODE                          _MMIO(0x14804)

Nitpick: this doesn't seem to be in the proper place and also breaks the file's convention of using tabs to move over to column 48 for the definition value.

This was something I actually forgot to fix. Thanks!

Quoted 2 lines · Andi Shyti

Quoted 1 line · Matt Roper

On Tue, Feb 20, 2024 at 03:35:26PM +0100, Andi Shyti wrote:

Quoted 38 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/gt/intel_gt.c +0 -0

index a425db5ed3a2..e19df4ef47f6 100644
@@ -168,6 +168,14 @@ static void init_unused_rings(struct intel_gt *gt)
 	}
 }
 

Quoted 9 lines

drivers/gpu/drm/i915/gt/intel_gt.c static void init_unused_rings(struct intel_gt *gt) · line 176

Quoted 6 lines

drivers/gpu/drm/i915/gt/intel_gt.c +6 -0

+static void intel_gt_apply_ccs_mode(struct intel_gt *gt)
+{
+	if (!IS_DG2(gt->i915))
+		return;
+
+	intel_uncore_write(gt->uncore, XEHP_CCS_MODE, 0);

This doesn't look right to me. A value of 0 means every cslice gets associated with CCS0.

Yes, that's what I'm trying to do. The behavior I'm looking for is this one:

/*
...
* With 1 engine (ccs0):
* slice 0, 1, 2, 3: ccs0
*
* With 2 engines (ccs0, ccs1):
* slice 0, 2: ccs0
* slice 1, 3: ccs1
*
* With 4 engines (ccs0, ccs1, ccs2, ccs3):
* slice 0: ccs0
* slice 1: ccs1
* slice 2: ccs2
* slice 3: ccs3
...
*/

Quoted 7 lines

where the user can configure runtime the mode, making sure that no client is connected to i915.

But, this needs to be written

As we are now forcing mode '1', then all cslices are connected with ccs0.

Right --- and that's what I'm pointing out as illegal. I think that code comment above was taken out of context from a different RFC series; that's not an accurate description of the behavior we want here.

First, the above comment is using ccs# to refer to userspace engines, not hardware engines. As a simple example, DG2-G11 only ever has a single CCS which userspace sees as "instance 0" but which is actually CCS1 at the hardware level. If you try to follow the comment above when programming CCS_MODE, you've assigned all of the cslices to a non-existent engine and assigned no cslices to the CCS engine that actually exists. For DG2-G10 (and I think DG2-G12), there are different combinations of fused-off / not-fused-off engines that will always show up in userspace as CCS0-CCSn, even if those don't match the hardware IDs.

Second, the above comment is assuming that you have a part with a maximum fusing config (i.e., all cslices present). Using DG2-G11 again as an example, there's also only a single cslice (cslice1), so if you tell CCS1 that it's allowed to use EUs from non-existent cslice0, cslice2, and cslice3, you might not get the behavior you were hoping for.

Quoted 10 lines

Quoted 9 lines

On a DG2-G11 platform, that will flat out break
compute since CCS0 is never present (G11 only has a single CCS and it's always the hardware's CCS1). Even on a G10 or G12 this could also break things depending on the fusing of your card if the hardware CCS0 happens to be missing.

Also, the register says that we need a field value of 0x7 for each cslice that's fused off. By passing 0, we're telling the CCS engine that it can use cslices that may not actually exist.

Quoted 2 lines

does it? Or do I need to write the id (0x0-0x3) of the user engine? That's how the mode is calculated in this algorithm.

0x0 - 0x3 are how you specify that a specific CCS engine can use the cslice. If the cslice can't be used at all (i.e., it's fused off), then you need to program a 0x7 to ensure no engines try to use the non-existent DSS/EUs.

Hi Matt,

first of all thanks a lot for the observations you are raising.

Quoted 3 lines · Matt Roper

Quoted 2 lines · Andi Shyti

Quoted 1 line · Matt Roper

On Tue, Feb 20, 2024 at 03:35:26PM +0100, Andi Shyti wrote:

...

Quoted 61 lines

Quoted 38 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/gt/intel_gt.c +0 -0

index a425db5ed3a2..e19df4ef47f6 100644
@@ -168,6 +168,14 @@ static void init_unused_rings(struct intel_gt *gt)
 	}
 }
 

Quoted 9 lines

drivers/gpu/drm/i915/gt/intel_gt.c static void init_unused_rings(struct intel_gt *gt) · line 176

Quoted 6 lines

drivers/gpu/drm/i915/gt/intel_gt.c +6 -0

+static void intel_gt_apply_ccs_mode(struct intel_gt *gt)
+{
+	if (!IS_DG2(gt->i915))
+		return;
+
+	intel_uncore_write(gt->uncore, XEHP_CCS_MODE, 0);

This doesn't look right to me. A value of 0 means every cslice gets associated with CCS0.

Yes, that's what I'm trying to do. The behavior I'm looking for is this one:

/*
...
* With 1 engine (ccs0):
* slice 0, 1, 2, 3: ccs0
*
* With 2 engines (ccs0, ccs1):
* slice 0, 2: ccs0
* slice 1, 3: ccs1
*
* With 4 engines (ccs0, ccs1, ccs2, ccs3):
* slice 0: ccs0
* slice 1: ccs1
* slice 2: ccs2
* slice 3: ccs3
...
*/

Quoted 7 lines

where the user can configure runtime the mode, making sure that no client is connected to i915.

But, this needs to be written

As we are now forcing mode '1', then all cslices are connected with ccs0.

Right --- and that's what I'm pointing out as illegal. I think that code comment above was taken out of context from a different RFC series; that's not an accurate description of the behavior we want here.

First, the above comment is using ccs# to refer to userspace engines, not hardware engines. As a simple example, DG2-G11 only ever has a single CCS which userspace sees as "instance 0" but which is actually CCS1 at the hardware level. If you try to follow the comment above when programming CCS_MODE, you've assigned all of the cslices to a non-existent engine and assigned no cslices to the CCS engine that actually exists. For DG2-G10 (and I think DG2-G12), there are different combinations of fused-off / not-fused-off engines that will always show up in userspace as CCS0-CCSn, even if those don't match the hardware IDs.

Quoted 6 lines

Second, the above comment is assuming that you have a part with a maximum fusing config (i.e., all cslices present). Using DG2-G11 again as an example, there's also only a single cslice (cslice1), so if you tell CCS1 that it's allowed to use EUs from non-existent cslice0, cslice2, and cslice3, you might not get the behavior you were hoping for.

if the hardware slices are fused off we wouldn't see them in a first place, right? And that's anyway a permanent configuration that wouldn't affect the patch.

BTW, is there any machine I can test this scenario?

Quoted 14 lines

Quoted 13 lines

Quoted 10 lines

On a DG2-G11 platform, that will flat out break
compute since CCS0 is never present (G11 only has a single CCS and it's always the hardware's CCS1). Even on a G10 or G12 this could also break things depending on the fusing of your card if the hardware CCS0 happens to be missing.

Also, the register says that we need a field value of 0x7 for each cslice that's fused off. By passing 0, we're telling the CCS engine that it can use cslices that may not actually exist.

does it? Or do I need to write the id (0x0-0x3) of the user engine? That's how the mode is calculated in this algorithm.

Quoted 4 lines

0x0 - 0x3 are how you specify that a specific CCS engine can use the cslice. If the cslice can't be used at all (i.e., it's fused off), then you need to program a 0x7 to ensure no engines try to use the non-existent DSS/EUs.

I planned to limit this to the only DG2 (and ATSM, of course). Do you think it would it help?

Andi