Re: [PATCH 06/21] drm/i915/guc: Use guc_class instead of engine_class in fw interface

[Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>]

On 30/08/2018 14:29, Lis, Tomasz wrote:
> On 2018-08-30 02:16, Lionel Landwerlin wrote:
> > On 29/08/2018 20:58, Michel Thierry wrote:
> > > +Lionel
> > > (please see below as this touches the lrca format & relates to OA 
> > > reporting too)
> > >
> > > On 8/29/2018 12:10 PM, Michal Wajdeczko wrote:
> > > > Until now the GuC and HW engine class has been the same, which allowed
> > > > us to use them interchangeable. But it is better to start doing the
> > > > right thing and use the GuC definitions for the firmware interface.
> > > >
> > > > We also keep the same class id in the ctx descriptor to be able to 
> > > > have
> > > > the same values in the driver and firmware logs.
> > > >
> > > > Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > > > Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
> > > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > > Cc: Tomasz Lis <tomasz.lis@xxxxxxxxx>
> Tested-by: Tomasz Lis <tomasz.lis@xxxxxxxxx>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_engine_cs.c  | 13 +++++++++++++
> > > >   drivers/gpu/drm/i915/intel_guc_fwif.h   |  7 +++++++
> > > >   drivers/gpu/drm/i915/intel_lrc.c        | 10 +++++++++-
> > > >   drivers/gpu/drm/i915/intel_ringbuffer.h |  2 ++
> > > >   4 files changed, 31 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> > > > b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > > index 1a34e8f..bc81354 100644
> > > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > > > @@ -85,6 +85,7 @@ struct engine_info {
> > > >       unsigned int hw_id;
> > > >       unsigned int uabi_id;
> > > >       u8 class;
> > > > +    u8 guc_class;
> > > >       u8 instance;
> > > >       /* mmio bases table *must* be sorted in reverse gen order */
> > > >       struct engine_mmio_base {
> > > > @@ -98,6 +99,7 @@ struct engine_info {
> > > >           .hw_id = RCS_HW,
> > > >           .uabi_id = I915_EXEC_RENDER,
> > > >           .class = RENDER_CLASS,
> > > > +        .guc_class = GUC_RENDER_CLASS,
> > > >           .instance = 0,
> > > >           .mmio_bases = {
> > > >               { .gen = 1, .base = RENDER_RING_BASE }
> > > > @@ -107,6 +109,7 @@ struct engine_info {
> > > >           .hw_id = BCS_HW,
> > > >           .uabi_id = I915_EXEC_BLT,
> > > >           .class = COPY_ENGINE_CLASS,
> > > > +        .guc_class = GUC_BLITTER_CLASS,
> > > >           .instance = 0,
> > > >           .mmio_bases = {
> > > >               { .gen = 6, .base = BLT_RING_BASE }
> > > > @@ -116,6 +119,7 @@ struct engine_info {
> > > >           .hw_id = VCS_HW,
> > > >           .uabi_id = I915_EXEC_BSD,
> > > >           .class = VIDEO_DECODE_CLASS,
> > > > +        .guc_class = GUC_VIDEO_CLASS,
> > > >           .instance = 0,
> > > >           .mmio_bases = {
> > > >               { .gen = 11, .base = GEN11_BSD_RING_BASE },
> > > > @@ -127,6 +131,7 @@ struct engine_info {
> > > >           .hw_id = VCS2_HW,
> > > >           .uabi_id = I915_EXEC_BSD,
> > > >           .class = VIDEO_DECODE_CLASS,
> > > > +        .guc_class = GUC_VIDEO_CLASS,
> > > >           .instance = 1,
> > > >           .mmio_bases = {
> > > >               { .gen = 11, .base = GEN11_BSD2_RING_BASE },
> > > > @@ -137,6 +142,7 @@ struct engine_info {
> > > >           .hw_id = VCS3_HW,
> > > >           .uabi_id = I915_EXEC_BSD,
> > > >           .class = VIDEO_DECODE_CLASS,
> > > > +        .guc_class = GUC_VIDEO_CLASS,
> > > >           .instance = 2,
> > > >           .mmio_bases = {
> > > >               { .gen = 11, .base = GEN11_BSD3_RING_BASE }
> > > > @@ -146,6 +152,7 @@ struct engine_info {
> > > >           .hw_id = VCS4_HW,
> > > >           .uabi_id = I915_EXEC_BSD,
> > > >           .class = VIDEO_DECODE_CLASS,
> > > > +        .guc_class = GUC_VIDEO_CLASS,
> > > >           .instance = 3,
> > > >           .mmio_bases = {
> > > >               { .gen = 11, .base = GEN11_BSD4_RING_BASE }
> > > > @@ -155,6 +162,7 @@ struct engine_info {
> > > >           .hw_id = VECS_HW,
> > > >           .uabi_id = I915_EXEC_VEBOX,
> > > >           .class = VIDEO_ENHANCEMENT_CLASS,
> > > > +        .guc_class = GUC_VIDEOENHANCE_CLASS,
> > > >           .instance = 0,
> > > >           .mmio_bases = {
> > > >               { .gen = 11, .base = GEN11_VEBOX_RING_BASE },
> > > > @@ -165,6 +173,7 @@ struct engine_info {
> > > >           .hw_id = VECS2_HW,
> > > >           .uabi_id = I915_EXEC_VEBOX,
> > > >           .class = VIDEO_ENHANCEMENT_CLASS,
> > > > +        .guc_class = GUC_VIDEOENHANCE_CLASS,
> > > >           .instance = 1,
> > > >           .mmio_bases = {
> > > >               { .gen = 11, .base = GEN11_VEBOX2_RING_BASE }
> > > > @@ -276,6 +285,9 @@ static void __sprint_engine_name(char *name, 
> > > > const struct engine_info *info)
> > > >       if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS))
> > > >           return -EINVAL;
> > > >   +    if (GEM_WARN_ON(info->guc_class >= GUC_MAX_ENGINE_CLASSES))
> > > > +        return -EINVAL;
> > > > +
> > > >       if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE))
> > > >           return -EINVAL;
> > > >   @@ -291,6 +303,7 @@ static void __sprint_engine_name(char *name, 
> > > > const struct engine_info *info)
> > > >       engine->i915 = dev_priv;
> > > >       __sprint_engine_name(engine->name, info);
> > > >       engine->hw_id = engine->guc_id = info->hw_id;
> > > > +    engine->guc_class = info->guc_class;
> > > >       engine->mmio_base = __engine_mmio_base(dev_priv, 
> > > > info->mmio_bases);
> > > >       engine->class = info->class;
> > > >       engine->instance = info->instance;
> > > > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h 
> > > > b/drivers/gpu/drm/i915/intel_guc_fwif.h
> > > > index 963da91..5b7a05b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> > > > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> > > > @@ -39,6 +39,13 @@
> > > >   #define GUC_VIDEO_ENGINE2        4
> > > >   #define GUC_MAX_ENGINES_NUM        (GUC_VIDEO_ENGINE2 + 1)
> > > >   +#define GUC_RENDER_CLASS    0
> > > > +#define GUC_VIDEO_CLASS        1
> > > > +#define GUC_VIDEOENHANCE_CLASS    2
> > > > +#define GUC_BLITTER_CLASS    3
> > > > +#define GUC_RESERVED_CLASS    4
> > > > +#define GUC_MAX_ENGINE_CLASSES    (GUC_RESERVED_CLASS + 1)
> > > > +
> > > >   /* Work queue item header definitions */
> > > >   #define WQ_STATUS_ACTIVE        1
> > > >   #define WQ_STATUS_SUSPENDED        2
> > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> > > > b/drivers/gpu/drm/i915/intel_lrc.c
> > > > index f8ceb9c..f4b9972 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > > @@ -249,7 +249,15 @@ static inline bool need_preempt(const struct 
> > > > intel_engine_cs *engine,
> > > >             /* TODO: decide what to do with SW counter (bits 55-60) */
> > > >   -        desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT;
> > > > +        /*
> > > > +         * Although GuC will never see this upper part as it fills
> > > > +         * its own descriptor, using the guc_class here will help 
> > > > keep
> > > > +         * the i915 and firmware logs in sync.
> > > > +         */
> > > > +        if (HAS_GUC_SCHED(ctx->i915))
> > > > +            desc |= (u64)engine->guc_class << 
> > > > GEN11_ENGINE_CLASS_SHIFT;
> > > > +        else
> > > > +            desc |= (u64)engine->class << GEN11_ENGINE_CLASS_SHIFT;
> > > >                                   /* bits 61-63 */
> > >
> > > OA also uses this upper part (see oa_get_render_ctx_id), so it's 
> > > something to be aware of.
> > >
> > > My opinion is that it's useful to have the lrca matching the 
> > > firmware logs, but OA should account of this change at it receives 
> > > what the fw sent to the hw. Which one is more important is for 
> > > others to decide (plus it only becomes a problem when engine-class 
> > > and guc-class start to deviate).
> > >
> > > Acked-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> > >
> > > -Michel
> >
> >
> > If GuC still behaves the same as the Gen9 firmware I was testing 
> > with, parts of the upper 32bits of the descriptor will end up in HW.
> >
> > Just make sure i915_perf.c is in sync with intel_lrc.c and it should 
> > be fine :)
> Tested on KBL; works fine for both enable_guc=2 and enable_guc=3.
>
> ./tests/perf --run-subtest=gen8-unprivileged-single-ctx-counters
> IGT-Version: 1.22-g11db680 (x86_64) (Linux: 4.19.0-rc1tli+ x86_64)
> Subtest gen8-unprivileged-single-ctx-counters: SUCCESS (0,058s)


But this patch affect Gen11, I think this is where you need to test, not 
on KBL.


> -Tomasz
> > > >       } else {
> > > >           GEM_BUG_ON(ctx->hw_id >= BIT(GEN8_CTX_ID_WIDTH));
> > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
> > > > b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > > > index 3f6920d..f47009f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > > > @@ -350,7 +350,9 @@ struct intel_engine_cs {
> > > >         enum intel_engine_id id;
> > > >       unsigned int hw_id;
> > > > +
> > > >       unsigned int guc_id;
> > > > +    u8 guc_class;
> > > >         u8 uabi_id;
> > > >       u8 uabi_class;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx