Repository navigation
fix(agc): wait for address-based writers before reading indirect dispatch arguments - #6786
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the indirect-argument read paths and lease-wait placement. Both CPU read sites hold GuestMemory::GpuMutex before FinishLeaseWork; the helper submits only when the target lease batch is still open and waits only through the newest batch recorded with a lease. Read-ahead remains speculative and the execution-time counts are re-read before a capture is adopted, with mismatches dropped. The new GPU regression covers address-based producers and the CPU fallback, and the PR reports repeated red/green runs plus neighboring tests. The GTA V result is carefully scoped: it passes the bad-count stop but still encounters a later device loss. I found no blocking issue.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@fdfawef324 thanks for the test. One problem on lavapipe (the CI device): case 6, "group counts read on the CPU behind an argument label of an earlier graphics batch", passes without the fix most of the time there, so on lavapipe it does not reliably guard the With all the production changes reverted to The PR body says case 6 fails on To see why, I added a temporary print of
So on lavapipe the case only goes red when the timing happens to keep the label pending. I did not check why the label is usually no longer pending when The rest checks out here. Cases 1-4 go red without the fix, but on lavapipe they hang with "completion label did not land" (ctest hits its timeout) rather than failing with the
Setup: Linux, lavapipe Mesa 26.2.4, default CMake build (CMAKE_BUILD_TYPE empty), AI-assisted: yes |
|
AMD hardware run, since the description lists AMD as untested, with each case on its own to answer the lavapipe question about case 6. Windows 11 (26300), Radeon 860M (RDNA 3.5, driver 32.0.13058.8001), WinLibs GCC 15.2.0, Release.
So on this device case 6 does guard the AI-assisted: yes (Claude Code). |
github-actions
Bot
added
needs rebase
3977f3c to
bc88a5d
Compare
|
@Dextroid17 the sixth case now requires the pending-label CPU fallback ( Published candidate CPU/mock graphics and PM4 pass 2/2. Conventions and a clean merge-tree against GPUs tested: NVIDIA GeForce RTX 3060 12 GB, driver 591.86 (32.0.15.9186), Vulkan 1.4.325, Windows Server 2025 10.0.26100. AI-assisted: yes (Claude Code and Codex) Co-Authored-By: Codex noreply@openai.com |
github-actions
Bot
added
needs rebase
|
While qualifying Sonic I found a separate LDS refusal-order defect, reported in #7794, with the tested two-file proposal on our fork. The current record path can allocate scratch before rejecting a split it already knows is unsupported. The original fixture accepts the earlier allocation/range exception; our stricter test fails 3/3 on each GPU with unchanged production code and passes after moving the existing refusal earlier. Your PR predates this finding and edits the same LDS diagnostic line. I propose keeping #6786 first, then carrying its AI-assisted: yes, Codex. |
…atch arguments
A CPU read of DISPATCH_INDIRECT arguments waited only through the flush
hook, which waits for the ranges recorded work noted as pending writes.
An address-based store (FLAT/GLOBAL, or through a descriptor the
recompiler cannot resolve to a static buffer) is not recorded as a
pending write: its batch leases the registered allocations, and only the
guest pin waiter finished that batch. So the thread-dimension,
misaligned and disabled paths, the fill-kernel path and the device's CPU
fallback read the arguments from before an address-based producer ran.
GTA V (PPSA04264) dies loading Story on such a read: a 64-thread clear
kernel read {160, 288, 0xffffffff} as thread dimensions
(3x288x4294967295 groups), bytes its 1-thread setup kernel had not yet
overwritten with {n, 1, 1}, and threw "workgroup count
3x288x4294967295 exceeds device limits".
Every CPU read of dispatch arguments at execution now first finishes the
recorder up to the newest batch recorded with a lease
(Graphics::FinishLeaseWork). The group read-ahead still reads without
waiting; a dispatch it resolved is adopted only when the execution-time
read gives the same counts, and one that differs is dropped and counted
in the [groupcap] line. The device limit error names the argument
address and cpuReason, and the driver adds the queue, path and
read-ahead outcome. The [indirect] read time includes the lease wait,
and the [recorder] sync-site table records the offset of
FinishLeaseWork's caller. The CPU reads that still miss address-based
writes (indirect draw arguments and count, the copy kernel's count
record, the awaits-title checks) are listed in the technical debt.
IndirectArgumentsPendingProducer covers the execution-time CPU argument
reads on compute queues and the pending-label CPU fallback in the Vulkan
device. Its sixth case records a real argument label behind a host Vulkan
event in an earlier batch and an address-based producer in the later open
batch. The event releases only after that producer batch is submitted;
a bounded watchdog releases for cleanup but fails the case. It requires
a deferred producer lease, an unsubmitted producer at the argument
decision, cpuReason 2 and the fresh arguments and counter result. Small
valid stale counts keep a broken-wait control finite on every driver.
Exact CPU-reason and watchdog diagnostics identify the failing path. This
replaces a spinning workload whose earlier label could retire before
the fallback decision on lavapipe, allowing a false green with cpuReason 0.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Codex <noreply@openai.com>
The direct-device case already drains its own work before returning. Skip the shared singleton wait after its explicit shutdown so the full suite can retain the fallback and counter assertions without throwing ProcessShutdown. Co-Authored-By: Codex <noreply@openai.com>
bc88a5d to
652f86e
Compare
|
Checked the lease wait before both CPU argument reads (driver and |
|
@fdfawef324 @adriadam10 while validating the post-merge main, I confirmed a separate test-gate problem: #7868. The fixture passes three times per RTX 5080 and Radeon Graphics and reaches |
6 participants
What
Execution-time CPU reads of indirect dispatch arguments could see stale counts while an earlier address-based producer still held a registry lease. Those writes need not appear in the pending-write ranges used by the guest-memory flush hook. Finish work through the newest leased batch before each execution-time CPU argument read, including the
VulkanDevicefallback. Re-read counts before adopting speculative read-ahead work, and discard a capture when its counts differ.The wait is conservative: it covers every batch through the newest leased batch without knowing which allocation a producer writes. It can submit and wait for currently recorded work. Other uncovered CPU reads remain documented in technical debt.
The sixth regression case now holds an actual earlier argument-label batch behind a host Vulkan event and leaves its address-based producer in a later open batch. Before the argument decision it requires a deferred producer lease, an unsubmitted producer, and a pending real label. It then requires
cpuReason == 2, fresh arguments and the expected final counter. The event releases after the producer batch is submitted; a five-second watchdog releases it for cleanup but fails the test. Small valid stale counts keep the removal control bounded on drivers that accept the stale dispatch. No production test switch was added. Cases 1–5 retain their workloads and checks; their shared failure text no longer assumes case 6's old stale tuple.The production implementation is unchanged by this coverage update. The final delta from prefinal
01d3bc1echanges only the pending-producer fixture: finite case-6 counts and explicit path/watchdog diagnostics. GTA V PPSA04264 previously passed its stale-count stop in composed runs with the original fix, then encountered a later BVH device loss. Story did not load. This update claims no new title milestone, memory reduction or speed improvement.Root's first managed GPU run at
3133913fpassed the first five cases and reached case 6's reason-2 path, then aborted because the fixture called the shared singleton wait after shutting that singleton down. The repaired candidate skips that common wait only for the direct-device case, which already waits through its own device before setting completion. The fallback, submission, arguments and counter assertions remain. That failed run is preserved and is not reported as a green.Tested
Candidate
bc88a5d2a6e1726724ae49dae562b464e266a882, based on pinned main84c1a4d9a26429cb483ec963195cff8ef25d128b:-O3. The final restored-green build took 26.195 s and the selective removal build 15.561 s, using the corrected nj admission PATH, a 4 GiB whole-job limit, affinity0xF, final-j2and nested-j1.agc_driver_graphicsandagc_driver_pm4pass 2/2 in 10.553 s. Conventions pass against the pinned base in 1.657 s.Graphics::FinishLeaseWork()immediately beforeGuestMemory::ReadinVulkanDevice::decideIndirect. Red and restored green have separate source/runtime pins and natural dirty trace identities. The original 143 READY pins were independently rehashed before the lifecycle edit, including all 22 selective-red pins. Their live inputs were preserved before the new build; the repaired candidate has a separate READY manifest.agc_driver_indirect_arguments_pending_producer,agc_driver_indirect_arguments_written_ahead,agc_driver_gpu_indirect_arguments_written_ahead,agc_driver_gated_dispatch_capture,agc_driver_prepared_shaders,agc_driver_prepared_shaders_indirect. Actual binaries and libraries were hash-checked before and after.5exits 1 in 5.98 s, with the actual RTX 3060/Vulkan-ready markers,cpuReason 2, producer batch 2 still unsubmitted at the watchdog, successfulvkSetEvent 0, and the exact missing-submission assertion. The frozen variant was built at pre-lifecycle head3133913f, natural dirty 1; its failure occurs inside the unchanged gate before the repaired common postlude. This is the intended regression failure, separate from the passing clean candidate. A prior dispatch was refused before GPU execution when its source-head pin changed; it is not counted as a test.676d423968a1a8fcc79c6247020ce22090a8e0d4; the candidate merges cleanly into it. Source review found no material interaction with merged fix(agc): store pending image results after the range's writers finish #6364: that wait applies to pending storage-image flushes, while the buffer-only argument fixture has no such image. These are source composition checks; execution above remains on the exact candidate based on84c1a4d9.Historical and external results remain separate from this candidate:
65d56a297; the detailed removal runs used mainaff725bf6and each selective condition failed 3/3 runs. These are earlier trees and the original fixture, not eight neighboring tests or current-candidate measurements. The old case 6 relied on timing and did not prove fallback selection on every driver.3977f3c2composed with main93e54cedd: case 6 often passed after removal because the fallback was not taken. Their instrumented green runs reported reason 0 in 10/10 runs; their whole original suite passed 3/3. This motivated the explicit pending-label and fallback checks. Their observations are not a new local lavapipe run or evidence of a reproduced production-fix defect.32.0.13058.8001, WinLibs GCC 15.2 Release, main17cfb1f4cplus original3977f3c2: case 6 failed 10/10 without the production changes and passed 10/10 with them. The whole suite failed 20/20 without the changes and passed 20/20 with them. These are their measurements on that older composition.The new fixture has not been tested locally on Linux or AMD. Neighboring #6360 addresses GPU-side argument counts; #5058's held prechecks and #6951's import-run memoization are outside this change. Existing overlap captures are not a fresh guarantee about every open PR or landing order.
Checklist
main; targeted current-main/indirect-argument overlap review completed (tested base and separate composition check qualified above)NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)Generated with Claude Code; coverage recovery and verification assisted by Codex.
Co-Authored-By: Codex noreply@openai.com