Repository navigation
fix(agc): don't pass a label wait on memory its own queue already reset - #13883
Conversation
A WAIT_REG_MEM passes as soon as guest memory satisfies its compare. A queue that resets a label right after its wait on it passed, and waits on it again before that reset reached memory (the store is recorded on the GPU behind other work, or still queued), found the old value in memory and passed the second wait without a new store from the other queue. Skyrim (PPSA03747) does this on queue 0x28: two passes on 0x112a800020 per signal of queue 0 let the two queues drift a step apart, and later signals overlap and get lost until both wait forever. Recorder::UnlandedOwnLabel reports the value a queue's own newest stores of a label leave once they land, when every awaited dword's newest tracked store is that queue's, still queued or in a batch not reaped yet, and no later recorded GPU write covers it. A wait whose memory satisfies the compare while such a store rejects it keeps waiting (at entry, in the poll loop and on the pipelined queue-0 path), and reaps while it sleeps so the store's entry leaves once it landed. Landing waits are unchanged.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
UnlandedOwnLabel can report an older recorded store as the newest own-queue store when a newer queued entry for the same dword belongs to another queue.
What I ran
Reviewed head 14bcc1a2064b6e6c61960c821c507929989509c3 against base 8f55414024c614b32548254e59e80bd3f0085231; the merge preview was clean. Environment: Debian Linux x86_64, Python 3.11.2. No local build or test was run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13883
gh pr checks 13883 --repo boykopovar/AnyPS5
git show 14bcc1a2064b6e6c61960c821c507929989509c3:core/libs/prx/libSceAgcDriver/Graphics/src/Recorder.cpp
git show 14bcc1a2064b6e6c61960c821c507929989509c3:core/libs/prx/libSceAgcDriver/Execution/src/Driver/Synchronization/WaitMemory.cppResults
| Check | Result |
|---|---|
| Diff and conventions | Passed. |
| Dependency/current-base preview | Passed. |
| Source review | Found a stale-entry selection case in the new queued/recorded label lookup. |
| Hosted CI | Still queued when checked. |
| Local build/tests | Not run under the no-build initial-review rule. |
Relevant unedited output
All conventions checks passed.
The precheck also reported that its ABI-audit and test-quality helper scripts were absent from the isolated PR checkout; those helper-launch errors are not source findings.
Findings
Blocking
unlandedOwnLabel first selects queuedLabels[dword] only when that entry's queue equals the caller. If a queued entry exists for another queue, it falls through to labels[dword]. With an older recorded entry belonging to the waiting queue, the function returns that old own-queue value even though a queued store from another queue is also tracked. The reverse ordering can also be misclassified: an older queued own-queue entry wins over a newer recorded entry from another queue. The default configuration keeps queued labels separate from recorded labels, so both maps can contain the same dword.
OwnStoreRejects then uses this stale own value to hold a memory-satisfied wait. Compare the recorded and queued entries by their stamps for each dword, then report an own store only if the newest tracked entry belongs to the waiting queue and is not overwritten. Add tests for both cross-map orderings. The current tests exercise an own queued entry and an other-queue recorded entry separately, but do not put entries from both maps at the same dword.
Non-blocking
None.
Sources
- Entry selection at Recorder.cpp, reviewed head.
- Separate queued-label behavior at Recorder.cpp, reviewed head and queued-label recording.
- Existing tests at Recorder.cpp, reviewed head.
- The wait guard consuming this result at WaitMemory.cpp, reviewed head and memory-satisfied path.
Not verified
- The hosted checks were queued; I did not wait for them to finish.
- I did not build or run the AGC tests, execute the reported Skyrim traces, or independently reproduce the reported GPU results.
- No negative-control test was run.
To get this to a good state
Select the newest tracked label entry across both maps and add regression cases for a newer queued other-queue entry and a newer recorded other-queue entry. I will re-check the same source paths and tests on the updated head.
|
@hugodearaujo Reviewed at An extracted-method CPU probe reproduces both cases; adding an own-queue high-dword entry makes the guard reject as expected. This is a remaining instance of the original race, rather than a new regression. Please account for relevant partial own stores when deciding whether memory may satisfy a wait, while preserving mask/compare semantics and allowing a genuinely later producer store to release it. Add regression cases for low-only and high-only resets of a wide wait, mixed ownership, and masks that ignore the reset bits. The existing mixed-ownership lookup assertion only checks A second point: one case can turn into a hang.
|
|
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.
|
✅ NVIDIA GPU test: passedPR head
Intermittent validation messages (1, seen once in the parallel run, not reproduced when the test was re-run alone 2 times; not counted against this PR)Unstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Validation IDs already present on main (3 IDs in 384 tests)Skipped (41)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
UnlandedOwnLabel took a queued entry only when it was the caller's own and fell back to the recorded one otherwise, so an older recorded store of the waiting queue counted as its newest even when another queue had a newer queued store of the dword, and an older own queued store won over a newer recorded store of another queue. It also answered nothing unless every awaited dword was the queue's own, so an 8-byte wait passed on memory that the queue's pending reset of one dword was about to change. The lookup now takes the newest tracked store of each dword, queued or recorded, by stamp, and reports the value and the dwords whose newest store is the queue's own and not overwritten. OwnStoreRejects composes what the wait would read once those stores land (own dwords from the table, the rest from memory) and keeps the wait only when that value fails the compare, so the wait's mask and function decide as before and another queue's later store of a dword still releases it.
|
Windows GPU run of head Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, Radeon RX 6700 XT with AMD proprietary driver 26.10.1 (Vulkan 1.4.315), WinLibs GCC 15.2.0 UCRT r7, Ninja, Release,
AI-assisted: yes (Claude Code). |
|
@MarcZX10 Thanks, fixed in @oneandonlydean Thanks for the CPU probe. The partial reset is fixed in New cases (wait-level ones call
On the CPU store in the window: you're right that the PR doesn't handle it. One correction about main, though. There, the wait passes, then the queue's own reset lands over the CPU's value, so memory ends with the reset value where hardware ends with the CPU's. Main loses that store too. With this PR the wait waits for the next store instead of passing once on a value that is then lost. I haven't changed this here, because the check you suggest isn't reliable for these labels yet:
I'd rather make that a follow-up with its own test: check only when the poll sleeps, and only for a dword whose entry has a closed generation. Tell me if you want it in this PR instead. On the lock: unchanged. The lookup runs only when memory already satisfies the compare, and it takes only the label table mutex, as AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The updated lookup compares queued and recorded stores by stamp per dword, resolving the cross-table ordering defect, and the added tests exercise both orderings and partial 64-bit waits.
What I ran
Reviewed head e11df016ff92148801f56aca1de34da6e6d0c9cf against PR base 8f55414024c614b32548254e59e80bd3f0085231 on Linux x86_64 with Python 3.11.2 and GCC 12.2.0. The precheck's merge preview was clean and the head/base stayed unchanged.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13883
git diff --check 8f55414024c614b32548254e59e80bd3f0085231...e11df016ff92148801f56aca1de34da6e6d0c9cf
python3 tools/check_conventions.py --base 8f55414024c614b32548254e59e80bd3f0085231 --head e11df016ff92148801f56aca1de34da6e6d0c9cfThe diff check, conventions, and merge preview passed. The precheck's ABI and test-quality helpers were missing from its isolated checkout; the ABI audit does not apply to this change. Hosted Linux was skipped and Windows is still running.
The author reports that the focused recorder/submit tests passed three times on Windows 11 with an RX 6700 XT and the validation layer loaded, with no VUIDs; the same per-test results were reported on main and the PR head. The author also reports two 480-second Skyrim runs without the prior wait freeze. I did not run these tests or gameplay reproduction myself.
Sources
- Per-dword selection of the newest recorded or queued label:
unlandedOwnLabel. - Cross-table ordering, wide-label and mask regressions: recorder tests and submit tests.
- Author's detailed test report and CPU-write discussion: PR comment.
Not verified
I did not build or run the C++ tests locally, independently repeat the GPU/Skyrim runs, or test on console. Windows CI is pending; Linux and specialized CI jobs were skipped.
The CPU-write race can stay in a separate follow-up: the author reports it also exists on the base branch, and solving it needs separate race-safe detection and tests. I don't see evidence here that a lock-free counter is needed; measure contention before adding that complexity.
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (4, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (3 IDs in 387 tests)Skipped (48)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Checked the unlanded-own-store lookup (newest entry per dword by stamp across the queued and recorded tables, mask/function handling in |
What
A
WAIT_REG_MEMpasses as soon as guest memory satisfies its compare. When a queue resets a label right after its wait on it passed and then waits on the same label again, the reset may not have reached memory yet: it is recorded on the GPU behind other work (or still queued on the worker), and memory still shows the other queue's old value. The second wait then passes on that stale value, without any new store from the other queue.The Elder Scrolls V: Skyrim (PPSA03747) runs into this during gameplay. Queue 0 and compute queue 0x28 hand off through two labels. Queue 0 stores 1 to
0x112a800020twice per round, and 0x28 waits for 1 and stores 0 after each wait. After both of those passes, 0x28 stores 1 to0x112a800b80, which queue 0 waits on and resets before its next round. A label trace (local logging, not part of this PR) shows queue 0x28 passing its wait on0x112a800020on stale memory: once in each of the two traced runs. Excerpt from the first run.storelines arenoteLabelStorecalls (the store's serial,stamp).waitlines arewaitMemoryreturns:exit 0is the poll loop's memory check,exit 1the memory check at entry, andmemoryis the value read on return.The second wait passed on the 1 from stamp 991575, which the first wait had already consumed. Queue 0x28's reset at stamp 991579 was recorded on the GPU 5.5 ms earlier and was still not in memory. Queue 0 made no store in between. After that extra pass, 0x28 counts its two passes per round from a different store of queue 0, so it sets
0x112a800b80after queue 0's first store of a round instead of after the second. Queue 0's wait on0x112a800b80then no longer means that 0x28 consumed and reset queue 0's second store. Queue 0 can start its next round while that store is still 1, and its next store of 1 merges with it: one reset covers both. Then 0x28 waits on0x112a800020for a store queue 0 already made, and queue 0 waits on0x112a800b80for a store 0x28 makes only after that wait. That is the deadlock below.On
mainthe window is mostly hidden:Driver::storeBufferFillcallsVulkanDevice::WaitIdle()for every buffer fill it stores on the CPU, and queue 0x28 makes such fills between its waits, so its reset usually lands in time. With that wait limited to the fill's range (a local experiment, not part of this PR), the title freezes every time: presents stop at 191 s and 322 s, withand queues 0x20 and 0x28 stay in those waits. Queue 0x20's label is a third pair of the same kind with queue 0, stuck because queue 0 is.
0x112a800020is the label from stop 4 of discussion #11508. #12923 fixed another way to pass on it:storedSincereleasing a later wait on a store the queue had already taken and reset. That path rejects the case above too (the reset took the store). Here the wait passes on guest memory, before the label history is consulted.How
Recorder::UnlandedOwnLabel(address, bytes, queue)reports, for each dword of a 4- or 8-byte label, whetherqueueowns its newest tracked store: either queued on the queue's worker and not recorded yet, or recorded in a batch the recorder has not reaped yet. The queued and the recorded table are compared by stamp, so the newest store of either kind counts. A store with a later recorded GPU write over it (overwritten) doesn't count. The result is the value those own stores leave and a mask of their dwords. Another queue's store that is newer than the queue's own takes that dword out of the mask. The lookup takes only the label table mutex, likeLookupLabel. An entry leaves the table when its batch is reaped, after the GPU stored the value. From then on, memory decides again.OwnStoreRejectsbuilds the value the wait would read once those own stores land: own dwords from the table, the other dwords from memory. It checks that value with the wait's own compare. InwaitMemory, memory satisfying the compare counts only if that value satisfies it too. This applies at entry, in the poll loop and on the pipelined queue-0 path inexecute. The wait's mask and function decide as before. A reset of a dword, or of bits, the mask ignores holds nothing, and a later store of another queue still releases the wait. While an own store holds the wait,pollServicereaps when it sleeps, the same as for a label behind a completion, so the entry leaves once the store landed.storedSince, the pending-label table, held at submit) and landing waits (requireMemory) are unchanged.Tested
Linux (Arch, RADV, RX 9070 XT), Release.
Driver cases in
agc_driver(tests/Submit.cpp,testLabelResetByItsQueueBeforeItLanded,testWideLabelPartlyResetByItsQueueBeforeItLanded). Queue 0x20 waits for queue 0's label, records a 4 MiB buffer fill on the GPU, resets the label and waits on it again. This runs three ways: a 4-byte wait; an 8-byte wait where the queue resets only the low dword; an 8-byte wait where it resets only the high dword. The reset goes to the GPU (checked withgpuLabels) and has not landed when the second wait runs, so that wait must stay blocked until queue 0 stores the label again. On the first revision of this PR (14bcc1a206), both 8-byte variants fail:an 8-byte wait passed on a label whose low dword its own queue had already reset, before the reset reached memory, or the wait expired, and the same for the high dword. The 4-byte variant passes there. Earlier, withOwnStoreRejectsreturning false, it failed in 2 of 2 runs. On this head all three pass, in 3 of 3 runs.Recorder::UnlandedOwnLabelcases inagc_driver_recorderandagc_recorder_completion_labels(tests/Recorder.cpp,unlandedOwnLabelTests): a queue's recorded store is its own and nobody else's; another queue's newer store replaces it; 8-byte composition; another queue's newer high dword leaves only the low dword own; an overwritten store doesn't count; a queued own store counts until forgotten; nothing is left after the batch is reaped.Wait-level cases in the same tests (
unlandedOwnStoreWaitTests). They callPm4::WaitSatisfiedUncheckedandOwnStoreRejectswith realWAIT_REG_MEMandWAIT_REG_MEM_64packets on real memory, so they check whether the wait passes:WAIT_REG_MEM_64with a zero high mask) and a 4-byte mask that ignores the changed bits let the wait pass; an own store that clears a compared bit holds it;On
14bcc1a206, 6 of these 16 cases fail: both cross-table orderings, low-only, high-only and both mixed-ownership cases. The others pass on both.^agc_.*(label|wait|queue|submit|pipeline|eop|gated|idle)(113 tests) with Khronos validation 1.4.363 (VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1,VK_LOADER_DEBUG=layershowsInsert instance layer "VK_LAYER_KHRONOS_validation"andInserted device layer "VK_LAYER_KHRONOS_validation"). Results are the same per test onmain828ac0b and on this head (111 passed, 2 failed), with noVUID-lines.agc_idle_publish_waitfails on both withthe idle publish's bytes are wrongon this GPU.agc_small_tiled_shadow_labeland bothagc_driver_copied_writer_label_ordertests skip on both.agc_driver_pipeline_profileis not built on both.Skyrim (PPSA03747), 480 s runs from the same save, Cross every 30 s until 420 s, same machine. fix(shader): decide each resource branch condition once per plan #11506 is included on every build because Skyrim needs it to reach gameplay. The range-limited fill wait above is only a way to make the race show up. It is not part of this PR.
main18f5949 + #11506main18f5949 + #11506 + this PR (first revision)Each frozen run ends with the three timeouts quoted in What. The runs that didn't freeze have no
WAIT_REG_MEMtimeout and are in Helgen gameplay at the end. The rows marked first revision carry an earlier revision (04beb65af0), cherry-picked cleanly onto those 18f5949-based builds. It differs from14bcc1a206only by code comments and a debug switch that was off in the runs. The this-head row is14bcc1a206ande11df016ff, cherry-picked cleanly onto the same repro build. Two of the frozen runs and bothmainruns also hadAPS5_PROFILE_DRAW=1and a trace window. The third frozen run and all the runs with this PR had neither.Checklist
main; no other open PR implements the same functionsNotImplemented_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)