Repository navigation
drm/asahi: g17: stop withholding compute doorbells - #278
Conversation
A compute publication with two or more unretired commands of its queue ahead of it withholds its doorbell. Only the compute scan of a later event pass rings it, once at most one command ahead lacks a completion record. Nothing guarantees that such a pass comes: if the last event pass for the commands ahead runs before that condition holds, the firmware sits idle on a command it was never told about, and every waiter on that command stalls until an unrelated firmware message runs the scan. Under a Vulkan game in a muvm guest (DXVK on Honeykrisp, which runs buffer copies as compute) this shows up as GPU-wide stalls of 330 to 400 ms, about ten a minute under steady input. Probes on the GPU mailbox show the firmware sending nothing for the whole stall. Each one opens right after a compute publication and ends when the compute scan rings a withheld compute doorbell, after which the queued work runs at once. The compositor's own frames stall with it. The follow-up commits closed some of the ways into this state (job timeouts, recovery, a record waiting for its stamp) but not the lost wakeup itself. Remove the withholding and everything built on it, so that every compute publication rings its doorbell again. This gives back the few microseconds per hop the withholding saved in a cross-queue semaphore ping-pong. This reverts: 1a3fd7f ("drm/asahi: withhold G17 compute doorbells behind two unretired commands") 26ee6b9 ("drm/asahi: drop G17 compute doorbells that another doorbell announced") 118c3b4 ("drm/asahi: announce withheld G17 compute doorbells whenever retirement stalls") 045f59c ("drm/asahi: announce withheld G17 compute doorbells after a recovery") fbb7695 ("drm/asahi: keep withholding G17 compute doorbells while a stamp is awaited") 1e11df2 ("drm/asahi: restart a withheld G17 compute doorbell another doorbell covered") Fixes: 1a3fd7f ("drm/asahi: withhold G17 compute doorbells behind two unretired commands") Signed-off-by: Jonathan Goyvaerts <jonathan.goyvaerts@gmail.com> Assisted-by: LLM (cherry picked from commit f5ca23b)
scottjones
left a comment
scottjones
left a comment
There was a problem hiding this comment.
We read this against the six commits it reverts and against the tree merged with #275 and #264, and it looks correct to us: the withholding code is fully gone, nothing in those PRs relied on it, and the mirror of f5ca23b is faithful. It only touches G17.
We're building a kernel with this plus #275, #276, #264 and #277 and will run it on our Neo (queue.depth, the stress suite and the synchronization CTS), since this exact form hasn't been on hardware yet. We'll report back here before approving.
scottjones
left a comment
scottjones
left a comment
There was a problem hiding this comment.
We read this against the six commits it reverts (all six are ancestors of integration/aurora) and against the tree merged with #275 and #264. We read source only. We then ran it only as part of the Neo stack run described below.
It does what it says. Every publication rings at g17/queue/compute.rs:2285, after the same SeqCst fence as before. withheld, due_doorbell, notify_compute, compute_written and compute_announced are gone from drivers/gpu/drm/asahi, on this head and in the merged tree. timeout_compute, cancel_compute and recovery's service() only lose the step that rang what was withheld, and now there's nothing withheld to ring. The fields settling() read all keep other users. We found nothing in #275 or #264 that relied on the removed code.
The mirror is faithful. Against f5ca23b the only hunk that differs is the dropped announce_withheld KUnit stub, which this tree doesn't have, as the body says. The other differences are context lines (demand in Port, report_blamed_render). The six reverted hashes are the right omacom ones.
Scope is g17 only, so T8140 and T6040. g15, m3 and the legacy paths don't use port.rs, queue/compute.rs or the g17 runtime, and mmu.rs isn't touched.
Two things to note:
- The Neo numbers came from aurora with AsahiLinux#570, AsahiLinux#571 and AsahiLinux#572 underneath (aeae3d9), and the J616s result from that team's daily kernel. This tree has none of those three yet, so aurora's numbers aren't from this form; ours below are, with four other PRs. Since it restores the doorbell behaviour from before 10-25, we don't think that should hold it.
- This should land before #277. #277 makes a stamp wait for its node to drain, and a withheld doorbell can keep a node from draining. The aurora thread also saw a signal_order case killed by the 30 s watchdog on AsahiLinux#570 to AsahiLinux#572 without this change. It was one run, but the mechanism fits.
Small one, not blocking: the cherry-pick has no Signed-off-by from its committer, unlike #264's. Worth adding if the branch gets touched again.
We ran the stack on a Neo through a lab job: integration/aurora 1358ab0 plus #278, #275, #276, #264, #277 (our build, clang 22, not the release toolchain), mesa-aurora 20261029 (672657e05e), gpubench db3fec6. CTS synchronization 28647 pass and synchronization2 36516 pass, 0 fail, every case reporting a result. No ETIMEDOUT, GPU recovery or lost device, and no stampless retirement, in the kernel log of the whole run. Nothing in that run points at this change. We then ran queue.depth (5 times) and sparse.churn (3 times) at 60 s each, on the same Neo and the same private Honeykrisp with omacom/mesa#20 (18ff4cdc9a), once on the stack and once on the kernel without it (7.2.11-10-1). Stack: queue.depth passed 5 of 5, about 810k-880k completions in the deep 128+64 phase per run (roughly 35k/s) with 0 errors and a worst latency of 45 ms; sparse.churn passed 3 of 3; no flagged kernel line. Without the stack: queue.depth lost the device in run 1 after 6.4 s (ETIMEDOUT at g17/runtime.rs:1291). Run 2, started right after on the same GPU, failed at 2.3 s with 0 checks, so it is the same failure and not a second sample. Then "GPU runtime stopped, device lost", and every later client got ENODEV until reboot. That's one job each on one Neo, with our clang 22 build rather than the release toolchain. It tests the five PRs together, so it doesn't separate them, and #278, #275 and #276 without #264 and #277 hasn't run anywhere on this tree. The other kernel is the 7.2.11-10-1 release, 88 commits behind 1358ab0; its only g17 difference is a logging commit. Throughput fell across the five runs (deep 880k to 811k, mixed 73k to 65k), which may be thermal on a fanless Mac. That failing run also logged an RTKit "failed buffer request for 0x2000 bytes (-22)" that the stack run didn't, and we don't know whether it matters. It says nothing about T6040.
It's a pure removal, it fixes a real lost wakeup, CI is green, and it fits the RC. We'd merge it first of the G17 stack.
|
Update on the line above that says this set hadn't run alone: it has now. We ran #278, #275 and #276 without #264 and #277 on a Neo, on integration/aurora 2dd4aab (our build, clang 22), with the private omacom/mesa#20 driver: queue.depth 5 of 5 at 60 s, sparse.churn 3 of 3, and no ETIMEDOUT, GPU recovery, lost device or stampless retirement in the kernel log. One job on one Mac, and no CTS on this set. |
Mirror of aurora-silicon/linux#573 by @jgoyvaerts, merged there as 9db30ff.
This removes the G17 compute-doorbell withholding (1a3fd7f and its five follow-ups). A withheld doorbell is only rung by the compute scan of a later event pass, and nothing guarantees one comes. When none does, the firmware sits idle on a command it was never told about. Every waiter on that command stalls, and the GPU stalls as a whole, until unrelated firmware traffic runs the scan. On a J616s it showed as 330–400 ms GPU-wide freezes under DXVK in a muvm guest, about ten a minute. The full analysis is in the original PR.
The cherry-pick (
-x) needed one conflict resolved: aurora'srecovery.rshas a KUnit stub forannounce_withheldthat this tree doesn't, so that hunk was dropped. The reverted commits are named by their omacom hashes.Testing:
--allwith no stalls or timeouts (its only failures are the known teardown log lines); queue.depth pass.The stall reproduces on J700 (Neo). A tool that times every GPU compute wait found 14–17 ms waits on multi-command submits (fill + clear-only render pass + fill, like DXVK), about 1 in 10,000 bursts, with withholding. With this change the worst wait is under 2 ms, and median waits move by only +0.5–2.4%. On the Neo some ~60 Hz event rescues the lost wakeup within one frame; on the J616s nothing did for 330–400 ms.