Intel-XE Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/xe/multi_queue: wait for secondary's own suspend in suspend_wait
@ 2026-07-14  5:26 Niranjana Vishwanathapura
  2026-07-14  5:33 ` ✓ CI.KUnit: success for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Niranjana Vishwanathapura @ 2026-07-14  5:26 UTC (permalink / raw)
  To: intel-xe; +Cc: matthew.brost

For a multi-queue group secondary, guc_exec_queue_suspend_wait() (and its
blocking variant) only waited on the primary's suspend, on the assumption
that the secondary's suspend is synchronous. It is not: the secondary's
suspend rides the sched-message worker (short-circuited, no GuC round-trip)
and completes asynchronously. When the primary was already suspended the
forward is a refcount-only transition that queues no new primary SUSPEND
and leaves the primary's suspend_pending clear, so the wait returned
immediately while the secondary's own suspend was still in flight. A
subsequent resume() then tripped the secondary's !suspend_pending assert.

Wait for the secondary's own suspend to complete before waiting on the
primary. On a timeout, ban the queue (which tears down the group) rather
than leave it with suspend_pending set - otherwise the preempt-fence and
hw-engine-group resume paths would resume it and hit the assert.

Factor the per-queue wait into guc_exec_queue_wait_suspend_done() and share
the orchestration between suspend_wait() and suspend_wait_blocking() via
guc_exec_queue_suspend_wait_common().

Assisted-by: Github-Copilot:Claude-opus-4.8
Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_submit.c | 126 ++++++++++++++++-------------
 1 file changed, 70 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index c70c77141a74..352f101b221f 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -2340,25 +2340,23 @@ static void guc_exec_queue_suspend_timeout_ban(struct xe_exec_queue *q)
 	}
 }
 
-static int guc_exec_queue_suspend_wait(struct xe_exec_queue *q)
+/*
+ * Wait for @q's own suspend to complete: suspend_pending cleared, or the queue
+ * killed / GuC stopped. With @blocking, wait uninterruptibly and do not handle
+ * VF recovery (for callers that must complete on behalf of a possibly
+ * cross-process queue); otherwise wait interruptibly.
+ *
+ * Returns 0 on completion or -ETIME on timeout. Interruptible waits may also
+ * return -EAGAIN (VF recovery in progress, retry) or -ERESTARTSYS (aborted by a
+ * signal; suspend_pending may still be set, so callers must not resume()
+ * without re-confirming the suspend).
+ */
+static int guc_exec_queue_wait_suspend_done(struct xe_exec_queue *q, bool blocking)
 {
 	struct xe_guc *guc = exec_queue_to_guc(q);
 	struct xe_device *xe = guc_to_xe(guc);
 	int ret;
 
-	/*
-	 * In multi-queue mode the primary owns the GuC scheduling context for
-	 * the whole group, so wait on the primary's suspend to complete. All
-	 * group members share the same GuC/device, so guc, xe and timeout above
-	 * are computed from @q directly.
-	 *
-	 * A secondary's suspend is short-circuited (no GuC round-trip) and, as
-	 * its SUSPEND message precedes the primary's on the shared FIFO
-	 * submit_wq, completes before the primary's. So waiting on the primary
-	 * is sufficient.
-	 */
-	q = xe_exec_queue_multi_queue_primary(q);
-
 	/*
 	 * Likely don't need to check exec_queue_killed() as we clear
 	 * suspend_pending upon kill but to be paranoid but races in which
@@ -2369,73 +2367,89 @@ static int guc_exec_queue_suspend_wait(struct xe_exec_queue *q)
 	 xe_guc_read_stopped(guc))
 
 retry:
-	if (IS_SRIOV_VF(xe))
+	if (blocking) {
+		if (IS_SRIOV_VF(xe))
+			ret = wait_event_timeout(guc->ct.wq, WAIT_COND, HZ * 5);
+		else
+			ret = wait_event_timeout(q->guc->suspend_wait, WAIT_COND,
+						 HZ * 5);
+	} else if (IS_SRIOV_VF(xe)) {
 		ret = wait_event_interruptible_timeout(guc->ct.wq, WAIT_COND ||
-						       vf_recovery(guc),
-						       HZ * 5);
-	else
+						       vf_recovery(guc), HZ * 5);
+	} else {
 		ret = wait_event_interruptible_timeout(q->guc->suspend_wait,
 						       WAIT_COND, HZ * 5);
+	}
 
-	if (vf_recovery(guc) && !xe_device_wedged((guc_to_xe(guc))))
+	if (!blocking && vf_recovery(guc) && !xe_device_wedged(xe))
 		return -EAGAIN;
 
-	if (!ret) {
-		guc_exec_queue_suspend_timeout_ban(q);
+	if (!ret)
 		return -ETIME;
-	} else if (IS_SRIOV_VF(xe) && !WAIT_COND) {
+	else if (!blocking && IS_SRIOV_VF(xe) && !WAIT_COND)
 		/* Corner case on RESFIX DONE where vf_recovery() changes */
 		goto retry;
-	}
 
 #undef WAIT_COND
 
-	/*
-	 * ret < 0 (-ERESTARTSYS): the interruptible wait was aborted by a
-	 * signal. The queue is not banned - the failure is in the waiter, not
-	 * the queue. The suspend is not confirmed complete, so suspend_pending
-	 * may still be set; callers must not resume() on this error without
-	 * re-confirming the suspend.
-	 */
 	return ret < 0 ? ret : 0;
 }
 
-static int guc_exec_queue_suspend_wait_blocking(struct xe_exec_queue *q)
+static int guc_exec_queue_suspend_wait_common(struct xe_exec_queue *q, bool blocking)
 {
-	struct xe_guc *guc = exec_queue_to_guc(q);
-	struct xe_device *xe = guc_to_xe(guc);
 	int ret;
 
 	/*
-	 * Uninterruptible variant of guc_exec_queue_suspend_wait() for callers
-	 * that must complete the wait on behalf of a queue possibly owned by a
-	 * different process (e.g. cleanup/undo paths). An interruptible wait
-	 * could return -ERESTARTSYS if the calling task is signalled, leaving
-	 * that queue suspended forever (cross-process DoS).
+	 * A secondary's suspend rides the sched-message worker (short-circuited,
+	 * no GuC round-trip) and so is not synchronous with
+	 * guc_exec_queue_suspend(): its own suspend_pending may still be set
+	 * here. Waiting on the primary alone is not sufficient - if the primary
+	 * was already suspended, the forward is a refcount-only transition that
+	 * queues no new primary SUSPEND and leaves the primary's suspend_pending
+	 * clear, so the primary wait would return immediately while the
+	 * secondary's suspend is still in flight, and a later resume() would trip
+	 * the secondary's !suspend_pending assert. So first wait for the
+	 * secondary's own suspend to complete, then wait on the primary.
 	 *
-	 * A timeout is still a real per-queue fault, so it bans and cleans up
-	 * like suspend_wait(). VF recovery is deliberately not handled (no
-	 * -EAGAIN) since a blocking caller cannot retry.
+	 * A timeout on either bans the queue (being multi-queue, that tears down
+	 * the whole group). A secondary suspend has no real GuC round-trip, so
+	 * its timeout is a software scheduler stall rather than a GuC fault, but
+	 * banning is still the safe recovery: otherwise the queue is left with
+	 * suspend_pending set and a subsequent resume() trips the !suspend_pending
+	 * assert.
 	 */
-	q = xe_exec_queue_multi_queue_primary(q);
-
-#define WAIT_COND \
-	(!READ_ONCE(q->guc->suspend_pending) ||	exec_queue_killed(q) || \
-	 xe_guc_read_stopped(guc))
+	if (xe_exec_queue_is_multi_queue_secondary(q)) {
+		ret = guc_exec_queue_wait_suspend_done(q, blocking);
+		if (ret == -ETIME)
+			guc_exec_queue_suspend_timeout_ban(q);
+		if (ret)
+			return ret;
+	}
 
-	if (IS_SRIOV_VF(xe))
-		ret = wait_event_timeout(guc->ct.wq, WAIT_COND, HZ * 5);
-	else
-		ret = wait_event_timeout(q->guc->suspend_wait, WAIT_COND, HZ * 5);
+	q = xe_exec_queue_multi_queue_primary(q);
+	ret = guc_exec_queue_wait_suspend_done(q, blocking);
+	if (ret == -ETIME)
+		guc_exec_queue_suspend_timeout_ban(q);
 
-#undef WAIT_COND
+	return ret;
+}
 
-	if (!ret) {
-		guc_exec_queue_suspend_timeout_ban(q);
-		return -ETIME;
-	}
+static int guc_exec_queue_suspend_wait(struct xe_exec_queue *q)
+{
+	return guc_exec_queue_suspend_wait_common(q, false);
+}
 
-	return 0;
+/*
+ * Uninterruptible variant of guc_exec_queue_suspend_wait() for callers that
+ * must complete the wait on behalf of a queue possibly owned by a different
+ * process (e.g. cleanup/undo paths). An interruptible wait could return
+ * -ERESTARTSYS if the calling task is signalled, leaving that queue suspended
+ * forever (cross-process DoS). VF recovery is deliberately not handled (no
+ * -EAGAIN) since a blocking caller cannot retry.
+ */
+static int guc_exec_queue_suspend_wait_blocking(struct xe_exec_queue *q)
+{
+	return guc_exec_queue_suspend_wait_common(q, true);
 }
 
 static void guc_exec_queue_resume(struct xe_exec_queue *q)
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-07-15 19:45 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-14  5:26 [PATCH] drm/xe/multi_queue: wait for secondary's own suspend in suspend_wait Niranjana Vishwanathapura
2026-07-14  5:33 ` ✓ CI.KUnit: success for " Patchwork
2026-07-14  6:14 ` ✓ Xe.CI.BAT: " Patchwork
2026-07-14  9:24 ` ✓ Xe.CI.FULL: " Patchwork
2026-07-15 19:45 ` [PATCH] " Matthew Brost

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox