Repository navigation
drm/asahi: g17: spare and bound the work a GPU recovery dropped (a bystander compositor keeps its device) - #1240
Conversation
… work it dropped When a GPU recovery cannot name the culprit (on M5 every firmware restart is attributed to the firmware, and classify() gives up when a guilty queue has no in-flight owner), it classifies nobody. Compute commands it dropped before they ran then come back from their queue's records as unexecuted (ENODATA), unclassified, and poisons() is true for unclassified ENODATA: the queue's terminal records the error and fails every queue of the VM, and the client's driver reads the VM status as a lost device. A Chromium GPU fault took Hyprland down this way: right after the faulting client's EIO, the log reported "GPU work of a client failed (ENODATA, ...)" for Hyprland too, and zink lost its device. The render side already handles this: groups a restart dropped are classified Spared before they fail with ENODATA. Do the same for compute: - When a compute queue's front comes back unexecuted after crossing a recovery, and the firmware reported no fault on the queue, classify every unclassified command of the queue Spared before quarantining it. All of them are ordered behind the front, so none of them ran. The queue then takes the existing spared path: its commands replay when their records prove they never ran, and otherwise fail with ENODATA without touching the VM status. - Without a recovery in between, a zero-duration record is the firmware's report of the queue's own failed command (see note_fault()). Those commands are classified Guilty and fail the VM as before. A fault report that arrives after the commands were spared as dropped reclassifies them Guilty, stops the queue from being replayed or returned to service, and queues the VM failure the report would have caused. - A compute replay that cannot be published was spared work: classify it Spared (unless guilty) before it fails with ENODATA, as dropped render groups are. - poisons(): ENODATA no longer poisons unless the command is guilty. A command that never ran cannot have faulted the GPU; a blamed render still fails with EIO, and a faulting compute command is classified Guilty first. The faulting client still fails: a blamed render fails with EIO, a watchdog timeout with ETIMEDOUT, and a reported compute fault now classifies its queue's work Guilty. What changes is only work that never ran on a queue nobody blamed. When the recovery attributes nothing, the command that was running when the firmware stopped and comes back with no record is spared too: its fence still fails with ENODATA, but its VM is not poisoned. A spared command is replayed only when its record shows it never ran, and a second fault on a replayed command is blamed on its VM (oldest_spared() counts replays), so this costs at most one more recovery. Completion::signalled() is added for note_fault(): a spared command whose fence already signalled is not reclassified. Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com> Assisted-by: LLM Claude
…a lost device or never signalling A fence on a scanout buffer that never signals wedges KMS: the atomic commit worker waits for it with no timeout, so no frame reaches the display again, even across compositor restarts. Work a recovery dropped must signal within a bound, and since it never ran, it must not cost its client the device either. Three paths did not do that: - A compute queue whose commands a recovery dropped waits, spared, for its retirement witness before it replays or fails them. When the queue's job watchdog expires first, timeout() failed them with ETIMEDOUT, and fail_while_owned() reports every error but ENODATA and ECANCELED to the client's VM status: the client lost its device for work it never got to run. They now fail with ENODATA. - That witness needs every entry to hold a ticket. A spared queue with an entry without one would never get it, and the entry's own watchdog does nothing for an unpublished packet. quarantine() now fails such spared work at once with ENODATA, and timeout_head() reports a queue whose spared work still waits, ticketed or not, so that other VMs' watchdogs see it as a failed blocker and defer to it within their bounded renewals rather than fail their own work immediately. - Render groups parked for replay after a recovery that do not publish within 10 s made replay_render_front() return ETIMEDOUT. That failed the GPU event service, which loses every client's device and stops the runtime. The parked groups are innocent work that never ran (replay::decide() parks only those), so they now fail with ENODATA like fail_dropped()'s, spared, and the replay pass goes on. Bounds: no new timeout. A spared compute fence is settled by its job's scheduler watchdog, TIMEOUT_MS (2 s) per expiry; on M5 a job is renewed only while it waits behind an older node of its own context and the firmware's progress word moves (at most 14 renewals), plus 4 for ray traversal and AS builds, so 2 s typically and 38 s at most after the quarantine. A parked render replay keeps its 10 s bound (background_stalled(), the same as the recovery handshake's wait). Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com> Assisted-by: LLM Claude
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c1cc06ccc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| packet.completion.classify(recovery::Class::Spared); | ||
| } | ||
| } | ||
| self.fail_replay(ENODATA, defer) |
There was a problem hiding this comment.
When a replay has been prepared but publish_render() returns early with EBUSY—for example, because the outer work ring is full at runtime/render/submit.rs:679-682—self.prepared still owns the front packet. This new timeout path calls fail_replay(), which empties only self.replay, leaving the prepared descriptor installed after signalling the packet's fence. Consequently, room() and direct_ready() remain false, and firmware_drained() also rejects the pair, leaving that slot unusable and unreclaimable even after backpressure clears. Discard the matching unpublished preparation before completing the stalled replay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and it went further than the timeout path. When publish_render() refused a replay before publishing it
(outer ring full, owner switch still releasing, recovery pending), replay_render_front() returned without dropping
the preparation. The next pass's prepare() then failed room() on that same preparation, so a replay refused once
could never be prepared again and always ran into the 10 s bound, and fail_stalled_replay() then left the descriptor
installed. Fixed in the follow-up commit on this branch (kept separate for review, squashed on request). replay_render_front() now discards an unpublished preparation whenever
publish_render() refuses it (runtime/render/replay.rs), so the next pass prepares the group again. fail_replay()
also discards a preparation it finds for the replay front before it fails the parked groups (queue/render.rs), so
no failure path leaves the pair unable to take work. The commit message carries the explanation.
| if completion.spared() || completion.guilty() { | ||
| continue; |
There was a problem hiding this comment.
After a command newly spared by this method is replayed, its completion retains Class::Spared, but publish_with_native_waits() constructs the replacement Active with spared_as_dropped = false. If the replay then receives Event::ComputeFault and a zero-duration record without another recovery, note_fault() skips reclassification because that flag is false, and this continue preserves the spared class despite the explicit fault report. quarantine(ENODATA) therefore suppresses the actual fault's VM failure and can replay the faulty command again. Preserve the dropped-work provenance across republication, or allow the replay's own fault to override its previous spared class.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The spared class lives on the completion, but spared_as_dropped lived on the Active, and
publish_with_native_waits() reset it for the replay. A fault report on the replay therefore never reached it, and
classify_unexecuted() kept the old Spared. Fixed in the follow-up commit on this branch (kept separate for review, squashed on request): the replay's Active now starts with
spared_as_dropped = packet.completion.spared(). A replay runs again from the start, so note_fault() now
reclassifies a faulting replay as Guilty and fails its VM, whether the report arrives before or after the
zero-duration record. If it arrives first, classify_unexecuted() skips the now guilty command and quarantine()
returns its VM status. If it arrives after, note_fault() clears spared_quarantine and queues the terminal, as it
does for first-run dropped work. The commit message carries the explanation.
Follow-up to the automated review of this pull request, kept as a separate
commit for review and squashed on request.
drm/asahi: g17: Fix fences of work a GPU recovery dropped failing as a lost device or never signalling
A fence on a scanout buffer that never signals wedges KMS: the atomic
commit worker waits for it with no timeout, so no frame reaches the
display again, even across compositor restarts. Work a recovery dropped
must signal within a bound, and since it never ran, it must not cost
its client the device either. Three paths did not do that:
- A compute queue whose commands a recovery dropped waits, spared, for
its retirement witness before it replays or fails them. When the
queue's job watchdog expires first, timeout() failed them with
ETIMEDOUT, and fail_while_owned() reports every error but ENODATA and
ECANCELED to the client's VM status: the client lost its device for
work it never got to run. They now fail with ENODATA.
- That witness needs every entry to hold a ticket. A spared queue with
an entry without one would never get it, and the entry's own watchdog
does nothing for an unpublished packet. quarantine() now fails such
spared work at once with ENODATA, and timeout_head() reports a queue
whose spared work still waits, ticketed or not, so that other VMs'
watchdogs see it as a failed blocker and defer to it within their
bounded renewals rather than fail their own work immediately.
- Render groups parked for replay after a recovery that do not publish
within 10 s made replay_render_front() return ETIMEDOUT. That failed
the GPU event service, which loses every client's device and stops
the runtime. The parked groups are innocent work that never ran
(replay::decide() parks only those), so they now fail with ENODATA
like fail_dropped()'s, spared, and the replay pass goes on.
A replay that publish_render() refused before publishing it (outer
work ring full, an owner switch still releasing, a recovery pending)
kept its prepared descriptor, so the next pass could not prepare it
again and the replay always ran into that bound; failing it then left
the descriptor installed, and the pair took no work again. Such a
descriptor is now discarded when the publication is refused, so the
next pass prepares the group again, and fail_replay() discards one it
finds for the front.
Bounds: no new timeout. A spared compute fence is settled by its job's
scheduler watchdog, TIMEOUT_MS (2 s) per expiry; on M5 a job is renewed
only while it waits behind an older node of its own context and the
firmware's progress word moves (at most 14 renewals), plus 4 for ray
traversal and AS builds, so 2 s typically and 38 s at most after the
quarantine. A parked render replay keeps its 10 s bound
(background_stalled(), the same as the recovery handshake's wait).
drm/asahi: g17: Fix a GPU recovery failing the compositor for compute work it dropped
When a GPU recovery cannot name the culprit (on M5 every firmware
restart is attributed to the firmware, and classify() gives up when a
guilty queue has no in-flight owner), it classifies nobody. Compute
commands it dropped before they ran then come back from their queue's
records as unexecuted (ENODATA), unclassified, and poisons() is true for
unclassified ENODATA: the queue's terminal records the error and fails
every queue of the VM, and the client's driver reads the VM status as a
lost device. A Chromium GPU fault took Hyprland down this way: right
after the faulting client's EIO, the log reported "GPU work of a client
failed (ENODATA, ...)" for Hyprland too, and zink lost its device.
The render side already handles this: groups a restart dropped are
classified Spared before they fail with ENODATA. Do the same for
compute:
- When a compute queue's front comes back unexecuted after crossing a
recovery, and the firmware reported no fault on the queue, classify
every unclassified command of the queue Spared before quarantining it.
All of them are ordered behind the front, so none of them ran. The
queue then takes the existing spared path: its commands replay when
their records prove they never ran, and otherwise fail with ENODATA
Compile-checked with W=1 CLIPPY=1 over drivers/gpu/drm/{asahi,apple}: no new
warnings against the base; checkpatch --strict clean; rustfmt unchanged.
Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com>
Assisted-by: LLM Claude
What this is
Two fixes to the G17 recovery path, on
integration/aurora(bdb9994). They change only what happens to work that a GPU recovery dropped before it ran, on queues the recovery did not blame:drm/asahi: g17: Fix a GPU recovery failing the compositor for compute work it dropped. Compute commands that come back unexecuted (ENODATA) after crossing a recovery, with no fault reported on their queue, are classified Spared before the queue is quarantined, as the render side already does for the groups a restart dropped. They replay when their records prove they never ran, otherwise fail with ENODATA, and in neither case touch their VM's status. A zero-duration record without a recovery in between stays the firmware's report of the queue's own failed command (Guilty, fails the VM as before), and a fault report that arrives late reclassifies spared work Guilty.poisons()no longer treats ENODATA as poisoning unless the command is guilty.drm/asahi: g17: Fix fences of work a GPU recovery dropped failing as a lost device or never signalling. Three paths let spared work either cost its client the device or never signal: the spared compute queue's job watchdog failed it with ETIMEDOUT (whichfail_while_owned()mirrors into the VM status as a lost device), now ENODATA; a spared queue with an unticketed entry waited for a witness that can never come, now fails at once with ENODATA, andtimeout_head()reports such a queue so other VMs' watchdogs defer to it within their bounded renewals; render groups parked for replay that do not publish within 10 s madereplay_render_front()return ETIMEDOUT, which failed the GPU event service and every client's device, now they fail Spared/ENODATA and the replay pass goes on. No new timeout: the compute bound is the job's scheduler watchdog (2 s per expiry, bounded renewals: about 2 s typically, 38 s worst on M5), the render replay bound stays 10 s.The problem
On the J716s (MacBook Pro 16-inch, M5 Pro, G17S) a GPU fault in one client took the compositor down every time. On M5 every firmware restart is attributed to the firmware and
classify()names nobody; the compute commands the recovery dropped then come back ENODATA, unclassified, and unclassified ENODATA poisoned the VM: the log saidGPU work of a client failed (ENODATA, ...); the client sees a lost devicefor Hyprland right after the faulting client's EIO, zink segfaulted indri_create_fence_fd, and Hyprland died. Three times in one session on 2026-11-08 (10:35, 10:40, 10:50 UTC) from Chromium-class clients; once the panel stayed black across compositor restarts (the display half of that is the companiondrm/applePR "Fix a GPU fence that never signals wedging the display"). Tracker: rolotrealanis98/m4-m5-linux#36. Analysis: GPU-RECOVERY-KILLS-COMPOSITOR.md and recovery/REPORT.md.The faulting client still fails as before: a blamed render with EIO, a watchdog timeout with ETIMEDOUT, a reported compute fault now classifies its queue's work Guilty first. What changes is only work that never ran on a queue nobody blamed. A spared command is replayed only when its record shows it never ran, and a second fault on a replayed command is blamed on its VM (
oldest_spared()counts replays), so the cost is at most one more recovery.Hardware result (J716s, G17S, firmware 27.3 beta 1 26C5035r)
Kernel 7.3.0-g12c445fa5371: our J716s tree (integration/aurora 33bec40 + #1012 + #1173 + the J716s DCP/board series) with these two commits, the
drm/applebounded wait (companion PR) and the two recovery-diagnostic commits (the "Name the cause and the clients of every M5 GPU recovery" and "M5 GPU fault experiment parameters" PRs) on top. Hyprland on zink, DCP display at 3456x2234@120 with VRR. Load for 10 min: 3x vkcube, glmark2-es2-wayland, two Qt Quick windows on the GPU backend, a Chromium WebGL tab (RESULTS.md 2026-11-08 12:08 UTC):GPU work of a client failed (EIO, ...)line). Hyprland kept its pid through the run, vkcube x3, both Qt Quick windows and Chromium kept running; no crash report, nopage-flip is awaiting, no RCU stall or hung task; swaps continued (1024 per 10 s after the load). The remaining 9.5 min of load produced no recovery.ENODATA ... lost deviceline and a zink SIGSEGV.Not observed on hardware: the spared-compute watchdog path (ENODATA in place of ETIMEDOUT) and the stalled-render-replay path of commit 2 did not fire in these runs (no
replay of N parked groups stalledline); commit 2 is validated as a no-regression there and by reading. An OOM at 12:47 UTC on this kernel was a Hyprland 0.57.1 direct-scanout retry storm (519 swapchain reconfigures, 1557 31 MB GBM buffers never released; the memory came back when the process died), not the kernel: RESULTS.md 12:43 and 13:15. No recovery ran in that boot.Checks
integration/aurorabdb9994 without conflict. Commit 1 differs from the tested patch by one 5-line addition:Completion::signalled()(a read of the existingsignalledflag), which the tested tree had from drm/asahi, drm/apple: M5 GPU and firmware-27 display on integration/aurora #1012 andintegration/auroradoes not;note_fault()uses it to leave an already signalled spared command alone. Commit 2 is the tested diff.make W=1 CLIPPY=1 drivers/gpu/drm/asahi/ drivers/gpu/drm/apple/(LLVM arm64, defconfig + asahi.config, rustc 1.99.1, clang 21) at the base and at each commit, objects rebuilt from scratch: see the Checks line below.checkpatch.pl --strict: 0 errors, 0 warnings, 0 checks on both.Compile check result:
drivers/gpu/drm/asahi/+drivers/gpu/drm/apple/withW=1 CLIPPY=1give the same 68 warnings at the base (bdb9994), at commit 1 and at commit 2; none new, none gone (all pre-existing clippy and deprecation notes). rustfmt on the.rsfiles each commit touches: no new deviation (the two files that already differ from rustfmt differ by the same hunks before and after).Risks
timeout_head()now reports a spared queue whose fences still wait, so other VMs' watchdogs defer to it within their bounded renewals rather than fail their own work at once: up to one more renewal round for them.Questions