From: Niranjana Vishwanathapura <[email protected]>
[ Upstream commit 65f40fa5022bcdb90d3a39e4948adf2aa16732a3 ] A consumer-issued suspend() can fail (e.g. the queue is killed, banned or wedged), leaving the queue un-suspended. The consumer must then not issue the matching resume(): resuming a queue that was never suspended is incorrect. Add an lr.suspended flag to struct xe_exec_queue that records whether a consumer suspend() succeeded and a matching resume() is still owed. Set it on a successful suspend() in the preempt-fence path, clear it on resume(), and only resume queues that have it set. In resume_and_reinstall_preempt_fences() also skip queues that have since been reset/killed/banned/wedged: such a queue's suspend may not have completed (suspend_pending can still be set, e.g. a preempt fence signalled with -ENOENT without waiting), so resuming it would trip the !suspend_pending assert in the backend. Leave it marked suspended and let teardown resolve its state. A queue is only ever suspended by a single consumer at a time (preempt-fence mode and hw engine group fault mode are mutually exclusive), so a single flag is sufficient. Assisted-by: Github-Copilot:Claude-opus-4.8 Signed-off-by: Niranjana Vishwanathapura <[email protected]> Reviewed-by: Matthew Brost <[email protected]> Link: https://patch.msgid.link/[email protected] Signed-off-by: Sasha Levin <[email protected]> --- LLM Generated explanations, may be completely bogus: This tree is **v7.3-rc4** (mainline). The Xe preempt-fence path this patch touches is present, and the change is a small, self-contained pairing fix for a real GuC suspend/resume protocol bug. ## Phase 1: Commit message forensics **Step 1.1: Subject line** - Record: `[drm/xe] [only resume] only resume exec queues that were actually suspended` - Subsystem: Intel Xe DRM. Verb “only resume” is a restriction/guard: do not resume a queue that was never successfully suspended. **Step 1.2: Tags** - Record: - `Assisted-by: Github-Copilot:Claude-opus-4.8` - `Signed-off-by: Niranjana Vishwanathapura` (author) - `Reviewed-by: Matthew Brost` (Xe maintainer) - `Link: https://patch.msgid.link/20260713202317.2187787-9- [email protected]` - No `Fixes:`, `Reported-by:`, `Tested-by:`, `Cc: stable` (absence is expected) - Ignore any pipeline `Signed-off-by: Sasha Levin` / `[Upstream commit …]` markers; none were on the real commit **Step 1.3: Body** - Record: `[Bug] Consumer `suspend()` can fail (killed/banned/wedged) leaving the queue un-suspended; the matching `resume()` was still issued. A second case: suspend started but the preempt fence was signalled `-ENOENT` without `suspend_wait()`, so `suspend_pending` can still be set; `resume()` then trips the backend `!suspend_pending` assert.` - `[Symptom]` Incorrect resume pairing; GuC backend assert; scheduling state left inconsistent. - `[Version]` None stated. Series title: “balance exec queue suspend/resume”. - `[Root cause]` Resume was unconditional. Fix: `lr.suspended` set only on successful `suspend()`, cleared on `resume()`, and skip reset/killed/banned/wedged queues. **Step 1.4: Hidden bug fix?** - Record: Not disguised. Explicit correctness fix. Covering letter also calls the series “fixes and hardens”; this patch is the pairing fix, not a pure cleanup. ## Phase 2: Diff analysis **Step 2.1: Inventory** - Record: 3 files, **+36 / -1** - `xe_exec_queue_types.h`: add `bool lr.suspended` (+12) - `xe_preempt_fence.c`: `preempt_fence_enable_signaling()` (+7) - `xe_vm.c`: `resume_and_reinstall_preempt_fences()` (+17 / -1) - Scope: single-subsystem surgical fix. **Step 2.2: Code flow** - Record: - **Before:** `suspend()` return stored in `pfence->error`; `resume_and_reinstall_preempt_fences()` always called `q->ops->resume(q)` for every queue on `vm->preempt.exec_queues`. - **After:** successful `suspend()` does `WRITE_ONCE(q->lr.suspended, true)`; resume only if `READ_ONCE(q->lr.suspended) && !q->ops->reset_status(q)`, then clears the flag. - Path: compute/long-running VM rebind worker (preempt-fence mode), not probe/init. **Step 2.3: Bug mechanism** - Record: `[Logic / state-machine pairing]` plus `[debug assert]` - At the parent of this commit, GuC resume was: ```2248:2254:drivers/gpu/drm/xe/xe_guc_submit.c // parent: 65f40fa5022bc^ xe_gt_assert(guc_to_gt(guc), !q->guc->suspend_pending); guc_exec_queue_try_add_msg(q, msg, RESUME); ``` - Failed `suspend()` returns `-EINVAL` *before* setting `suspend_pending`. Unconditional `resume()` still queues a GuC RESUME. - `preempt_fence_work_func()` on `reset_status()` signals `-ENOENT` without `suspend_wait()`, so `suspend_pending` can still be true when resume runs. - `guc_exec_queue_allowed_to_change_state()` is `!killed/banned/wedged && registered` — it does **not** check RESET. A reset-but-not-yet- killed queue can take the resume path and call `enable_scheduling()`. **Step 2.4: Fix quality** - Record: Obviously correct pairing (resume only if suspend succeeded and queue is not in reset/kill). Minimal. Adds an internal flag only. WRITE_ONCE/READ_ONCE match the comment that only one consumer suspends at a time (preempt-fence vs hw-engine-group fault mode). Regression risk: low; skipping resume is limited to queues already failing, which the commit says teardown owns. ## Phase 3: Git history **Step 3.1: Blame / introduction** - Record: Unconditional `q->ops->resume(q)` in `resume_and_reinstall_preempt_fences()` dates to `dd08ebf6c3525` (“drm/xe: Introduce a new DRM driver for Intel GPUs”), i.e. Xe’s introduction (v6.8-era). Long-standing, not last-month code. **Step 3.2: Fixes: tag** - Record: No `Fixes:` tag. Buggy resume site is original Xe code, not a recent regression-only commit. **Step 3.3: Related file history** - Record: This is **patch 1/6** of “drm/xe: balance exec queue suspend/resume”. Later commits in this tree that use `lr.suspended` / continue the series: - `dad2af2da9ead` ban on suspend timeout - `412c885692cad` uninterruptible suspend wait - `c373eaaada2fe` hw_engine_group propagate suspend failures (sets `lr.suspended` there) - `3c316b09072c2` suspend refcount - `5d5a729cc9780` multi-queue preempt primary - Patch 1 is standalone for the preempt-fence path. Later patches are not required for this hunk to be correct. **Step 3.4: Author** - Record: Niranjana Vishwanathapura, regular Xe contributor. Reviewed by Matthew Brost, Xe maintainer (`MAINTAINERS`: INTEL DRM XE DRIVER). **Step 3.5: Dependencies** - Record: Needs existing `q->ops->suspend`, `resume`, `reset_status`, and `struct xe_exec_queue` `lr` — all present here. No new helper from later series patches. Can apply standalone. ## Phase 4: Mailing list / external **Step 4.1: Original discussion** - Record: `b4 dig -c 65f40fa5022bc` → https://patch.msgid.link/202607132 [email protected] - Revisions: v1 (2026-06-24, 1/5) → v2 → v3 (1/6) → v5 covering letter 2026-07-13. Applied version is v5 1/6. - Covering letter: consumers issued resume even when suspend never took effect; “with the GuC backend it can trip the `!suspend_pending` assertion or leave scheduling state inconsistent.” - No NAKs in the saved thread mbox (series posts only; `Reviewed-by` was already on the submission). **Step 4.2: Recipients (`b4 dig -w`)** - Record: `[email protected]`, Matthew Brost, Thomas Hellström (both Xe maintainers). Right list and maintainers. **Step 4.3: Bug report** - Record: No syzbot, bugzilla, or user `Reported-by`. Author-driven correctness fix. **Step 4.4: Series** - Record: 6 patches. Patch 1 = this commit (preempt-fence pairing). Patch 4 applies the same flag to hw engine group. Patch 6 is multi- queue GuC preempt behavior. This commit does not depend on 2–6. **Step 4.5: Stable list** - Record: lore.kernel.org/stable search for this subject did not show a dedicated stable request or rejection (unrelated 7.2/6.18 review mails matched generic words). ## Phase 5: Semantic / call-chain **Step 5.1: Functions** - Record: `preempt_fence_enable_signaling`, `resume_and_reinstall_preempt_fences`. New field `xe_exec_queue.lr.suspended`. **Step 5.2: Callers** - Record: `resume_and_reinstall_preempt_fences()` is called once from the preempt rebind worker after `arm_preempt_fences()` (“point of no return”). `preempt_fence_enable_signaling` is the dma-fence `enable_signaling` callback for Xe preempt fences. **Step 5.3: Callees** - Record: `q->ops->suspend` / `resume` / `reset_status` (GuC backend is the real one; execlist ops are NIY stubs). **Step 5.4: Reachability** - Record: Yes from userspace. `xe_vm_in_preempt_fence_mode()` is long- running compute VMs (`XE_VM_FLAG_LR_MODE` and not fault mode). Triggered when the VM rebind worker runs (BO moves, userptr invalidation, eviction) while compute exec queues are attached. Concurrent GPU reset/kill of a queue during that window is the bad path. **Step 5.5: Similar patterns** - Record: Same `lr.suspended` discipline later in `xe_hw_engine_group.c` (series patch 4). Confirms this is the intended pairing primitive, not a one-off hack. ## Phase 6: This tree **Step 6.1: Buggy code in this tree?** - Record: Yes. Tree is `v7.3-rc4-27-gf0100363d8c37`, `VERSION=7 PATCHLEVEL=3 SUBLEVEL=0 EXTRAVERSION=-rc4`. Xe files exist. Parent of `65f40fa5022bc` still has unconditional `q->ops->resume(q)`. Bug introduced with Xe (`dd08ebf6c3525`), well before 7.3. **Step 6.2: Backport difficulty** - Record: For this tree, the surrounding structs/ops match the diff: clean apply. (This commit is already an ancestor of HEAD; that is used only to inspect the code, not as a “already selected” reason.) **Step 6.3: Related fixes already here** - Record: No earlier alternate fix of this pairing. Later series patches extend the same flag to other callers; they do not replace this preempt-fence fix. ## Phase 7: Subsystem context **Step 7.1** - Record: `[drivers/gpu/drm/xe] [IMPORTANT]` — Intel Xe (Lunar Lake and newer). Not core mm/net, but a supported production GPU driver. **Step 7.2** - Record: High activity (many Xe commits in this tree). The bug itself is old pairing, not a brand-new subsystem. ## Phase 8: Impact and risk **Step 8.1: Who is affected** - Record: Xe users with long-running/compute exec queues in preempt- fence mode (Intel compute / Level Zero style workloads), during VM rebind plus queue reset/kill/ban/wedge. **Step 8.2: Trigger** - Record: Not every boot. Concurrent rebind + queue failure/reset. Userspace-reachable via compute VM + memory movement + hang/reset or queue destroy. Timing-dependent. **Step 8.3: Failure mode** - Record: `xe_gt_assert` is **debug-only** (`CONFIG_DRM_XE_DEBUG`); production distro kernels will not WARN there. Production still queues GuC RESUME without a completed suspend. For killed queues, `allowed_to_change_state` makes the resume handler mostly a no-op. For **RESET but not yet killed** queues, resume can `enable_scheduling()` on a context hang recovery is tearing down — GPU scheduling inconsistency / possible hang of that engine, not a kernel oops. - Severity: **HIGH** for the reset-window GuC protocol break; **MEDIUM** if only dead queues are poked (mostly no-op + debug WARN). **Step 8.4: Risk-benefit** - Record: Benefit: medium-high for Xe compute (prevents illegal resume / enable on reset queues). Risk: low (36 lines, error/teardown path, no UAPI). Ratio favors taking it. ## Phase 9: Synthesis **Step 9.1: Evidence** FOR: - Real pairing bug: resume of never-suspended or still-pending-suspend queues. - Concrete backend contract: `guc_exec_queue_resume()` asserts `!suspend_pending` and (in production) still submits RESUME. - Reset vs kill mismatch: `reset_status` includes RESET; `allowed_to_change_state` does not — so old resume can re-enable a reset context. - Small, obviously correct, maintainer-reviewed (Matthew Brost). - Self-contained for preempt-fence; this tree has the code. - No new userspace API. AGAINST: - Series 1/6; later patches continue related hardening (not required here). - No `Reported-by` / syzbot. - Healthy queues on the success path are unchanged; the skip is for already-failing queues. - Primary wording in the commit is the debug assert. UNRESOLVED: - No public user crash report proving a full-device GPU hang in the wild. - Stable list had no explicit ACK/NAK for this subject. **Step 9.2: Stable rules** 1. Obviously correct and tested? **PASS** — pairing is straightforward; v1–v5 review; maintainer `Reviewed-by`. 2. Real bug affecting users? **PASS** — reachable compute rebind + reset/kill path; illegal GuC RESUME. 3. Important (crash/security/corruption/deadlock)? **PASS** — not a kernel oops in production, but GuC protocol violation and `enable_scheduling()` on a reset queue is a serious GPU-driver failure (hang / inconsistent scheduling), which is in the “etc.” class for driver stability. 4. Small and contained? **PASS** — 36 lines, 3 Xe files. 5. No new features/APIs? **PASS** — internal flag only. (Series patch 6 is a feature; this patch is not.) 6. Can apply to this tree? **PASS** — Xe preempt-fence path and ops exist; hunk is against the local structures. **Step 9.3: Exception category** - Record: None (not a device ID, quirk, DT, build, or docs fix). Qualifies as a normal bugfix. **Step 9.4: Decision** This is a small, reviewed correctness fix for a real suspend/resume imbalance on a supported Intel GPU compute path. It does not add features, applies to this tree’s Xe code, and the production risk of the bug is illegal GuC resume (including re-enabling a reset context), not merely a debug WARN. That meets stable rules for this checkout. ### What it solves for users of this tree On Xe long-running (preempt-fence) VMs, the rebind worker always resumed every attached exec queue. If `suspend()` had failed or the queue was reset before suspend finished, that resume violated the GuC backend contract. This patch records a successful suspend and only resumes those queues, so already-failed queues are left to teardown instead of getting a spurious RESUME. ### Risk vs benefit Benefit is avoiding GuC protocol errors and possible engine re-enable during reset. Risk is low: one flag, two call sites, no UAPI, skip limited to reset/killed queues. ### Concerns It is patch 1/6; hw-engine-group pairing is a later commit and is not required for this hunk. No in-the-wild report. Debug asserts are compiled out of production, so the user-visible production issue is the protocol/state bug, not a WARN. ### Verification - [Phase 1] Parsed commit `65f40fa5022bc`: tags as listed; no Fixes/Reported-by/Cc-stable. - [Phase 2] Diff: +36/-1 in three Xe files; resume became conditional on `lr.suspended && !reset_status()`. - [Phase 2] Parent `guc_exec_queue_suspend` returns `-EINVAL` for killed/banned/wedged without setting `suspend_pending`; parent `guc_exec_queue_resume` asserts `!suspend_pending` then `try_add_msg(RESUME)`. - [Phase 2] `xe_assert.h`: `xe_gt_assert` effective only with `CONFIG_DRM_XE_DEBUG`. - [Phase 2] `guc_exec_queue_allowed_to_change_state()` ignores RESET; `guc_exec_queue_reset_status()` includes RESET; `xe_guc_exec_queue_reset_handler()` sets reset then `trigger_cleanup` — window verified in `xe_guc_submit.c`. - [Phase 3] `git log -S 'resume_and_reinstall_preempt_fences' -- drivers/gpu/drm/xe/xe_vm.c` → `dd08ebf6c3525`. - [Phase 3] Series neighbors confirmed via `git log --author=Niranjana Vishwanathapura -- drivers/gpu/drm/xe/`. - [Phase 3] `git merge-base --is-ancestor 65f40fa5022bc HEAD` succeeds (used to inspect, not as a selection argument). `git merge-base --is- ancestor 65f40fa5022bc v7.2` fails; present since `v7.3-rc1`. - [Phase 4] `b4 dig -c 65f40fa5022bc` / `-a` / `-w` / `-m /tmp/xe- suspend.mbox`: v5 1/6, maintainers CC’d, covering letter quoted above; mbox had no NAK replies. - [Phase 4] lore.kernel.org/stable search: no dedicated discussion of this patch. - [Phase 5] Only caller of `resume_and_reinstall_preempt_fences` is the rebind worker in `xe_vm.c` (~line 569). - [Phase 6] `git describe HEAD` → `v7.3-rc4-27-gf0100363d8c37`; `make kernelversion` → `7.3.0-rc4`. - [Phase 7] `MAINTAINERS`: Matthew Brost is Xe maintainer; he reviewed the patch. - [Phase 8] Failure mode: production GuC RESUME without completed suspend; debug WARN only if `CONFIG_DRM_XE_DEBUG`. - UNVERIFIED: no public user GPU-hang log for this exact race. **YES** drivers/gpu/drm/xe/xe_exec_queue_types.h | 12 ++++++++++++ drivers/gpu/drm/xe/xe_preempt_fence.c | 7 +++++++ drivers/gpu/drm/xe/xe_vm.c | 18 +++++++++++++++++- 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h index 38906cb7608ca..c5024acf4aaf0 100644 --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h @@ -139,6 +139,18 @@ struct xe_exec_queue { u32 seqno; /** @lr.link: link into VM's list of exec queues */ struct list_head link; + /** + * @lr.suspended: Tracks whether the consumer-issued suspend() + * succeeded and a matching resume() is still owed. suspend() can + * fail (e.g. killed/banned/wedged), leaving the queue + * un-suspended, so consumers must only resume() queues that were + * actually suspended. Set by the suspend caller on success and + * cleared by the resume caller. A queue is only ever suspended by + * a single consumer at a time (preempt-fence mode and hw engine + * group fault mode are mutually exclusive), so a single flag is + * sufficient. + */ + bool suspended; } lr; #define XE_EXEC_QUEUE_TLB_INVAL_PRIMARY_GT 0 diff --git a/drivers/gpu/drm/xe/xe_preempt_fence.c b/drivers/gpu/drm/xe/xe_preempt_fence.c index 83fbeea5aa201..014a5980febd0 100644 --- a/drivers/gpu/drm/xe/xe_preempt_fence.c +++ b/drivers/gpu/drm/xe/xe_preempt_fence.c @@ -63,6 +63,13 @@ static bool preempt_fence_enable_signaling(struct dma_fence *fence) struct xe_exec_queue *q = pfence->q; pfence->error = q->ops->suspend(q); + /* + * Record a successful suspend so the rebind worker only resumes queues + * that were actually suspended; a failed suspend() leaves the queue + * un-suspended and must not be paired with a resume(). + */ + if (!pfence->error) + WRITE_ONCE(q->lr.suspended, true); queue_work(q->vm->xe->preempt_fence_wq, &pfence->preempt_work); return true; } diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index a4272bc248ec7..b139ee46208ad 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -195,7 +195,23 @@ static void resume_and_reinstall_preempt_fences(struct xe_vm *vm, xe_vm_assert_held(vm); list_for_each_entry(q, &vm->preempt.exec_queues, lr.link) { - q->ops->resume(q); + /* + * Only resume queues whose suspend() actually succeeded. A + * failed suspend() (e.g. killed/banned/wedged) leaves the queue + * un-suspended, so it must not be resumed. + * + * Also skip queues that have since been reset/killed/banned/ + * wedged: their suspend may not have completed (suspend_pending + * can still be set, e.g. a preempt fence signalled with -ENOENT + * without waiting), so resuming would trip the !suspend_pending + * assert in the backend. Such queues are being torn down anyway, + * so leave them marked suspended and let teardown resolve their + * state. + */ + if (READ_ONCE(q->lr.suspended) && !q->ops->reset_status(q)) { + WRITE_ONCE(q->lr.suspended, false); + q->ops->resume(q); + } drm_gpuvm_resv_add_fence(&vm->gpuvm, exec, q->lr.pfence, DMA_RESV_USAGE_BOOKKEEP, DMA_RESV_USAGE_BOOKKEEP); -- 2.53.0
