Re: [PATCH 2/5] drm/i915/icl: Use hw engine class, instance to find irq handler

Michel Thierry <michel.thierry@xxxxxxxxx> · Thu, 5 Apr 2018 11:17:17 -0700

On 4/5/2018 10:10 AM, Daniele Ceraolo Spurio wrote:


On 05/04/18 07:00, 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.

v3: rebase on top of rps intr
     use correct class / instance limits (Michel)
v4: split engine/other handling

Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_irq.c | 102 
++++++++++++++++++++++------------------
  drivers/gpu/drm/i915/i915_reg.h |   4 +-
  2 files changed, 59 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c 
b/drivers/gpu/drm/i915/i915_irq.c
index 27aee25429b7..f984bf12a0b6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2732,47 +2732,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;
@@ -2799,7 +2761,57 @@ 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
+gen11_other_irq_handler(struct drm_i915_private * const i915,
+            const u8 instance, const u16 iir)
+{
+    WARN_ONCE(1, "unhandled other interrupt instance=0x%x, iir=0x%x\n",
+          instance, iir);
+}
+
+static void
+gen11_engine_irq_handler(struct drm_i915_private * const i915,
+             const u8 class, const u8 instance, const u16 iir)
+{
+    struct intel_engine_cs *engine;
+
+    if (instance <= MAX_ENGINE_INSTANCE)
+        engine = i915->engine_class[class][instance];
+    else
+        engine = NULL;
+

bikeshed: if we initialize the engine pointer to null above we can skip 
the else case here.

+    if (likely(engine))
+        return gen8_cs_irq_handler(engine, iir);
+
+    WARN_ONCE(1, "unhandled engine interrupt class=0x%x, 
instance=0x%x\n",
+          class, instance);
+}
+
+static void
+gen11_gt_identity_handler(struct drm_i915_private * const i915,
+              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);
+
+    if (unlikely(!intr)) {
+        DRM_ERROR("class=0x%x, instance=0x%x, intr expected!\n",
+              class, instance);

This is not an error, it is possible to have an empty iir with double 
buffering. With the log removed:


I agree with Daniele, unlikely but possible.

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>

So with that, also

Reviewed-by: Michel Thierry <michel.thierry@xxxxxxxxx>

Thanks,
Daniele

+        return;
+    }
+
+    if (class <= COPY_ENGINE_CLASS)
+        return gen11_engine_irq_handler(i915, class, instance, intr);
+
+    if (class == OTHER_CLASS)
+        return gen11_other_irq_handler(i915, instance, intr);
+
+    WARN_ONCE(1, "unknown interrupt class=0x%x, instance=0x%x, 
intr=0x%x\n",
+          class, instance, intr);
  }
  static void
@@ -2824,12 +2836,10 @@ 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);
-
-            if (unlikely(!iir))
-                continue;
+            const u32 ident = gen11_gt_engine_identity(i915,
+                                   bank, bit);
-            gen11_gt_engine_irq_handler(i915, bank, bit, iir);
+            gen11_gt_identity_handler(i915, ident);
          }
          /* 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 b2a2d8fbbc68..d4b5fba7a2dc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6998,7 +6998,9 @@ enum {
  #define GEN11_INTR_IDENTITY_REG0    _MMIO(0x190060)
  #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_ENGINE_INTR(x)    ((x) & 0xffff)
  #define GEN11_INTR_IDENTITY_REG(x)    _MMIO(0x190060 + (x * 4))

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







