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

[Michel Thierry]

Hi,

On 3/12/2018 7:41 AM, Mika Kuoppala wrote:
> 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>
> ---
>   drivers/gpu/drm/i915/i915_irq.c | 64 ++++++++++++++---------------------------
>   drivers/gpu/drm/i915/i915_reg.h |  2 ++
>   2 files changed, 23 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 828f3104488c..49816d0a380b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2733,47 +2733,9 @@ static void __fini_wedge(struct wedge_me *w)
>   	     (W)->i915;							\
>   	     __fini_wedge((W)))
>   
> -static void
> -gen11_gt_engine_irq_handler(struct drm_i915_private * const i915,
> -			    const unsigned int bank,
> -			    const unsigned int engine_n,
> -			    const u16 iir)
> -{
> -	struct intel_engine_cs ** const engine = i915->engine;
> -
> -	switch (bank) {
> -	case 0:
> -		switch (engine_n) {
> -
> -		case GEN11_RCS0:
> -			return gen8_cs_irq_handler(engine[RCS], iir);
> -
> -		case GEN11_BCS:
> -			return gen8_cs_irq_handler(engine[BCS], iir);
> -		}
> -	case 1:
> -		switch (engine_n) {
> -
> -		case GEN11_VCS(0):
> -			return gen8_cs_irq_handler(engine[_VCS(0)], iir);
> -		case GEN11_VCS(1):
> -			return gen8_cs_irq_handler(engine[_VCS(1)], iir);
> -		case GEN11_VCS(2):
> -			return gen8_cs_irq_handler(engine[_VCS(2)], iir);
> -		case GEN11_VCS(3):
> -			return gen8_cs_irq_handler(engine[_VCS(3)], iir);
> -
> -		case GEN11_VECS(0):
> -			return gen8_cs_irq_handler(engine[_VECS(0)], iir);
> -		case GEN11_VECS(1):
> -			return gen8_cs_irq_handler(engine[_VECS(1)], iir);
> -		}
> -	}
> -}
> -
>   static u32
> -gen11_gt_engine_intr(struct drm_i915_private * const i915,
> -		     const unsigned int bank, const unsigned int bit)
> +gen11_gt_engine_identity(struct drm_i915_private * const i915,
> +			 const unsigned int bank, const unsigned int bit)
>   {
>   	void __iomem * const regs = i915->regs;
>   	u32 timeout_ts;
> @@ -2800,7 +2762,7 @@ gen11_gt_engine_intr(struct drm_i915_private * const i915,
>   	raw_reg_write(regs, GEN11_INTR_IDENTITY_REG(bank),
>   		      GEN11_INTR_DATA_VALID);
>   
> -	return ident & GEN11_INTR_ENGINE_MASK;
> +	return ident;
>   }
>   
>   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;
>   
> -			gen11_gt_engine_irq_handler(i915, bank, bit, iir);
> +			class = GEN11_INTR_ENGINE_CLASS(ident);
> +			if (unlikely(class >= MAX_ENGINE_CLASS))

MAX_ENGINE_CLASS points to the last CLASS, so this should be

	if (unlikely(class > MAX_ENGINE_CLASS))

as you originally had in the v1 of this patch.
(GuC interrupts will be reported with class = OTHER_CLASS).

> +				continue;
> +
> +			instance = GEN11_INTR_ENGINE_INSTANCE(ident);
> +			if (unlikely(instance >= MAX_ENGINE_INSTANCE))
> +				continue;
> +

Same here, MAX_ENGINE_INSTANCE is inclusive too.

That's why we have the GEM_WARN_ON's in intel_engine_setup().

> +			engine = i915->engine_class[class][instance];
> +			if (unlikely(!engine))
> +				continue;
> +
> +			gen8_cs_irq_handler(engine, iir);
>   		}
>   
>   		/* Clear must be after shared has been served for engine */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e6a8c0ee7df1..065825290482 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7118,6 +7118,8 @@ enum {
>   #define GEN11_INTR_IDENTITY_REG1	_MMIO(0x190064)
>   #define  GEN11_INTR_DATA_VALID		(1 << 31)
>   #define  GEN11_INTR_ENGINE_MASK		(0xffff)
> +#define  GEN11_INTR_ENGINE_CLASS(x)	(((x) & GENMASK(18, 16)) >> 16)
> +#define  GEN11_INTR_ENGINE_INSTANCE(x)	(((x) & GENMASK(25, 20)) >> 20)
>   
>   #define GEN11_INTR_IDENTITY_REG(x)	_MMIO(0x190060 + (x * 4))
>   
>