Re: [Intel-gfx] [PATCH 3/5] drm/i915/mtl: add GSC CS interrupt support

[Ceraolo Spurio, Daniele]

On 10/31/2022 5:55 AM, Tvrtko Ursulin wrote:

On 28/10/2022 18:00, Ceraolo Spurio, Daniele wrote:

On 10/28/2022 1:38 AM, Tvrtko Ursulin wrote:

On 27/10/2022 23:15, Daniele Ceraolo Spurio wrote:

The GSC CS re-uses the same interrupt bits that the GSC used in older
platforms. This means that we can now have an engine interrupt coming
out of OTHER_CLASS, so we need to handle that appropriately.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>

Cc: Matt Roper <matthew.d.ro...@intel.com>
---

  drivers/gpu/drm/i915/gt/intel_gt_irq.c | 78 ++++++++++++++------------

  1 file changed, 43 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c

index f26882fdc24c..34ff1ee7e931 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c

@@ -81,35 +81,27 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8 instance,

            instance, iir);
  }
  -static void
-gen11_engine_irq_handler(struct intel_gt *gt, const u8 class,
-             const u8 instance, const u16 iir)

+static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 instance)

  {
-    struct intel_engine_cs *engine;
-
-    /*

-     * Platforms with standalone media have their media engines in another

-     * GT.
-     */
-    if (MEDIA_VER(gt->i915) >= 13 &&

-        (class == VIDEO_DECODE_CLASS || class == VIDEO_ENHANCEMENT_CLASS)) {

-        if (!gt->i915->media_gt)
-            goto err;
+    struct intel_gt *media_gt = gt->i915->media_gt;
  -        gt = gt->i915->media_gt;
+    /* we expect the non-media gt to be passed in */
+    GEM_BUG_ON(gt == media_gt);
+
+    if (!media_gt)
+        return gt;
+
+    switch (class) {
+    case VIDEO_DECODE_CLASS:
+    case VIDEO_ENHANCEMENT_CLASS:
+        return media_gt;
+    case OTHER_CLASS:

+        if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, GSC0))

+            return media_gt;
+        fallthrough;
+    default:
+        return gt;
      }
-
-    if (instance <= MAX_ENGINE_INSTANCE)
-        engine = gt->engine_class[class][instance];
-    else
-        engine = NULL;
-
-    if (likely(engine))
-        return intel_engine_cs_irq(engine, iir);
-
-err:

-    WARN_ONCE(1, "unhandled engine interrupt class=0x%x, instance=0x%x\n",

-          class, instance);
  }
    static void

@@ -118,12 +110,24 @@ gen11_gt_identity_handler(struct intel_gt *gt, 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);
+    struct intel_engine_cs *engine;
        if (unlikely(!intr))
          return;
  -    if (class <= COPY_ENGINE_CLASS || class == COMPUTE_CLASS)
-        return gen11_engine_irq_handler(gt, class, instance, intr);
+    /*

+     * Platforms with standalone media have the media and GSC engines in

+     * another GT.
+     */
+    gt = pick_gt(gt, class, instance);
+
+    if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE)
+        engine = gt->engine_class[class][instance];
+    else
+        engine = NULL;
+
+    if (engine)
+        return intel_engine_cs_irq(engine, intr);

Drive by observation - you could fold the above two ifs into one since engine appears unused afterwards.

engine can be NULL in both branches of the if statement, so to get a unified if we'd have to do something like:

if (class <= MAX_ENGINE_CLASS && instance <= MAX_ENGINE_INSTANCE) {

         struct intel_engine_cs *engine = gt->engine_class[class][instance];

         if (engine)
                 return intel_engine_cs_irq(engine, intr);
}

Is this what you are suggesting?

Right, two ifs are needed after all. Well at least it would avoid the pointless engine = NULL assignment. Up to you.

Absence of any out-of-range class/instance logging is intentional?

There is already a log further down in this function.

Daniele

Regards,

Tvrtko