Re: [PATCH v2 2/2] drm/i915/gt: Enable only one CCS for compute workload

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 21/02/2024 11:19, Andi Shyti wrote:
> Hi Tvrtko,
> 
> On Wed, Feb 21, 2024 at 08:19:34AM +0000, Tvrtko Ursulin wrote:
>> On 21/02/2024 00:14, Andi Shyti wrote:
>>> On Tue, Feb 20, 2024 at 02:48:31PM +0000, Tvrtko Ursulin wrote:
>>>> On 20/02/2024 14:35, Andi Shyti wrote:
>>>>> Enable only one CCS engine by default with all the compute sices
>>>>
>>>> slices
>>>
>>> Thanks!
>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>>>> index 833987015b8b..7041acc77810 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>>>> @@ -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
>>>>> +		 */
>>>>> +		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 :-)
>>
>> 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.

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:

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c
index 833987015b8b..4c33f30612c4 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
@@ -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;
@@ -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
@@ -238,11 +238,15 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
                  */
                 engine_rename(engine,
                               intel_engine_class_repr(engine->class),
-                             name_instance);
+                             engine->uabi_instance);
  
-               if (engine->uabi_class == I915_NO_UABI_CLASS)
+               if (uabi_class == I915_NO_UABI_CLASS)
                         continue;
  
+               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);


>>> 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.
>>>
>>>> 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?
> 
> 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