Repository navigation
drm/asahi: g17: count the stamp fallback from the drain of the command's node (stacked on #264) - #277
Conversation
…d's node The firmware stores the completion stamps of an ioctl's commands only once every kick counted on the ioctl's work-state node has completed: a node's completion word reaches the host's submitted count, and only then do the compute and render stamps move. A command whose record or pass record is present therefore waits for its stamp for as long as the rest of its ioctl runs. A command buffer of a dozen render passes and their copies, or a compute command beside a render that waits on another queue, keeps the stamps of its early members back for tens to hundreds of milliseconds. The 50 ms stampless fallback measured from the record, so it retired those members on their records while the firmware was still running their node: their fences, their VM pins and their render leases were released before the post-maintenance stamp the retirement path otherwise requires. On the Neo, one CTS synchronization2 pass retired about 9.8k compute commands this way. Each of them had kicks of its own node still running (the node's completion word below the host count), and the stamp covered it 4 to 300 ms later (median 28 ms). Count the bound from the drain instead. Once it has passed, a compute command or a render group whose stamps lag asks its node whether every counted kick completed; if not, the stamps are not late and the wait starts again. A drained node whose stamps still lag keeps the existing fallback, so a group whose stamps stop one kick short still retires on its pass record, and the watchdog's expiry still retires an executed group without waiting for the node. A node that cannot be read keeps the old bound. Signed-off-by: DjDeveloperr <hey@djdev.me> (cherry picked from commit effb9e0)
scottjones
left a comment
scottjones
left a comment
There was a problem hiding this comment.
We read this at 7eaf9b6 on #264's branch, and ran it on a Neo with #278, #275, #276 and #264.
This closes the common case from #264. Compute (g17/queue/compute.rs:1048) and both render checks (g17/queue/render.rs:1065, 1328) now ask the node before retiring without stamps, and only published kicks count, so a full ring can't wait on itself. Our small model found 0 new deadlocks. We want this with #264.
What keeps this a COMMENT is recovery. Replay publishes again and adds to the node's count (g17/queue/compute.rs:2189, g17/queue/render.rs:844). From source we can't tell whether firmware counts a kick dropped by recovery on the completion word. If it doesn't, an innocent client's node never drains after another VM's recovery. Compute would then fail its VM through timeout (g17/queue/compute.rs:1811) and render would wait for the watchdog, where the old bound rescued both. None of our runs or your qualification exercised recovery. A run with an innocent client beside a forced hang, showing the node drains, or a fallback when it doesn't, is what would get our approval here, and on #264 with it.
Worth fixing or noting, not blocking:
- The bound doesn't count from the drain, despite the comments at g17/queue/compute/retirement.rs:75 and g17/queue/render/retirement.rs:159. The first sample that sees a drained node retires at once (compute/retirement.rs:116, render/retirement.rs:197), so a drain just before it retires after 0 ms. A compute command behind the front can be past the bound on its first sample (g17/queue/compute.rs:934). A one-bit latch restarting the wait on the first drained sample would make the comments true.
- expire_stamp_wait still skips the node (g17/queue/render.rs:1108). A group that used to leave at 50 ms now reaches that whenever its ioctl outlasts the watchdog, with #264's full exposure. We'd like a separate log line for retiring with the node still running.
- The comment at compute/retirement.rs:84 ("cannot fail its VM") is no longer true.
- The 1 ms poll now lasts as long as the sibling kicks run (g17/queue/compute.rs:871, g17/runtime/events.rs:513). No A/B row has a long job beside a short member.
T6040: g17/context.rs:92 reads a completion word only observed on 26A428. If it doesn't advance on 25G83, drained() is false forever and the fallback is gone. Mesa at 18ff4cdc9a refuses that chip today, so we'd want one run reading the word before that changes.
The mirror is faithful: the patch matches effb9e0 except hunk offsets and one context line.
On a Neo with our clang 22 build: CTS synchronization and synchronization2 0 fail with no stampless retirement, against the 9.1k to 9.7k compute retirements the AsahiLinux#570 and AsahiLinux#571 bodies report without this. Then queue.depth 5/5 and sparse.churn 3/3 with omacom/mesa#20. The three-PR build without this also logged none under those stress runs, so only the CTS result speaks to this gate. One job per kernel, one Neo, and it tests the set rather than this PR.
For the RC, fold this into #264 or land it directly after, with no release between them, and after #278 and #276.
Mirror of aurora-silicon/linux#572 (merged as aeae3d9), cherry-picked with
-xonto #264's branch (neo/g17-render-stamp-fallback-20261028), so this PR is stacked on #264. It applied without conflicts. KUnitasahi_g17*passes 130/130 on this tree, including both new cases. The hardware results below were measured on the aurora build.This also answers the visibility question on #264: the 50 ms fallback fired thousands of times per synchronization CTS pass because it retired members of ioctls whose work-state node the firmware was still running. With this change it no longer does.
One CTS
synchronization2pass retires about 9.8k G17 compute commands on the 50 ms stampless fallback ("lifetime total N compute commands retired on their completion record after 50 ms without a completion stamp"). That happens on stock and with aurora-silicon/linux#570/AsahiLinux#571. Asynchronizationpass retires about 9.1k more. These stamps aren't missing. The firmware stores them later than the fallback assumed.Cause
The firmware stores the completion stamps of an ioctl's commands only after every kick counted on the ioctl's work-state node has completed. A command whose record (compute) or pass record (render) is already present waits for its stamp for as long as the rest of its ioctl runs.
The 50 ms bound was measured from the record, so it retired those members on their records while the firmware was still running their node. It released their fences, VM pins and render leases before the post-maintenance stamp that the retirement path otherwise requires. This is the visibility window chriskearney asked about on #264. In these cases it wasn't a stalled stamp: the node was still running.
Evidence from a diagnostic build (this PR's base plus report tracepoints). It covered one full 6-worker
synchronization2pass (36516 pass, 0 non-pass), with the work-state node read at every fallback:signal_order's single command buffer of twelve read operations, with the compute copies between them (12 renders + 13 computes = 37 kicks), where each render waits on the other queue.The fallback added no latency here. The ioctl's out-fences wait for all its members, so they signalled at node completion either way. A
synchronization2pass takes 3 min 47 s with or without this change.Fix
Count the bound from the drain of the command's node instead of from its record:
WorkStateLease::drained(): node completion word ≥ host count).The node is read only when the bound has already passed, so the common path is unchanged.
What a stampless retirement releases, and when that can still happen
Retirement (
Deferred::Retired→Completion::complete()/release_after_retirement()) does the following:Output::publish());vm_job) and, for a render, its render-slot lease;With this change none of that happens on the fallback while the command's node still runs. A stampless retirement now needs one of two things:
expire_stamp_wait(), which sets the latch to 0). That one is unchanged and doesn't consult the node.The aurora-silicon/linux#555 stall is still covered
The original render stall (aurora-silicon/linux#555) had the stamps stop one kick short while the group and later groups had executed, and no later event moved them:
No hardware repro of a drained-node stall exists today. In this PR's qualification boot, nothing retired stampless: no render or compute fallback line at all, including the first heavy CTS load after boot, where aurora-silicon/linux#555's stall used to show up once per boot.
Qualification
KUnit:
asahi_g17*155/155 under QEMU arm64, 16K pages (aurora-asahi-1028.kunitconfig). Two new cases:asahi_g17_compute_stamp_wait::a_stamp_waits_for_the_rest_of_its_ioctl;asahi_g17_render_retirement::stamps_wait_for_the_rest_of_the_ioctl, which also checks that the watchdog's expiry doesn't wait for the node.On the omacom tree (mirror, stacked on drm/asahi: retire a G17 render group on its pass record when its stamps stall (stacked on #244) #264) it is 130/130.
Neo, stock kit
clpcd2with this change as its asahi module (drm/asahi: g17: keep a work-state node for the other engine aurora-silicon/linux#570 + drm/asahi: g17: admit a context's ioctls to work states in submission order aurora-silicon/linux#571 base). One fresh one-shot boot:PERFORMANCE-REGRESSION-PASS.synchronization2: 36516 Pass, 0 non-pass, 3 min 47 s.synchronization: 28647 Pass, 0 non-pass, 3 min 14 s.synchronization2and ~9.1k forsynchronization.Diagnostic A/B with the node gate as a knob (same boot,
synchronization2): 9792 fallbacks with the knob off, 17 with it on. Those 17 retired on the record the moment their node drained and their stamp landed 0.0–0.3 ms later. The production change restarts the bound at that point instead, so they retire on their stamps.Performance
Same-boot A/B on the Neo. A diagnostic twin of this change has a knob that restores the bound from the record (the old behaviour). Arms were alternated with rotated order, 6 rounds, quietdesk on. Medians, with the change vs the old bound (positive = faster with the change):
All of this is noise. The two arms run the same instructions on these rows: the node is read only once a stamp wait has passed 50 ms, and no stampless line appeared during the A/B (the harness stops at the first one). Every median with the change lies within the old arm's own spread over its rounds (render.light d32: old 26.76–27.85 µs, with the change 27.44; compute.light d8: old 15.11–15.53 µs, with the change 15.37). The cross-module regression gate above passed as well.