Repository navigation
fix(agc): treat other-shaped views of a depth surface's memory as reused memory - #1495
Conversation
…sed memory Engines alias transient render targets, so a texture can sit at the address of a depth surface after the memory has been handed to a color image: a 384x384 R11G11B10 texture over a 384x384 D32S8 surface, say. Sampling it threw "sampling the depth plane ... is not implemented" and the draw was skipped. A view with another extent, or a depth-plane view in a format that cannot hold the plane (neither the plane's own R32F / R16 unorm nor a T# that reads the raw depth bits), is now treated as memory reuse: the lookup falls through to the ordinary texture path instead of the depth surface. Stencil-plane views keep their current handling.
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Good improvement to depth surface texture matching. This adds proper validation to ensure the depth format matches between the requested resource and existing depth surfaces, preventing incorrect texture reuse that could cause rendering issues.
Strengths:
- Clean implementation with proper validation of width, height, and depth format
- Good handling of both depth-only and stencil surfaces
- Proper use of ShaderRecompiler::DepthBitsTextureWidth for format detection
- Correct distinction between D16 and D32 formats
This will improve rendering correctness by preventing mismatched depth texture reuse. Ready to merge.
|
Another case this rule fixes, from Astro's Playroom (PPSA01325), Windows 11 / RTX 3060 Laptop:
The preference matters when both shapes are resident: a newest-first search can return the other one. That could be a follow-up; it's not needed for this PR. |
|
Another title that hits this: Hollow Knight: Silksong (PPSA12544) on Linux (RX 9070 XT), after lowering the in-game resolution scale. Before this PR: With this PR, that stop is gone. The same reuse then happens on the stencil plane, which this PR leaves as it was: The extent matches, but an R8G8B8A8 view cannot be a stencil read: const bool stencil = target.stencilAddress != 0 && resource.baseAddress == target.stencilAddress;
if (stencil && ResolveTextureFormat(resource.format) != VK_FORMAT_R8_UINT) return nullptr;I only ran it locally, so I'm leaving it here in case you want it in this PR rather than opening a separate one. Base for these runs: |
|
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.
|
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the fix, and for the title logs. The rule looks right to me, but it needs a test before it can merge:
- No test covers this. Fixes for a title-specific crash need a Linux test that reproduces the case (CONTRIBUTING: tests next to their component, registered in ctest). Suggested cases in the existing
libSceAgcDrivergraphics tests: a depth surface plus a texture at the same address with (a) a different extent, (b) the same extent but a colour format that cannot hold the plane (e.g. R11G11B10), (c) a 2D array view of a different extent (the Astro's Playroom case reported by @orbz22). Each must go through the ordinary texture path instead of throwing, and a texture of the surface's own extent and R32F/R16 must still hitDepthSurface::Sampled. The new tests must fail onmain. DepthSurface.cpp:178-179: please pin the "raw depth bits" exception (DepthBitsTextureWidth) with a test row too, so the!depthBitsbranch is covered both ways.- @hugodearaujo reports (Silksong, PPSA12544) the same reuse on the stencil plane with an R8G8B8A8 view, which this PR leaves as is. If you add the
R8_UINTcheck for the stencil plane here, add a test row for it as well. Otherwise it is a separate PR. - CI covers the build only; the title runs (PPSA16064 / PPSA01325 / PPSA12544) are not something we can reproduce, hence the test request.
…agc-depth-memory-reuse
agc_driver_graphics registers a D32 and a D16 surface on a mock device and looks up views at their addresses. Views of the surface's own extent in R32F or R16, and T#s that read the raw depth bits, sample the depth plane; a view of another extent, a 2D array view of another extent, an R11G11B10 view, an R32 uint view without a depth layout and an R32F view of a D16 surface return null and take the ordinary texture path. On main the first of these throws in DepthSurface::Sampled.
|
Thanks. I added
On current |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the test. Checked: the new cases pass, and they fail with DepthSurface.cpp reverted (negative check); libSceAgcDriver ctest 270/270 green on current main. Merging once CI is green.
…reuse fix(agc): treat other-shaped views of a depth surface's memory as reused memory
What
Engines alias transient render targets, so a texture can sit at the address of a depth surface after that memory has been handed to a color image: a 384x384 R11G11B10 texture over a 384x384 D32S8 surface, for example.
DepthSurfaceTexturesent every texture at a depth surface's address toDepthSurface::Sampled, which threwsampling the depth plane ... is not implemented, and the draw was skipped.Now a view with a different extent, or a depth-plane view in a format that cannot hold the plane, is treated as reused memory. "Cannot hold the plane" means it is neither the plane's own R32F / R16 unorm nor a T# that reads the raw depth bits. The lookup returns null, so the texture goes through the ordinary texture path, as it would at any other address. Stencil-plane views keep their current handling.
#425 rewrites
DepthSurfaceTexturefor array and cube depth reads but has no reuse rule. The two touch the same function, so whichever merges second needs a small rebase.Tested
Windows 11, NVIDIA RTX 5070 (driver 610.88).
... as a 384x384 texture of guest format 36 (vk 122), tile mode 7 ...) was skipped every frame. It now records, and that skip no longer appears in the log.ctest: 426 of 426 pass on currentmainwith this PR and fix(agc): prefer the pending image of exactly a sampled surface over an aliasing mip chain #1494 (unrelated files) applied.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)