Skip to content

fix(graphics): allow storage textures in reused depth memory - #2904

Merged
oneandonlydean merged 3 commits into
boykopovar:mainfrom
Scramblrrrr:codex/depth-storage-reuse
Oct 12, 2026
Merged

oneandonlydean merged 3 commits into
boykopovar:mainfrom
Scramblrrrr:codex/depth-storage-reuse

Conversation

@Scramblrrrr

@Scramblrrrr Scramblrrrr commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

What

A cached depth surface at a temporary GPU address can block a later storage texture whose extent or format cannot represent that depth plane. Match the newest surface on the same device by extent and plane format/raw depth layout before refusing actual depth/stencil storage access. Incompatible views use the ordinary storage path.

Rebased onto main bca7a29. Keep #419's RequireNoMultisampleTarget guard before the depth admission check. Move the existing limitation under TechnicalDebt's Functional section. Rendered depth is not written back to guest bytes, and descriptor compatibility does not establish allocation lifetime or writer ownership. Matching depth/stencil storage access still throws on this standalone branch; #425/#454 provide broader transfer/retirement and their integration must keep this admission check.

Tested

Windows 11 / GCC 15.2 / Release / SPIRV-Tools OFF / RTX 5070 Ti, Vulkan required, head ef218ef:

  • agc_driver_graphics, agc_driver_depth_fast_clear, agc_driver_depth_storage_reuse: 3/3 passed, 6.10 s.
  • GPU fixture distinguishes rendered depth 0.5 from guest bytes containing 0.25. Matching sampled depth reads rendered depth (D16 allows one UNORM quantization step). Different-extent R32 over D32 and same-extent R32 over D16 storage views load/store guest 0.25.
  • Source conventions and git diff --check passed. No full-suite or new standalone title run claimed.
  • Earlier negative control restoring the address-only guard failed on the different-extent view; restored source passed. Earlier integrated 007 remained black. Independent Linux High on Life and Windows Demon's Souls feat(agc): depth planes sampled as 2D arrays, cubes and integer views #425 cross-tests are in comments; those are integrated stacks, not standalone title validation of this head.

Checklist

@github-actions github-actions Bot added the intake paused Closed while PR intake is limited to authors with at least 10 PRs merged into main label Oct 10, 2026
@github-actions

Copy link
Copy Markdown

Thank you for your interest in the project! We are sorry, but we cannot accept more PRs while we work through the existing backlog. This PR has been automatically closed and labeled "intake paused". This is not a judgment of the quality of your changes.

@github-actions github-actions Bot closed this Oct 10, 2026
@oneandonlydean oneandonlydean removed the intake paused Closed while PR intake is limited to authors with at least 10 PRs merged into main label Oct 11, 2026
@github-actions github-actions Bot added agc libSceAgc* needs rebase Conflicts with main, rebase needed labels Oct 11, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

I reopened this because it fixes the stop on main directly, so it doesn't have to wait for #425 or #454. Before it can go in:

  • rebase onto main (one conflict in DepthSurface.cpp: main added DepthSurfaceHolds at the same spot, keep both);
  • tick the "Based on current main" box (the conventions check fails on it) and mark it ready for review;
  • add a TechnicalDebt line: the depth plane is never written back to guest memory, so a reused storage view reads guest memory, not the depth bits;
  • add a test that runs on the device: a storage image of another extent, and one of the same extent in a format that can't hold the plane, both keep the guest bytes, and a view of the plane's own extent and format still gets the depth value.

@Mouhand-Kaddo

Copy link
Copy Markdown
Contributor

Additional platform validation of standalone PR head 3514a17a13e30c4820aa3e33b3af3266deadea06 (2026-10-11):

Linux: passed — Arch Linux in WSL2 (kernel 6.6.87.2), GCC 16.2.1 20260810; CMake 4.3.1 / Ninja; Release, BUILD_TESTING=ON.

1 checks, 0 failures, 0 CTest skips. Built the selected targets and dependencies; this is focused validation, not a full build or full-suite run.

Checks: agc_driver_graphics.

Command after building: ctest --test-dir <build> --verbose --no-tests=error --timeout 30 -R "^agc_driver_graphics$".

This scope checks CPU-side behavior; no GPU execution result is claimed.

Internal limitation: heap-mirror subcases returned early because the guest arena was not write-watched in WSL. Zero CTest skips does not include that early return.

No title/gameplay validation. These results cover the standalone head, not a merge with current main.

@blawesom

Copy link
Copy Markdown
Contributor

Hi! This PR currently conflicts with main, so I rebased it onto the latest main (d3e84f85, includes #3296). The conflicts resolved as follows:

  • DepthSurface.cpp and its header: main added DepthSurfaceHolds and the HTILE metadata helpers at the same spot as this PR's DepthSurfaceAt(context, words, resource) overload — kept both sides.
  • DepthSurfaceReuse.cpp: kept both sides' test additions; renamed the PR's local stencil constant to stencil16 to avoid shadowing the stencil address constant main added in the same function.

It builds and agc_driver_graphics_tests passes locally.

If it helps, the rebased branch is available to pull:

git fetch https://github.com/blawesom/AnyPS5 rebase/pr-2904
git reset --hard FETCH_HEAD

Feel free to ignore this if you'd rather rebase yourself.

@Tamalero

Copy link
Copy Markdown
Contributor

Tested on High on Life (PPSA15388 v01.120.000, Unreal Engine 4): this fixes its stop in the intro. With #425 and #454 ported, the run aborted the same way in 2 of 2 runs:

compute shader 0x20336f0000: AGC graphics: storage image access to depth surface 0x20b16a0000 (240x135, vk format 124) as a 120x68 image of vk format 97, dimension 1, 1 mips is not implemented

#454's retirement doesn't catch it, because the storage write itself is what reuses the memory. With this PR's two commits on top, the intro plays through and the title reaches its main menu and a save load. That abort hasn't come back in any run since.

Details

Setup: CachyOS (Linux 7.2), RTX 4090 (NVIDIA 615.78), GCC, Release. main f5c0fe00 plus my local integration patches and ports of #423/#1077, #425 and #454.

One conflict with #425, in ShaderResources.cpp. I kept #425's path that seeds a storage texture from a depth surface, but only when your stricter DepthSurfaceAt(context, words, viewed) matches (extent plus plane format). A storage descriptor that only shares a depth surface's address is now an ordinary storage texture. That is the reused-memory case here: a 120x68 R32 view of 240x135 D16 memory. The depth and storage tests pass (10/10). DepthSurfaceReuse sits inside agc_driver_graphics, which stops earlier on my tree on an unrelated span check, so that case hasn't run.

Tested and written by Claude (AI assistant) on a contributor's machine.

@Scramblrrrr

Copy link
Copy Markdown
Contributor Author

Thanks for reopening and for the concrete validation requirements. I applied the conflict resolutions from blawesom's rebase (keeping DepthSurfaceHolds/HTILE helpers and the stencil16 test name) onto current main 1e8fff6 in an isolated worktree, preserving original authorship. Windows build is underway. I added the requested technical-debt disclosure and am validating a real GPU depth producer versus guest bytes in two reuse cases, with a matching sampled plane as the control. I will keep this draft until device checks and conventions pass. Thanks also to Tamalero for the independent High on Life cross-test; I am not treating its integrated result as standalone GPU validation.

AI-assisted (Codex).

@Scramblrrrr
Scramblrrrr force-pushed the codex/depth-storage-reuse branch from 3514a17 to 0beaafe Compare October 11, 2026 12:32
@Scramblrrrr
Scramblrrrr marked this pull request as ready for review October 11, 2026 12:32
@Scramblrrrr

Copy link
Copy Markdown
Contributor Author

Updated to 0beaafe on main 1e8fff6; mergeable and marked ready. Windows graphics + depth fast-clear + new device reuse test passed 3/3. The new test has a real depth draw, matching sampled-plane control, and load/store readback for both incompatible storage descriptors. Restoring the old address-only guard makes it fail, restored source passes. TechnicalDebt now explicitly records the missing depth-to-guest writeback and descriptor-only limitation. The description distinguishes matching sampled depth from still-unsupported matching storage on this standalone branch. Thanks blawesom for the conflict resolutions and Tamalero for the cross-test. AI-assisted (Codex).

@Scramblrrrr

Copy link
Copy Markdown
Contributor Author

CI limitation on the new head: conventions run 38139635004 fails during actions/checkout, before the checker executes, with fatal: No url found for submodule path 3rdparty/imgui in .gitmodules. The Build run 38139638334 has the same checkout failure in the Linux/Windows relinker jobs. Base main 1e8fff6 contains that gitlink without a .gitmodules entry; this PR does not change submodules. Local source/body conventions and the selected Windows device checks passed. CI is not green, and I am not claiming full CI validation. AI-assisted (Codex).

@Scramblrrrr

Copy link
Copy Markdown
Contributor Author

After #3511 repaired main checkout, rebased again onto a11c8f7: head 4fdbe91. All six modified source/test/debt files are byte-identical to 0beaafe; selected Windows checks passed again, 3/3 in 5.56 s. Local source/body conventions pass and latest conventions CI is green. Remains ready for review. No full build CI claim. AI-assisted (Codex).

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 11, 2026
@fdfawef324

Copy link
Copy Markdown
Contributor

Tested together with #425 on Demon's Souls (PPSA01342 v1.005, US), Windows, RTX 3060 (NVIDIA 591.86): main a11c8f7 + #3871 + #425 at 6d59983d3 + this PR at 4fdbe916e.

The merge with #425 conflicts in cachedStorageTexture. I kept #425's body (stencil-plane Require, lookupStorageTexture, then SeedStorageFromDepth) and seed from depth only when this PR's DepthSurfaceAt(context, words, viewed) matches, the same resolution queeeef described on #1409.

Result: the storage image access to depth/stencil surface 0x282e10000 is not implemented stop at compute shader 0x24893fb00 (PlayStation Studios logo) is gone in seven runs. The title reaches its language, brightness and voice settings screens and the opening cinematic. The next stop is unrelated: bindless image table: 18 distinct keys exceed the 16 slots (compute shader 0x248947300, #757's area).

AI-assisted: yes (Claude Code)

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Conflicts with main in ShaderResources.cpp: #419 added RequireNoMultisampleTarget just above the guard in cachedStorageTexture. Please rebase and keep both.

While you're there, the TechnicalDebt line sits above ### Build, outside any section; move it under ### Functional.

@Scramblrrrr
Scramblrrrr force-pushed the codex/depth-storage-reuse branch from 4fdbe91 to ef218ef Compare October 12, 2026 00:31
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 12, 2026
@Scramblrrrr

Copy link
Copy Markdown
Contributor Author

Rebased onto bca7a29, new head ef218ef. Kept RequireNoMultisampleTarget first and the descriptor-aware DepthSurfaceAt(context, words, viewed) guard after it. The debt entry now appears only under ### Functional. Windows RTX5070Ti with required Vulkan: graphics/depth_fast_clear/depth_storage_reuse 3/3 pass, 6.10s; conventions and whitespace pass. No full-suite claim.

Thanks fdfawef324 for the seven-run Demon's Souls cross-test and the exact #425 integration resolution. Updated description keeps the integrated-title evidence separate from standalone checks. AI-assisted: yes (Codex).

@github-actions

github-actions Bot commented Oct 12, 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
#425 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp L840-844
#2490 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp L840-844
#2491 core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp L840-844
#4318 docs/dev/TechnicalDebt.md L389
#4411 docs/dev/TechnicalDebt.md L389-463

@SleglJan

Copy link
Copy Markdown
Contributor

Linux cross-test with 007 First Light (PPSA11386 v01.018.000) on an integrated stack, not this standalone head. Setup: CachyOS, RTX 3080, NVIDIA 615.71.09, GCC 16.2.1, Release, bubblewrap sandbox.

Stack. main 62f8758 plus:

Merge. I cherry-picked 4de5726, b9b83ce and ef218ef. There was one code conflict, in cachedStorageTexture, resolved as in the High on Life and Demon's Souls comments above: seeding from depth happens only when DepthSurfaceAt(context, words, viewed) matches.

Runs. First level load, same save and warm caches, one run each:

depth-storage errors in the log how the run ended
without this PR 33 × storage image access to depth surface … is not implemented (breakdown below) Guest segfault during the load. One run, so I'm not attributing it.
with this PR 0 The next stop, which is unrelated: registered program refers to an incompatible shader binary type. The tessellation draw wants type 5 (LS), and the code is registered as type 3 (merged LS+HS).

The 33 errors without this PR:

  • 28 × a 1024x256 vk format 37 (R8G8B8A8_UNORM) view of a 128x128 D16_UNORM/D32_SFLOAT or 256x256 D16 surface;
  • 3 × a 512x512, 10-mip vk format 122 view of a 128x128 D16 surface;
  • 2 × a same-extent vk format 100 (R32_SFLOAT) view of a 128x128 D16 surface.

The sampled-texture path catches 31 of them and keeps its snapshot. For the other 2, a color target stays non-resident. The 2 same-extent R32_SFLOAT views are the D16 case in DepthStorageReuse; the rest differ in extent.

Tests. On the #2904 build of this stack, the 13 targets matching agc_driver_(graphics|depth_|image_table|runtime|storage) pass, including agc_driver_depth_storage_reuse.

Not checked. The level doesn't render yet, so there's no visual check of these passes.

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* docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants