Re: [PATCH 3/5] drm/i915/mtl: add GSC CS interrupt support

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx>]

On 10/31/2022 5:55 AM, Tvrtko Ursulin wrote:
> On 28/10/2022 18:00, Ceraolo Spurio, Daniele wrote:
> > On 10/28/2022 1:38 AM, Tvrtko Ursulin wrote:
> > > On 27/10/2022 23:15, Daniele Ceraolo Spurio wrote:
> > > > The GSC CS re-uses the same interrupt bits that the GSC used in older
> > > > platforms. This means that we can now have an engine interrupt coming
> > > > out of OTHER_CLASS, so we need to handle that appropriately.
> > > >
> > > > Signed-off-by: Daniele Ceraolo Spurio 
> > > > <daniele.ceraolospurio@xxxxxxxxx>
> > > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > > ---
> > > >   drivers/gpu/drm/i915/gt/intel_gt_irq.c | 78 
> > > > ++++++++++++++------------
> > > >   1 file changed, 43 insertions(+), 35 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c 
> > > > b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > > > index f26882fdc24c..34ff1ee7e931 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > > > @@ -81,35 +81,27 @@ gen11_other_irq_handler(struct intel_gt *gt, 
> > > > const u8 instance,
> > > >             instance, iir);
> > > >   }
> > > >   -static void
> > > > -gen11_engine_irq_handler(struct intel_gt *gt, const u8 class,
> > > > -             const u8 instance, const u16 iir)
> > > > +static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 
> > > > instance)
> > > >   {
> > > > -    struct intel_engine_cs *engine;
> > > > -
> > > > -    /*
> > > > -     * Platforms with standalone media have their media engines in 
> > > > another
> > > > -     * GT.
> > > > -     */
> > > > -    if (MEDIA_VER(gt->i915) >= 13 &&
> > > > -        (class == VIDEO_DECODE_CLASS || class == 
> > > > VIDEO_ENHANCEMENT_CLASS)) {
> > > > -        if (!gt->i915->media_gt)
> > > > -            goto err;
> > > > +    struct intel_gt *media_gt = gt->i915->media_gt;
> > > >   -        gt = gt->i915->media_gt;
> > > > +    /* we expect the non-media gt to be passed in */
> > > > +    GEM_BUG_ON(gt == media_gt);
> > > > +
> > > > +    if (!media_gt)
> > > > +        return gt;
> > > > +
> > > > +    switch (class) {
> > > > +    case VIDEO_DECODE_CLASS:
> > > > +    case VIDEO_ENHANCEMENT_CLASS:
> > > > +        return media_gt;
> > > > +    case OTHER_CLASS:
> > > > +        if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, 
> > > > GSC0))
> > > > +            return media_gt;
> > > > +        fallthrough;
> > > > +    default:
> > > > +        return gt;
> > > >       }
> > > > -
> > > > -    if (instance <= MAX_ENGINE_INSTANCE)
> > > > -        engine = gt->engine_class[class][instance];
> > > > -    else
> > > > -        engine = NULL;
> > > > -
> > > > -    if (likely(engine))
> > > > -        return intel_engine_cs_irq(engine, iir);
> > > > -
> > > > -err:
> > > > -    WARN_ONCE(1, "unhandled engine interrupt class=0x%x, 
> > > > instance=0x%x\n",
> > > > -          class, instance);
> > > >   }
> > > >     static void
> > > > @@ -118,12 +110,24 @@ gen11_gt_identity_handler(struct intel_gt 
> > > > *gt, const u32 identity)
> > > >       const u8 class = GEN11_INTR_ENGINE_CLASS(identity);
> > > >       const u8 instance = GEN11_INTR_ENGINE_INSTANCE(identity);
> > > >       const u16 intr = GEN11_INTR_ENGINE_INTR(identity);
> > > > +    struct intel_engine_cs *engine;
> > > >         if (unlikely(!intr))
> > > >           return;
> > > >   -    if (class <= COPY_ENGINE_CLASS || class == COMPUTE_CLASS)
> > > > -        return gen11_engine_irq_handler(gt, class, instance, intr);
> > > > +    /*
> > > > +     * Platforms with standalone media have the media and GSC 
> > > > engines in
> > > > +     * another GT.
> > > > +     */
> > > > +    gt = pick_gt(gt, class, instance);
> > > > +
> > > > +    if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE)
> > > > +        engine = gt->engine_class[class][instance];
> > > > +    else
> > > > +        engine = NULL;
> > > > +
> > > > +    if (engine)
> > > > +        return intel_engine_cs_irq(engine, intr);
> > >
> > > Drive by observation - you could fold the above two ifs into one 
> > > since engine appears unused afterwards.
> >
> > engine can be NULL in both branches of the if statement, so to get a 
> > unified if we'd have to do something like:
> >
> > if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) {
> >          struct intel_engine_cs *engine = 
> > gt->engine_class[class][instance];
> >          if (engine)
> >                  return intel_engine_cs_irq(engine, intr);
> > }
> >
> > Is this what you are suggesting?
>
> Right, two ifs are needed after all. Well at least it would avoid the 
> pointless engine = NULL assignment. Up to you.
>
> Absence of any out-of-range class/instance logging is intentional?

There is already a log further down in this function.

Daniele

> Regards,
>
> Tvrtko