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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> ---
>>>   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