Quoting Mika Kuoppala (2019-08-07 11:31:14) > Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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 <chris@xxxxxxxxxxxxxxxxxx> > > --- > > 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 I needed some short way of identifying the proto-name from the final name for debugging. (It has to be kept short to fit inside the small embedded name[].) After some deliberation, I went with class' as being easy to distinguish and not likely to match other shorthand. I thought about keeping engine->name and engine->uabi_name, but I think that leads to longer term confusion. Our debug traces will say one thing, but uAPI another, with no clear correlation. It's only a name and I expect we will revisit the topic again. > > + 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. We're a bit tight on characters. > > + > > + 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 ' ? Yup. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
