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@xxxxxxxxx>Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> Cc: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx> --- 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.cindex 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 3I 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?
Otherwise I'll try the 3-bits thing and see how that looks. 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, TvrtkoRegards, 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.hindex 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.hindex 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.hindex 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;
