Repository navigation
drm/asahi: g17: admit a context's ioctls to work states in submission order (stacked on #275) - #276
Conversation
… order Each engine admits a context's ioctls in submission order, but the render and compute engines advance independently. Under deep mixed load, render admission takes work-state nodes for ioctls far ahead of an older ioctl that is still waiting for its turn on the compute queue. gpubench-stress queue.depth keeps 128 compute and 64 render submissions in flight on one queue, each render followed by a compute copy. On a Neo (A18 Pro), every completion of the context stops within 0.5-4 s: compute and render-pair stamps stop together, newly published compute never runs, and the client loses the device. Capping the context's live nodes at 32, 48 or 56 does not prevent this. Native macOS on the same machine and firmware runs the same load (one MTLCommandQueue, 128 compute and 64 render command buffers in flight, each render followed by a compute copy) for 60 s without an error. Metal schedules a queue's command buffers in commit order across engines, with at most 36 between scheduling and completion. An ioctl now takes its node only while at most 8 older ioctls of its context have neither taken one nor left without one. Native scheduling happens at commit, on one thread. Here each engine admits from its own scheduler, so a strictly ordered turn costs a scheduler round trip whenever the other engine's ioctl is ready first; that slowed a windowed Vulkan client. Under the queue.depth load, run-ahead limits up to 16 held and 32 did not, so 8 leaves a factor of two. The turn is tracked under a context mutex taken inside the work-storage lock. Ioctls that fail or are cancelled before taking a node are remembered within 1024 ordinals of the turn. One that leaves further ahead makes the context's admission unordered, as before, rather than stall it. A refused publisher waits on its engine's admission domain, and the turn moving past a refusal wakes both domains. Contexts whose ioctls are never refused, such as single-engine ones, take no wake-ups from this. The last-node rule stays: render ioctls still hold nodes while their copies wait in the compute queue. Link: aurora-silicon/gpubench#5 Signed-off-by: DjDeveloperr <hey@djdev.me> (cherry picked from commit c510e9d)
scottjones
left a comment
scottjones
left a comment
There was a problem hiding this comment.
We read this at 1d3726b on #275's branch, and ran it on a Neo with #278 and #275.
The rule does what the body says. We looked for deadlocks, lost wakeups and fairness problems with #275's nodes and found none:
- Ordinals and job pushes both happen under the queue lock (file.rs:1448, g17/queue/mod.rs:462), after the in-syncs are resolved, so an ioctl can't wait on a fence from a later one.
- At most 9 ioctls at or past the turn hold nodes, and there are 63, so order never refuses the turn itself. An exhaustive small model found 0 deadlocks.
- The domain is sampled before acquire (g17/runtime.rs:1859, g17/runtime/render/submit.rs:163), so a refusal can't sleep through a turn move.
- A refused render holds no pair yet (submit.rs:161, then 178), so it can't block another client through a slot.
Three things we'd like looked at, none blocking:
- A compute command refused by the turn looks like it spins. prepare_compute queues a housekeeping pass and waits on
progress(g17/runtime.rs:1863, 1865), each pass notifiesprogress(g17/runtime.rs:2349), and that reruns prepare. Each round takes the device mutex. We sampled worker CPU during queue.depth: about 130-180% of a core across G17 and event workers in the deep phase, none above 61%. That doesn't clear it, because top can't isolate this loop and a wait-based loop wouldn't pin a core. Can you check it with your counters? If it's real, skipping queue_events for a turn refusal should fix it. - The unordered fallback is one-way and silent (g17/context/identity.rs:154), and a client can reach it by failing an ioctl after publish (g17/queue/mod.rs:205). We'd add a dev_warn_once.
- g17/context.rs:452 calls release_state() with
workandorderheld, the same note as on #275. compute.light d64 at -6.6% is worth repeating, and the "Regression gate" line in the body has the table pasted into it.
The mirror is faithful: the patch is byte-identical to c510e9d. Scope is g17 only, so T8140 and T6040, and RUN_AHEAD 8 was only measured on A18 Pro firmware. We'd expect any T6040 cost to be throughput rather than a stall, but we'd like one run before Mesa admits that chip.
We ran 2dd4aab + #278, #275, #276 and the five-PR build with #264 and #277, each through a private Vulkan driver with omacom/mesa#20: queue.depth 5/5 and sparse.churn 3/3 at 60 s, no flagged kernel line. The 7.2.11-10-1 release lost the device in its first queue.depth at 6.4 s, then had no GPU until reboot. That shows the set survives on omacom's tree. Isolating this change still rests on your same-boot unordered control. The limits: one job per kernel, one Neo, our clang 22 build, no T6040, no CTS on the three-PR build. Throughput also fell across the five runs on both builds (mixed 75k to 67k here), possibly thermal; we didn't check.
For the RC, this is more change than we'd normally want this late, but #275 alone is worse. Land it with #275, no release between them, after #278. Its base is #275's branch, so it needs merging there or retargeting.
scottjones
left a comment
scottjones
left a comment
There was a problem hiding this comment.
Approving again at the same head: retargeting from #275's branch to integration/aurora dismissed the earlier approval. Our review above stands.
|
What happened here, for the record. #278 and #275 merged this morning through the queue. This PR was based on #275's branch, so I retargeted it to integration/aurora. That dismissed the approval, so I approved again at the same head (1d3726b). No CI had ever run on this PR, because CI only runs on PRs aimed at integration/aurora, and the retarget didn't start it, so I closed and reopened the PR to trigger it. No code changed. It goes in the queue once #275 is now in integration/aurora without this, and its description says it can turn a client timeout into a GPU stall alone, so this should land before the next linux-aurora pin. |
Mirror of aurora-silicon#571, cherry-picked with
-xonto #275's branch (aurora/g17-work-state-engine-reserve), so this PR is stacked on #275; it applied without conflicts. The touched files (g17/context.rs, g17/context/identity.rs, g17/runtime.rs) are identical on both trees. KUnitasahi_g17*passes 132/132 on this tree. The hardware results below were measured on the aurora build.This fixes the GPU stall that AsahiLinux#570 left open ("Still open"), tracked in aurora-silicon/gpubench#5. Under deep mixed render and compute load on one queue, every completion of a G17 context stops within seconds.
What native does differently
The comparison ran natively on the same Neo (A18 Pro, macOS 26.6.2, same GPU firmware), using a Metal copy of
queue.depth:MTLCommandQueue;Every native run passed:
Per-command-buffer scheduled/completed handler times, kernel and GPU timestamps, and a long head command buffer with 191 behind it show how Metal schedules:
commit()never blocks; the rest wait in the driver.Linux has no such order. Each engine admits a context's ioctls in submission order, but render and compute advance independently. Render admission takes work-state nodes for ioctls far ahead of an older ioctl still waiting on the compute queue.
Fix
An ioctl takes its work-state node only while at most 8 older ioctls of its context have neither taken one nor left without one: ordered admission across engines with a bounded run-ahead.
Strict order (run-ahead 0) is the closest copy of native, and it passes the repro too. Native schedules at commit on one thread, though. Here each engine admits from its own scheduler, so a strictly ordered turn costs a scheduler round trip whenever the other engine's ioctl is ready first. That cost showed up in a windowed Vulkan client (below).
Evidence (Neo, diagnostic knobs,
queue.depthc128+r64 on one queue)The run-ahead is empirical. 16 is the largest value that passed, and 32 the first that failed, so 8 leaves a factor of two.
Qualification
asahi_g17*153/153 under QEMU arm64 with 16K pages. That includes four newasahi_g17_work_statescases: run-ahead admission, ioctls leaving early pass the turn, the unordered fallback, and no wake-ups without a refusal. On the omacom tree (omacom/linux-aurora mirror) it is 132/132.clpcd2) with this change as its asahi module:Strict order (run-ahead 0) left vkmark's shading scene about 9% slower in 4 of 11 rounds. Run-ahead 8 showed that once in 23 rounds, and its dedicated 10-round A/B above was a tie.-REGRESSION-PASS`.
synchronization2group: 36516 Pass, 0 non-pass, the same as stock.queue.depthc128+r64 repro: 4 × 30 s pass, plus a 60 s run that completed about 2M checked submissions with 0 errors (the harness's own 60 s timeout cut its verdict line). No kernel GPU lines during any repro run.Performance
Same-boot A/B on the Neo, with this change vs contexts created with the old unordered admission (a diagnostic build only), arms rotated, quietdesk on. Medians, ordered vs unordered:
Strict order (run-ahead 0) left vkmark's shading scene about 9% slower in 4 of 11 rounds. Run-ahead 8 showed that once in 23 rounds, and its dedicated 10-round A/B above was a tie.
Head-of-line microbench (gpubench-stress diagnostic): queue 0's head ioctl waits on ~5 ms of queue-1 work while 4 independent ioctls of the other engine are submitted behind it. In both directions, the independent ioctls finished after the long work in 0 of 60 loops with ordered and unordered admission alike. Queue order before admission already serializes them on this stack, so ordered admission adds no head-of-line wait.