Repository navigation
fix(agc): store a written storage image over a depth plane into the surface - #5113
Conversation
|
First Windows and Intel run. Setup: Windows 11 (26200), WinLibs GCC 15.2.0 r7, Release, CI configure flags, an RTX 4070 Laptop GPU (NVIDIA 595.97) and an Intel Raptor Lake-S iGPU (32.0.101.6790). The head dfc28bd merged into
Written with AI assistance. |
|
Such surfaces exist when one depth plane is bound with different stencil planes, because Both of these work on
An outside image takes the same loop, so it also lands in the oldest pairing. Repro: a copy of const DepthTarget first{address, address + BlockBytes / 2, {Side, Side}, VK_FORMAT_D32_SFLOAT_S8_UINT, 0.25f, 7};
const DepthTarget second{address, address + BlockBytes / 2 + 8192, {Side, Side}, VK_FORMAT_D32_SFLOAT_S8_UINT, 0.5f, 3};
DepthSurfaceView(context, first);
DepthSurfaceView(context, second);
const auto depth = DepthSurfaceStorage(context, {}, PlaneResource(address, Format32Float), 0); // second's view, reads 0.5
fill(*depth, 0.75f);
DepthSurfaceStoreBack(context, *depth);
// read back through DepthSurfaceStorage(...) again and count 0.75
auto layered = PlaneResource(address, Format32Float);
layered.dimension = TextureDimension::k2DArray;
DepthSurfaceStorage(context, {}, layered, 0); // replaces `depth` on the second pairing
DepthSurfaceStoreBack(context, *depth); // should throwResults (WSL2 Ubuntu, GCC 15.2, CI CMake configuration, Release, lavapipe):
A fix I tried locally: first run the identity and replaced checks on every surface, then run the for (const auto& surface : surfaces())
if (surface->context.device == context.device && surface->StoreBack(storage)) return true; // own view, or throw for a replaced one
for (auto it = surfaces().rbegin(); it != surfaces().rend(); ++it)
if ((*it)->context.device == context.device && (*it)->StoreBackCovering(storage)) return true; // the new CoversPlane loop, moved out of StoreBackWith that change (+8 lines), the PR's test and the repro both pass. AI-assisted: written with Claude Code; I checked the code paths and ran what is described above. |
|
@nikowolf123 thanks, you're right on both points. I pushed 6d1893f with your fix:
Your repro is now the
AI-assisted: yes |
#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>
…urface DepthSurfaceStoreBack took only the storage image a depth surface had handed out for its plane. A storage image over the same plane that the surface did not hand out (one cached before the surface existed, kept by a dispatch whose resources revalidate from their records) was written and never reached the depth image: later depth tests and plane reads used the surface's old contents. A written storage image that covers a depth surface's depth or stencil plane exactly (the plane's address and extent, level 0, one 2D layer, the plane's 32-bit float, 16-bit unorm or 8-bit uint format) is now copied into that plane when it is stored back, as the surface's own view is. A view the surface's own view replaced still throws. Uncharted: Legacy of Thieves Collection (PPSA05684) writes a 640x360 downsampled depth with a compute shader and then tests its mist particles against it as a depth target; the depth target stayed at its clear and the mist drew over foreground geometry.
…ts plane DepthSurfaceStoreBack walked the surfaces oldest first and let the first one whose plane the storage image covers take the copy. A depth plane bound with several stencil planes has one surface per pairing, so the copy could land in an older pairing while DepthSurfaceStorage, DepthSurfaceTexture and DepthSurfaceAt read the newest one, and a store back through a replaced view was accepted by an older pairing instead of throwing. The surface's own view and the replaced views are now checked on every surface first; only then does the newest surface covering the plane take the copy, the order DepthSurfaceStorage resolves a plane in. Reported with the repro and the fix by nikowolf123; the repro is the new RunPairings test. AI-assisted: yes
|
Rebased onto current |
6d1893f to
b3e5acc
Compare
What
DepthSurfaceStoreBackonly took the storage image a depth surface had handed out for its plane (DepthSurfaceStorage). A storage image over the same plane that the surface did not hand out was written but never reached the depth image, so later depth tests and plane reads used the surface's old contents. For example:Changes in
Graphics/src/DepthSurface.cpp:copyPlanetakes the storage image to copy from or to, instead of always the plane's own one.Test in
tests/execution/DepthStorage.cpp:StorageTexture::Create, filled with 0.125 and stored back, reaches the depth surface (read back through the surface's own storage view);The first check fails on
main.Uncharted: Legacy of Thieves Collection (PPSA05684) writes a 640x360 downsampled depth with a compute shader, then tests its mist particles against that memory as a depth target. The storage image was created before the depth surface, and the dispatch kept reusing it, so the depth target stayed at its clear value and the mist drew over the foreground cage in the main menu. On that title's branch, an equivalent local change (making such a written storage image the plane's writer, taken at the next bind) puts the mist behind the cage. That branch carries other depth work, so this PR's exact code was verified by the test, not in the title.
Related: #454 (retiring a surface when another writer covers it) and #5003 (2D array storage views of a depth plane) also touch storage access to depth planes. Neither stores an outside view back into the plane.
Tested
BUILD_TESTING=ON.guest_kernel_fcntl, which passed when rerun. Conventions check passed.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)