Skip to content

fix(agc): order direct reads after copied writes - #1359

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
TibixDev:pr/copied-write-ordering
Oct 13, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
TibixDev:pr/copied-write-ordering

Conversation

@TibixDev

@TibixDev TibixDev commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

What

Complete overlapping copied-buffer writebacks and CPU completion labels before fresh or reused shader resources read guest memory directly. Apply the same ordering to sampled/storage texture contents and compression metadata, before accepting current refresh/revalidation proofs. Disjoint work and GPU-only writes stay asynchronous; completion callbacks cannot recurse into these waits.

Rebased only this correctness change on d146ec1b6. The old #1324/#1357 import stack is removed. Current import retirement, span/window behavior and storage refresh proofs are retained. The old TechnicalDebt addition belonged to the removed import stack and is absent from this diff.

Depends on: none. Related #1157 is a separate DMA-wait optimization.

Tested

Fedora 44, GCC 16 Release, RX 6600/RADV and lavapipe. The standalone agc_copied_direct_ordering test passes on both with Vulkan core and synchronization validation and no validation diagnostics or skipped cases. It covers fresh/reused direct buffers, dispatch/draw copied-writer registries, overlapping completion labels, cached images, compression metadata, disjoint consumers and GPU-only no-wait behavior. Fixtures use the current typed-image binding ABI.

With the same regression fixture and all production files restored to main d146ec1b6, lavapipe fails with fresh direct consumer ran before copied write-back. Restoring this fix passes.

Seven focused CTests pass: copied/direct ordering, graphics, guest memory, import unmap, storage upload lifetime/failure, and descriptor pool recycling (3.47 seconds total). Conventions and whitespace checks pass. No title attribution or Windows result is claimed for this change; full Linux/Windows CI and maintainer re-review remain.

Checklist

  • Main d146ec1b6; related work checked and unrelated prerequisites removed.
  • One topic.
  • No new code comments.
  • Unsupported paths still throw; UI stubs are documented.
  • No notes, logs, or images in the repository.
  • No new third-party code.
  • General behavior.
  • Depends on: none.
  • AI-assisted: yes.

@TibixDev

TibixDev commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased on eaa6ccea. Updated #1357 with #1051’s Linux writable-range check and owner-aware failure caching. This fixes the merge-build compile failure while retaining the byte budget and existing ordering/remap behavior.

Fedora 44 Release: agc_driver_graphics_tests, guest_memory_tests, agc_driver_recorder_tests, agc_recorder_kept_bytes_tests build; the targeted tests and contribution checks pass. Windows CI is pending.
RADV and lavapipe: --copied-direct-only pass with core and synchronization validation; no validation errors.

@xXPBSXx

xXPBSXx commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Windows data point for this PR, with a title. Slay the Princess (PPSA24423) intermittently aborts with AGC graphics: vertex buffer aliases the render target (#4369). The cause I found there: an indexed draw (explicit DRAW_INDEX, 6 32-bit indices, read in place from guest memory) occasionally reads garbage indices (0xff000000 x 6) from its index buffer, so the vertex range becomes about 32 GiB; in other runs the same draws read valid indices (0 1 2 2 3 0), and no command in the stream writes that memory. That looks like an ordering problem between a write and a direct read, which is what this PR is about.

Tree: main ffa1984 + this PR (clean merge, head from refs/pull/1359), relinked, Windows 10, MinGW GCC Release, RTX 4080 SUPER, 150 s per run, 3 runs: no abort in any of the 3 runs (no what(), no FATAL; the logs end in normal [shadow] seed / shader-cache lines). Earlier today on trees without this PR the same title aborted in 3 of the 4 runs I made (79 to 122 s in). I did not rerun the exact same main as a control, so this is a suggestion, not a measurement of the effect: 0 of 3 against about 3 of 4 earlier. No claim about the image, and I did not run this PR's own tests on Windows.

AI-assisted (Claude Code).

@xXPBSXx

xXPBSXx commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Correction to my comment above: I ran the control. Plain main ffa1984 (the same tree without this PR), relinked, same title, 3 runs of 150 s: no abort in any of them either (no what(), no FATAL). So the 0-of-3 result with this PR does not show an effect of the PR; the aborts I saw earlier today (3 of 4) came from trees on an older main (e7c70a2, with #4408, #3840 and my diagnostic prints), and I do not know what removed them (a later change on main, the diagnostic prints changing the timing, or chance). Please disregard my suggestion that this PR fixes the intermittent index data; I have no evidence for it.

AI-assisted (Claude Code).

@Scramblrrrr

Copy link
Copy Markdown
Contributor

Source-only older-stack Windows cross-test port prepared at https://github.com/Scramblrrrr/AnyPS5/tree/codex/first-light-copied-direct-ordering head6175e8551 (author retained from d799e59). Resolved overlaps by preserving existing import build-floor bookkeeping, runtime table helper and depth retirement, with completion waits before those decisions. Conventions pass. Not built/staged and no native or 007 result for this port yet; current game still runs the preceding artifact. Will run the added ordering fixture after the quiet import-window comparison finishes. AI-assisted: yes (Codex).

@Scramblrrrr

Copy link
Copy Markdown
Contributor

Native Windows cross-test of the isolated port6175e85512b0d8e76e930948f3635a0bab0825a9: copied/direct ordering, storage CPU snapshot, upload lifetime and upload failure all passed1.73s, no skips, RTX5070Ti/GCC15.2. Negative control restored only the three production source files to parentc4981fb10 while retaining your new fixture/shader: agc_copied_direct_ordering fails in0.35s with "fresh direct consumer ran before copied write-back" (CTest8). Production fix restored; rebuilding for the final restored check. Nothing staged into the game yet. SleglJan has reported a separate terrain readback/completion-label ordering case on share/007-hull-v1; coordinating comparison before combining changes. AI-assisted: yes (Codex).

@Scramblrrrr

Copy link
Copy Markdown
Contributor

Combined Windows cross-test with #4017 now built: cfeae7d, https://github.com/Scramblrrrr/AnyPS5/tree/codex/first-light-retirement-ordering-cross-test . Your credited6175 port (canonical d799e59) applies cleanly on the locked-retirement5dd integration. Native copied_direct_ordering, host_import_retirement, driver_host_import_at_map, storage_cpu_snapshot, storage_upload_lifetime/failure all pass2.05s; graphics, bda_device, tessellation pass5.32s. All nine execute without CTest skips. Separate production-only negative control for your ordering behavior is recorded above; no new causality claim from these combined checks.

Guarded warm007 run started with graphics SHA2560D503946D34725F16ECDAA41B581313ABE0021B5CC9CB2E0D063DFF192AE80AA. Same cache/config; no profile/capture or compute optimization disable. Goal is to distinguish copied-write ordering from the unsupported image-table operations that remain in the previous run. No gameplay/FPS success claim yet. AI-assisted: Codex.

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased onto current main (conflicts were adjacency only: the test list in libSceAgcDriver/CMakeLists.txt and GuestBufferMemory::UploadFinish, where I kept main's StorageTexture::FlushPending loop together with the new completion waits). Checked on RADV: agc_copied_direct_ordering, agc_storage_written_buffer_order, agc_host_import_*, agc_storage_*, agc_descriptor_pool_*, guest_memory and agc_driver_graphics pass. The regression fixture failing without the fix is covered by the author's lavapipe run and @Scramblrrrr's Windows run (RTX 5070 Ti); merging once the full Build is green.

@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
#4889 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp L5755
#4978 core/libs/prx/libSceAgcDriver/CMakeLists.txt

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 13, 2026
TibixDev and others added 2 commits October 13, 2026 12:34
Wait for overlapping CPU writebacks and completion labels before importing buffers or accepting cached image resources. Cover texture data and compression metadata, retain asynchronous disjoint and GPU-only dependencies, and avoid recursive completion waits.

Reproduced fresh-buffer and cached-image failures with CPU Vulkan before fixing them. Regression covers fresh/reused bindings, dispatch/draw copied writers, texel/metadata updates, labels and no-wait cases; passes CPU Vulkan and RX 6600 synchronization validation. Graphics and guest-memory suites pass. The Skyrim loading GPU fault remains unattributed.

AI-assisted implementation and independent code review.
…n Windows

Co-Authored-By: Claude <noreply@anthropic.com>
@adriadam10

Copy link
Copy Markdown
Collaborator

The full Build failed on Windows only in agc_copied_direct_ordering (0xc0000135, DLL not found): the test was registered without the PATH prepend of libc that the other recorder tests get on WIN32, and without SKIP_RETURN_CODE 77 for hosts with no Vulkan device. I rebased onto current main (clean) and added one commit with both. With the production changes reverted agc_copied_direct_ordering fails, with them it passes (RADV). Merging once the full Build is green.

@adriadam10
adriadam10 force-pushed the pr/copied-write-ordering branch from 62f9702 to 3949fb9 Compare October 13, 2026 10:47
@adriadam10
adriadam10 merged commit a164b1d into boykopovar:main Oct 13, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants