Chris Wilson <[email protected]> writes:
> During engine setup, we may find that some engines are fused off causing
> a misalignment between internal names and the instances seen by users,
> e.g. (I915_ENGINE_CLASS_VIDEO_DECODE, 1) may be vcs2 in hardware.
> Normally this is invisible to the user, but a few debug interfaces (and
> our own internal tracing) use the original HW name not the name the user
> would expect as formed their class:instance tuple. Replace our internal
> name with the uabi name for consistency, for example in error states.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111311
> Signed-off-by: Chris Wilson <[email protected]>
> ---
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 37 ++++----------------
> drivers/gpu/drm/i915/gt/intel_engine_user.c | 21 +++++++++++
> drivers/gpu/drm/i915/gt/intel_engine_user.h | 2 ++
> drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 19 +++++-----
> 4 files changed, 37 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index d0befd6c023a..d38c114b0964 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -55,30 +55,6 @@
>
> #define GEN8_LR_CONTEXT_OTHER_SIZE ( 2 * PAGE_SIZE)
>
> -struct engine_class_info {
> - const char *name;
> - u8 uabi_class;
> -};
> -
> -static const struct engine_class_info intel_engine_classes[] = {
> - [RENDER_CLASS] = {
> - .name = "rcs",
> - .uabi_class = I915_ENGINE_CLASS_RENDER,
> - },
> - [COPY_ENGINE_CLASS] = {
> - .name = "bcs",
> - .uabi_class = I915_ENGINE_CLASS_COPY,
> - },
> - [VIDEO_DECODE_CLASS] = {
> - .name = "vcs",
> - .uabi_class = I915_ENGINE_CLASS_VIDEO,
> - },
> - [VIDEO_ENHANCEMENT_CLASS] = {
> - .name = "vecs",
> - .uabi_class = I915_ENGINE_CLASS_VIDEO_ENHANCE,
> - },
> -};
> -
> #define MAX_MMIO_BASES 3
> struct engine_info {
> unsigned int hw_id;
> @@ -259,11 +235,11 @@ static u32 __engine_mmio_base(struct drm_i915_private
> *i915,
> return bases[i].base;
> }
>
> -static void __sprint_engine_name(char *name, const struct engine_info *info)
> +static void __sprint_engine_name(struct intel_engine_cs *engine)
> {
> - WARN_ON(snprintf(name, INTEL_ENGINE_CS_MAX_NAME, "%s%u",
> - intel_engine_classes[info->class].name,
> - info->instance) >= INTEL_ENGINE_CS_MAX_NAME);
> + GEM_WARN_ON(snprintf(engine->name, sizeof(engine->name), "%s'%u",
^ ?
unexpected
> + intel_engine_class_repr(engine),
> + engine->instance) >= sizeof(engine->name));
> }
>
> void intel_engine_set_hwsp_writemask(struct intel_engine_cs *engine, u32
> mask)
> @@ -292,8 +268,6 @@ static int intel_engine_setup(struct intel_gt *gt, enum
> intel_engine_id id)
> const struct engine_info *info = &intel_engines[id];
> struct intel_engine_cs *engine;
>
> - GEM_BUG_ON(info->class >= ARRAY_SIZE(intel_engine_classes));
> -
> BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH));
> BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH));
>
> @@ -317,11 +291,12 @@ static int intel_engine_setup(struct intel_gt *gt, enum
> intel_engine_id id)
> engine->i915 = gt->i915;
> engine->gt = gt;
> engine->uncore = gt->uncore;
> - __sprint_engine_name(engine->name, info);
> engine->hw_id = engine->guc_id = info->hw_id;
> engine->mmio_base = __engine_mmio_base(gt->i915, info->mmio_bases);
> +
> engine->class = info->class;
> engine->instance = info->instance;
> + __sprint_engine_name(engine);
>
> /*
> * To be overridden by the backend on setup. However to facilitate
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> index 68fda1ac3c60..03b4ace578d7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> @@ -127,6 +127,22 @@ static void set_scheduler_caps(struct drm_i915_private
> *i915)
> i915->caps.scheduler = 0;
> }
>
> +const char *intel_engine_class_repr(struct intel_engine_cs *engine)
I dont mind the repr, but we have been using _str.
> +{
> + static const char *uabi_names[] = {
> + [RENDER_CLASS] = "rcs",
> + [COPY_ENGINE_CLASS] = "bcs",
> + [VIDEO_DECODE_CLASS] = "vcs",
> + [VIDEO_ENHANCEMENT_CLASS] = "vecs",
> + };
> +
> + if (engine->class >= ARRAY_SIZE(uabi_names) ||
> + !uabi_names[engine->class])
> + return "xxx";
or "unknown" shrug.
> +
> + return uabi_names[engine->class];
> +}
> +
> void intel_engines_driver_register(struct drm_i915_private *i915)
> {
> u8 uabi_instances[4] = {};
> @@ -149,6 +165,11 @@ void intel_engines_driver_register(struct
> drm_i915_private *i915)
> GEM_BUG_ON(engine->uabi_class >= ARRAY_SIZE(uabi_instances));
> engine->uabi_instance = uabi_instances[engine->uabi_class]++;
>
> + /* Replace the internal name with the final user facing name */
> + scnprintf(engine->name, sizeof(engine->name), "%s%u",
> + intel_engine_class_repr(engine),
> + engine->uabi_instance);
Hmm, this was the reason for ' ?
-Mika
> +
> 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 9e5f113e5027..d1a08f336e70 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_user.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.h
> @@ -17,6 +17,8 @@ intel_engine_lookup_user(struct drm_i915_private *i915, u8
> class, u8 instance);
>
> unsigned int intel_engines_has_context_isolation(struct drm_i915_private
> *i915);
>
> +const char *intel_engine_class_repr(struct intel_engine_cs *engine);
> +
> void intel_engine_add_user(struct intel_engine_cs *engine);
> void intel_engines_driver_register(struct drm_i915_private *i915);
>
> diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_cs.c
> b/drivers/gpu/drm/i915/gt/selftest_engine_cs.c
> index cfaa6b296835..d0f4217b148a 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_engine_cs.c
> @@ -12,19 +12,16 @@ static int intel_mmio_bases_check(void *arg)
>
> for (i = 0; i < ARRAY_SIZE(intel_engines); i++) {
> const struct engine_info *info = &intel_engines[i];
> - char name[INTEL_ENGINE_CS_MAX_NAME];
> u8 prev = U8_MAX;
>
> - __sprint_engine_name(name, info);
> -
> for (j = 0; j < MAX_MMIO_BASES; j++) {
> u8 gen = info->mmio_bases[j].gen;
> u32 base = info->mmio_bases[j].base;
>
> if (gen >= prev) {
> - pr_err("%s: %s: mmio base for gen %x "
> - "is before the one for gen %x\n",
> - __func__, name, prev, gen);
> + pr_err("%s(class:%d, instance:%d): mmio base
> for gen %x is before the one for gen %x\n",
> + __func__, info->class, info->instance,
> + prev, gen);
> return -EINVAL;
> }
>
> @@ -32,17 +29,17 @@ static int intel_mmio_bases_check(void *arg)
> break;
>
> if (!base) {
> - pr_err("%s: %s: invalid mmio base (%x) "
> - "for gen %x at entry %u\n",
> - __func__, name, base, gen, j);
> + pr_err("%s(class:%d, instance:%d): invalid mmio
> base (%x) for gen %x at entry %u\n",
> + __func__, info->class, info->instance,
> + base, gen, j);
> return -EINVAL;
> }
>
> prev = gen;
> }
>
> - pr_info("%s: min gen supported for %s = %d\n",
> - __func__, name, prev);
> + pr_debug("%s: min gen supported for {class:%d, instance:%d} is
> %d\n",
> + __func__, info->class, info->instance, prev);
> }
>
> return 0;
> --
> 2.23.0.rc1
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
