Skip to content

fix(agc): copy depth planes whose detiled rows are padded to the swizzle block - #4722

Closed
alfalfa-star wants to merge 1 commit into
boykopovar:mainfrom
alfalfa-star:fix/agc-depth-plane-padded-layout
Closed

alfalfa-star wants to merge 1 commit into
boykopovar:mainfrom
alfalfa-star:fix/agc-depth-plane-padded-layout

Conversation

@alfalfa-star

@alfalfa-star alfalfa-star commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What

DepthPlaneCopy (from #425, now on main) refuses any depth plane whose linear layout was not tightly packed: geometry.sliceLinearBytes == sliceBytes(), where sliceBytes() is width × height × texel size.

Kena: Bridge of Spirits (PPSA01802) samples its point-light shadow maps as 128x128 D16 cubes, through an R16 view in a 64 KiB Z swizzle. That block holds 256x128 texels of 16 bits, so the detiled row is 512 bytes and a face is 64 KiB, while the copy assumed 256-byte rows and 32 KiB faces:

copy 128x128 D16 layers 6 slice 32768; resource 128x128 fmt 7 tile 4 dim 3 (cube); geometry layers 6 slice 65536 pitch 512

Every shadow projection draw that bound one was skipped, about 4,400 per 4-minute run. The scene was then lit as if no point light cast a shadow, and the image came out white or heavily overexposed.

  • The staging buffer is sized for the surface's linear layout, using the last face's LinearLayerOffset plus the first mip's pitch × height. It's reallocated, if the constructor's packed size is too small, only before the copy is first recorded; a later, larger layout is still refused.
  • Each face's region (in record, which only copies the layers that changed) uses LinearLayerOffset(layer) plus the mip's linearOffset, with bufferRowLength = pitch / texel size and bufferImageHeight = the mip's height. This now applies to both the slices copied from a live depth surface and the ones the detiler writes from guest memory, which already landed at LinearLayerOffset.
  • The geometry still has to match the copy's layer count, and a pitch narrower than the plane or a short mip still throws.

Tested

Arch Linux, KDE Plasma, GCC, Release, RADV (RX 9070 XT, Mesa 26.2.4), on main 0214069 (rebased onto main's reworked DepthPlaneCopy with kept guest layers).

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 in TechnicalDebt
  • 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 (feat(agc): depth planes sampled as 2D arrays, cubes and integer views #425 is merged)
  • AI-assisted: yes (Claude Code)

…zle block

A depth plane copy refused any surface whose linear layout was not tightly
packed. PPSA01802 (Kena: Bridge of Spirits) samples its point-light shadow
maps, 128x128 D16 cubes, through an R16 view in a 64 KiB Z swizzle. That
block holds 256x128 texels of 16 bits, so each detiled row is 512 bytes and
each face 64 KiB, while the copy assumed 256-byte rows and 32 KiB faces.
Every shadow projection draw was skipped, and the scene was lit as if
nothing cast a shadow.

The staging buffer now has the surface's linear layout. Each face lands
at LinearLayerOffset, with the row length and height of the first mip,
for both the slices copied from a live depth surface and the ones
detiled from guest memory. The buffer grows to that layout before the
copy is first recorded; a later, larger layout is still refused.

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

Copy link
Copy Markdown
Contributor

Cross-reference: #4873 implements the same padded depth-plane copy (same LinearLayerOffset/bufferRowLength region mechanism) against the older inline path. Since this PR already matches main's record() refactor and carries the Kena motivating case, it looks like the more current of the two — flagging the duplication so the maintainers can choose once rather than review twice. (This PR still shows needs-rebase for staleness; I can rebase it on request.)

GustavoGamarra95 pushed a commit to GustavoGamarra95/AnyPS5 that referenced this pull request Oct 13, 2026
Taken from boykopovar#4722 (Kena: Bridge of Spirits, PPSA01802): its point-light
shadow maps are 128x128 D16 cubes read through an R16 view in a 64 KiB Z
swizzle, whose block holds 256x128 texels of 16 bits, so each detiled
row is 512 bytes and each face 64 KiB instead of 256 bytes and 32 KiB.
Face 0 comes from the live surface, faces 1-5 from guest memory.
@Sanjaysrinivas Sanjaysrinivas mentioned this pull request Oct 13, 2026
11 tasks done
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>
@oneandonlydean oneandonlydean self-assigned this Oct 14, 2026
@alfalfa-star

Copy link
Copy Markdown
Contributor Author

@oneandonlydean #4873 now carries this fix. Since its rebase it sits in record() and uses the same per-layer LinearLayerOffset and padded bufferRowLength. It keeps this PR's Kena (PPSA01802) 128x128 D16 cube test, adds the 32x32 case from PPSA29343, and grows the staging buffer for larger layouts, so it covers everything this PR does. I'm fine with #4873 going in instead, and I'll close this one when it merges. Thanks @GustavoGamarra95 for keeping the Kena case.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 14, 2026
adriadam10 pushed a commit to GustavoGamarra95/AnyPS5 that referenced this pull request Oct 14, 2026
Taken from boykopovar#4722 (Kena: Bridge of Spirits, PPSA01802): its point-light
shadow maps are 128x128 D16 cubes read through an R16 view in a 64 KiB Z
swizzle, whose block holds 256x128 texels of 16 bits, so each detiled
row is 512 bytes and each face 64 KiB instead of 256 bytes and 32 KiB.
Face 0 comes from the live surface, faces 1-5 from guest memory.
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Closing in favour of #4873, which is now on main and carries this fix (per-layer LinearLayerOffset with the padded bufferRowLength) together with your Kena (PPSA01802) 128x128 D16 cube test, as you suggested. Thanks for the original fix.

MichalSy pushed a commit to MichalSy/AnyPS5 that referenced this pull request Oct 14, 2026
Taken from boykopovar#4722 (Kena: Bridge of Spirits, PPSA01802): its point-light
shadow maps are 128x128 D16 cubes read through an R16 view in a 64 KiB Z
swizzle, whose block holds 256x128 texels of 16 bits, so each detiled
row is 512 bytes and each face 64 KiB instead of 256 bytes and 32 KiB.
Face 0 comes from the live surface, faces 1-5 from guest memory.

(cherry picked from commit 2910741)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* docs needs rebase Conflicts with main, rebase needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants