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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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?

Regards,

Tvrtko