Repository navigation
fix(agc): order deferred import settles with main-queue accesses - #8527
adriadam10 merged 3 commits into
Conversation
|
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.
|
|
@eliasbruno124-dev thanks, the barriers and the alias removal look right. Two questions before merge:
|
|
@eliasbruno124-dev #8527 conflicts with In case it saves you a step, there's a merge of
To take it: For #9724's option 1, we built the synchronous settle on top of #8527:
|
…tisampled DCC import Rush 2 Recompiled (PPSA69423) and Wave Race 64 Recompiled (PPSA69424) stop on Windows with "initial multisampled DCC contents require watched guest memory" (boykopovar#9724). Their multisampled color target lives in a 784 KiB host import made at map time. An import under 1 MiB stays compared until a later lookup reaps its deferred settle read, and the titles draw to the target first. When an initial multisampled DCC import needs a range whose import still waits for its settle read, submit that read now if it is still wanted, wait for its fence and return the import to the write watch before the check. This builds on boykopovar#8527's ordering: the read goes through its main-queue batch with barriers around it, so it runs after every earlier main-queue access, and the wait happens under the GPU mutex that submission holds. An import that is not a whole number of 64 KiB blocks, such as these titles' 0xc4000 bytes, never got a deferred read, and Rewatch refuses a range that does not start and end on a block. Such an import now waits for this path alone. It returns to the write watch with its partial blocks whole, each stamped as a CPU store, unless another compared import or a live storage image shares them. Imports that stay compared for any other reason still do, an explicitly unwatched range is still refused, and APS5_NO_DEFERRED_SETTLES still keeps these imports compared.
…tisampled DCC import Rush 2 Recompiled (PPSA69423) and Wave Race 64 Recompiled (PPSA69424) stop on Windows with "initial multisampled DCC contents require watched guest memory" (boykopovar#9724). Their multisampled color target lives in a 784 KiB host import made at map time. An import under 1 MiB stays compared until a later lookup reaps its deferred settle read, and the titles draw to the target first. When an initial multisampled DCC import needs a range whose import still waits for its settle read, submit that read now if it is still wanted, wait for its fence and return the import to the write watch before the check. This builds on boykopovar#8527's ordering: the read goes through its main-queue batch with barriers around it, so it runs after every earlier main-queue access, and the wait happens under the GPU mutex that submission holds. An import that is not a whole number of 64 KiB blocks, such as these titles' 0xc4000 bytes, never got a deferred read, and Rewatch refuses a range that does not start and end on a block. Such an import now waits for this path alone. It returns to the write watch with its partial blocks whole, each stamped as a CPU store, unless another compared import or a live storage image shares them. Imports that stay compared for any other reason still do, an explicitly unwatched range is still refused, and APS5_NO_DEFERRED_SETTLES still keeps these imports compared.
|
@eliasbruno124-dev the synchronous settle for #9724 is up as a draft, #9941. It's stacked on this PR:
If you'd rather carry the settle in #8527 yourself, say so, and we'll close #9941 and move the fix over to you. |
|
The race reproduces in an existing test on current Windows 11 Pro 10.0.26300, WinLibs GCC 15.2.0 r7, Release. NVIDIA GeForce RTX 3060 Ti (driver 617.42) and Intel Arc Graphics (Meteor Lake, On The branch conflicts with
No AI-assisted: yes (Claude Code) |
|
@BKlaasWerkman thank you, that's a useful independent check: the hazard on For the PR's own tests: |
|
@EagleBearerYS ran them. Setup:
In B, on both GPUs, the hazards are the write-order pairs above and one AI-assisted: yes (Claude Code) |
|
@BKlaasWerkman thank you for running both tests, with and without the fix, on both GPUs, and the wider settle and import set too. It's useful for #9941 as well, which carries this PR's commits. |
|
The full suite under synchronization validation,
This PR ( No other hazard signature appears anywhere in the full runs on either GPU, so on these two devices this PR covers every synchronization error the suite produces. AI-assisted: yes (Claude Code) |
60ab136 to
a0ecec3
Compare
|
@oneandonlydean addressed in
Local Windows/RTX 3060 Laptop 617.42 comparison, real Khronos synchronization validation and caches disabled: the same 28 tests on main's production file and this head. Main has four failed cases and five synchronization reports; this head passes 28/28 with no skips or validation errors. The old unbounded wait also fails the new bounded-wait assertion. Conventions and whitespace checks pass. @BKlaasWerkman your report caught a real limitation in the test result: byte assertions can pass despite the race. The new ordering tests now fail CTest on synchronization/validation error output when the layer is enabled. They do not promise to detect every race without validation. Thanks for the independent NVIDIA/Intel comparisons. @EagleBearerYS please keep the initial-DCC correction in #9941; it is a separate change. This branch contains only deferred ordering and its review correction, so #9941 can follow this updated head. The earlier GTA III evidence remains tied to its original commits. These review tests establish correctness and failure handling, not an FPS improvement. Prepared with AI assistance. |
…tisampled DCC import Rush 2 Recompiled (PPSA69423) and Wave Race 64 Recompiled (PPSA69424) stop on Windows with "initial multisampled DCC contents require watched guest memory" (boykopovar#9724). Their multisampled color target lives in a 784 KiB host import made at map time. An import under 1 MiB stays compared until a later lookup reaps its deferred settle read, and the titles draw to the target first. When an initial multisampled DCC import needs a range whose import still waits for its settle read, submit that read now if it is still wanted, wait for its fence and return the import to the write watch before the check. This builds on boykopovar#8527's ordering: the read goes through its main-queue batch with barriers around it, so it runs after every earlier main-queue access, and the wait happens under the GPU mutex that submission holds. An import that is not a whole number of 64 KiB blocks, such as these titles' 0xc4000 bytes, never got a deferred read, and Rewatch refuses a range that does not start and end on a block. Such an import now waits for this path alone. It returns to the write watch with its partial blocks whole, each stamped as a CPU store, unless another compared import or a live storage image shares them. Imports that stay compared for any other reason still do, an explicitly unwatched range is still refused, and APS5_NO_DEFERRED_SETTLES still keeps these imports compared.
|
Checked the diff against the two points in the earlier review: the settle batch's queue/pool uses ( |
What
Order Windows deferred host-import settle reads with earlier and later main-queue accesses to the same memory. GTA III reports
SYNC-HAZARD-WRITE-RACING-READwhen a main-queue fill races the deferred four-byte copy on the separate settle queue.Allocate batch commands from the main pool and submit on the main queue, with barriers around the copies. Submission remains asynchronous on the CPU. Reuse the existing import buffer, removing the alternate-family alias from this path. Retirement checks a bounded 60-second fence wait and propagates failure before releasing the batch's resources.
The ordering tests cover both settle-family configurations and reject reported
SYNC-HAZARD-orVUID-messages when validation is enabled. Their byte assertions alone can pass on unfixed drivers. A separate regression injects timeout and device-loss results, checks error propagation and retained imports, then verifies exactly one release after a successful wait. The first-use check requires earlier main-queue reads to finish before rewatching. Both comparison arms include the scratch-copy barriers needed by the existing canaries.Rebased linearly onto
f40cdebce2, keeping both recorder selection flags from the conflict. Main queue/pool access is protected byGuestMemory::GpuMutex, including queue-0 commits, presentation and command allocation/free. Overlap refreshed on 2026-10-23: #9941 owns the separate synchronous initial-DCC settle change; #9670 adds a separate presentation queue. Neither is a dependency or a replacement for this correction.Tested
ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1, sequentialctest -V.GuestBufferMemory.cppfrom mainf40cdebce2versus candidatea0ecec3d3. Baseline: four failing cases, twoREAD-RACING-WRITEand threeWRITE-RACING-READreports. Candidate: 28/28 pass, no skips or validation errors; longest case 2.94 seconds. The set covers deferred settles, both ordering configurations, retirement errors, import lifetime, flip stores and VideoOut presentation.git diff --checkpass.cabd80f47andc64cfdee2: identical resources/settings, default deferred settles, empty private caches, no inputs or other unmerged PRs, all 61 observed runtime modules verified. Each observation was stopped after 300 seconds rather than reaching a normal guest exit.SYNC-HAZARD-WRITE-RACING-READVUID-VkImageViewCreateInfo-usage-02275Those races concern a main-queue
vkCmdFillBufferand the settlevkCmdCopyBuffer, over 4096-, 65536- and 12288-byte buffers. The separate image-view errors remain. Renderer exports show the opening logo, not gameplay. This title evidence belongs to the named pre-rebase commits; the new review correction has native comparison coverage, not a fresh title run.No FPS or loading-time gain is claimed. Main-queue batching latency, a real device-loss event, other GPU vendors, Linux and macOS were not measured locally. The earlier title observations had validation/profiling/readback enabled, and Git metadata work overlapped part of the candidate observation.
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)