[Intel-gfx] [PATCH] drm/i915/gsc: Mark internal GSC engine with reserved uabi class

[Daniele Ceraolo Spurio]

On 11/15/2023 3:02 AM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> 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
> internal class as -1.
>
> At the same time remove special handling for the name and complete
> the uabi_classes array so internal class is automatically correctly
> assigned.
>
> Engine will show as 65535:0 other0 in the logs/traces which should
> be unique enough.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Fixes: 194babe26bdc ("drm/i915/mtl: don't expose GSC command streamer to the user")
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> ---
> Daniele I borrowed most of your commit text as is, hope you don't mind but
> I was lazy. See if you like this solution. It is also untested so lets see.

I'm ok with this approach. As you said the naming is unique so we can 
easily identify the engine. I've tested this locally with a small change 
(see below) and I see the expected values in the logs.

> ---
>   drivers/gpu/drm/i915/gt/intel_engine_user.c | 37 ++++++++++++---------
>   1 file changed, 21 insertions(+), 16 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..7693ccfac1f9 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> @@ -41,12 +41,15 @@ void intel_engine_add_user(struct intel_engine_cs *engine)
>   	llist_add(&engine->uabi_llist, &engine->i915->uabi_engines_llist);
>   }
>   
> +#define I915_NO_UABI_CLASS (-1)

I see the lkp is complaining about using this for comparison against a 
u16. When I locally tried to reduce this to u16 my compiler also 
complained that we're assigning it to a u8 in the uabi_classes array, so 
I've just set I915_NO_UABI_CLASS directly to 255 and it all worked as 
expected. With that fix, or an alternative change to work with all the 
involved types:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>

Daniele

> +
>   static const u8 uabi_classes[] = {
>   	[RENDER_CLASS] = I915_ENGINE_CLASS_RENDER,
>   	[COPY_ENGINE_CLASS] = I915_ENGINE_CLASS_COPY,
>   	[VIDEO_DECODE_CLASS] = I915_ENGINE_CLASS_VIDEO,
>   	[VIDEO_ENHANCEMENT_CLASS] = I915_ENGINE_CLASS_VIDEO_ENHANCE,
>   	[COMPUTE_CLASS] = I915_ENGINE_CLASS_COMPUTE,
> +	[OTHER_CLASS] = I915_NO_UABI_CLASS, /* Not exposed to users, no uabi class. */
>   };
>   
>   static int engine_cmp(void *priv, const struct list_head *A,
> @@ -200,6 +203,7 @@ 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;
>   	struct legacy_ring ring = {};
>   	struct list_head *it, *next;
>   	struct rb_node **p, *prev;
> @@ -216,27 +220,28 @@ 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;
> -		}
> -
>   		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;
>   
> -		GEM_BUG_ON(engine->uabi_class >=
> -			   ARRAY_SIZE(i915->engine_uabi_class_count));
> -		engine->uabi_instance =
> -			i915->engine_uabi_class_count[engine->uabi_class]++;
> -
> -		/* Replace the internal name with the final user facing name */
> +		/*
> +		 * Replace the internal name with the final user and log facing
> +		 * name.
> +		 */
>   		engine_rename(engine,
>   			      intel_engine_class_repr(engine->class),
> -			      engine->uabi_instance);
> +			      name_instance);
> +
> +		if (engine->uabi_class == I915_NO_UABI_CLASS)
> +			continue;
>   
>   		rb_link_node(&engine->uabi_node, prev, p);
>   		rb_insert_color(&engine->uabi_node, &i915->uabi_engines);