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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 11/16/2023 12:44 AM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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.
>
> v2:
>   * Fix uabi class u8 vs u16 type confusion.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: 194babe26bdc ("drm/i915/mtl: don't expose GSC command streamer to the user")
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> # v1

My r-b stands.

Thanks,
Daniele

> ---
>   drivers/gpu/drm/i915/gt/intel_engine_user.c | 39 ++++++++++++---------
>   1 file changed, 22 insertions(+), 17 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..833987015b8b 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);
>   }
>   
> -static const u8 uabi_classes[] = {
> +#define I915_NO_UABI_CLASS ((u16)(-1))
> +
> +static const u16 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);