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 12:08, Tvrtko Ursulin wrote:
> 
> 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;

Here you just add the ccs skip condition.

Anyway.. I rushed it a bit so see what you think.

Regards,

Tvrtko

> 
> +               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