[PATCH v2 2/2] drm/i915/gt: Enable only one CCS for compute workload
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Wed Feb 21 12:11:44 UTC 2024
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
More information about the dri-devel
mailing list