All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler
@ 2018-03-12 14:33 Mika Kuoppala
  2018-03-12 14:41 ` Mika Kuoppala
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Mika Kuoppala @ 2018-03-12 14:33 UTC (permalink / raw)
  To: intel-gfx

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%

Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@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..67240112bb14 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))
+				continue;
+
+			instance = GEN11_INTR_ENGINE_INSTANCE(ident);
+			if (unlikely(instance > MAX_ENGINE_INSTANCE))
+				continue;
+
+			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))
 
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler
  2018-03-12 14:33 [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler Mika Kuoppala
@ 2018-03-12 14:41 ` Mika Kuoppala
  2018-03-12 14:52   ` Chris Wilson
  2018-03-12 22:47   ` Michel Thierry
  2018-03-12 15:50 ` ✓ Fi.CI.BAT: success for drm/i915/icl: Use hw engine class, instance to find irq handler (rev2) Patchwork
  2018-03-12 20:20 ` ✗ Fi.CI.IGT: failure " Patchwork
  2 siblings, 2 replies; 9+ messages in thread
From: Mika Kuoppala @ 2018-03-12 14:41 UTC (permalink / raw)
  To: intel-gfx

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@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@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))
+				continue;
+
+			instance = GEN11_INTR_ENGINE_INSTANCE(ident);
+			if (unlikely(instance >= MAX_ENGINE_INSTANCE))
+				continue;
+
+			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))
 
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler
  2018-03-12 14:41 ` Mika Kuoppala
@ 2018-03-12 14:52   ` Chris Wilson
  2018-03-12 15:09     ` Daniele Ceraolo Spurio
  2018-03-12 22:47   ` Michel Thierry
  1 sibling, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2018-03-12 14:52 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

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@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@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.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler
  2018-03-12 14:52   ` Chris Wilson
@ 2018-03-12 15:09     ` Daniele Ceraolo Spurio
  2018-03-12 15:23       ` Chris Wilson
  0 siblings, 1 reply; 9+ messages in thread
From: Daniele Ceraolo Spurio @ 2018-03-12 15:09 UTC (permalink / raw)
  To: Chris Wilson, Mika Kuoppala, intel-gfx



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@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@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).

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler
  2018-03-12 15:09     ` Daniele Ceraolo Spurio
@ 2018-03-12 15:23       ` Chris Wilson
  2018-03-12 16:01         ` Daniele Ceraolo Spurio
  0 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2018-03-12 15:23 UTC (permalink / raw)
  To: Daniele Ceraolo Spurio, Mika Kuoppala, intel-gfx

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@intel.com>
> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@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?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/icl: Use hw engine class, instance to find irq handler (rev2)
  2018-03-12 14:33 [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler Mika Kuoppala
  2018-03-12 14:41 ` Mika Kuoppala
@ 2018-03-12 15:50 ` Patchwork
  2018-03-12 20:20 ` ✗ Fi.CI.IGT: failure " Patchwork
  2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-03-12 15:50 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/icl: Use hw engine class, instance to find irq handler (rev2)
URL   : https://patchwork.freedesktop.org/series/39793/
State : success

== Summary ==

Series 39793v2 drm/i915/icl: Use hw engine class, instance to find irq handler
https://patchwork.freedesktop.org/api/1.0/series/39793/revisions/2/mbox/

---- Known issues:

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:431s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:436s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:385s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:538s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:298s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:514s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:511s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:510s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:497s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:408s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:582s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:594s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:425s
fi-gdg-551       total:288  pass:180  dwarn:0   dfail:0   fail:0   skip:108 time:316s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:533s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:403s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:426s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:474s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:435s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:473s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:468s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:512s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:639s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:438s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:531s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:542s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:505s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:490s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:427s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:436s
fi-snb-2520m     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:398s
Blacklisted hosts:
fi-cnl-drrs      total:288  pass:257  dwarn:3   dfail:0   fail:0   skip:28  time:543s
fi-glk-j5005 failed to collect. IGT log at Patchwork_8307/fi-glk-j5005/run0.log

1bf8f00fed0b78f5d286304deb1f1526b10aeaca drm-tip: 2018y-03m-12d-11h-28m-33s UTC integration manifest
4fd9de60b0b4 drm/i915/icl: Use hw engine class, instance to find irq handler

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8307/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler
  2018-03-12 15:23       ` Chris Wilson
@ 2018-03-12 16:01         ` Daniele Ceraolo Spurio
  0 siblings, 0 replies; 9+ messages in thread
From: Daniele Ceraolo Spurio @ 2018-03-12 16:01 UTC (permalink / raw)
  To: Chris Wilson, Mika Kuoppala, intel-gfx



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@intel.com>
>>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread
* ✗ Fi.CI.IGT: failure for drm/i915/icl: Use hw engine class, instance to find irq handler (rev2)
  2018-03-12 14:33 [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler Mika Kuoppala
  2018-03-12 14:41 ` Mika Kuoppala
  2018-03-12 15:50 ` ✓ Fi.CI.BAT: success for drm/i915/icl: Use hw engine class, instance to find irq handler (rev2) Patchwork
@ 2018-03-12 20:20 ` Patchwork
  2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2018-03-12 20:20 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/icl: Use hw engine class, instance to find irq handler (rev2)
URL   : https://patchwork.freedesktop.org/series/39793/
State : failure

== Summary ==

---- Possible new issues:

Test kms_chv_cursor_fail:
        Subgroup pipe-a-128x128-top-edge:
                fail       -> PASS       (shard-apl)
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-spr-indfb-draw-pwrite:
                pass       -> FAIL       (shard-apl)

---- Known issues:

Test gem_eio:
        Subgroup in-flight-contexts:
                incomplete -> PASS       (shard-apl) fdo#105341
Test kms_sysfs_edid_timing:
                warn       -> PASS       (shard-apl) fdo#100047
Test pm_lpsp:
        Subgroup screens-disabled:
                fail       -> PASS       (shard-hsw) fdo#104941

fdo#105341 https://bugs.freedesktop.org/show_bug.cgi?id=105341
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#104941 https://bugs.freedesktop.org/show_bug.cgi?id=104941

shard-apl        total:3467 pass:1826 dwarn:1   dfail:0   fail:8   skip:1632 time:12987s
shard-hsw        total:3467 pass:1773 dwarn:1   dfail:0   fail:1   skip:1691 time:12032s
shard-snb        total:3467 pass:1364 dwarn:1   dfail:0   fail:2   skip:2100 time:7226s
Blacklisted hosts:
shard-kbl        total:3408 pass:1913 dwarn:1   dfail:0   fail:7   skip:1485 time:9138s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8307/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler
  2018-03-12 14:41 ` Mika Kuoppala
  2018-03-12 14:52   ` Chris Wilson
@ 2018-03-12 22:47   ` Michel Thierry
  1 sibling, 0 replies; 9+ messages in thread
From: Michel Thierry @ 2018-03-12 22:47 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

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@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@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))
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-03-12 22:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-12 14:33 [PATCH] drm/i915/icl: Use hw engine class, instance to find irq handler Mika Kuoppala
2018-03-12 14:41 ` Mika Kuoppala
2018-03-12 14:52   ` Chris Wilson
2018-03-12 15:09     ` Daniele Ceraolo Spurio
2018-03-12 15:23       ` Chris Wilson
2018-03-12 16:01         ` Daniele Ceraolo Spurio
2018-03-12 22:47   ` Michel Thierry
2018-03-12 15:50 ` ✓ Fi.CI.BAT: success for drm/i915/icl: Use hw engine class, instance to find irq handler (rev2) Patchwork
2018-03-12 20:20 ` ✗ Fi.CI.IGT: failure " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.