[Intel-gfx] [PATCH] drm/i915/gsc: Assign a uabi class number to the GSC CS
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Tue Nov 14 21:09:26 UTC 2023
On 11/14/2023 9:20 AM, Tvrtko Ursulin wrote:
>
> On 14/11/2023 17:03, Daniele Ceraolo Spurio wrote:
>> On 11/13/2023 8:46 AM, Tvrtko Ursulin wrote:
>>> On 13/11/2023 15:51, Daniele Ceraolo Spurio wrote:
>>>> On 11/10/2023 4:00 AM, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 09/11/2023 23:53, Daniele Ceraolo Spurio wrote:
>>>>>> The GSC CS is not exposed to the user, so we skipped assigning a
>>>>>> uabi
>>>>>> class number for it. However, the trace logs use the uabi class and
>>>>>> instance to identify the engine, so leaving uabi class unset
>>>>>> makes the
>>>>>> GSC CS show up as the RCS in those logs.
>>>>>> Given that the engine is not exposed to the user, we can't add a new
>>>>>> case in the uabi enum, so we insted internally define a kernel
>>>>>> reserved class using the next free number.
>>>>>>
>>>>>> Fixes: 194babe26bdc ("drm/i915/mtl: don't expose GSC command
>>>>>> streamer to the user")
>>>>>> Signed-off-by: Daniele Ceraolo Spurio
>>>>>> <daniele.ceraolospurio at intel.com>
>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>>>>>> Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
>>>>>> Cc: Matt Roper <matthew.d.roper at intel.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/i915/gt/intel_engine_user.c | 17 ++++++++---------
>>>>>> drivers/gpu/drm/i915/gt/intel_engine_user.h | 4 ++++
>>>>>> drivers/gpu/drm/i915/i915_drm_client.h | 2 +-
>>>>>> drivers/gpu/drm/i915/i915_drv.h | 2 +-
>>>>>> 4 files changed, 14 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>>>>> index 118164ddbb2e..3fd32bedd6e7 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
>>>>>> @@ -47,6 +47,7 @@ static const u8 uabi_classes[] = {
>>>>>> [VIDEO_DECODE_CLASS] = I915_ENGINE_CLASS_VIDEO,
>>>>>> [VIDEO_ENHANCEMENT_CLASS] = I915_ENGINE_CLASS_VIDEO_ENHANCE,
>>>>>> [COMPUTE_CLASS] = I915_ENGINE_CLASS_COMPUTE,
>>>>>> + [OTHER_CLASS] = I915_KERNEL_RSVD_CLASS,
>>>>>
>>>>> Could we set it to -1 (aka no uabi class) to avoid needing to
>>>>> maintain the new macros?
>>>>>
>>>>> And then just teach intel_engines_driver_register to skip -1.
>>>>> Would also need teaching engine_rename to handle -1.
>>>>>
>>>>> Might end up a smaller and more maintainable patch - worth a try
>>>>> do you think?
>>>>
>>>> That was my initial idea as well, but the issue with this approach
>>>> is the engine_uabi_class_count[] array, which is sized based on the
>>>> number of uabi classes, so having class -1 would needlessly
>>>> increase its size a lot even when using a u8. I thought about
>>>> limiting the class entry to 3
>>>
>>> I was thinking the -1 entry wouldn't be in that array since -1 is
>>> not uabi class by its very definition. It is not reachable from the
>>> outside so no need to be there.
>>
>> The issue there is that the array is used to assign uabi_instance,
>> but I can hardcode that to 0 for now since we don't expect any new
>> engines of that class. would that work for you?
>
> My issue is that the starting position here is GSC engine has no uabi
> class, therefore it shouldn't have an uabi instance either, therefore
> the fact you cannot use the array called engine_*uabi*_class_count to
> assign it something which it shouldn't have, shouldn't be an argument. ;)
Agreed. The problem is that we use the uabi_class/instance for things
that aren't strictly uabi (i.e. the traces) so we need to assign a value
to those even if we don't need it for actual uabi purposes.
>
>> Otherwise I'll try the 3-bits thing and see how that looks.
>
> 3-bits.. I have to admit I don't know what you mean by that?
I was referring to limiting the uabi class index to 3 bits to limit the
max array size to 8 and use -1 for the GSC uabi_class.
>
> I can try to sketch out tomorrow what I have in mind. Lets go from
> there. I might fail and concede there is nothing to be gained by using
> a different approach.
sounds good.
Thanks,
Daniele
>
> Regards,
>
> Tvrtko
>
>>
>> Daniele
>>
>>>
>>>> bits so the array would max out at 8 entries, but that seemed to be
>>>> getting a bit too convoluted. I can give it a go if you think it's
>>>> be cleaner overall.
>>>
>>> I had a feeling it would be, but without trying it out I won't claim
>>> 100%.
>>>
>>>> Note that this patch does not introduce any new macros that would
>>>> need to be maintained. I915_LAST_UABI_ENGINE_CLASS already existed
>>>> (I just moved it from one file to another) and is the only one that
>>>> changes when a new "real" uabi class is added; the other defines
>>>> are based on this one. This also implies that if a new uabi class
>>>> is added then I915_KERNEL_RSVD_CLASS would be bumped to the next
>>>> free number, which would cause the GSC to show as a different uabi
>>>> class in newer logs; considering that i915 is on its way out, a new
>>>> class seems very unlikely so I thought it'd be an acceptable
>>>> compromise to keep things simple.
>>>>
>>>>>
>>>>>> };
>>>>>> static int engine_cmp(void *priv, const struct list_head *A,
>>>>>> @@ -138,7 +139,7 @@ const char *intel_engine_class_repr(u8 class)
>>>>>> [COPY_ENGINE_CLASS] = "bcs",
>>>>>> [VIDEO_DECODE_CLASS] = "vcs",
>>>>>> [VIDEO_ENHANCEMENT_CLASS] = "vecs",
>>>>>> - [OTHER_CLASS] = "other",
>>>>>> + [OTHER_CLASS] = "gsc",
>>>>>
>>>>> Maybe unrelated?
>>>>
>>>> no. Before this patch, we hardcoded "gsc" below when calling
>>>> engine_rename() for it. With this patch, we use the name from this
>>>> array, so it needs to be updated. The GEM_WARN_ON below was added
>>>> to make sure we don't get different engines in OTHER_CLASS that
>>>> might not match the "gsc" naming.
>>>
>>> Ah okay, one special casing for another, a wash I guess.
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Tvrtko
>>>>>
>>>>>> [COMPUTE_CLASS] = "ccs",
>>>>>> };
>>>>>> @@ -216,14 +217,8 @@ void intel_engines_driver_register(struct
>>>>>> drm_i915_private *i915)
>>>>>> if (intel_gt_has_unrecoverable_error(engine->gt))
>>>>>> continue; /* ignore incomplete engines */
>>>>>> - /*
>>>>>> - * We don't want to expose the GSC engine to the users,
>>>>>> but we
>>>>>> - * still rename it so it is easier to identify in the
>>>>>> debug logs
>>>>>> - */
>>>>>> - if (engine->id == GSC0) {
>>>>>> - engine_rename(engine, "gsc", 0);
>>>>>> - continue;
>>>>>> - }
>>>>>> + /* The only engine we expect in OTHER_CLASS is GSC0 */
>>>>>> + GEM_WARN_ON(engine->class == OTHER_CLASS && engine->id
>>>>>> != GSC0);
>>>>>> GEM_BUG_ON(engine->class >= ARRAY_SIZE(uabi_classes));
>>>>>> engine->uabi_class = uabi_classes[engine->class];
>>>>>> @@ -238,6 +233,10 @@ void intel_engines_driver_register(struct
>>>>>> drm_i915_private *i915)
>>>>>> intel_engine_class_repr(engine->class),
>>>>>> engine->uabi_instance);
>>>>>> + /* We don't want to expose the GSC engine to the users */
>>>>>> + if (engine->id == GSC0)
>>>>>> + continue;
>>>>>> +
>>>>>> rb_link_node(&engine->uabi_node, prev, p);
>>>>>> rb_insert_color(&engine->uabi_node, &i915->uabi_engines);
>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.h
>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_user.h
>>>>>> index 3dc7e8ab9fbc..dd31805b2a5a 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.h
>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.h
>>>>>> @@ -11,6 +11,10 @@
>>>>>> struct drm_i915_private;
>>>>>> struct intel_engine_cs;
>>>>>> +#define I915_LAST_UABI_ENGINE_CLASS I915_ENGINE_CLASS_COMPUTE
>>>>>> +#define I915_KERNEL_RSVD_CLASS (I915_LAST_UABI_ENGINE_CLASS + 1)
>>>>>> +#define I915_MAX_UABI_CLASSES (I915_KERNEL_RSVD_CLASS + 1)
>>>>>> +
>>>>>> struct intel_engine_cs *
>>>>>> intel_engine_lookup_user(struct drm_i915_private *i915, u8
>>>>>> class, u8 instance);
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.h
>>>>>> b/drivers/gpu/drm/i915/i915_drm_client.h
>>>>>> index 67816c912bca..c42cb2511348 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_drm_client.h
>>>>>> +++ b/drivers/gpu/drm/i915/i915_drm_client.h
>>>>>> @@ -12,7 +12,7 @@
>>>>>> #include <uapi/drm/i915_drm.h>
>>>>>> -#define I915_LAST_UABI_ENGINE_CLASS I915_ENGINE_CLASS_COMPUTE
>>>>>> +#include "gt/intel_engine_user.h"
>>>>>> struct drm_file;
>>>>>> struct drm_printer;
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>>>> index f3be9033a93f..a718b4cb5a2d 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>>> @@ -238,7 +238,7 @@ struct drm_i915_private {
>>>>>> struct list_head uabi_engines_list;
>>>>>> struct rb_root uabi_engines;
>>>>>> };
>>>>>> - unsigned int
>>>>>> engine_uabi_class_count[I915_LAST_UABI_ENGINE_CLASS + 1];
>>>>>> + unsigned int engine_uabi_class_count[I915_MAX_UABI_CLASSES];
>>>>>> /* protects the irq masks */
>>>>>> spinlock_t irq_lock;
>>>>
>>
More information about the Intel-gfx
mailing list