Re: [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_handler764 604-160
Total: Before=1506953, After=1506793, chg -0.01%

v2: handle class/instance overflow correctly (Mika)

Suggested-by: Daniele Ceraolo Spurio 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Signed-off-by: Mika Kuoppala 
---
  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, _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 

Re: [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_handler764 604-160
Total: Before=1506953, After=1506793, chg -0.01%

v2: handle class/instance overflow correctly (Mika)

Suggested-by: Daniele Ceraolo Spurio 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Signed-off-by: Mika Kuoppala 
---
   static void
@@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * const 
i915,
  }
   
  for_each_set_bit(bit, _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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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_handler764 604-160
> >> Total: Before=1506953, After=1506793, chg -0.01%
> >>
> >> v2: handle class/instance overflow correctly (Mika)
> >>
> >> Suggested-by: Daniele Ceraolo Spurio 
> >> Cc: Daniele Ceraolo Spurio 
> >> Cc: Chris Wilson 
> >> Cc: Tvrtko Ursulin 
> >> Signed-off-by: Mika Kuoppala 
> >> ---
> >>   static void
> >> @@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * 
> >> const i915,
> >>  }
> >>   
> >>  for_each_set_bit(bit, _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?

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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniele Ceraolo Spurio]

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_handler764 604-160
Total: Before=1506953, After=1506793, chg -0.01%

v2: handle class/instance overflow correctly (Mika)

Suggested-by: Daniele Ceraolo Spurio 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
Cc: Tvrtko Ursulin 
Signed-off-by: Mika Kuoppala 
---
  static void
@@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * const 
i915,
 }
  
 for_each_set_bit(bit, _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).





-   gen11_gt_engine_irq_handler(i915, bank, bit, iir);
+   class = GEN11_INTR_ENGINE_CLASS(ident);
+   if (unlikely(class >= MAX_ENGINE_CLASS))
+   continue;
+
+   instance = GEN11_INTR_ENGINE_INSTANCE(ident);
+   if (unlikely(instance >= MAX_ENGINE_INSTANCE))
+   continue;
+
+   engine = i915->engine_class[class][instance];
+   if (unlikely(!engine))
+   continue;


Spurious interrupts be spurious; but we are can enable interrupts on a
per-engine basis, right?


irq enable registers are per-class for gen11.

Daniele


-Chris


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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_handler764 604-160
> Total: Before=1506953, After=1506793, chg -0.01%
> 
> v2: handle class/instance overflow correctly (Mika)
> 
> Suggested-by: Daniele Ceraolo Spurio 
> Cc: Daniele Ceraolo Spurio 
> Cc: Chris Wilson 
> Cc: Tvrtko Ursulin 
> Signed-off-by: Mika Kuoppala 
> ---
>  static void
> @@ -2825,12 +2787,28 @@ gen11_gt_irq_handler(struct drm_i915_private * const 
> i915,
> }
>  
> for_each_set_bit(bit, _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.

> -   gen11_gt_engine_irq_handler(i915, bank, bit, iir);
> +   class = GEN11_INTR_ENGINE_CLASS(ident);
> +   if (unlikely(class >= MAX_ENGINE_CLASS))
> +   continue;
> +
> +   instance = GEN11_INTR_ENGINE_INSTANCE(ident);
> +   if (unlikely(instance >= MAX_ENGINE_INSTANCE))
> +   continue;
> +
> +   engine = i915->engine_class[class][instance];
> +   if (unlikely(!engine))
> +   continue;

Spurious interrupts be spurious; but we are can enable interrupts on a
per-engine basis, right?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx