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

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

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@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?

There is already a log further down in this function.

Daniele

>
> Regards,
>
> Tvrtko