[Intel-gfx] [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler

[Daniele Ceraolo Spurio]

On 12/03/18 08:23, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2018-03-12 15:09:31)
>>
>>
>> On 12/03/18 07:52, Chris Wilson wrote:
>>> Quoting Mika Kuoppala (2018-03-12 14:41:31)
>>>> Interrupt identity register we already read from hardware
>>>> contains engine class and instance fields. Leverage
>>>> these fields to find correct engine to handle the interrupt.
>>>>
>>>> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-160 (-160)
>>>> Function                                     old     new   delta
>>>> gen11_irq_handler                            764     604    -160
>>>> Total: Before=1506953, After=1506793, chg -0.01%
>>>>
>>>> v2: handle class/instance overflow correctly (Mika)
>>>>
>>>> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> Signed-off-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>>>> ---
>>>>    static void
>>>> @@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * const i915,
>>>>                   }
>>>>    
>>>>                   for_each_set_bit(bit, &intr_dw, 32) {
>>>> -                       const u16 iir = gen11_gt_engine_intr(i915, bank, bit);
>>>> +                       const u32 ident = gen11_gt_engine_identity(i915,
>>>> +                                                                  bank, bit);
>>>> +                       const u16 iir = ident & GEN11_INTR_ENGINE_MASK;
>>>> +                       u8 class, instance;
>>>> +                       struct intel_engine_cs *engine;
>>>>    
>>>>                           if (unlikely(!iir))
>>>>                                   continue;
>>>
>>> Now if (!ident) or actually just use u32 iir as we can pass it straight
>>> through to cs_irq_handler.
>>
>> Can't use (!ident) here because bit 31 (iir valid) or the class/instance
>> bits might be set when the iir is empty (because we had a buffered irq
>> that we actually already handled).
> 
> If there's a valid bit, surely that's the one we want to be testing?
> 

We do already test the valid bit in gen11_gt_engine_intr and return zero 
from that function if the bit doesn't go to 1 in a reasonable time.

> If the low iir bits are 0, that's fine. The question is whether it's
> common enough to worry about; and I note it's marked as unlikely() so
> it seems like we can just let it fallthrough and do nothing.
> -Chris
> 

Difficult to say how common it is going to be yet. If my reading of the 
specs is correct, double buffering is only active between reading 
GEN11_GT_INTR_DW and clearing it, so the iir might be empty if the 
second interrupt lands between reading GEN11_GT_INTR_DW and reading the 
iir (in which case both events will be handled in the first round).
I'm ok with just falling through for now and we can revisit once we have 
better data.

Daniele