Repository navigation
drm/asahi: g17: keep innocent clients alive across another client's recovery - #433
jgoyvaerts wants to merge 2 commits into
Conversation
…ecovery When one client hangs the GPU and the firmware recovers, other clients lost their device too. On a J616s (25G83) three reason-0 recoveries of one dEQP client took down Hyprland (frozen desktop), Chromium, Electron and a muvm GPU worker; a hang in headless vkmark took down T3 Code the same way. Each failed before publication, and fail_unpublished() reported the error, which Honeykrisp treats as a lost device: - Innocent work waited only briefly for a recovery to settle: render admission behind a spared, quarantined pair for 1 s, the render publication for 2 s, render and compute for 200 polls of 1 ms while a recovery was pending, a spared compute drain for 200 polls, and the compute admission, render capacity and busy waits for 2 s (the last with ETIMEDOUT, which also marks the VM lost). A spared pair drains only when the watchdog settles the dropped work, after one or more 2 s polls: EBUSY. - A command whose input fence, same-VM predecessor or compute producer had failed was cancelled (inherited ECANCELED), so the consumer of innocent work that the firmware dropped in the recovery failed too, and so did a replay of such a command; an ordered output publication inherited its predecessor's error and reported it. Wait instead, while a recovery is pending or the owner's pair is spared and draining, for up to RECOVERY_WAIT_MS from the start of the wait: the longest the watchdog defers work behind another VM before it settles it, ray work included (26 s). The compute admission deadline becomes extendable for this; each render wait keeps its own clock, so its usual bound applies afresh once the recovery ends instead of having been spent during it. A guilty pair's publication still fails after a second, and a guilty VM's sticky status fails its next submission at once. Do not fail a command because an input, a predecessor or a producer failed. As in drm_sched, only its own state decides (cancellation and its VM status); a signalled producer, failed or not, orders nothing. The producer's client sees its own error. The queue's per-engine failed-ordinal record, which existed only to cancel later commands, goes. Do not report ECANCELED from fail_unpublished(), the ordered output publication, or fail_while_owned() for spared work, as complete() already does: a cancellation is not this client's device loss, and a guilty VM carries its own sticky error. The UAPI text for PER_COMMAND_INPUT_SYNCS and VM_STATUS says so. Trade-off: a command no longer waits for a failed same-VM predecessor that is still retained on a quarantined pair; if that predecessor is replayed later, the two run out of order (wrong contents for a frame, not a lost device). Signed-off-by: Jonathan Goyvaerts <jonathan.goyvaerts@gmail.com> Assisted-by: LLM
A failed fence is not proof that its firmware command has retired. Quarantined render and compute work can still own memory, and an abandoned render can replay. Dropping that dependency lets its consumer execute before the retained producer, then lets the producer overwrite the consumer's result after replay. Keep the longer recovery waits and avoid reporting ECANCELED as device loss, but retain cancellation of commands with failed inputs or internal predecessors. Restore error propagation through ordered output publication and replay admission as well. The cancellation reaches the fence without poisoning an innocent VM, while no successful consumer can bypass a retained failed producer. Keep the existing input-sync UAPI contract. Add a regression case for failed predecessor admission. Independent work can still proceed; making failed-input consumers execute needs a separate retirement guarantee first. Assisted-by: LLM Signed-off-by: Ryan Murray <ryan@aurorasilicon.org>
RyanTheTide
left a comment
RyanTheTide
left a comment
There was a problem hiding this comment.
I pushed Ryan fixup 986ec63. A failed fence does not prove that its firmware producer has retired. Dropping that dependency allowed a consumer to run before retained work that could later replay and overwrite its result.
The fix keeps the longer recovery waits and avoids recording ECANCELED as an innocent VM's device loss. It restores failed-input and predecessor cancellation, replay admission checks and ordered output error propagation. Consumers of failed work remain cancelled; allowing them to execute needs a separate retirement guarantee. The input-sync UAPI contract is preserved. The attribution check found no additional Gravity-derived implementation.
The failed-predecessor regression test passes in Asahi KUnit 86/86. The fixup also resolves the long-line warning and is checkpatch-clean. Apple-dcp passes 63/63. The first defconfig plus asahi.config WERROR build passed; the shipping and final fixup-tree builds are still running.
This is not merge-ready until the M1 Pro and M2 one-shot checks pass and the common config dependency in #438 is reviewed. I have not run a new G17 recovery campaign on a J616s.
|
I re-read the PR body, reviews and both comment lists. There are currently no inline review threads or new replies here. The retained-producer ordering issue is resolved conservatively by 986ec63: longer recovery waits and cancellation handling remain, while failed dependencies still cancel their consumers. Running those consumers requires a separate retirement guarantee. My current verdict remains not merge-ready pending the final universal build, the required M1 Pro/M2 one-shot checks and the #438 config review. The new predecessor regression test passes in Asahi KUnit 86/86. |
|
Final software gates now pass on the combined tree containing these heads and the proposed common config correction in #438. Both defconfig plus asahi.config and the Fedora-base recipe build Image, modules and DTBs with WERROR enabled. The Fedora recipe also covers KUNIT=m. Every requested asahi.config setting survives resolution. Asahi KUnit passes 86/86 and apple-dcp host tests pass 63/63. The targeted J713/J616s binding and DTB checks have no diagnostics. The conservative ordering fix and failed-predecessor regression test pass. This remains not merge-ready until the M1 Pro/M2 candidate boots pass and #438 is reviewed. |
|
M2 no-regression qualification now passes with this head in the combined source tree 031f7dec92a1bef6b72509b08c5c597f50474adb. The approved one-shot ran 7.3.0-aurora-t3028. A native Apple GPU compute/readback check verified all 4096 words, the checked kernel fault counters stayed at zero, and device counts, active services and SELinux enforcing matched the baseline. This uses the installed system Mesa to exercise the candidate kernel. The machine is back on 73w, where the same check passes again. The test kernel, its GRUB entry and fallback units have been removed. The saved default, boot.bin and pinned DTB setting are unchanged. The M1 Pro candidate check and the #438 common-config review are still outstanding, so this remains not merge-ready. |
|
Final qualification for 986ec63. The combined test source is 031f7dec92a1bef6b72509b08c5c597f50474adb, tree 36854d3a72e4efdf2c352166498ab436adaf3c48. Both defconfig plus asahi.config and the Fedora-base shipping recipe pass the full Image/modules/DTB build with WERROR enabled. Every requested fragment value survives olddefconfig. Asahi KUnit passes 86/86, apple-dcp host tests pass 63/63, and targeted J713/J616s binding/DTB checks have no diagnostics. Strict source-style checks pass; commit sign-offs are checked separately. The attribution audit found no new material Gravity implementation blocks. Existing GPL-2.0-only Cody Ho and Niklas Sheth credit is preserved. The same candidate image passed approved M1 Pro and M2 one-shot smoke checks against fresh normal baselines. Native GPU compute/readback verified all 4096 words on each device, kernel fault counters stayed at zero, and baseline devices, active services and SELinux enforcing were preserved. Both machines are back on 73w with the checks passing again. Test kernels, task GRUB entries and fallback units are removed. The saved defaults, stock boot.bin files and pinned DTB setting are unchanged. Ryan fixup 986ec63 keeps longer recovery waits and avoids poisoning innocent VMs for cancellation, while preserving failed-dependency cancellation and ordered publication. The failed-predecessor regression test passes. Failed-input consumers remain cancelled; running them requires a separate physical-retirement guarantee. No code hold remains under that conservative scope. Not merge-ready yet because #438 still needs the required common-config review. |
…ecovery A recovery for one client's hang still killed the others: a CS2 reason-0 recovery cancelled 42 commands and Hyprland got ECANCELED from fail_unpublished(), which Honeykrisp treats as a lost device, so the desktop froze until a reboot. - ECANCELED is not reported as this client's failure from fail_unpublished(), the ordered output publication, or fail_while_owned() for spared work. Work the firmware listed as interrupted keeps reporting its failure. - Publication waits for a pending recovery, or for this owner's spared pair to drain, for up to RECOVERY_WAIT_MS (26 s) instead of the usual 2 s: the compute admission deadline extends (Admission:: extend_for_recovery) in ensure_compute, in the recovery waits of compute_packet including the input-source wait, and in Gate::enter_until, whose gate only a recovery closes; the render waits keep a WaitClock whose usual bound starts afresh once the recovery ends, and render admission extends its deadline while a recovery is pending. A spared quarantined pair waits until the caller's deadline; a guilty pair's publication still fails after a second. - Commands with failed inputs or predecessors stay cancelled: a failed fence does not prove its producer retired. The compute deadline host tests move to the longer recovery budget, and a mutant now checks that removing the extension fails them. Link: aurora-silicon#433 Co-developed-by: Ryan Murray <ryan@aurorasilicon.org> Signed-off-by: Ryan Murray <ryan@aurorasilicon.org> Signed-off-by: Jonathan Goyvaerts <jonathan.goyvaerts@gmail.com> Assisted-by: LLM
When one client hangs the GPU and the firmware recovers, other clients lose their device too. On a J616s (T6040, 25G83), three reason-0 recoveries of a single dEQP client took down Hyprland (a frozen desktop), Chromium, an Electron app and a muvm GPU worker. A hang in headless vkmark took down another app the same way.
None of the victims had work on the GPU that failed. Each one failed before publication, and
fail_unpublished()reported that error, which Honeykrisp treats as a lost device. There were two causes.1. Waits shorter than a recovery
Innocent work waited only briefly for a recovery to settle:
ETIMEDOUT, which also marks the VM lost.A spared pair drains only once the watchdog settles the dropped work, after one or more 2 s polls, so all of these lost the race.
Now, while a recovery is pending or the owner's pair is spared and draining, these waits last up to
RECOVERY_WAIT_MSfrom the start of the wait. That is the longest the watchdog defers work behind another VM before it settles it, ray work included (POLL_MS * (1 + EXECUTION_RENEWALS + MAX_RENEWALS), 26 s).Admissiondeadline can now be extended for this.WaitClock), so its usual bound applies afresh once the recovery ends, instead of being used up during it.2. Inherited cancellation
A command was cancelled (inherited
ECANCELED) when its input fence, same-VM predecessor or compute producer had failed. So when the firmware dropped innocent work in a recovery, that work's consumer failed too, and so did a replay of such a command. An ordered output publication inherited its predecessor's error and reported it.This follows drm_sched instead: a dependency's error doesn't fail the consumer. Only the command's own state decides, meaning its cancellation and its VM's status. A signalled producer, failed or not, orders nothing, and the producer's client sees its own error. The queue's per-engine failed-ordinal record existed only to cancel later commands, so it goes.
fail_unpublished(), the ordered output publication andfail_while_owned()for spared work no longer reportECANCELED, which matches whatcomplete()already does. A cancellation isn't this client's device loss, and a guilty VM carries its own sticky error.UAPI
No new interface, but the documented contract changes. The text for
DRM_ASAHI_FEATURE_PER_COMMAND_INPUT_SYNCSand the VM-status query is updated:ECANCELED.Trade-off: a command no longer waits for a failed same-VM predecessor that is still held on a quarantined pair. If that predecessor is replayed later, the two run out of order. The cost is wrong contents for one frame, not a lost device.
Tested: J616s, 7.3 daily tree (this commit on top of the integration tree). The client that kept hanging is
dEQP-VK.memory.pipeline_barrier.graphics.all_device.1048576_vertex_buffer_stride_2. I ran it next to glmark2 and the desktop:KUnit (asahi, QEMU) 107/107, including new tests for the extendable admission deadline,
spared_quarantined()and a failed predecessor. checkpatch: one line-length warning.Not in this PR: the firmware crash we see when that same case triggers firmware-requested TVB growth under heavy device churn. That crash is inside the firmware (a NULL+0x210 read) and needs more work on our side first.
Base:
integration/aurora(420a1c8f5).🤖 Generated with Claude Code