Re: [PATCH 1/3] drm/i915: Rename engines with to match their user interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux