[Intel-gfx] [PATCH v3 0/3] Resolve suspend-resume racing with GuC destroy-context-worker

[Intel-gfx] [PATCH v3 0/3] Resolve suspend-resume racing with GuC destroy-context-worker

[Alan Previn]

This series is the result of debugging issues root caused to
races between the GuC's destroyed_worker_func being triggered
vs repeating suspend-resume cycles with concurrent delayed
fence signals for engine-freeing.

The reproduction steps require that an app is launched right
before the start of the suspend cycle where it creates a
new gem context and submits a tiny workload that would
complete in the middle of the suspend cycle. However this
app uses dma-buffer sharing or dma-fence with non-GPU
objects or signals that eventually triggers a FENCE_FREE
via__i915_sw_fence_notify that connects to engines_notify ->
free_engines_rcu -> intel_context_put ->
kref_put(&ce->ref..) that queues the worker after the GuCs
CTB has been disabled (i.e. after i915-gem's suspend-late).

This sequence is a corner-case and required repeating this
app->suspend->resume cycle ~1500 times across 4 identical
systems to see it once. That said, based on above callstack,
it is clear that merely flushing the context destruction worker,
which is obviously missing and needed, isn't sufficient.

Because of that, this series adds additional patches besides
the obvious (Patch #1) flushing of the worker during the
suspend flows. It also includes (Patch #2) closing a race
between sending the context-deregistration H2G vs the CTB
getting disabled in the midst of it (by detecing the failure
and unrolling the guc-lrc-unpin flow) and (Patch #32) not
infinitely waiting in intel_gt_pm_wait_timeout_for_idle
when in the suspend-flow.

NOTE: We are still observing one more wakeref leak from gt
but not necessarilty guc. We are still debugging this so
this series will very likely get rev'd up again.

Changes from prior revs:
   v2: - Patch #2 Restructure code in guc_lrc_desc_unpin so
         it's more readible to differentiate (1)direct guc-id
         cleanup ..vs (2) sending the H2G ctx-destroy action ..
         vs (3) the unrolling steps if the H2G fails.
       - Patch #2 Add a check to close the race sooner by checking
         for intel_guc_is_ready from destroyed_worker_func.
       - Patch #2 When guc_submission_send_busy_loop gets a
         failure from intel_guc_send_busy_loop, we need to undo
         i.e. decrement the outstanding_submission_g2h.
       - Patch #3 In wait_for_suspend, fix checking of return from
         intel_gt_pm_wait_timeout_for_idle to now use -ETIMEDOUT
         and add documentation for intel_wakeref_wait_for_idle.
         (Rodrigo).

Alan Previn (3):
  drm/i915/guc: Flush context destruction worker at suspend
  drm/i915/guc: Close deregister-context race against CT-loss
  drm/i915/gt: Timeout when waiting for idle in suspending

 drivers/gpu/drm/i915/gt/intel_engine_cs.c     |  2 +-
 drivers/gpu/drm/i915/gt/intel_gt_pm.c         |  7 +-
 drivers/gpu/drm/i915/gt/intel_gt_pm.h         |  7 +-
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 81 ++++++++++++++++---
 .../gpu/drm/i915/gt/uc/intel_guc_submission.h |  2 +
 drivers/gpu/drm/i915/gt/uc/intel_uc.c         |  2 +
 drivers/gpu/drm/i915/intel_wakeref.c          | 14 +++-
 drivers/gpu/drm/i915/intel_wakeref.h          |  6 +-
 8 files changed, 101 insertions(+), 20 deletions(-)


base-commit: f8d21cb17a99b75862196036bb4bb93ee9637b74
-- 
2.39.0

[Intel-gfx] [PATCH v3 1/3] drm/i915/guc: Flush context destruction worker at suspend

[Alan Previn]

When suspending, flush the context-guc-id
deregistration worker at the final stages of
intel_gt_suspend_late when we finally call gt_sanitize
that eventually leads down to __uc_sanitize so that
the deregistration worker doesn't fire off later as
we reset the GuC microcontroller.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Tested-by: Mousumi Jana <mousumi.jana@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 5 +++++
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 2 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c             | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index cabdc645fcdd..0ed44637bca0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1578,6 +1578,11 @@ static void guc_flush_submissions(struct intel_guc *guc)
 	spin_unlock_irqrestore(&sched_engine->lock, flags);
 }
 
+void intel_guc_submission_flush_work(struct intel_guc *guc)
+{
+	flush_work(&guc->submission_state.destroyed_worker);
+}
+
 static void guc_flush_destroyed_contexts(struct intel_guc *guc);
 
 void intel_guc_submission_reset_prepare(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
index c57b29cdb1a6..b6df75622d3b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
@@ -38,6 +38,8 @@ int intel_guc_wait_for_pending_msg(struct intel_guc *guc,
 				   bool interruptible,
 				   long timeout);
 
+void intel_guc_submission_flush_work(struct intel_guc *guc);
+
 static inline bool intel_guc_submission_is_supported(struct intel_guc *guc)
 {
 	return guc->submission_supported;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 98b103375b7a..eb3554cb5ea4 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -693,6 +693,8 @@ void intel_uc_suspend(struct intel_uc *uc)
 		return;
 	}
 
+	intel_guc_submission_flush_work(guc);
+
 	with_intel_runtime_pm(&uc_to_gt(uc)->i915->runtime_pm, wakeref) {
 		err = intel_guc_suspend(guc);
 		if (err)
-- 
2.39.0

[Intel-gfx] [PATCH v3 2/3] drm/i915/guc: Close deregister-context race against CT-loss

[Alan Previn]

If we are at the end of suspend or very early in resume
its possible an async fence signal could lead us to the
execution of the context destruction worker (after the
prior worker flush).

Even if checking that the CT is enabled before calling
destroyed_worker_func, guc_lrc_desc_unpin may still fail
because in corner cases, as we traverse the GuC's
context-destroy list, the CT could get disabled in the mid
of it right before calling the GuC's CT send function.

We've witnessed this race condition once every ~6000-8000
suspend-resume cycles while ensuring workloads that render
something onscreen is continuously started just before
we suspend (and the workload is small enough to complete
and trigger the queued engine/context free-up either very
late in suspend or very early in resume).

In such a case, we need to unroll the unpin process because
guc-lrc-unpin takes a gt wakeref which only gets released in
the G2H IRQ reply that never comes through in this corner
case. That will cascade into a kernel hang later at the tail
end of suspend in this function:

   intel_wakeref_wait_for_idle(&gt->wakeref)
   (called by) - intel_gt_pm_wait_for_idle
   (called by) - wait_for_suspend

Doing this unroll and keeping the context in the GuC's
destroy-list will allow the context to get picked up on
the next destroy worker invocation or purged as part of a
major GuC sanitization or reset flow.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Tested-by: Mousumi Jana <mousumi.jana@intel.com>
---
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 76 ++++++++++++++++---
 1 file changed, 65 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 0ed44637bca0..db7df1217350 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -235,6 +235,13 @@ set_context_destroyed(struct intel_context *ce)
 	ce->guc_state.sched_state |= SCHED_STATE_DESTROYED;
 }
 
+static inline void
+clr_context_destroyed(struct intel_context *ce)
+{
+	lockdep_assert_held(&ce->guc_state.lock);
+	ce->guc_state.sched_state &= ~SCHED_STATE_DESTROYED;
+}
+
 static inline bool context_pending_disable(struct intel_context *ce)
 {
 	return ce->guc_state.sched_state & SCHED_STATE_PENDING_DISABLE;
@@ -612,6 +619,8 @@ static int guc_submission_send_busy_loop(struct intel_guc *guc,
 					 u32 g2h_len_dw,
 					 bool loop)
 {
+	int ret;
+
 	/*
 	 * We always loop when a send requires a reply (i.e. g2h_len_dw > 0),
 	 * so we don't handle the case where we don't get a reply because we
@@ -622,7 +631,11 @@ static int guc_submission_send_busy_loop(struct intel_guc *guc,
 	if (g2h_len_dw)
 		atomic_inc(&guc->outstanding_submission_g2h);
 
-	return intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop);
+	ret = intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop);
+	if (ret)
+		atomic_dec(&guc->outstanding_submission_g2h);
+
+	return ret;
 }
 
 int intel_guc_wait_for_pending_msg(struct intel_guc *guc,
@@ -3173,12 +3186,13 @@ static void guc_context_close(struct intel_context *ce)
 	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
 }
 
-static inline void guc_lrc_desc_unpin(struct intel_context *ce)
+static inline int guc_lrc_desc_unpin(struct intel_context *ce)
 {
 	struct intel_guc *guc = ce_to_guc(ce);
 	struct intel_gt *gt = guc_to_gt(guc);
 	unsigned long flags;
 	bool disabled;
+	int ret;
 
 	GEM_BUG_ON(!intel_gt_pm_is_awake(gt));
 	GEM_BUG_ON(!ctx_id_mapped(guc, ce->guc_id.id));
@@ -3188,19 +3202,33 @@ static inline void guc_lrc_desc_unpin(struct intel_context *ce)
 	/* Seal race with Reset */
 	spin_lock_irqsave(&ce->guc_state.lock, flags);
 	disabled = submission_disabled(guc);
-	if (likely(!disabled)) {
-		__intel_gt_pm_get(gt);
-		set_context_destroyed(ce);
-		clr_context_registered(ce);
-	}
-	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
 	if (unlikely(disabled)) {
+		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
 		release_guc_id(guc, ce);
 		__guc_context_destroy(ce);
-		return;
+		return 0;
 	}
 
-	deregister_context(ce, ce->guc_id.id);
+	/* GuC is active, lets destroy this context,
+	 * but at this point we can still be racing with
+	 * suspend, so we undo everything if the H2G fails
+	 */
+
+	/* Change context state to destroyed and get gt-pm */
+	__intel_gt_pm_get(gt);
+	set_context_destroyed(ce);
+	clr_context_registered(ce);
+
+	ret = deregister_context(ce, ce->guc_id.id);
+	if (ret) {
+		/* Undo the state change and put gt-pm if that failed */
+		set_context_registered(ce);
+		clr_context_destroyed(ce);
+		intel_gt_pm_put(gt);
+	}
+	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
+
+	return 0;
 }
 
 static void __guc_context_destroy(struct intel_context *ce)
@@ -3268,7 +3296,22 @@ static void deregister_destroyed_contexts(struct intel_guc *guc)
 		if (!ce)
 			break;
 
-		guc_lrc_desc_unpin(ce);
+		if (guc_lrc_desc_unpin(ce)) {
+			/*
+			 * This means GuC's CT link severed mid-way which could happen
+			 * in suspend-resume corner cases. In this case, put the
+			 * context back into the destroyed_contexts list which will
+			 * get picked up on the next context deregistration event or
+			 * purged in a GuC sanitization event (reset/unload/wedged/...).
+			 */
+			spin_lock_irqsave(&guc->submission_state.lock, flags);
+			list_add_tail(&ce->destroyed_link,
+				      &guc->submission_state.destroyed_contexts);
+			spin_unlock_irqrestore(&guc->submission_state.lock, flags);
+			/* Bail now since the list might never be emptied if h2gs fail */
+			break;
+		}
+
 	}
 }
 
@@ -3279,6 +3322,17 @@ static void destroyed_worker_func(struct work_struct *w)
 	struct intel_gt *gt = guc_to_gt(guc);
 	int tmp;
 
+	/*
+	 * In rare cases we can get here via async context-free fence-signals that
+	 * come very late in suspend flow or very early in resume flows. In these
+	 * cases, GuC won't be ready but just skipping it here is fine as these
+	 * pending-destroy-contexts get destroyed totally at GuC reset time at the
+	 * end of suspend.. OR.. this worker can be picked up later on the next
+	 * context destruction trigger after resume-completes
+	 */
+	if (!intel_guc_is_ready(guc))
+		return;
+
 	with_intel_gt_pm(gt, tmp)
 		deregister_destroyed_contexts(guc);
 }
-- 
2.39.0

[Intel-gfx] [PATCH v3 3/3] drm/i915/gt: Timeout when waiting for idle in suspending

[Alan Previn]

When suspending, add a timeout when calling
intel_gt_pm_wait_for_idle else if we have a lost
G2H event that holds a wakeref (which would be
indicative of a bug elsewhere in the driver),
driver will at least complete the suspend-resume
cycle, (albeit not hitting all the targets for
low power hw counters), instead of hanging in the kernel.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Tested-by: Mousumi Jana <mousumi.jana@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c |  2 +-
 drivers/gpu/drm/i915/gt/intel_gt_pm.c     |  7 ++++++-
 drivers/gpu/drm/i915/gt/intel_gt_pm.h     |  7 ++++++-
 drivers/gpu/drm/i915/intel_wakeref.c      | 14 ++++++++++----
 drivers/gpu/drm/i915/intel_wakeref.h      |  6 ++++--
 5 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index dfb69fc977a0..4811f3be0332 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -688,7 +688,7 @@ void intel_engines_release(struct intel_gt *gt)
 		if (!engine->release)
 			continue;
 
-		intel_wakeref_wait_for_idle(&engine->wakeref);
+		intel_wakeref_wait_for_idle(&engine->wakeref, 0);
 		GEM_BUG_ON(intel_engine_pm_is_awake(engine));
 
 		engine->release(engine);
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index 5a942af0a14e..ca46aee72573 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -289,6 +289,8 @@ int intel_gt_resume(struct intel_gt *gt)
 
 static void wait_for_suspend(struct intel_gt *gt)
 {
+	int timeout_ms = CONFIG_DRM_I915_MAX_REQUEST_BUSYWAIT ? : 10000;
+
 	if (!intel_gt_pm_is_awake(gt))
 		return;
 
@@ -301,7 +303,10 @@ static void wait_for_suspend(struct intel_gt *gt)
 		intel_gt_retire_requests(gt);
 	}
 
-	intel_gt_pm_wait_for_idle(gt);
+	/* we are suspending, so we shouldn't be waiting forever */
+	if (intel_gt_pm_wait_timeout_for_idle(gt, timeout_ms) == -ETIMEDOUT)
+		gt_warn(gt, "bailing from %s after %d milisec timeout\n",
+			__func__, timeout_ms);
 }
 
 void intel_gt_suspend_prepare(struct intel_gt *gt)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
index 6c9a46452364..5358acc2b5b1 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
@@ -68,7 +68,12 @@ static inline void intel_gt_pm_might_put(struct intel_gt *gt)
 
 static inline int intel_gt_pm_wait_for_idle(struct intel_gt *gt)
 {
-	return intel_wakeref_wait_for_idle(&gt->wakeref);
+	return intel_wakeref_wait_for_idle(&gt->wakeref, 0);
+}
+
+static inline int intel_gt_pm_wait_timeout_for_idle(struct intel_gt *gt, int timeout_ms)
+{
+	return intel_wakeref_wait_for_idle(&gt->wakeref, timeout_ms);
 }
 
 void intel_gt_pm_init_early(struct intel_gt *gt);
diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
index 718f2f1b6174..383a37521415 100644
--- a/drivers/gpu/drm/i915/intel_wakeref.c
+++ b/drivers/gpu/drm/i915/intel_wakeref.c
@@ -111,14 +111,20 @@ void __intel_wakeref_init(struct intel_wakeref *wf,
 			 "wakeref.work", &key->work, 0);
 }
 
-int intel_wakeref_wait_for_idle(struct intel_wakeref *wf)
+int intel_wakeref_wait_for_idle(struct intel_wakeref *wf, int timeout_ms)
 {
-	int err;
+	int err = 0;
 
 	might_sleep();
 
-	err = wait_var_event_killable(&wf->wakeref,
-				      !intel_wakeref_is_active(wf));
+	if (!timeout_ms)
+		err = wait_var_event_killable(&wf->wakeref,
+					      !intel_wakeref_is_active(wf));
+	else if (wait_var_event_timeout(&wf->wakeref,
+					!intel_wakeref_is_active(wf),
+					msecs_to_jiffies(timeout_ms)) < 1)
+		err = -ETIMEDOUT;
+
 	if (err)
 		return err;
 
diff --git a/drivers/gpu/drm/i915/intel_wakeref.h b/drivers/gpu/drm/i915/intel_wakeref.h
index ec881b097368..302694a780d2 100644
--- a/drivers/gpu/drm/i915/intel_wakeref.h
+++ b/drivers/gpu/drm/i915/intel_wakeref.h
@@ -251,15 +251,17 @@ __intel_wakeref_defer_park(struct intel_wakeref *wf)
 /**
  * intel_wakeref_wait_for_idle: Wait until the wakeref is idle
  * @wf: the wakeref
+ * @timeout_ms: Timeout in ms, 0 means never timeout.
  *
  * Wait for the earlier asynchronous release of the wakeref. Note
  * this will wait for any third party as well, so make sure you only wait
  * when you have control over the wakeref and trust no one else is acquiring
  * it.
  *
- * Return: 0 on success, error code if killed.
+ * Returns 0 on success, -ETIMEDOUT upon a timeout, or the unlikely
+ * error propagation from wait_var_event_killable if timeout_ms is 0.
  */
-int intel_wakeref_wait_for_idle(struct intel_wakeref *wf);
+int intel_wakeref_wait_for_idle(struct intel_wakeref *wf, int timeout_ms);
 
 struct intel_wakeref_auto {
 	struct drm_i915_private *i915;
-- 
2.39.0

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for Resolve suspend-resume racing with GuC destroy-context-worker (rev3)

[Patchwork]

== Series Details ==

Series: Resolve suspend-resume racing with GuC destroy-context-worker (rev3)
URL   : https://patchwork.freedesktop.org/series/121916/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

Re: [Intel-gfx] [PATCH v3 2/3] drm/i915/guc: Close deregister-context race against CT-loss

[Rodrigo Vivi]

On Sat, Sep 09, 2023 at 08:58:45PM -0700, Alan Previn wrote:
> If we are at the end of suspend or very early in resume
> its possible an async fence signal could lead us to the
> execution of the context destruction worker (after the
> prior worker flush).
> 
> Even if checking that the CT is enabled before calling
> destroyed_worker_func, guc_lrc_desc_unpin may still fail
> because in corner cases, as we traverse the GuC's
> context-destroy list, the CT could get disabled in the mid
> of it right before calling the GuC's CT send function.
> 
> We've witnessed this race condition once every ~6000-8000
> suspend-resume cycles while ensuring workloads that render
> something onscreen is continuously started just before
> we suspend (and the workload is small enough to complete
> and trigger the queued engine/context free-up either very
> late in suspend or very early in resume).
> 
> In such a case, we need to unroll the unpin process because
> guc-lrc-unpin takes a gt wakeref which only gets released in
> the G2H IRQ reply that never comes through in this corner
> case. That will cascade into a kernel hang later at the tail
> end of suspend in this function:
> 
>    intel_wakeref_wait_for_idle(&gt->wakeref)
>    (called by) - intel_gt_pm_wait_for_idle
>    (called by) - wait_for_suspend
> 
> Doing this unroll and keeping the context in the GuC's
> destroy-list will allow the context to get picked up on
> the next destroy worker invocation or purged as part of a
> major GuC sanitization or reset flow.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Tested-by: Mousumi Jana <mousumi.jana@intel.com>
> ---
>  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 76 ++++++++++++++++---
>  1 file changed, 65 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 0ed44637bca0..db7df1217350 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -235,6 +235,13 @@ set_context_destroyed(struct intel_context *ce)
>  	ce->guc_state.sched_state |= SCHED_STATE_DESTROYED;
>  }
>  
> +static inline void
> +clr_context_destroyed(struct intel_context *ce)
> +{
> +	lockdep_assert_held(&ce->guc_state.lock);
> +	ce->guc_state.sched_state &= ~SCHED_STATE_DESTROYED;
> +}
> +
>  static inline bool context_pending_disable(struct intel_context *ce)
>  {
>  	return ce->guc_state.sched_state & SCHED_STATE_PENDING_DISABLE;
> @@ -612,6 +619,8 @@ static int guc_submission_send_busy_loop(struct intel_guc *guc,
>  					 u32 g2h_len_dw,
>  					 bool loop)
>  {
> +	int ret;
> +
>  	/*
>  	 * We always loop when a send requires a reply (i.e. g2h_len_dw > 0),
>  	 * so we don't handle the case where we don't get a reply because we
> @@ -622,7 +631,11 @@ static int guc_submission_send_busy_loop(struct intel_guc *guc,
>  	if (g2h_len_dw)
>  		atomic_inc(&guc->outstanding_submission_g2h);
>  
> -	return intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop);
> +	ret = intel_guc_send_busy_loop(guc, action, len, g2h_len_dw, loop);
> +	if (ret)
> +		atomic_dec(&guc->outstanding_submission_g2h);
> +
> +	return ret;
>  }
>  
>  int intel_guc_wait_for_pending_msg(struct intel_guc *guc,
> @@ -3173,12 +3186,13 @@ static void guc_context_close(struct intel_context *ce)
>  	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
>  }
>  
> -static inline void guc_lrc_desc_unpin(struct intel_context *ce)
> +static inline int guc_lrc_desc_unpin(struct intel_context *ce)
>  {
>  	struct intel_guc *guc = ce_to_guc(ce);
>  	struct intel_gt *gt = guc_to_gt(guc);
>  	unsigned long flags;
>  	bool disabled;
> +	int ret;
>  
>  	GEM_BUG_ON(!intel_gt_pm_is_awake(gt));
>  	GEM_BUG_ON(!ctx_id_mapped(guc, ce->guc_id.id));
> @@ -3188,19 +3202,33 @@ static inline void guc_lrc_desc_unpin(struct intel_context *ce)
>  	/* Seal race with Reset */
>  	spin_lock_irqsave(&ce->guc_state.lock, flags);
>  	disabled = submission_disabled(guc);
> -	if (likely(!disabled)) {
> -		__intel_gt_pm_get(gt);
> -		set_context_destroyed(ce);
> -		clr_context_registered(ce);
> -	}
> -	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
>  	if (unlikely(disabled)) {
> +		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
>  		release_guc_id(guc, ce);
>  		__guc_context_destroy(ce);
> -		return;
> +		return 0;
>  	}
>  
> -	deregister_context(ce, ce->guc_id.id);
> +	/* GuC is active, lets destroy this context,
> +	 * but at this point we can still be racing with
> +	 * suspend, so we undo everything if the H2G fails
> +	 */
> +
> +	/* Change context state to destroyed and get gt-pm */
> +	__intel_gt_pm_get(gt);
> +	set_context_destroyed(ce);
> +	clr_context_registered(ce);
> +
> +	ret = deregister_context(ce, ce->guc_id.id);
> +	if (ret) {
> +		/* Undo the state change and put gt-pm if that failed */
> +		set_context_registered(ce);
> +		clr_context_destroyed(ce);
> +		intel_gt_pm_put(gt);

This is a might_sleep inside a spin_lock! Are you 100% confident no WARN
was seeing during the tests indicated in the commit msg?

> +	}
> +	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
> +
> +	return 0;

If you are always returning 0, there's no pointing in s/void/int...

>  }
>  
>  static void __guc_context_destroy(struct intel_context *ce)
> @@ -3268,7 +3296,22 @@ static void deregister_destroyed_contexts(struct intel_guc *guc)
>  		if (!ce)
>  			break;
>  
> -		guc_lrc_desc_unpin(ce);
> +		if (guc_lrc_desc_unpin(ce)) {
> +			/*
> +			 * This means GuC's CT link severed mid-way which could happen
> +			 * in suspend-resume corner cases. In this case, put the
> +			 * context back into the destroyed_contexts list which will
> +			 * get picked up on the next context deregistration event or
> +			 * purged in a GuC sanitization event (reset/unload/wedged/...).
> +			 */
> +			spin_lock_irqsave(&guc->submission_state.lock, flags);
> +			list_add_tail(&ce->destroyed_link,
> +				      &guc->submission_state.destroyed_contexts);
> +			spin_unlock_irqrestore(&guc->submission_state.lock, flags);
> +			/* Bail now since the list might never be emptied if h2gs fail */
> +			break;
> +		}
> +
>  	}
>  }
>  
> @@ -3279,6 +3322,17 @@ static void destroyed_worker_func(struct work_struct *w)
>  	struct intel_gt *gt = guc_to_gt(guc);
>  	int tmp;
>  
> +	/*
> +	 * In rare cases we can get here via async context-free fence-signals that
> +	 * come very late in suspend flow or very early in resume flows. In these
> +	 * cases, GuC won't be ready but just skipping it here is fine as these
> +	 * pending-destroy-contexts get destroyed totally at GuC reset time at the
> +	 * end of suspend.. OR.. this worker can be picked up later on the next
> +	 * context destruction trigger after resume-completes

who is triggering the work queue again?
I mean, I'm wondering if it is necessary to re-schedule it from inside...
but also wonder if this is safe anyway...

> +	 */
> +	if (!intel_guc_is_ready(guc))
> +		return;
> +
>  	with_intel_gt_pm(gt, tmp)
>  		deregister_destroyed_contexts(guc);
>  }
> -- 
> 2.39.0
> 

Re: [Intel-gfx] [PATCH v3 1/3] drm/i915/guc: Flush context destruction worker at suspend

[Rodrigo Vivi]

On Sat, Sep 09, 2023 at 08:58:44PM -0700, Alan Previn wrote:
> When suspending, flush the context-guc-id
> deregistration worker at the final stages of
> intel_gt_suspend_late when we finally call gt_sanitize
> that eventually leads down to __uc_sanitize so that
> the deregistration worker doesn't fire off later as
> we reset the GuC microcontroller.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Tested-by: Mousumi Jana <mousumi.jana@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 5 +++++
>  drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h | 2 ++
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c             | 2 ++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index cabdc645fcdd..0ed44637bca0 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1578,6 +1578,11 @@ static void guc_flush_submissions(struct intel_guc *guc)
>  	spin_unlock_irqrestore(&sched_engine->lock, flags);
>  }
>  
> +void intel_guc_submission_flush_work(struct intel_guc *guc)
> +{
> +	flush_work(&guc->submission_state.destroyed_worker);
> +}
> +
>  static void guc_flush_destroyed_contexts(struct intel_guc *guc);
>  
>  void intel_guc_submission_reset_prepare(struct intel_guc *guc)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> index c57b29cdb1a6..b6df75622d3b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> @@ -38,6 +38,8 @@ int intel_guc_wait_for_pending_msg(struct intel_guc *guc,
>  				   bool interruptible,
>  				   long timeout);
>  
> +void intel_guc_submission_flush_work(struct intel_guc *guc);
> +
>  static inline bool intel_guc_submission_is_supported(struct intel_guc *guc)
>  {
>  	return guc->submission_supported;
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 98b103375b7a..eb3554cb5ea4 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -693,6 +693,8 @@ void intel_uc_suspend(struct intel_uc *uc)
>  		return;
>  	}
>  
> +	intel_guc_submission_flush_work(guc);
> +
>  	with_intel_runtime_pm(&uc_to_gt(uc)->i915->runtime_pm, wakeref) {
>  		err = intel_guc_suspend(guc);
>  		if (err)
> -- 
> 2.39.0
> 

Re: [Intel-gfx] [PATCH v3 1/3] drm/i915/guc: Flush context destruction worker at suspend

[Teres Alexis, Alan Previn]

On Thu, 2023-09-14 at 11:35 -0400, Vivi, Rodrigo wrote:
> On Sat, Sep 09, 2023 at 08:58:44PM -0700, Alan Previn wrote:
> > When suspending, flush the context-guc-id
> > deregistration worker at the final stages of
> > intel_gt_suspend_late when we finally call gt_sanitize
> > that eventually leads down to __uc_sanitize so that
> > the deregistration worker doesn't fire off later as
> > we reset the GuC microcontroller.
> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > Tested-by: Mousumi Jana <mousumi.jana@intel.com>
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
alan:snip
alan: thanks for the RB Rodrigo.

Re: [Intel-gfx] [PATCH v3 2/3] drm/i915/guc: Close deregister-context race against CT-loss

[Teres Alexis, Alan Previn]

(cc Anshuman who is working directly with the taskforce debugging this)

Thanks again for taking the time to review this patch.
Apologies for the tardiness, rest assured debug is still ongoing.

As mentioned in prior comments, the signatures and frequency are
now different compared to without the patches of this series.
We are still hunting for data as we are suspecting a different wakeref
still being held with the same trigger event despite.

That said, we will continue to rebase / update this series but hold off on actual
merge until we can be sure we have all the issues resolved.

On Thu, 2023-09-14 at 11:34 -0400, Vivi, Rodrigo wrote:
> On Sat, Sep 09, 2023 at 08:58:45PM -0700, Alan Previn wrote:
> > If we are at the end of suspend or very early in resume
> > its possible an async fence signal could lead us to the
alan:snip


> > @@ -3188,19 +3202,33 @@ static inline void guc_lrc_desc_unpin(struct intel_context *ce)
> >  	/* Seal race with Reset */
> >  	spin_lock_irqsave(&ce->guc_state.lock, flags);
> >  	disabled = submission_disabled(guc);
> > 
alan:snip
> > +	/* Change context state to destroyed and get gt-pm */
> > +	__intel_gt_pm_get(gt);
> > +	set_context_destroyed(ce);
> > +	clr_context_registered(ce);
> > +
> > +	ret = deregister_context(ce, ce->guc_id.id);
> > +	if (ret) {
> > +		/* Undo the state change and put gt-pm if that failed */
> > +		set_context_registered(ce);
> > +		clr_context_destroyed(ce);
> > +		intel_gt_pm_put(gt);
> 
> This is a might_sleep inside a spin_lock! Are you 100% confident no WARN
> was seeing during the tests indicated in the commit msg?
alan: Good catch - i dont think we saw a WARN - I'll go back and check
with the task force - i shall rework this function to get that outside the lock.

> 
> > +	}
> > +	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
> > +
> > +	return 0;
> 
> If you are always returning 0, there's no pointing in s/void/int...
Alan: agreed - will change to void.
> > 
> > 

alan:snip
> > @@ -3279,6 +3322,17 @@ static void destroyed_worker_func(struct work_struct *w)
> >  	struct intel_gt *gt = guc_to_gt(guc);
> >  	int tmp;
> >  
> > +	/*
> > +	 * In rare cases we can get here via async context-free fence-signals that
> > +	 * come very late in suspend flow or very early in resume flows. In these
> > +	 * cases, GuC won't be ready but just skipping it here is fine as these
> > +	 * pending-destroy-contexts get destroyed totally at GuC reset time at the
> > +	 * end of suspend.. OR.. this worker can be picked up later on the next
> > +	 * context destruction trigger after resume-completes
> 
> who is triggering the work queue again?

alan: short answer: we dont know - and still hunting this
(getting closer now.. using task tgid str-name lookups).
in the few times I've seen it, the callstack I've seen looked like this:

[33763.582036] Call Trace:
[33763.582038]  <TASK>
[33763.582040]  dump_stack_lvl+0x69/0x97
[33763.582054]  guc_context_destroy+0x1b5/0x1ec
[33763.582067]  free_engines+0x52/0x70
[33763.582072]  rcu_do_batch+0x161/0x438
[33763.582084]  rcu_nocb_cb_kthread+0xda/0x2d0
[33763.582093]  kthread+0x13a/0x152
[33763.582102]  ? rcu_nocb_gp_kthread+0x6a7/0x6a7
[33763.582107]  ? css_get+0x38/0x38
[33763.582118]  ret_from_fork+0x1f/0x30
[33763.582128]  </TASK>

I did add additional debug-msg for tracking and I recall seeing this sequence via independant callstacks in the big picture:
	i915_sw_fence_complete > __i915_sw_fence_complete -> __i915_sw_fence_notify(fence, FENCE_FREE) -> <..delayed?..>
	[ drm fence sync func ] <...> engines_notify > call_rcu(&engines>rcu, free_engines_rcu) <..delayed?..>
	free_engines -> intel_context_put -> ... [kref-dec] --> guc_context_destroy

Unfortunately, we still don't know why this initial "i915_sw_fence_complete" is coming during suspend-late.
NOTE1: in the cover letter or prior comment, I hope i mentioned the reproduction steps where
it only occurs when having a workload that does network download that begins downloading just
before suspend is started but completes before suspend late. We are getting close to finding
this - taking time because of the reproduction steps.

Anshuman can chime in if he is seeing new signatures with different callstack /
events after this patch is applied.


> I mean, I'm wondering if it is necessary to re-schedule it from inside...
> but also wonder if this is safe anyway...

alan: so my thought process, also after consulting with John and Daniele, was:
since we have a link list to collect the list of contexts that need to be dereigstered,
and since we have up to 64k (32k excluding multi-lrc) guc-ids, there really is
risk is just keeping it inside the link list until we reach one of the following:

1. full suspend happens and we actually reset the guc - in which case we will remove
   all contexts in this link list and completely destroy them and release all references
   immediately without calling any h2g. (that cleanup will occurs nearly the end of
   gem's suspend late, after which this worker will flush and if it had pending contexts
   would have bailed with !intel_guc_is_ready.

2. suspend is aborted so we come back into the resume steps and we eventually, at some point
   get another request completion that signals a context freeing that we end up retriggering
   this worker in which we'll find two contexts ready to be deregistered.


> 
> > +	 */
> > +	if (!intel_guc_is_ready(guc))
> > +		return;
> > +
> >  	with_intel_gt_pm(gt, tmp)
> >  		deregister_destroyed_contexts(guc);
> >  }
> > -- 
> > 2.39.0
> > 

Re: [Intel-gfx] [PATCH v3 2/3] drm/i915/guc: Close deregister-context race against CT-loss

[Gupta, Anshuman]

> -----Original Message-----
> From: Teres Alexis, Alan Previn <alan.previn.teres.alexis@intel.com>
> Sent: Friday, September 22, 2023 11:32 PM
> To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Gupta, Anshuman
> <anshuman.gupta@intel.com>
> Subject: Re: [PATCH v3 2/3] drm/i915/guc: Close deregister-context race
> against CT-loss
> 
> (cc Anshuman who is working directly with the taskforce debugging this)
> 
> Thanks again for taking the time to review this patch.
> Apologies for the tardiness, rest assured debug is still ongoing.
> 
> As mentioned in prior comments, the signatures and frequency are now
> different compared to without the patches of this series.
> We are still hunting for data as we are suspecting a different wakeref still being
> held with the same trigger event despite.
> 
> That said, we will continue to rebase / update this series but hold off on actual
> merge until we can be sure we have all the issues resolved.
> 
> On Thu, 2023-09-14 at 11:34 -0400, Vivi, Rodrigo wrote:
> > On Sat, Sep 09, 2023 at 08:58:45PM -0700, Alan Previn wrote:
> > > If we are at the end of suspend or very early in resume its possible
> > > an async fence signal could lead us to the
> alan:snip
> 
> 
> > > @@ -3188,19 +3202,33 @@ static inline void guc_lrc_desc_unpin(struct
> intel_context *ce)
> > >  	/* Seal race with Reset */
> > >  	spin_lock_irqsave(&ce->guc_state.lock, flags);
> > >  	disabled = submission_disabled(guc);
> > >
> alan:snip
> > > +	/* Change context state to destroyed and get gt-pm */
> > > +	__intel_gt_pm_get(gt);
> > > +	set_context_destroyed(ce);
> > > +	clr_context_registered(ce);
> > > +
> > > +	ret = deregister_context(ce, ce->guc_id.id);
> > > +	if (ret) {
> > > +		/* Undo the state change and put gt-pm if that failed */
> > > +		set_context_registered(ce);
> > > +		clr_context_destroyed(ce);
> > > +		intel_gt_pm_put(gt);
> >
> > This is a might_sleep inside a spin_lock! Are you 100% confident no
> > WARN was seeing during the tests indicated in the commit msg?
> alan: Good catch - i dont think we saw a WARN - I'll go back and check with the
> task force - i shall rework this function to get that outside the lock.
> 
> >
> > > +	}
> > > +	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
> > > +
> > > +	return 0;
> >
> > If you are always returning 0, there's no pointing in s/void/int...
> Alan: agreed - will change to void.
> > >
> > >
> 
> alan:snip
> > > @@ -3279,6 +3322,17 @@ static void destroyed_worker_func(struct
> work_struct *w)
> > >  	struct intel_gt *gt = guc_to_gt(guc);
> > >  	int tmp;
> > >
> > > +	/*
> > > +	 * In rare cases we can get here via async context-free fence-signals
> that
> > > +	 * come very late in suspend flow or very early in resume flows. In
> these
> > > +	 * cases, GuC won't be ready but just skipping it here is fine as these
> > > +	 * pending-destroy-contexts get destroyed totally at GuC reset time at
> the
> > > +	 * end of suspend.. OR.. this worker can be picked up later on the next
> > > +	 * context destruction trigger after resume-completes
> >
> > who is triggering the work queue again?
> 
> alan: short answer: we dont know - and still hunting this (getting closer now..
> using task tgid str-name lookups).
> in the few times I've seen it, the callstack I've seen looked like this:
> 
> [33763.582036] Call Trace:
> [33763.582038]  <TASK>
> [33763.582040]  dump_stack_lvl+0x69/0x97 [33763.582054]
> guc_context_destroy+0x1b5/0x1ec [33763.582067]
> free_engines+0x52/0x70 [33763.582072]  rcu_do_batch+0x161/0x438
> [33763.582084]  rcu_nocb_cb_kthread+0xda/0x2d0 [33763.582093]
> kthread+0x13a/0x152 [33763.582102]  ?
> rcu_nocb_gp_kthread+0x6a7/0x6a7 [33763.582107]  ? css_get+0x38/0x38
> [33763.582118]  ret_from_fork+0x1f/0x30 [33763.582128]  </TASK>
Alan above trace is not due to missing GT wakeref, it is due to a intel_context_put(),
Which  called asynchronously by rcu_call(__free_engines), we need insert rcu_barrier() to flush all
rcu callback in late suspend.

Thanks,
Anshuman.
> 
> I did add additional debug-msg for tracking and I recall seeing this sequence via
> independant callstacks in the big picture:
> 	i915_sw_fence_complete > __i915_sw_fence_complete ->
> __i915_sw_fence_notify(fence, FENCE_FREE) -> <..delayed?..>
> 	[ drm fence sync func ] <...> engines_notify > call_rcu(&engines>rcu,
> free_engines_rcu) <..delayed?..>
> 	free_engines -> intel_context_put -> ... [kref-dec] -->
> guc_context_destroy
> 
> Unfortunately, we still don't know why this initial "i915_sw_fence_complete"
> is coming during suspend-late.
> NOTE1: in the cover letter or prior comment, I hope i mentioned the
> reproduction steps where it only occurs when having a workload that does
> network download that begins downloading just before suspend is started
> but completes before suspend late. We are getting close to finding this - taking
> time because of the reproduction steps.
> 
> Anshuman can chime in if he is seeing new signatures with different callstack /
> events after this patch is applied.
> 
> 
> > I mean, I'm wondering if it is necessary to re-schedule it from inside...
> > but also wonder if this is safe anyway...
> 
> alan: so my thought process, also after consulting with John and Daniele, was:
> since we have a link list to collect the list of contexts that need to be
> dereigstered, and since we have up to 64k (32k excluding multi-lrc) guc-ids,
> there really is risk is just keeping it inside the link list until we reach one of the
> following:
> 
> 1. full suspend happens and we actually reset the guc - in which case we will
> remove
>    all contexts in this link list and completely destroy them and release all
> references
>    immediately without calling any h2g. (that cleanup will occurs nearly the end
> of
>    gem's suspend late, after which this worker will flush and if it had pending
> contexts
>    would have bailed with !intel_guc_is_ready.
> 
> 2. suspend is aborted so we come back into the resume steps and we
> eventually, at some point
>    get another request completion that signals a context freeing that we end up
> retriggering
>    this worker in which we'll find two contexts ready to be deregistered.
> 
> 
> >
> > > +	 */
> > > +	if (!intel_guc_is_ready(guc))
> > > +		return;
> > > +
> > >  	with_intel_gt_pm(gt, tmp)
> > >  		deregister_destroyed_contexts(guc);
> > >  }
> > > --
> > > 2.39.0
> > >

Re: [Intel-gfx] [PATCH v3 2/3] drm/i915/guc: Close deregister-context race against CT-loss

[Teres Alexis, Alan Previn]

> 


> > alan:snip
> > > > @@ -3279,6 +3322,17 @@ static void destroyed_worker_func(struct
> > work_struct *w)
> > > >  	struct intel_gt *gt = guc_to_gt(guc);
> > > >  	int tmp;
> > > > 
> > > > +	/*
> > > > +	 * In rare cases we can get here via async context-free fence-signals
> > that
> > > > +	 * come very late in suspend flow or very early in resume flows. In
> > these
> > > > +	 * cases, GuC won't be ready but just skipping it here is fine as these
> > > > +	 * pending-destroy-contexts get destroyed totally at GuC reset time at
> > the
> > > > +	 * end of suspend.. OR.. this worker can be picked up later on the next
> > > > +	 * context destruction trigger after resume-completes
> > > 
> > > who is triggering the work queue again?
> > 
> > alan: short answer: we dont know - and still hunting this (getting closer now..
> > using task tgid str-name lookups).
> > in the few times I've seen it, the callstack I've seen looked like this:
> > 
> > [33763.582036] Call Trace:
> > [33763.582038]  <TASK>
> > [33763.582040]  dump_stack_lvl+0x69/0x97 [33763.582054]
> > guc_context_destroy+0x1b5/0x1ec [33763.582067]
> > free_engines+0x52/0x70 [33763.582072]  rcu_do_batch+0x161/0x438
> > [33763.582084]  rcu_nocb_cb_kthread+0xda/0x2d0 [33763.582093]
> > kthread+0x13a/0x152 [33763.582102]  ?
> > rcu_nocb_gp_kthread+0x6a7/0x6a7 [33763.582107]  ? css_get+0x38/0x38
> > [33763.582118]  ret_from_fork+0x1f/0x30 [33763.582128]  </TASK>

> Alan above trace is not due to missing GT wakeref, it is due to a intel_context_put(),
> Which  called asynchronously by rcu_call(__free_engines), we need insert rcu_barrier() to flush all
> rcu callback in late suspend.
> 
> Thanks,
> Anshuman.
> > 
Thanks Anshuman for following up with the ongoing debug. I shall re-rev accordingly.
...alan

Re: [Intel-gfx] [PATCH v3 2/3] drm/i915/guc: Close deregister-context race against CT-loss

[Teres Alexis, Alan Previn]

On Thu, 2023-09-14 at 11:34 -0400, Vivi, Rodrigo wrote:
> On Sat, Sep 09, 2023 at 08:58:45PM -0700, Alan Previn wrote:
alan:snip
> 
> > +	/* Change context state to destroyed and get gt-pm */
> > +	__intel_gt_pm_get(gt);
> > +	set_context_destroyed(ce);
> > +	clr_context_registered(ce);
> > +
> > +	ret = deregister_context(ce, ce->guc_id.id);
> > +	if (ret) {
> > +		/* Undo the state change and put gt-pm if that failed */
> > +		set_context_registered(ce);
> > +		clr_context_destroyed(ce);
> > +		intel_gt_pm_put(gt);
> 
> This is a might_sleep inside a spin_lock! Are you 100% confident no WARN
> was seeing during the tests indicated in the commit msg?
> 
> > +	}
> > +	spin_unlock_irqrestore(&ce->guc_state.lock, flags);
> > +
> > +	return 0;
> 
> If you are always returning 0, there's no pointing in s/void/int...
alan: Hi Rodrigo - i actually forget that i need the error value returned
so that _guc_context_destroy can put the context back into the
guc->submission_state.destroyed_contexts linked list. So i clearly missed returning
the error in the case deregister_context fails.
> 
> >  }
> >  
> >  static void __guc_context_destroy(struct intel_context *ce)
> > @@ -3268,7 +3296,22 @@ static void deregister_destroyed_contexts(struct intel_guc *guc)
> >  		if (!ce)
> >  			break;
> >  
> > -		guc_lrc_desc_unpin(ce);
> > +		if (guc_lrc_desc_unpin(ce)) {
> > +			/*
> > +			 * This means GuC's CT link severed mid-way which could happen
> > +			 * in suspend-resume corner cases. In this case, put the
> > +			 * context back into the destroyed_contexts list which will
> > +			 * get picked up on the next context deregistration event or
> > +			 * purged in a GuC sanitization event (reset/unload/wedged/...).
> > +			 */
> > +			spin_lock_irqsave(&guc->submission_state.lock, flags);
> > +			list_add_tail(&ce->destroyed_link,
> > +				      &guc->submission_state.destroyed_contexts);
> > +			spin_unlock_irqrestore(&guc->submission_state.lock, flags);
alan:snip