Repository navigation
fix(agc): publish occlusion counts before the ready marker - #5213
Conversation
|
Thanks for tracking this down. Reading the diff against Also, the description says the asynchronous dump lacks a host-visible barrier; if that is the cause, adding the barrier on the GPU path would keep it asynchronous. I only read the code and did not run anything. AI-assisted (Claude Code). |
dd58f49 to
8fef4b4
Compare
|
Good points, thanks @xXPBSXx! Regarding the host-visible barrier: Regarding the default: |
8fef4b4 to
d860294
Compare
Asynchronous GPU sample counter dumps record into command buffer runs that are submitted asynchronously, or queued when label runs per batch are active. Titles whose guest CPU visibility or occlusion query paths directly read sample dump buffers (such as Unreal Engine 4's visibility thread) poll memory without waiting on a GPU fence. Because guest memory uses write watching rather than read trapping, these CPU reads do not hit flush hooks, observing unready memory or zero passed samples and causing false-negative visibility culling and flickering geometry. Allow falling back to synchronous CPU sample counter dumps via APS5_SYNC_SAMPLE_DUMPS or APS5_NO_GPU_SAMPLE_DUMPS, while keeping GPU sample dumps enabled by default to preserve performance on titles that benefit from asynchronous GPU evaluation.
d860294 to
8ce4060
Compare
CnRJay
left a comment
There was a problem hiding this comment.
As it stands the PR is an opt-in switch, so the default behaviour is unchanged. That makes it a reasonable diagnostic, but it isn't a fix users will find: Hello Neighbor 2 still flickers unless someone sets APS5_SYNC_SAMPLE_DUMPS=1.
A CPU seeing "not ready" while the GPU hasn't finished is also what happens on the console. UE4's visibility thread handles a not-ready query by keeping the previous result, so polling before completion shouldn't cause flicker by itself. Flicker suggests the CPU sees the result as ready with wrong counts, which points to the order in which the dump's words become visible:
- If the sample counts and the ready/end marker are written by the same command, or by commands with no dependency between them, nothing orders those writes as the host observes them while the batch is in flight. A barrier to
HOST_READonly makes writes visible once the host waits on a fence. A polling CPU can then see the marker before the counts. - Writing the counts first, then the marker in a separate command after an execution and memory dependency, with the marker store being the last write of the dump, matches how the console's CP writes them and closes that window in practice on all desktop drivers. That would keep dumps asynchronous for every title and only change the order of two stores.
To confirm the cause: a trace of one flickering query slot, logging the dump's words at the moment the guest reads them, would show whether the ready word is set while the counts are still the previous or zero values.
|
High on Life (PPSA15388, UE4) is another title this fixes, together with #5232. Tested at the upstairs window from #5488 and the #5232 report.
A second symptom also went away. With #5232 alone, objects that the first-person weapon passed over came back a few frames late. I had put that down to UE4 reading occlusion results from earlier frames, but with this PR added they no longer disappear behind the weapon at all. The occlusion trace from #5488 agrees: the three large steady queries (0.7–1.6 M samples) read correctly in 55 of 55 frames, with no zero frame. The combination matters. Without #5232, this same CPU-side path only counted finished work. Many queries in a row then shared one stale total, and at low frame rates doors vanished (on RDNA2 at about 1 FPS, see #5488). With #5232's ordering in place, the per-query totals on this path are real, and they move smoothly as the camera sways. Details
Tested and written by Claude (AI assistant) on a contributor's machine. |
A polling CPU can observe bit 63 before the sample counts when one dispatch stores both. Clear that bit and publish each half first, then set it from a later dispatch once those stores are visible to the host. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The default path no longer stores the count and bit 63 in one dispatch. Each dump now clears bit 63 and writes the high half, then writes the low half, then sets bit 63. Each of those dispatches waits until the previous stores are visible to the host, so a poll that sees the ready bit also sees the counts. Pushed in 6b164f2. |
|
NVIDIA run at this head (
|
|
The three phases are in the right order, but at 6b164f2 phase 2 crashes lavapipe, and the occlusion dump tests that would catch it are not in ctest. Phase 2 reads the dump back, and lavapipe faults on that read. With this PR merged into main, If phase 2 stores the whole high word from the same fold, the read goes away and the order stays the same: both count halves first, bit 63 last. The counter then has to move in phase 2 instead of phase 1: SampleDumps.comp void main() {
- if (parameters.phase == 2u) {
- for (uint dump = parameters.firstDump; dump < parameters.firstDump + parameters.dumpCount; ++dump) {
- Words target = Words(uvec2(parameters.dumps.words[dump * 4u], parameters.dumps.words[dump * 4u + 1u]));
- for (uint db = 0u; db < 16u; ++db) target.words[db * 4u + 1u] |= 0x80000000u;
- }
- return;
- }
uint low = parameters.counter.words[0];
@@
for (uint db = 0u; db < 16u; ++db) {
if (parameters.phase == 0u) target.words[db * 4u + 1u] = db == 0u ? (high & 0x7fffffffu) : 0u;
- else target.words[db * 4u] = db == 0u ? low : 0u;
+ else if (parameters.phase == 1u) target.words[db * 4u] = db == 0u ? low : 0u;
+ else target.words[db * 4u + 1u] = (db == 0u ? high : 0u) | 0x80000000u;
}
}
- if (parameters.phase == 0u && parameters.dumpCount != 0u) return;
+ if (parameters.phase != 2u && parameters.dumpCount != 0u) return;
The dump tests aren't in ctest. tests/Recorder.cpp and CMakeLists.txt (against main) pendingKeyStoreTests(device, recorder);
std::cout << "Pending DCC key store tests passed\n";
return 0;
}
+ if (argc == 2 && std::string_view(argv[1]) == "--sample-dumps-only") {
+ if (device.GetContext().hostImportAlignment == 0) {
+ std::cout << "skipped, host imports are unavailable\n";
+ return 77;
+ }
+ if (!device.GetContext().bufferDeviceAddress) {
+ std::cout << "skipped, the device lacks buffer device addresses\n";
+ return 77;
+ }
+ sampleDumpTests(device, recorder);
+ std::cout << "Occlusion counter dump tests passed\n";
+ return 0;
+ } set_tests_properties(agc_shadowed_key_stores PROPERTIES SKIP_RETURN_CODE 77 TIMEOUT 30)
+ add_test(NAME agc_sample_dumps COMMAND agc_driver_recorder_tests --sample-dumps-only)
+ set_tests_properties(agc_sample_dumps PROPERTIES SKIP_RETURN_CODE 77 TIMEOUT 30)
if(WIN32)
- set_tests_properties(agc_pending_key_stores agc_shadowed_key_stores PROPERTIES ENVIRONMENT_MODIFICATION
+ set_tests_properties(agc_pending_key_stores agc_shadowed_key_stores agc_sample_dumps PROPERTIES ENVIRONMENT_MODIFICATION
"PATH=path_list_prepend:$<TARGET_FILE_DIR:libc>")
endif()With both changes on this PR merged into main e34c479, What the default path changes, and #5488. A dump ends with the same words as on main: DB 0 holds the total with bit 63 set, DBs 1 to 15 hold bit 63 alone, and the counter moves once. Only the order in which a polling CPU can see them changes. To check that order, I used a local probe (not for this PR). It records 256 query slots per batch (clear, draw, begin dump, draw, end dump), and the CPU polls DB 0's begin and end words and DB 15's end word while the batch runs:
With this PR the poller does catch dumps in flight. On main, the RTX 5080 never showed it a half-written dump, so this probe can't reproduce the race there. The new order is still the safe one, but this is no evidence that the #5488 dropouts come from this race. A readback taken after the GPU goes idle, like the per-flip trace in #5488, reads the same values at this head as on main. @Tamalero's 55 clean frames (comment) used the synchronous path ( Cost. Each fold now runs three dispatches and two more barriers. Since #5232, a store after a dump starts a new group, so with a clear before each query slot (the pattern in #5232's test) this happens once per slot. On the RTX 5080, one probe batch of 256 query slots, from recording to idle, took a median of 2.37 ms on main, 6.8 ms on this PR and 4.8 ms with the change above (3 runs of 200 batches each). That is about 17 µs per query slot for this PR and about 9.4 µs with the change. What that costs in a title needs a frame-time run, like the one @xXPBSXx asked for. AI-assisted: written with Claude Code; I checked the diff at 6b164f2 against main's dump path, the words each phase writes, the llvmpipe JIT code at the crash, |
adriadam10
left a comment
There was a problem hiding this comment.
The three-phase order (high half without bit 63, low half, bit 63 last) is right, but two things block merging:
core/libs/prx/libSceAgcDriver/Graphics/shaders/SampleDumps.comp:18-24: phase 2 doestarget.words[...] |= 0x80000000u, which reads throughtarget. As @nikowolf123 reported, lavapipe runs one extra masked iteration, loads the next (unwritten, 0) list entry and faults at 0x4 insampleDumpTests. Phases 0 and 1 only store, so they are fine. Please apply the fix from their comment: phase 2 stores the whole high word from the same fold ((db == 0u ? high : 0u) | 0x80000000u) and phase 1 no longer folds the counter, so the read goes away and bit 63 is still the last write.- The occlusion dump tests (
sampleDumpTests,tests/Recorder.cpp:5278) only run whenagc_driver_recorder_testsis started by hand and are not in ctest, so CI cannot catch a regression like the one above. Please register a ctest entry for them (CI runs these GPU tests on lavapipe) and check it passes there.
Note on testing: we could not run sampleDumpTests to the end on our lavapipe setup, so the lavapipe crash is taken from the report above; the ordering fix needs a run on lavapipe after the change.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Phase 2 now stores the whole high word, |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the re-push at b526a11:
- Phase 2 of
SampleDumps.compnow stores the whole high word and never reads the dump;--sample-dumps-onlyon lavapipe (Mesa 25.2.8, LLVM 20) passes, without the fault reported at 6b164f2. agc_sample_dumpsis registered in ctest and passes on RADV; the rest of the libSceAgcDriver test scope is green.
Merging once the full Build is green.
What
Asynchronous GPU sample counter dumps used to store the sample count and ready bit 63 from one compute dispatch. A guest CPU that polls the dump can observe that bit set while the count words are still the previous or zero values.
The dump now publishes in three dispatches. Each one waits until the previous stores are visible to the host:
APS5_SYNC_SAMPLE_DUMPSorAPS5_NO_GPU_SAMPLE_DUMPSstill forces the synchronous CPU dump. GPU dumps stay the default.agc_sample_dumpsrunssampleDumpTeststhroughagc_driver_recorder_tests --sample-dumps-only.Tested
SampleDumps.compcompiles with glslangvulkan1.1.agc_sample_dumpswas not run on lavapipe or another GPU in this pass.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)