Skip to content

fix(agc): retire an unwritten native multisample depth surface on guest access - #6727

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
simonbronk:fix/native-msaa-depth-unwritten-guest-access
Oct 22, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
simonbronk:fix/native-msaa-depth-unwritten-guest-access

Conversation

@simonbronk

Copy link
Copy Markdown
Contributor

What

Part of #6570. A guest read over the depth or HTILE range of a resident native multisample depth surface throws sample-preserving guest publication of native multisample depth is not implemented. That includes a surface the host never wrote after its import. In that case its samples are still the clear value the import took from the guest HTILE, so the guest bytes already describe the surface and nothing needs publishing.

This depends on #6557 (its commit is the first one here). Both edit the same debt entry, and the title needs both.

The title's case, measured with local prints:

  1. Onimusha imports a 128x64, 4-sample D32 target (HTILE without ITERATE_256).
  2. It draws into it once with GREATER_OR_EQUAL depth tests, no depth or stencil writes and no color target.
  3. A compute dispatch then reads the same memory as a structured buffer (buffer_load_dwordx4 with idxen, 96-byte records) and stores into it.

Details are in #6570.

Tested

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: fix(agc): import native multisample depth whose HTILE is not ITERATE_256 #6557
  • Unblocks: PPSA27836 Onimusha: Way of the Sword (PPSA27836) #5769
  • GPUs tested: NVIDIA GeForce RTX 5070, driver 617.42, Windows 11 Pro 26200; AMD Radeon gfx1036 iGPU, Windows 11 Pro 26200
  • AI-assisted: yes

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 1191 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 1368 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 1395 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 2434 docs/dev/debt/libSceAgcDriver.md
PR 4072 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 4448 docs/dev/debt/libSceAgcDriver.md
PR 4449 docs/dev/debt/libSceAgcDriver.md
PR 5058 docs/dev/debt/libSceAgcDriver.md
PR 5372 docs/dev/debt/libSceAgcDriver.md
PR 5431 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 5441 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 5609 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 5925 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md
PR 6186 docs/dev/debt/libSceAgcDriver.md
PR 6188 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 6194 docs/dev/debt/libSceAgcDriver.md
PR 6202 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 6223 docs/dev/debt/libSceAgcDriver.md
PR 6273 docs/dev/debt/libSceAgcDriver.md
PR 6280 docs/dev/debt/libSceAgcDriver.md
PR 6292 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 6300 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp
docs/dev/debt/libSceAgcDriver.md
PR 6412 docs/dev/debt/libSceAgcDriver.md
PR 6481 docs/dev/debt/libSceAgcDriver.md
PR 6539 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 6548 docs/dev/debt/libSceAgcDriver.md
PR 6549 docs/dev/debt/libSceAgcDriver.md
PR 6615 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 6741 docs/dev/debt/libSceAgcDriver.md
PR 6808 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp
docs/dev/debt/libSceAgcDriver.md
PR 6867 docs/dev/debt/libSceAgcDriver.md
PR 6868 docs/dev/debt/libSceAgcDriver.md
PR 6879 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 6946 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md
PR 6959 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md
PR 7201 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 7256 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md
PR 7275 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 7604 docs/dev/debt/libSceAgcDriver.md
PR 7618 docs/dev/debt/libSceAgcDriver.md
PR 7642 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md
PR 7705 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 7803 docs/dev/debt/libSceAgcDriver.md
PR 7893 docs/dev/debt/libSceAgcDriver.md
PR 8105 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 8246 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 8281 docs/dev/debt/libSceAgcDriver.md
PR 8395 docs/dev/debt/libSceAgcDriver.md
PR 8424 docs/dev/debt/libSceAgcDriver.md
PR 8527 docs/dev/debt/libSceAgcDriver.md
PR 8592 docs/dev/debt/libSceAgcDriver.md
PR 8658 core/libs/prx/libSceAgcDriver/tests/execution/NativeMultisampleDepth.cpp
PR 8672 docs/dev/debt/libSceAgcDriver.md
PR 8999 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md
PR 9020 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md
PR 9335 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md
PR 9356 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
PR 9387 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md
PR 9508 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md
PR 9592 docs/dev/debt/libSceAgcDriver.md
PR 9721 docs/dev/debt/libSceAgcDriver.md
PR 9748 core/libs/prx/libSceAgcDriver/Graphics/src/Draw.cpp
docs/dev/debt/libSceAgcDriver.md

@github-actions github-actions Bot added docs agc libSceAgc* labels Oct 16, 2026
KingDarkHN added a commit to KingDarkHN/AnyPS5 that referenced this pull request Oct 16, 2026
…HTILE clears

Guest titles (e.g. Sonic Superstars) render into a native multisampled colour
target and then reuse that guest memory for something else while the driver
still guards it as a resident target; every such access aborted and closed the
title as soon as a level began. Drop a reused multisampled colour guard
instead of throwing (issue boykopovar#5933).

Supporting fixes that fall out of the same depth paths:

- Honour a guest HTILE fast clear the driver has no producer note for when the
  whole HTILE proves the surface's clear value and neither the depth nor the
  stencil plane changed, instead of aborting.
- Retire a depth surface only over the bytes its plane actually stores. The
  tiled footprint rounds the plane up to whole 128x128 blocks, so a
  neighbouring allocation could land in the padding and wrongly retire a
  surface it never reused.
- Accept a stencil plane whose clear state is in the raw plane when HTILE
  stencil metadata is disabled, and accept a clear-proven HTILE without
  ITERATE_256 or pipe alignment.

A guest read over a resident native multisample depth guard is not retired: it
keeps refusing ("sample-preserving guest publication of native multisample
depth is not implemented"), matching the contract of boykopovar#6727 for an unwritten
surface, until sample-preserving publication of the depth, stencil and HTILE
planes to guest memory exists. The read-site classification, whole-fill HTILE
note and exact-plane overlap work are intended to be rebased onto boykopovar#6727 once it
lands.

Tests: agc_driver_native_multisample_depth, agc_driver_depth_fast_clear and
agc_driver_depth_surface_last_writer exercise the new paths; the full
agc_driver suite stays at its pre-existing pass rate.
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 17, 2026
@simonbronk
simonbronk force-pushed the fix/native-msaa-depth-unwritten-guest-access branch from 74413ec to 0e4eccd Compare October 18, 2026 13:42
@github-actions github-actions Bot added unblocks title Names a title ID on the Unblocks line needs rebase Conflicts with main, rebase needed and removed needs rebase Conflicts with main, rebase needed labels Oct 18, 2026
@simonbronk
simonbronk force-pushed the fix/native-msaa-depth-unwritten-guest-access branch from 0e4eccd to c72a102 Compare October 18, 2026 21:47
@github-actions github-actions Bot added needs rebase Conflicts with main, rebase needed and removed needs rebase Conflicts with main, rebase needed labels Oct 18, 2026
@simonbronk
simonbronk force-pushed the fix/native-msaa-depth-unwritten-guest-access branch from c72a102 to f2eb6a7 Compare October 19, 2026 22:21
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 19, 2026
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 19, 2026
@oneandonlydean oneandonlydean self-assigned this Oct 20, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Thanks. The first commit on this branch (the ITERATE_256 HTILE import) is already on main, and the branch now conflicts with main in tests/execution/NativeMultisampleDepth.cpp and the libSceAgcDriver debt file. Could you rebase onto current main so the PR carries only "retire an unwritten native multisample depth surface on guest access"? I'll review it on the rebased head.

@simonbronk
simonbronk force-pushed the fix/native-msaa-depth-unwritten-guest-access branch 2 times, most recently from f47436b to 8328c2f Compare October 20, 2026 22:01
@simonbronk

Copy link
Copy Markdown
Contributor Author

@oneandonlydean rebased onto current main (bcdb03375). The PR now carries only "retire an unwritten native multisample depth surface on guest access" (8328c2f).

  • Conflicts: main's 84e3b73 added a depthWritten parameter to DepthSurfaceView. I kept it at both Draw.cpp call sites and added this PR's NoteDepthSurfaceWrite next to them, so the plane mirroring and the host-write mark are independent.
  • Debt entry: I shortened this PR's addition to the native multisampled depth entry (1,922 → 1,778 characters; it was 1,403 on main, already over the 1,000 warning). The content is unchanged.

Tested on Windows 11 Pro 26200, WinLibs GCC 15.2.0 r7, Release, ctest -R "agc|shader|video_out":

  • RTX 5070 (617.42): 879/880 passed. agc_driver_storage_read_during_idle_store failed once under -j 8; it passes 5/5 alone on this head and 3/3 on main.
  • AMD gfx1036 iGPU: 879/880 passed. agc_driver_tessellation fails 3/3 on main as well.
  • agc_driver_native_multisample_depth passes on both.
  • agc_driver_image_array_window_past_layer_limit doesn't build with MinGW on main (alignas(65536), test(agc): align the array window test's texels at run time #8310 fixes it), so it was excluded.

AI-assisted: yes

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 20, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

@simonbronk Thanks for the rebase. The conflict resolution against 84e3b73 looks right (both call sites keep depthWritten), and the retire-on-read logic matches main's release retirement. Two small requests:

  1. NoteDepthSurfaceWrite takes surfacesMutex and walks every depth surface on every depth- or stencil-writing draw, including single-sample draws in titles that never create a resident depth guard. Could it return early when there are no guards? Guards are only created together with a fresh surface (whose hostWritten starts false), so this is exact:
    void NoteDepthSurfaceWrite(const Context& context, const DepthTarget& target) {
        if (target.samples == 1u || !anyDepthGuard().load(std::memory_order_acquire)) return;
        ...
  2. The Draw.cpp marking isn't covered yet: the new case calls NoteDepthSurfaceWrite directly. The fixture already does a real device->Draw(state, draw, initial) with depthWrite = true. Could you add a guest-read TargetRefuses(...) right after it, and if it's easy, a depth-test-only draw followed by a read that retires? A missed mark there would silently drop the host's depth instead of throwing, so it's the path most worth pinning.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 21, 2026
…st access

A guest read over the depth or HTILE range of a native multisample depth
surface threw even when the host had not written the surface since its
import. Its samples are then still the clear value the import took from
the guest HTILE, so the guest bytes already describe it. Retire such a
surface and its guards instead, and keep the refusal for a surface the
host wrote: draws with depth or stencil writes, depth/stencil copies and
storage write-back.

Onimusha: Way of the Sword (PPSA27836) imports a 4-sample D32 target,
draws into it once with depth tests only and no color target, then reads
the memory back as a structured buffer in a compute dispatch.

Unblocks PPSA27836.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@simonbronk

Copy link
Copy Markdown
Contributor Author

@oneandonlydean both done, on a rebase onto main d6eafa5 (one conflict in DepthSurface.hpp, where main added DepthPlaneStoreCounts; I kept both).

  1. NoteDepthSurfaceWrite now returns at once for a single-sample target or when anyDepthGuard() is false, as you suggested.

  2. CheckDrawFeedback now covers the Draw.cpp marking through device->Draw:

    • right after the depth-writing draw, a guest read of the surface TargetRefuses(... "sample-preserving guest publication"), and the surface stays resident;
    • a second target gets a depth-test-only draw (depthWrite = false). A guest read of it then returns its guest bytes and retires it.

    The direct NoteDepthSurfaceWrite calls stay only in the fixtures that don't draw.

Tested: only partly, and I'd ask you to run the full CI on this head.

  • Builds: Linux, Ubuntu 24.04 x86-64 container (Colima/Rosetta), GCC 13, both RelWithDebInfo with ASan and Release, merged onto main d6eafa5. check_conventions.py --base origin/main passes; its only warning is the debt entry's length, which was already over 1,000 characters on main.
  • agc_driver_native_multisample_depth could not be exercised there. That VM's kernel has no asynchronous userfaultfd write protection ("write watch unavailable"), so every native depth import is refused with "requires watched accessible guest memory". The test fails the same way on main d6eafa5 and on this head (lavapipe, ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1, Khronos validation), so neither run tells anything about the change. The only VUID- in both logs is the same VUID-VkDeviceCreateInfo-pNext-pNext, from Ubuntu's 1.3.275 layer not knowing a newer structure.
  • The Windows RTX 5070 / AMD iGPU runs from the previous head are not repeated on this one.

AI-assisted: yes

@simonbronk
simonbronk force-pushed the fix/native-msaa-depth-unwritten-guest-access branch from 8328c2f to bf0806e Compare October 22, 2026 09:11
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 22, 2026
@EagleBearerYS

Copy link
Copy Markdown
Contributor

@simonbronk a merge-order note: #9533, with #9717 and #9742 stacked on it, passes the draw's stencilTest to DepthSurfaceView at the two calls in Draw.cpp where this PR adds NoteDepthSurfaceWrite after the view. Both also reword the native multisampled depth sampling entry in the debt file. Composed, each call keeps the extra argument and your write note, and the entry keeps both changes. #9533 goes after this PR and rebases over it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* docs unblocks title Names a title ID on the Unblocks line

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants