Skip to content

fix(agc): start a new depth surface from the storage writes over its plane - #5446

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
Nesyth:fix/agc-depth-surface-from-storage-writes
Oct 14, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
Nesyth:fix/agc-depth-surface-from-storage-writes

Conversation

@Nesyth

@Nesyth Nesyth commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

What

A depth surface the driver creates for a depth target starts from DB_DEPTH_CLEAR: the DepthSurface constructor clears the new image and never reads guest memory. Storage writes made to the plane's memory before any surface existed at that address therefore do not reach it. At that time the view is an ordinary storage image, since a plane's own storage view (#4500) exists only once its surface does. The first depth test against the plane reads the clear value instead of what the kernel wrote.

DepthSurfaceView now gives a surface it creates for a target without HTILE (DB_HTILE_DATA_BASE 0), and without an earlier surface whose planes it takes over (#4683), the depth those writes left (Graphics/src/DepthSurface.cpp):

  • It looks for a storage image at the depth plane's address: first one with results pending, which may be larger than the plane (StorageTexture::FindPending), then a cached one of exactly the plane's size (FindLive).
  • It takes the image only when it fits the plane: the plane's width and height, one level, a 2D or 2D array view, and an R16 format (UINT, UNORM, SINT, SNORM or SFLOAT) for a 16-bit plane (D16, D16S8) or an R32 format (SFLOAT, UINT or SINT) for a 32-bit one (D32, D32S8).
  • The image is first brought up to date with guest memory (Refresh). Its layer 0 is then copied into the surface's depth aspect through the plane's staging buffer: in the active recorder, which keeps the image alive until the copy has run, or else in a batch of its own that is submitted and waited for.
  • Only the depth aspect is copied. The stencil plane of a new D32S8 surface still starts from DB_STENCIL_CLEAR.
  • These surfaces still start from DB_DEPTH_CLEAR, as on main:
    • a surface with HTILE, where the HTILE state decides, so an HTILE clear after the storage write wins;
    • a multisampled surface;
    • a surface the device stores in another format than the guest's (16-bit depth with stencil on a device without D16_UNORM_S8_UINT, fix(agc): store D16 depth with stencil as D24 or D32 where the device lacks it #3846);
    • a surface with no fitting image at its address (another extent, level count, dimension or format size).
  • A surface that already exists is never seeded again, including one DepthSurfaceView finds again for the same target. fix(agc): store a written storage image over a depth plane into the surface #5113 keeps such a surface up to date when a storage image over its plane is stored back.
  • DepthSurfaceView now takes GuestMemory::GpuMutex() (recursive) before the surfaces' mutex, the same order DepthSurfaceTexture uses, because the lookup and Refresh run under the GPU mutex. Its only caller in the driver is the draw path.
  • docs/dev/debt/libSceAgcDriver.md records the rule. On the console the plane is the memory the kernel wrote, so there is nothing to seed; none of this is measured there.

Ready or Not (PPSA18605) clears its shadow maps this way. Traced with temporary logging on my bring-up build (run 205): six 1024x1024 D16 planes were each written through a storage image, then bound as a depth target for the first time and created from DB_DEPTH_CLEAR 0.0 without HTILE. On main the two operators in the main menu are black silhouettes, while the PS5 shows a red rim light on them. With this change they are lit (see Tested).

Test: new tests/execution/DepthSurfaceFromStorage.cpp (ctest agc_driver_depth_surface_from_storage). A compute kernel stores row 0 of a 64x32 plane whose memory holds 0.25, 32 values from 0 to 0.94, through a storage view. A full-screen draw at depth 0.5 with LESS then tests against a new surface over the plane, whose clear value is 1.0. The draw passes 15 of the 2048 pixels when the surface took the storage image (the stored values above 0.5) and all 2048 when the surface started from its clear value:

  • a new D32 surface over a pending R32F view, a new D16 surface over a pending R16 unorm view, and a new D32 surface over a flushed R32F view (FindLive): 15 each;
  • a new D32 surface over a pending two-layer R32F 2D array view, which stores into layer 0: 15. The image is larger than the plane, so only FindPending finds it; the case requires FindLive to miss it;
  • a new D32 surface whose HTILE was cleared (NoteHtileDepthClear) after the storage write: 2048;
  • a new D32S8 surface over a pending R32F view: 15. A stencil EQUAL 0 draw without the depth test then passes all 2048 pixels (the stencil plane kept its clear value).

Not covered by a test:

  • The multisample and host format check, and the image's extent, level, dimension and format checks.
  • Which image wins when a pending one and a cached one of the plane's size are both at the address: FindPending comes first, so the one with pending results.
  • Skipping a new surface that takes its planes from an earlier one: without that check, the depth tests (ctest -R depth, 10 tests) still pass. It keeps an image that is still cached at the address from overwriting depth drawn after the image was written.

Related, both merged:

Tested

Windows 11 Pro 10.0.22631, AMD Ryzen 7 7800X3D, AMD Radeon RX 7900 XTX, WinLibs GCC 15.2.0, Release, BUILD_TESTING=ON, this head on main 32e810d:

  • ctest -R "^agc_" -j 2: 539 of 544 pass, with 4 skips. The failure, agc_driver_layered_volume_vertex_no_gpl ("vertex draw: slice 1 texel (0, 0) channel 0 is 64, expected 255"), also fails on main 4ae78dc without this change on this GPU.
  • One change at a time, rebuilding and running agc_driver_depth_surface_from_storage:
    • on main 32e810d without this change, it fails in the five new-surface cases ("a new D32 depth surface over a pending R32F storage write: 2048 pixels passed, expected 15", and the same for D16, the flushed image, the two-layer view and D32S8);
    • taking the image for a surface with HTILE as well (without target.htileAddress == 0): "a new D32 depth surface whose HTILE was cleared after a storage write: 15 pixels passed, expected 2048";
    • without FindLive: "a new D32 depth surface over a flushed R32F storage write: 2048 pixels passed, expected 15";
    • with FindLive alone, without FindPending: "a new D32 depth surface over a pending R32F storage write through a two-layer 2D array view: 2048 pixels passed, expected 15".
  • tools/check_conventions.py --base origin/main passes.

Ready or Not (PPSA18605), main menu, measured before this rebase on main 18c438b, which main dacd785 differs from only in libSceAgcDriver (#4949). The runtime's libs held the 127 modules built from 18c438b, with libSceAgcDriver swapped per run, and app.exe is the title relinked by that commit's relinker. APS5_HOST_IMPORT_MIB=10240, and the runtime's saved\ps5\engine.ini raises UE's render fence timeout (g.TimeoutForBlockOnRenderFence=1800000). The red light is measured on the 9 menu frames each run dumped (1280x720), in the 500x380 box around the operators, as the share of pixels with R >= 60 and R at least twice G and B:

image

Main menu, one dumped frame (1280x720) from each session. Top left: main 18c438b (run 700). Top right: main + this PR (run 701). Bottom left: main + #5113 (run 702). Bottom right: the PS5, for reference. The two operators stand in front of a red light. On the PS5 it lights their outlines. On main and with #5113 alone they are black silhouettes; with this PR they get the same rim light.

Linux: not run.

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: none
  • Unblocks: PPSA18605 Ready or Not (PPSA18605) #4184
  • GPUs tested: AMD Radeon RX 7900 XTX, AMD Software Adrenalin 26.8.1 (driver 32.0.31041.1004), Windows 11 Pro 10.0.22631
  • AI-assisted: yes

@github-actions github-actions Bot added docs agc libSceAgc* labels Oct 13, 2026
RINNECODER added a commit to RINNECODER/AnyPS5 that referenced this pull request Oct 14, 2026
#484)

* fix(agc): sample 10_10_10_2 SNorm and SInt textures

Guest texture formats 51 (SNorm) and 55 (SInt) of the 10-bit packed group
were missing from the texture lookup and the descriptor format table, so a
descriptor using them threw 'unsupported guest texture format' while the
UNorm (50) and UInt (54) siblings worked. Map them to
A2B10G10R10_SNORM_PACK32 / A2B10G10R10_SINT_PACK32, classify them for
sampling (float / signed integer), and list SINT among the integer
formats. TextureFormat.cpp and RdnaDescriptorFormat.cpp are shared with
our Metal path.

Metal has no signed RGB10A2 pixel format, so MetalTexture still refuses
these two formats, now by name ("no supported pixel format for guest
format 51") instead of failing the shared lookup. Recorded in
TechnicalDebt.

Test: metal_utility_tests (TextureResource.mm) checks that 51 and 55
resolve to the signed packings, are four bytes and not block compressed,
and that a native texture refuses them by name. The upstream
RunTextureFormatTests cases (tests/TextureFormat.cpp) are carried too;
they build in the Vulkan graphics tests, not on macOS.

Ported from upstream PR boykopovar#5149
(cherry picked from commit c6a4223)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj

* fix(agc): share a depth plane's contents across its stencil pairings on Metal

A title can bind one depth plane with several stencil planes (or one
stencil plane with several depth planes). MetalDepthSurfaceCache keys a
surface by both plane addresses, so each pairing got its own textures,
cleared on creation: depth written through one pairing never reached the
others, and later draws tested against stale depth.

The cache now records, per depth and per stencil plane address, the
surface that bound it last. Binding a single-sample surface without
DB_DEPTH_VIEW slices takes each of its planes from that surface when both
have the same format and extent: a blit copies the texture layer, or, for
a combined Depth32Float_Stencil8 texture, only that plane's aspect through
a buffer. The blit is committed on the backend's queue, after the passes
that wrote the other surface and before the draw that binds this one.
Multisampled surfaces and depth-array slices keep their own planes, as
before. Clear() forgets the owners.

Upstream does this in the Vulkan DepthSurface.cpp (vkCmdCopyImage, plus a
pending fast clear carried over); our Metal path has no HTILE fast-clear
state, so only the copy applies.

Test: metal_utility_tests (DepthResource.mm, sharedPlanes) binds a depth
plane with a second stencil plane, binds the same pairing again, goes
back to the first pairing, binds the stencil plane with another depth
plane and binds a pairing of another extent, for D16S8 and D32S8. It
fails without this change ("Sampled depth plane returned incorrect GPU
values").

Ported from upstream PR boykopovar#4683

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj

* fix(agc): retire a Metal depth surface once another writer covers its memory

A depth surface lives only in its Metal textures: nothing writes it back
to guest memory. When a color target or another depth target is later
bound over its memory, the surface still answered every view of that
address, so a texture of another layout there threw ("resident depth
view is incompatible") and a matching one read the dead depth.

The surface is now retired by the last writer:
- binding a color target drops the depth surfaces whose depth or stencil
  plane it overlaps from the draw's resident surfaces (the ones views
  and depth-array gathers search);
- binding a depth target moves its surface last and retires the other
  surfaces its planes overlap: another stencil pairing of its depth
  plane, or a surface of another extent at its address. The lookup then
  finds the surface bound last, not the one bound first.
The cache keeps a retired surface; binding its target again brings it
back with the texels it held (with boykopovar#4683's change, a plane another
pairing wrote since is copied over first).

Not ported: upstream also retires a surface on a written storage image
over its planes and, with write watching, on CPU or driver stores. Our
Metal path has no storage view of a depth plane (a storage image at its
address throws) and no write tracking for this cache, so those cases keep
throwing. Recorded in TechnicalDebt.

Test: metal_utility_tests (DepthResource.mm, retiredDepth): writes next
to the planes retire nothing; the surface being bound is kept; a writer
inside the depth or stencil plane retires it; the cache still returns it;
binding a depth target of another extent at the address retires the
other surface.

Ported from upstream PR #454

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj

* docs(agc): record the Metal depth-plane views and storage writes not modeled

Upstream boykopovar#4722, boykopovar#4873, boykopovar#5054, boykopovar#5113 and boykopovar#5446 fix the Vulkan depth-plane
copy (padded detiled rows and slices, a cube with a mip chain) and the
storage writes over a depth plane. Our Metal path has neither: a resident
depth surface is served only as a single-level 2D view or a 2D array of
its extent, and a storage image over its plane is bound to guest memory.
Record that in TechnicalDebt instead of porting the fixes.

Refs upstream-pr-batch-1

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj

---------

Co-authored-by: xXPBSXx <20614131+xXPBSXx@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 14, 2026
@KauaSantos702

Copy link
Copy Markdown
Contributor

NVIDIA run at this head (284011b8b), adding a vendor to the AMD results in the body: no agc_* test fails.

  • NVIDIA GeForce RTX 5070 (Blackwell), driver 32.0.16.1714, Windows 11
  • WinLibs GCC 15.2.0 r7, Release, BUILD_TESTING=ON
  • ctest -R '^agc_' -j2: 484 tests, 483 passed, 0 failed, 1 skipped. The skipped test is agc_driver_stencil_ref_export, because the device lacks VK_EXT_shader_stencil_export; it is also skipped on main.

@Nesyth
Nesyth force-pushed the fix/agc-depth-surface-from-storage-writes branch from 284011b to 2cf6b09 Compare October 14, 2026 18:46
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 14, 2026
@adriadam10 adriadam10 self-assigned this Oct 14, 2026
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked: the merged tree builds and agc_driver_depth_surface_from_storage passes on Linux; with DepthSurface.cpp reverted it fails, so the test pins the change. The rest of the libSceAgcDriver scope is green except agc_driver_tessellation, which segfaults on main as well. Lock order (GpuMutex before the surfaces mutex), the recorder/batch copy path and the debt entry look right; thanks for the NVIDIA run, @KauaSantos702. Merging once the full Build is green.

@adriadam10
adriadam10 merged commit 038c83b into boykopovar:main Oct 14, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants