drm/i915: Fix up locking around dumping requests lists

The debugfs dump of requests was confused about what state requires
the execlist lock versus the GuC lock. There was also a bunch of
duplicated messy code between it and the error capture code.

So refactor the hung request search into a re-usable function. And
reduce the span of the execlist state lock to only the execlist
specific code paths. In order to do that, also move the report of hold
count (which is an execlist only concept) from the top level dump
function to the lower level execlist specific function. Also, move the
execlist specific code into the execlist source file.

v2: Rename some functions and move to more appropriate files (Daniele).
v3: Rename new execlist dump function (Daniele)

Fixes: dc0dad365c5e ("drm/i915/guc: Fix for error capture after full GPU reset with GuC")
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: Michael Cheng <michael.cheng@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Bruce Chang <yu.bruce.chang@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230127002842.3169194-4-John.C.Harrison@Intel.com

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index 0e24af5efee9c6aa3050630e2b6eac5a27879140..b58c30ac8ef0264cdf9f8f469e73f93f192d1b5d 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -250,8 +250,8 @@ void intel_engine_dump_active_requests(struct list_head *requests,
 ktime_t intel_engine_get_busy_time(struct intel_engine_cs *engine,
                                   ktime_t *now);
 
-struct i915_request *
-intel_engine_execlist_find_hung_request(struct intel_engine_cs *engine);
+void intel_engine_get_hung_entity(struct intel_engine_cs *engine,
+                                 struct intel_context **ce, struct i915_request **rq);
 
 u32 intel_engine_context_size(struct intel_gt *gt, u8 class);
 struct intel_context *

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index a86bdbee7a6bed333a42b32051dd9a9ce232372e..9f703f255d7216a123c559e40665bb7991190cb7 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -2114,17 +2114,6 @@ static void print_request_ring(struct drm_printer *m, struct i915_request *rq)
        }
 }
 
-static unsigned long list_count(struct list_head *list)
-{
-       struct list_head *pos;
-       unsigned long count = 0;
-
-       list_for_each(pos, list)
-               count++;
-
-       return count;
-}
-
 static unsigned long read_ul(void *p, size_t x)
 {
        return *(unsigned long *)(p + x);
@@ -2216,11 +2205,11 @@ void intel_engine_dump_active_requests(struct list_head *requests,
        }
 }
 
-static void engine_dump_active_requests(struct intel_engine_cs *engine, struct drm_printer *m)
+static void engine_dump_active_requests(struct intel_engine_cs *engine,
+                                       struct drm_printer *m)
 {
+       struct intel_context *hung_ce = NULL;
        struct i915_request *hung_rq = NULL;
-       struct intel_context *ce;
-       bool guc;
 
        /*
         * No need for an engine->irq_seqno_barrier() before the seqno reads.
@@ -2229,29 +2218,20 @@ static void engine_dump_active_requests(struct intel_engine_cs *engine, struct d
         * But the intention here is just to report an instantaneous snapshot
         * so that's fine.
         */
-       lockdep_assert_held(&engine->sched_engine->lock);
+       intel_engine_get_hung_entity(engine, &hung_ce, &hung_rq);
 
        drm_printf(m, "\tRequests:\n");
 
-       guc = intel_uc_uses_guc_submission(&engine->gt->uc);
-       if (guc) {
-               ce = intel_engine_get_hung_context(engine);
-               if (ce)
-                       hung_rq = intel_context_get_active_request(ce);
-       } else {
-               hung_rq = intel_engine_execlist_find_hung_request(engine);
-               if (hung_rq)
-                       hung_rq = i915_request_get_rcu(hung_rq);
-       }
-
        if (hung_rq)
                engine_dump_request(hung_rq, m, "\t\thung");
+       else if (hung_ce)
+               drm_printf(m, "\t\tGot hung ce but no hung rq!\n");
 
-       if (guc)
+       if (intel_uc_uses_guc_submission(&engine->gt->uc))
                intel_guc_dump_active_requests(engine, hung_rq, m);
        else
-               intel_engine_dump_active_requests(&engine->sched_engine->requests,
-                                                 hung_rq, m);
+               intel_execlists_dump_active_requests(engine, hung_rq, m);
+
        if (hung_rq)
                i915_request_put(hung_rq);
 }
@@ -2263,7 +2243,6 @@ void intel_engine_dump(struct intel_engine_cs *engine,
        struct i915_gpu_error * const error = &engine->i915->gpu_error;
        struct i915_request *rq;
        intel_wakeref_t wakeref;
-       unsigned long flags;
        ktime_t dummy;
 
        if (header) {
@@ -2300,13 +2279,8 @@ void intel_engine_dump(struct intel_engine_cs *engine,
                   i915_reset_count(error));
        print_properties(engine, m);
 
-       spin_lock_irqsave(&engine->sched_engine->lock, flags);
        engine_dump_active_requests(engine, m);
 
-       drm_printf(m, "\tOn hold?: %lu\n",
-                  list_count(&engine->sched_engine->hold));
-       spin_unlock_irqrestore(&engine->sched_engine->lock, flags);
-
        drm_printf(m, "\tMMIO base:  0x%08x\n", engine->mmio_base);
        wakeref = intel_runtime_pm_get_if_in_use(engine->uncore->rpm);
        if (wakeref) {
@@ -2352,8 +2326,7 @@ intel_engine_create_virtual(struct intel_engine_cs **siblings,
        return siblings[0]->cops->create_virtual(siblings, count, flags);
 }
 
-struct i915_request *
-intel_engine_execlist_find_hung_request(struct intel_engine_cs *engine)
+static struct i915_request *engine_execlist_find_hung_request(struct intel_engine_cs *engine)
 {
        struct i915_request *request, *active = NULL;
 
@@ -2405,6 +2378,33 @@ intel_engine_execlist_find_hung_request(struct intel_engine_cs *engine)
        return active;
 }
 
+void intel_engine_get_hung_entity(struct intel_engine_cs *engine,
+                                 struct intel_context **ce, struct i915_request **rq)
+{
+       unsigned long flags;
+
+       *ce = intel_engine_get_hung_context(engine);
+       if (*ce) {
+               intel_engine_clear_hung_context(engine);
+
+               *rq = intel_context_get_active_request(*ce);
+               return;
+       }
+
+       /*
+        * Getting here with GuC enabled means it is a forced error capture
+        * with no actual hang. So, no need to attempt the execlist search.
+        */
+       if (intel_uc_uses_guc_submission(&engine->gt->uc))
+               return;
+
+       spin_lock_irqsave(&engine->sched_engine->lock, flags);
+       *rq = engine_execlist_find_hung_request(engine);
+       if (*rq)
+               *rq = i915_request_get_rcu(*rq);
+       spin_unlock_irqrestore(&engine->sched_engine->lock, flags);
+}
+
 void xehp_enable_ccs_engines(struct intel_engine_cs *engine)
 {
        /*

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index 18ffe55282e594c38e5e7b90c4e78003ccfe72a7..3c573d41d40468700f7027f8a8ae6bd0dacb47a5 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -4150,6 +4150,33 @@ void intel_execlists_show_requests(struct intel_engine_cs *engine,
        spin_unlock_irqrestore(&sched_engine->lock, flags);
 }
 
+static unsigned long list_count(struct list_head *list)
+{
+       struct list_head *pos;
+       unsigned long count = 0;
+
+       list_for_each(pos, list)
+               count++;
+
+       return count;
+}
+
+void intel_execlists_dump_active_requests(struct intel_engine_cs *engine,
+                                         struct i915_request *hung_rq,
+                                         struct drm_printer *m)
+{
+       unsigned long flags;
+
+       spin_lock_irqsave(&engine->sched_engine->lock, flags);
+
+       intel_engine_dump_active_requests(&engine->sched_engine->requests, hung_rq, m);
+
+       drm_printf(m, "\tOn hold?: %lu\n",
+                  list_count(&engine->sched_engine->hold));
+
+       spin_unlock_irqrestore(&engine->sched_engine->lock, flags);
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftest_execlists.c"
 #endif

diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.h b/drivers/gpu/drm/i915/gt/intel_execlists_submission.h
index a1aa92c983a51adacaf3f4b68f6ade944f386822..d2c7d45ea0623b4d113b128ff5b42f730fedcedf 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.h
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.h
@@ -32,6 +32,10 @@ void intel_execlists_show_requests(struct intel_engine_cs *engine,
                                                        int indent),
                                   unsigned int max);
 
+void intel_execlists_dump_active_requests(struct intel_engine_cs *engine,
+                                         struct i915_request *hung_rq,
+                                         struct drm_printer *m);
+
 bool
 intel_engine_in_execlists_submission_mode(const struct intel_engine_cs *engine);
 

diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 9e2d17785a9a88bb58058d24dd1981c5eb3e8b2c..b20bd6365615b5bce67973af6d265e37af320b41 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1596,35 +1596,15 @@ capture_engine(struct intel_engine_cs *engine,
 {
        struct intel_engine_capture_vma *capture = NULL;
        struct intel_engine_coredump *ee;
-       struct intel_context *ce;
+       struct intel_context *ce = NULL;
        struct i915_request *rq = NULL;
-       unsigned long flags;
 
        ee = intel_engine_coredump_alloc(engine, ALLOW_FAIL, dump_flags);
        if (!ee)
                return NULL;
 
-       ce = intel_engine_get_hung_context(engine);
-       if (ce) {
-               intel_engine_clear_hung_context(engine);
-               rq = intel_context_get_active_request(ce);
-               if (!rq || !i915_request_started(rq))
-                       goto no_request_capture;
-       } else {
-               /*
-                * Getting here with GuC enabled means it is a forced error capture
-                * with no actual hang. So, no need to attempt the execlist search.
-                */
-               if (!intel_uc_uses_guc_submission(&engine->gt->uc)) {
-                       spin_lock_irqsave(&engine->sched_engine->lock, flags);
-                       rq = intel_engine_execlist_find_hung_request(engine);
-                       if (rq)
-                               rq = i915_request_get_rcu(rq);
-                       spin_unlock_irqrestore(&engine->sched_engine->lock,
-                                              flags);
-               }
-       }
-       if (!rq)
+       intel_engine_get_hung_entity(engine, &ce, &rq);
+       if (!rq || !i915_request_started(rq))
                goto no_request_capture;
 
        capture = intel_engine_coredump_add_request(ee, rq, ATOMIC_MAYFAIL);