Repository navigation
fix(agc): share a depth plane's contents across its stencil pairings - #4683
Conversation
github-actions
Bot
added
agc
b019187 to
35c4b5c
Compare
|
Combined source branch is prepared on current main43e3a70f8143f014f621c48abbdb137fffd7346e: https://github.com/Scramblrrrr/AnyPS5/tree/codex/depth-filter-plane-pairing-current-main , head9d5d0f94965dd100a52015abf78bec34349ddaf1. This retains both #4646 commits and author-preserved #4683 head35c4b5cde, resolving only DepthSurface.cpp conflicts. Negative admission check now lives at the start of planeSurface; both addresses are admitted before a newly created surface is constructed, and device cleanup keeps both key and owner-map removals. All #4683 pairing/copy tests and #4646 recorder lookup/CLI coverage remain. Source/diff-check/conventions pass. Not built/staged; native combined validation will follow after the separate007 test ends. No game/FPS claim, no competing PR. AI-assisted: Codex. |
|
Combined Windows validation completed for9d5d0f94965dd100a52015abf78bec34349ddaf1 on main43e3a70f8143f014f621c48abbdb137fffd7346e: both #4646 commits plus author-preserved #4683 35c4b5c. Fresh graphics/recorder target builds, RTX5070Ti/GCC15.2/Release/toolsOFF. Full mocked agc_driver_graphics PASS4.67s and native agc_depth_surface_lookup PASS0.55s,2/2 in5.56s, no skip. Production-only negative restores DepthSurface.cpp from preceding814a830e2 (filter kept, pairing fix removed), fixtures/CMake unchanged: graphics FAIL "a depth plane paired with another stencil plane must carry its depth over", native lookup control PASS,2-test total5.16s. Restore/touch/rebuild positive: graphics4.84s/native lookup0.35s,2/2 PASS5.24s. Source clean/diff-check/conventions pass. All both PRs' fixture/registration changes retained; address guard is in the common planeSurface helper before lookup/fallback scan, keys admitted before construction and cleared by device. Branch: https://github.com/Scramblrrrr/AnyPS5/tree/codex/depth-filter-plane-pairing-current-main . This is combined PR compatibility/native validation, not a game cross-test or real-GPU readback of pairing copies. None of these generic build artifacts were staged into007; its ongoing runtime stack remains separate. No FPS claim. AI-assisted: Codex. |
|
One integration concern before I carry this into007: that runtime stack also contains #454's last-writer retirement (headde90e241e, on #425), unlike the isolated current-main9d5d0f949 validation above. Binding a different overlapping depth target retires older surfaces; color/storage/CPU/driver reuse can retire them too. A compatible plane-pairing switch should carry the source aspect even if that source's whole surface was retired by the pairing switch. Conversely, a real color/storage/CPU overwrite must invalidate that plane's owner so an old depth image cannot be copied back over reused memory. Simply ignoring retired owners loses the first case; simply copying any matching owner risks the second. This is source-level reasoning, not evidence that #4683 fixes007's black scene, and the main-only fixture does not cover the combination. Have you cross-tested owner tracking with #454, or do you have a combined head to try? I can cross-test the pairing and last-writer controls on Windows. Until then, the public main+#4646+#4683 branch9d5d0f949 remains separate from the007 game stack. AI-assisted: Codex. |
A depth surface is keyed by its depth and stencil addresses, so a depth plane bound with different stencil planes became separate Vulkan images. Depth written through one pairing and HTILE fast clears consumed by one were invisible to the others, and later draws tested against stale depth. Track which surface owns the current contents of each depth and stencil plane address. Binding a plane through another surface of the same format, sample count and extent copies that aspect over, or inherits a pending clear. Sampling a plane address reads the surface that bound it last.
35c4b5c to
80d8bd0
Compare
|
Thanks for carrying it with #4646 and for the Windows runs. No, I have not cross-tested it with #454. My stack is main from 10 Oct plus #425, #757/#758 and #2076, without #454. I agree with both cases you describe. Here is how I would combine them; it is untested with #454:
In short, ownership follows the plane's memory, and #454's last-writer rule decides when an owner is no longer valid. If you have a combined head with #454 I can run it on my side. Note that PPSA05684 does not exercise #454's retirement, as far as I know. AI-assisted: yes |
|
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.
|
|
Checked on current main (merged cleanly): |
|
Rebase handoff — rebased onto current Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release,
To pick it up: Head is |
#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>
…pthSurface Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pthSurface Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5 participants
What
A depth surface is looked up by its depth address, stencil address, extent, format and sample count. When a title binds one depth plane with several stencil planes, each pairing gets its own Vulkan image. Depth written through one pairing, and an HTILE fast clear consumed by one, never reach the others, so later draws test against stale depth.
Changes in
Graphics/src/DepthSurface.cpp:vkCmdCopyImage. If the previous surface still has a pending fast clear for that aspect, the clear is carried over instead.TRANSFER_SRCusage for the copy.Related: #454 retires a depth surface once another writer covers its memory. It does not carry contents between surfaces that share a plane.
Test in
tests/DepthSurfaceReuse.cpp:It fails on
mainwith "a depth plane paired with another stencil plane must carry its depth over".Uncharted: Legacy of Thieves Collection (PPSA05684) binds the depth plane of its main menu scene with three stencil planes. Its tile-lighting compute writes a constant colour into sky pixels (depth == 1.0) and relies on the sky dome draw (depth test LEQUAL) to cover them. That draw tested against a stale image, failed everywhere, and the menu background showed a magenta gradient instead of the sky, cliff and fog. With this change the menu renders correctly. Frame rate is unchanged (8.5 fps with the change, 8.3 without).
Tested
BUILD_TESTING=ON.maindoes not run it yet).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)