Repository navigation
fix(agc): copy a depth surface's plane through the texture's padded linear slices - #4873
Conversation
github-actions
Bot
added
needs rebase
|
Super Meat Boy 3D (PPSA31162, UE5) data point, Windows 11, RTX 3080 (610.74), WinLibs GCC 15.2.0 r7, Release. Without this PR (on The PR no longer merges cleanly: AI-assisted (Claude Code); I ran the above on my machine. |
|
Cross-reference before further review effort: #4722 (alfalfa-star) implements the same padded depth-plane copy, and its version is already aligned with Both PRs make the depth-plane copy handle linearly padded layouts with the same core mechanism — A plain rebase of this PR would therefore mean porting its region computation into |
32d213c to
2910741
Compare
|
@blawesom thanks for the cross-reference. I rebased this PR onto
A fix tested on only one of them could get the per-face offset right and the row length wrong, or the other way around. Both cases fail without the fix with The staging buffer grows whenever a larger layout shows up, keeping the previous one alive for copies already recorded, instead of refusing a larger layout after the first copy. Thanks also to @ps54all for the Super Meat Boy 3D (PPSA31162) run. |
#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>
|
Super Meat Boy 3D (PPSA31162, UE5) retest on the rebased head, Windows 11, RTX 3080 (610.74), WinLibs GCC 15.2.0 r7, Release: main The AI-assisted (Claude Code); I ran the above on my machine. |
…inear slices The plane copy laid its staging buffer out as tight slices while the detiler writes the guest's linear layout, which pads rows and slices: a 32x32 D16 cube in a 64 KiB swizzle has 16384-byte slices, not 2048, and a 128x128 one has 512-byte rows and 64 KiB slices. Each region in record now takes the layer's LinearLayerOffset plus the mip's linear offset and its pitch as the row length, for both the slices copied from a live depth surface and the ones detiled from guest memory. The staging buffer grows to the padded size, keeping the buffer it replaces alive for copies already recorded, and a mismatch says which geometry it saw.
A 32x32 D16 depth surface read as a six-face cube through a depth-bits R16_UINT view: its faces are narrower than a 64 KiB block, so the texture's linear slices are 16384 bytes, not the plane's 2048. Face 0 comes from the live surface, faces 1-5 from guest memory. The draw helper now sizes its color target with ColorTargetLayout so the small target is accepted.
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.
|
Rebased onto current |
2910741 to
86c6691
Compare
|
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.
|
5 participants
What
Follow-up to #425 for depth surfaces whose linear layout is padded.
DepthPlaneCopylaid its staging buffer out as tight slices, while the detiler writes the guest's linear layout, which pads rows and slices, so the copy refused these surfaces withdepth plane copy geometry does not match its layers. Two shapes of padding reach it:DescribeSurfacegives 16384-byte slices where the copy expected 2048: the padding is in the slice size;Changes:
record()takes the layer'sLinearLayerOffsetplus the mip's linear offset, and the mip's pitch as the row length, for both the slices copied from a live depth surface and the ones detiled from guest memory. The staging buffer grows to the padded size whenever a larger layout shows up, keeping the buffer it replaces alive for copies already recorded, and a mismatch says which geometry it saw.agc_driver_depth_plane_samplingsamples both cubes (face 0 from the live surface, faces 1-5 from guest memory). The draw helper sizes its color target withColorTargetLayout, since a linear 32x32 target is padded to a 64-pixel pitch. The 128x128 case is taken from fix(agc): copy depth planes whose detiled rows are padded to the swizzle block #4722 with its authorship kept.Overlaps #4722, which implements the same fix for the 128x128 case; this PR carries its test case too, so the maintainers can pick one. Rebased onto
mainafter #2490/#2491: the fix now lives inrecord()with itscopiedLayers/uploadLayerssplit.Tested
Linux, RTX 3050 Laptop (NVIDIA 615.78.08), on
maindacd785. Both new cases fail without the fix with the message above (the 32x32 case also on its own) and pass with it; thedepthctests andagc_driver_graphicspass, andtools/check_conventions.pypasses. Beast of Reincarnation (PPSA29343) gets past this copy on currentmain; it then stops at a separate refusal of a multi-layer depth storage view, which we are still working on. @ps54all also ran this PR on Super Meat Boy 3D (PPSA31162, Windows, RTX 3080): it reaches its main menu instead of stopping at this refusal.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)