Repository navigation
fix(agc): order deferred write-backs after queued draws' index reads - #6914
Conversation
408f564 to
ec5175a
Compare
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
|
@moong15 thanks for this. The new test Cause, from my runs: after I did not check whether Test-side fix for const auto landBy = std::chrono::steady_clock::now() + std::chrono::seconds(5);
do {
AgcDriver::GuestMemory::FlushGpuWrites(reinterpret_cast<std::uint64_t>(reinterpret_cast<std::uintptr_t>(Pool)), PoolBytes);
AgcDriverWaitIdle_nid_postfix();
} while (IndicesIntact() && std::chrono::steady_clock::now() < landBy);
Require(!IndicesIntact(), "queued index read: the image's results never reached the memory they cover, so the run above proves nothing");
The negative control works. Setup: Linux, lavapipe Mesa 26.2.4 (LLVM 23.1.1), default CMake build ( AI-assisted: yes |
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
The cause is well shown (a queued draw reads its indices at commit, a later write-back lands in the same bytes first, Stellar Blade reproduces it on every menu load, 5 of 5 runs fail without the fix and run 8 with the switch off fails again). The test fails with the switch set. Two questions and one measurement.
HeldReadsrecords only the index range of a queued draw. What else does a queued draw read from guest memory at commit rather than at prepare? Indirect argument buffers, vertex data fetched through a BDA range, and descriptor words that are re-read incommitDrawwould be held to the same bug. Please list whatcommitDrawreads from guest memory and show that the others are snapshotted at prepare, so this is the whole class and not one instance.- A write-back that stays pending until the draws holding the range have committed moves a store later. What happens when the draws never commit because the pipeline is blocked behind a label that waits on this write-back (a store over the held range that a
WAIT_REG_MEMdepends on)? The hold must not be able to wait on itself; a test with a label wait over a held range would show it. - "No measurable cost on the
[draw] pipelinedline" is one line from one run. Post the summary lines of main (switch off) and this PR at the menu, same scene, noAPS5_PROFILE_*, since the write-backs it delays are on the path to the flip.
ec5175a to
a436481
Compare
|
Updated in 7471618. What a queued draw reads at commit.
Registers, user data, push constants, BDA/SRT constants, indirect arguments and descriptor words are read or snapshotted at prepare. Indirect draws never take the pipelined branch. At commit, the index buffer (11-14) was already held. The vertex buffer reads (16) and the pipelined DMA copy source (17) were not; both are held now. Can the hold wait on itself. The draws always commit: the commit thread pops items without waiting on any label or drain, and the GPU mutex is always taken before the pipeline mutex, so there is no cycle. The real gap was in the other direction, as you suspected: a store that the hold refused was never retried. A Tests.
The test's vertex data is reached through a buffer descriptor, so the separate vertex-attribute loop has no case of its own. 160 runs, 8 in parallel, with The menu summary lines, main against this PR, follow in a separate comment. |
|
Correction to my earlier reply: the retry I described there is gone. In game runs it stored a refused write-back after the holding draws committed, which could land after the title had already reused that memory for the next frame's indices, and the draw then read texels as indices. The hold now stays as it was, and a
Same machine and scene, from start to the in-game scene with Enter pressed every 20 s:
|
|
NVIDIA and Windows data point at the current head
The earlier head Not checked: whether the new test fails on AI-assisted: yes (Claude Code). |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
A Linux run on NVIDIA and lavapipe, since the description has Windows on AMD only.
Machine: Ubuntu 26.04, i7-8700, GCC 15.2, Release, BUILD_TESTING=ON. NVIDIA GeForce RTX 3070 (driver 595.99.02) and lavapipe (Mesa 26.0.3, LLVM 21.1.8) on the same host. PR merged onto main 12a71f3f5 (clean). ANYPS5_NO_SHADER_CACHE=1, -j1 --timeout 30.
| Run | RTX 3070 | lavapipe |
|---|---|---|
agc_driver_queued_index_read_order |
pass 3/3 | pass |
same, --repeat until-fail:30, serial |
30/30 | 30/30 |
same, APS5_NO_QUEUED_DRAW_READ_ORDER=1 |
fails: queued index read: the queued draw's commit stored the image's results over its index buffer |
same failure |
^agc (637 tests), PR vs main |
no new failures (635/637; agc_driver_f16_fma_denormal_modes and agc_driver_fma_mix_f16_denormal_modes fail on main on this GPU too) |
637/637 on both |
I ran the repeats serially only, so this does not cover the parallel, cold-cache case @Dextroid17 reported.
Not run: Windows, AMD, Stellar Blade or any title.
GPUs tested: NVIDIA GeForce RTX 3070 (595.99.02) and lavapipe (Mesa 26.0.3), Ubuntu 26.04
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Linux NVIDIA data point: Ubuntu 24.04.4, RTX 4000 SFF Ada Generation, driver 595.99.02, GCC 13.3.0, Release. This PR at |
8d26c40 to
ce35d8f
Compare
|
Rebased onto @CnRJay, about your third point, the menu summary lines. These come from one build (the game branch carrying this fix) with the fix on and with
The two pairs disagree. Pair 1 has the fix about 3 ms slower at p50, and pair 2 shows no difference. Two pairs can't settle a 3 ms gap at the menu. After the menu there is nothing to compare: with the fix off, every run aborts on the index read while loading from the menu (150 s and 165 s), and with it on, the game reaches the in-game scene (323 s and 380 s). If a 3 ms menu cost would matter here, I can run another pair on this rebased head and post it. |
|
Tested on Linux (Arch), #6656, #6914 and #6249 together fail two tests that #6249 adds, and #6656, #6914, #6249 and #6146 together fail one that #6146 adds. Each is a three-way interaction: no pair of the PRs fails (2 of 2 for every row,
( What I found: the copy holds back its own source. #6656 makes Evidence on the three-PR merge: Fix tried (two small hunks on top of #6914): Who should act: #6914's author (the fix belongs next to their held read; the read hold is what is new). #6656 only turned the path on by default. AI-assisted: yes |
|
A second host for @nicolasfalesy's #6249 result, on a different
Not run: #6146 or the four-PR merge, the #6249 + #6656 and #6914 + #6656 pairs, the AI-assisted: yes |
ce35d8f to
488275f
Compare
|
@nicolasfalesy, @Dextroid17, thanks. Confirmed and fixed in 6647ccd, rebased onto The self-hold doesn't need #6656 or #6249: on The fix is the one you described. Test:
I left the large copy out of this test. On Windows 11, AMD Radeon RX 9070 XT, AMD 26.8.1 (LLPC). |
|
@moong15 I re-ran this on
Not run: #6146 or the four-PR merge, AI-assisted: yes |
2ee52e7 to
845d587
Compare
github-actions
Bot
added
the
unblocks title
845d587 to
4ffb7f6
Compare
A pipelined draw read its index buffer from guest memory at commit time, after the packets behind it had run, so a later dispatch's deferred storage-image write-back could land in the bytes first. Stellar Blade recycles its full-screen quad's index buffer for a 66x66 image when the menu loads, and aborted with "vertex fetch exceeds descriptor record count". Deferred write-backs now wait while a queued draw still holds a read of the range. APS5_NO_QUEUED_DRAW_READ_ORDER=1 restores the old order; [draw] reports held read ranges and stores. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ndex read Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e stores they refused Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s checked Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erence Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… hold counters report
4ffb7f6 to
45d012e
Compare
|
Checked the held-read ranges (index, buffer descriptors, vertex attributes, DMA source), the Added two commits on your branch: a rebase onto current |
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
Thanks for the pairs, and for fixing the race in the test.
I agree two pairs can't settle 3 ms, but the PR already has a counter that can. The hold only changes behaviour when a write-back overlaps an index range a queued draw still has to read. The earlier runs held 27 stores in one run and 324 and 65 in another, whole-run totals. At about 250 frames per 10 s window that is at most a store held every few frames, which is too little work to cost 3 ms of Driver.Packet on every frame. If the [draw] pipelined lines for the same windows show the same small numbers of held reads and stores on the fix-on runs, the pair-1 difference is run-to-run spread; if a window shows hundreds of holds, it isn't. Please post the held read ranges and held stores per 10 s window next to the table.
Pair 1's fix-off run also lost its second window to the start of loading at about 25 s (127 presents), so only the 10-20 s window of that pair is comparable. A third pair run in the opposite order (off first) would remove the ordering effect.
8 participants
What
A pipelined draw (the draw pipeline, depth 8) reads its index buffer from guest memory when it is committed, not when its packet is processed. By then the packets queued behind it may already have run, and a later dispatch's deferred storage-image write-back can land in the same bytes first. The draw then reads indices that are really image texels.
Stellar Blade hits this every time the main menu loads. Each frame it sets
INDEX_BASEto the same allocation for a 6-index, 32-bit full-screen quad, then recycles that allocation for a 66x66 storage image (format 75). Once the image's write-back overtakes the queued quad, the quad's indices are garbage and the frame aborts:Change:
DrawPipeline::HeldReadsrecords the guest ranges each queued draw still reads at commit: its index range, and the extent of every guest buffer descriptor and vertex attribute its stages bind. A pipelined DMA copy holds its source range the same way.Texture.cpp). AWAIT_REG_MEMover that range stores it when it polls (WaitMemory.cpp), so the wait is satisfied without waiting for another flush.APS5_NO_QUEUED_DRAW_READ_ORDER=1restores the old order.[draw] pipelinedprofile line reports read ranges and stores held, and the[storage]line counts the stores released after the reads committed.agc_driver_queued_index_read_order, in three phases: a queued draw's index read and its vertex read must still see their original bytes when a later dispatch's write-back covers them, and aWAIT_REG_MEMover a held range, whose value only the deferred write-back produces, must be satisfied.Tested
Windows 11, MinGW build. Stellar Blade (PPSA13197 v01.141), same path to the main menu each run:
APS5_NO_QUEUED_DRAW_READ_ORDER=1)Holding the stores had no measurable cost on the
[draw] pipelinedline.ctest -j 8on this branch: 1348/1352. The four failures also fail on this machine without this change:agc_driver_quad_pixel_maskandagc_driver_layered_volume_vertex_no_gpl(fail on main),select_tests(needs symlink rights) andagc_driver_gpu_map_on_fiber.agc_driver_queued_index_read_orderpasses. It fails withAPS5_NO_QUEUED_DRAW_READ_ORDER=1, when only the index range is held (vertex phase), and without the write-back store from the wait (wait phase).agc_driver_queued_index_read_order, 8 in parallel, withANYPS5_NO_SHADER_CACHE=1: 0 failures.Checklist
main; no other open PR implements the same functions.mdfiles or images added to the repository🤖 Generated with Claude Code