Skip to content

fix(agc): retire a depth surface on a storage write only when the write covers a whole plane - #7050

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
SP4C3B4R-8:fix/depth-retire-whole-plane
Oct 19, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
SP4C3B4R-8:fix/depth-retire-whole-plane

Conversation

@SP4C3B4R-8

Copy link
Copy Markdown
Contributor

What

Since #454 a written storage image retires every depth surface its described size overlaps, and a retired surface reads guest memory, which never received the depth image. ee6aba54 reported on #454 that PPSA15304 writes storage views that overlap the scene depth without covering it (the largest reaches 0x11e0000 of the 0x12b0000 depth plane), so the whole depth was thrown away and the scene came out almost black; they keep #454 reverted on their branch.

  • A written storage view now retires a depth surface only when the bytes its written level holds, per array layer or depth slice (CoveredMipBytes: whole tile blocks, rows without padding), cover the surface's whole depth or stencil plane. A partial overlap leaves the surface alive, as before fix(libSceAgcDriver): retire a depth surface once another writer covers its memory #454.
  • Thick 3D views hold no whole block per slice and retire nothing.
  • The cheap overlap check runs first (DepthSurfaceOverlaps), so a written storage view that touches no depth surface does not describe its layers.

By ee6aba54's descriptor table this keeps the 2848x1600 scene depth alive and still retires the 512x512 surface whose planes the 4 MiB views cover.

Tested

Windows 11 Pro (26300), MinGW-w64 GCC 15.2.0, Release, Ryzen 7 7800X3D, RTX 5070 Ti.

  • agc_driver_depth_storage_reuse now checks, for D32, that a half-width storage write keeps the surface sampled from the depth image (0.5), and that a 128x128 R32 write over the whole 64 KiB plane retires it (0.25 from memory). With the old retire on any overlap it fails ("depth reuse returned the wrong GPU value at lane 0").
  • All 30 depth and storage ctests (-R "depth|agc_storage|storage_one_slice|storage_write_back|storage_cache|resource_cache_storage") pass.
  • Not tested in PPSA15304 (I don't have it); ee6aba54 offered to compare the frame at the same spot.
  • tools/check_conventions.py --base origin/main passes.

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
  • Fixes: none
  • Unblocks: PPSA15304 (regression reported on fix(libSceAgcDriver): retire a depth surface once another writer covers its memory #454)
  • GPUs tested: NVIDIA GeForce RTX 5070 Ti, driver 617.42, Windows 11 Pro 26300
  • AI-assisted: yes (Claude). I reviewed and take responsibility for every line.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GN8FJFTedayRXLHythXkZ4

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Thanks @SP4C3B4R-8, the partial-overlap fix looks right: the PPSA15304 view no longer retires the scene depth. One change requested before merge, plus a heads-up about #7044.

Full-plane writes at non-block-aligned extents no longer retire (regression vs main)

CoveredMipBytes counts only whole tile blocks (wholeColumns = width / blockWidth, wholeRows = height / blockHeight, TextureTiling.cpp:296-297), but depthBytes() is the padded plane (DepthSliceBytes). So a write can only cover a plane whose extent is a multiple of the block size, and real depth planes almost never are:

  • D32 2848x1600 is 0x12b0000 = 23x13 = 299 blocks of 64 KiB. A full 2848x1600 R32 write at the same address covers 22x12 = 264 whole blocks, so the surface is not retired. 1920x1080 is the same (1080/128 = 8.4).
  • The PR's own fixture (64x32 D32, one block) needs a 128x128 write to retire. A 64x32 write in a format DepthSurfaceStorage doesn't serve (e.g. R32_UINT) has wholeColumns = 0 and never retires.

The effect is that after a whole-plane overwrite in another format or layout, the plane's sampled view returns the stale depth image again, which is the pre-#454 behaviour, now limited to block-aligned planes.

Ask: count the edge blocks as covered when the written view reaches the plane's extent (for example in CoveredBy: same base address and block layout, written extent in bytes per row and rows at least the plane's). Partial overlaps should keep the surface alive, as they do now. Please also add a test case: a full-extent write at the fixture's own 64x32 in a non-served format must retire (expect 0.25 from memory).

Heads-up: conflict with #7044

#7044 and this PR merge cleanly as text but don't compile together: both add bool DepthSurfaceOverlaps(VkDevice, uint64, uint64) (two definitions in DepthSurface.cpp, two declarations in DepthSurface.hpp). #7044's version also checks surface->active; either body is correct for both callers. Whichever lands second needs to drop its copy. No action needed yet, just so it isn't a surprise.

After the fix we'll run full CI on the new head and the usual game checks.

@github-actions github-actions Bot added the agc libSceAgc* label Oct 17, 2026
@github-actions

github-actions Bot commented Oct 17, 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
PR 6146 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp L1914
PR 7154 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp L10
core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp L1006

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 1368 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 1395 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 5499 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 6177 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6188 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6192 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6194 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6202 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6223 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6292 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6300 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 6340 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6412 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6498 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6615 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6647 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6683 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 6718 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6727 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 6804 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6808 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 6820 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6937 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6946 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6947 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6970 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6972 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 6977 core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 7002 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 7201 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
PR 7233 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
PR 7258 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 7340 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 7390 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 7484 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 7618 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 7642 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 7650 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
PR 7689 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp

@keravyn-ops

keravyn-ops commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

AMD / Windows run of this PR at 25c6bb0: Radeon 780M (RDNA 3, 0x15bf), AMD driver 25.10.20.01 (32.0.21020.1007), Windows 11 Home 10.0.26100, WinLibs GCC 15.2.0 r7, Release.

  • agc_driver_depth_storage_reuse: pass (0.33 s)
  • agc_driver_f16_fma_denormal_modes: pass (0.32 s)
  • agc_driver_linear_target_resident: pass (0.31 s)
  • agc_driver_graphics: pass (4.97 s)
  • agc_driver_prepared_graphics, agc_driver_prepared_unregistered_graphics, agc_driver_prepared_graphics_prebuild_unset, agc_driver_prepared_graphics_prebuild_0, agc_driver_prepared_graphics_prebuild_1: pass

AI-assisted: yes (runs made on the device above).

@ee6aba54

Copy link
Copy Markdown
Contributor

@SP4C3B4R-8 Ran it at the same spot. PPSA15304, start position of the first area after loading the save, frame dumps (APS5_DUMP_FRAMES) of the title's output at the same camera position, mean gray of the whole frame and of its lower two thirds (0-255). Both builds are plain main bf2053816, the second with this PR (25c6bb032) merged; nothing else differs.

Build Mean gray Lower 2/3 Scene
main 47.5 58.8 tree trunks and background nearly black, hard black ground shadows
main + this PR 98.5 101.8 trunks, ground and background lit

So this PR fixes the darkening, consistent with the 2848x1600 scene depth no longer being retired; the scene is lit as in our builds with #454 reverted (92.5 to 96.6 there).

Both runs used APS5_NO_RECT_INDIRECT=1: since #5979, red and blue blotches appear between the tree canopies in this scene (reported on #5979). The switch does not change the brightness; plain main without it measured 50.8.

Ubuntu 24.04, kernel 7.0.0, NVIDIA GeForce RTX 5080, driver 595.99.02, Release build.

AI-assisted: yes

…te covers a whole plane

Since boykopovar#454 a written storage image retires every depth surface its described size overlaps, and a retired
surface reads guest memory, which never received the depth image. PPSA15304 writes storage views that
overlap the scene depth without covering it (the largest reaches 0x11e0000 of its 0x12b0000 depth plane),
so the whole depth was thrown away and the scene came out almost black.

A written storage view now retires a surface only when the bytes its written level holds, per array layer
or depth slice (CoveredMipBytes, whole tile blocks, no padding), cover the surface's whole depth or stencil
plane; a partial overlap leaves the surface alive, as before boykopovar#454. Thick 3D views hold no whole block per
slice and retire nothing. agc_driver_depth_storage_reuse now checks that a half-width write keeps the
surface sampled from the depth image and that a write over the whole plane retires it.

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

Copy link
Copy Markdown
Contributor Author

@oneandonlydean thanks, both done in a714d1b (rebased onto main ed01d79).

Full-plane writes at non-block-aligned extents: a written view now also retires a plane when it reaches the plane's extent with the same block layout: same base address (per written layer), same bytes per element, 64 KiB blocks with the depth block's extent (128x128 for 32-bit, 256x128 for 16-bit, 256x256 for stencil), the same blocks per row, and width and height at least the plane's (WrittenStorageExtent, CoveredBy). Partial overlaps keep the surface alive as before, and the whole-block byte rule still covers the rest.

Test: DepthStorageReuse.cpp writes the fixture's own 64x32 through an R32_UINT view in 64KB_S_X (not served by DepthSurfaceStorage, not a depth-bits layout) and expects the sampled plane to read 0.25 from memory; the 128x128 case runs after a new depth draw. The depth address is now 64 KiB aligned inside the buffer, since a tiled view's base is block aligned. With the new rule disabled the test fails at that read.

#7044: it merged first, so this PR now uses its DepthSurfaceOverlaps and drops its own copy.

Local: 37 depth/storage ctests and the agc suite pass (RTX 5070 Ti, Windows 11); check_conventions.py passes.

@savvaniss

Copy link
Copy Markdown
Contributor

Intel and AMD Windows data: agc_driver_depth_storage_reuse fails on main and passes with this PR on an Intel UHD 770 and an AMD RX 9070 XT.

Windows 11 Pro 26300, WinLibs GCC 15.2.0 r7, Release, every execution test built, ANYPS5_NO_SHADER_CACHE=1, serial CTest. Intel UHD Graphics 770 (8086:a780), driver 32.0.101.7088. AMD Radeon RX 9070 XT (1002:7550), driver 32.0.31041.1004, with VkCache emptied first. This head a714d1b37 merged into main f3059421f. "Without" means this PR's test files on main f3059421f, with none of its other changes.

Tests, ×3 UHD 770 RX 9070 XT
This head: agc_driver_depth_storage_reuse pass pass
Without fails: depth reuse returned the wrong GPU value at lane 0: got 1048576000, expected 1056964608 same

I also ran full CTest with this PR and the 14 other open AGC PRs I tested in the same batch merged together (#7647, #7641, #7630, #7627, #7603, #7600, #7591, #7579, #7535, #7533, #7474, #7462, #7048, #7650). Neither GPU showed a failure beyond main's own, except #7650's new test.

AI-assisted: yes (Claude Code)

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants