Repository navigation
Upstream PR batch 1 U8b: Metal depth, MSAA and fill-kernel correctness - #484
Merged
Merged
Conversation
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
…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
… 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
…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
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7af80f366
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| depthSurfaces.push_back(depth); | ||
| } | ||
| // A color target over a depth surface's memory writes where its planes were. | ||
| for (const auto& color : state.colors) RetireDepthSurfaces(depthSurfaces, color.address, color.bytes, depth.get()); |
There was a problem hiding this comment.
When a multisampled color target overlaps a resident depth plane only in its second or later fragment allocation, this range covers just one fragment because ColorTarget::bytes is computed from width, height, and element size without samples. The same file's MetalColorSurface::OverlapsMappings correctly uses target.bytes * target.samples; use that full logical footprint here too, otherwise the depth surface remains resident and later texture reads can return stale depth data.
Useful? React with 👍 / 👎.
Comment on lines
+681
to
+684
| RetireDepthSurfaces(depthSurfaces, target.address, Graphics::DepthSliceBytes(target.extent, d16 ? 2u : 4u) * target.samples); | ||
| if (target.stencilAddress != 0) { | ||
| RetireDepthSurfaces(depthSurfaces, target.stencilAddress, Graphics::DepthSliceBytes(target.extent, 1u) * target.samples); | ||
| } |
There was a problem hiding this comment.
For a layered depth draw, this only retires surfaces overlapping the first slice. The later-slice loop at lines 696–703 removes surfaces only when their depth address exactly equals that slice's address, so a previously resident target whose plane begins inside a later slice (or overlaps only its stencil plane) survives and can subsequently serve stale texels. Apply the same depth/stencil range retirement to each generated slice before adding it.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This was referenced Oct 14, 2026
RINNECODER
added a commit
that referenced
this pull request
Oct 14, 2026
…es, Metal depth retirement, null degamma images) (#498) * fix(relinker): decode scalar compares behind address-size and segment prefixes A scalar compare carrying an address-size override (67 C5 FA C2 ..., 67 F3 0F C2 ...) or with F2/F3 on either side of another legacy prefix was not decoded, so it stayed in its Rosetta block and could still lose the later load of its destination. Legacy prefixes are now read in any order, the last F2/F3 being the mandatory one; F2/F3 before a VEX escape is still not a compare. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(agc): retire depth surfaces over every MSAA fragment and every layered slice on Metal A multisampled color target now retires the depth surfaces over all of its fragments (bytes * samples), as MetalColorSurface::OverlapsMappings already measures it. Each extra slice of a layered depth draw retires the surfaces over its depth and stencil planes, not only one at exactly its depth address. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(recompiler): skip null images before FORCE_DEGAMMA validation A null T# (TYPE=0 or zero base) reads zero whatever its other words say, so a null image left with an RGBA8_UINT format under a FORCE_DEGAMMA sampler no longer fails the one-slice-volume check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(agc): pin a swizzled T# sampled through the FORCE_DEGAMMA sRGB view An R/B-swapped UNORM and sRGB texture sampled under FORCE_DEGAMMA keeps its mapping once. Measured: a Metal view of a view takes its swizzle relative to the root texture, so passing the parent's swizzle is right and an identity swizzle drops the mapping. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(agc): keep a stencil-only target's absent depth plane absent in its layered slices The extra slices of a layered draw offset the depth address even when it is zero (a stencil-only target), and the range retirement then treated that fabricated address as a depth plane. DepthArraySlice leaves an absent plane at zero. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
RINNECODER
added a commit
that referenced
this pull request
Oct 15, 2026
A multisampled depth or stencil plane was taken to span DepthSliceBytes(extent) * samples: the single-sample 128x128 (or 256x128, 256x256) block padding times the sample count. A 64 KiB swizzle block holds 64 KiB / (bytes per texel * samples) pixels, so a 4x 1920x1080 D32 plane pads to 64x64 blocks: 33.4 MB, not 35.4 MB. Since depth surfaces are retired when another depth target is bound over their planes (#484), the overstated extent reached a single-sample depth target the title allocates right after its 4x scene depth. Binding that target retired the 4x scene depth surface, so later passes that sample scene depth (fog of war, water, atmosphere) read stale guest memory and washed the 3D map out in white and pale blue. DepthPlaneBytes(extent, bytes per texel, samples) gives the plane size with the sample count's block dimensions (equal to DepthSliceBytes for a single sample) and replaces DepthSliceBytes * samples in the depth surface overlap, retirement and mapping checks. Test: metal_utility_tests (DepthResource.mm, multisampledDepthPlane) checks the plane sizes and that a single-sample target 0x1FF0000 bytes after a 4x 1920x1080 one retires neither; it fails without this change ("Binding a depth target after a multisampled one retired it"). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj
RINNECODER
added a commit
that referenced
this pull request
Oct 15, 2026
A multisampled depth or stencil plane was taken to span DepthSliceBytes(extent) * samples: the single-sample 128x128 (or 256x128, 256x256) block padding times the sample count. A 64 KiB swizzle block holds 64 KiB / (bytes per texel * samples) pixels, so a 4x 1920x1080 D32 plane pads to 64x64 blocks: 33.4 MB, not 35.4 MB. Since depth surfaces are retired when another depth target is bound over their planes (#484), the overstated extent reached a single-sample depth target the title allocates right after its 4x scene depth. Binding that target retired the 4x scene depth surface, so later passes that sample scene depth (fog of war, water, atmosphere) read stale guest memory and washed the 3D map out in white and pale blue. DepthPlaneBytes(extent, bytes per texel, samples) gives the plane size with the sample count's block dimensions (equal to DepthSliceBytes for a single sample) and replaces DepthSliceBytes * samples in the depth surface overlap, retirement and mapping checks. Test: metal_utility_tests (DepthResource.mm, multisampledDepthPlane) checks the plane sizes and that a single-sample target 0x1FF0000 bytes after a 4x 1920x1080 one retires neither; it fails without this change ("Binding a depth target after a multisampled one retired it"). Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upstream PR batch 1, unit U8b: Metal depth, MSAA and fill-kernel correctness. Three of the eighteen upstream PRs are ported, one commit each; the other fifteen are skipped, each with a reason below. A fourth commit adds only a TechnicalDebt entry.
No performance claims: not measured.
Refs upstream-pr-batch-1
Ported
TextureFormat.cppandRdnaDescriptorFormat.cpptables. Metal has no signed RGB10A2 pixel format, soMetalTexturestill refuses them, now naming the format instead of failing the shared lookup. Added to TechnicalDebt.MetalDepthSurfaceCacherecords which surface last bound each depth plane address and each stencil plane address. Binding a single-sample surface (no DB_DEPTH_VIEW slices) copies a plane from that surface when the format and extent match. For Depth32Float_Stencil8 the copy takes only that plane's aspect, through a buffer. Multisampled surfaces and depth-array slices keep their own planes.MetalDrawbinds a color target, it drops from the draw's resident depth list every depth surface whose planes the target overlaps. Binding a depth target moves that surface to the end of the list and drops the other surfaces its planes overlap. The cache keeps them, so binding the target again brings the surface back. Upstream's storage-image rule and CPU/driver-store rule are not ported: our path has no storage view of a depth plane and no write tracking for this cache. Added to TechnicalDebt.Skipped
DepthPlaneCopy. Metal has no such copy. It samples a resident plane through a texture view, and a depth array throughGatherDepthSlices(layer views or blits), so no guest linear layout is involved.DepthPlaneCopygather. Metal has no such check. Note: Metal cannot sample a depth cube over resident surfaces at all (residentViewthrows for any non-2D view). That is a separate, larger gap.MetalDraw::residentView, so there is no store-back to fix.Graphics/MetalorExecution/Metal).Execution/src/Driver/Memory/BufferFill.cpp, which the Rosetta/Metal build does not compile. Metal runs these kernels as real compute dispatches, so their stores land in guest memory. Upstream needs them recognized for its HTILE/DCC-key fast paths, which Metal does not model.MultisampleTargetfor FMASK decompress. NeitherMultisampleTargetnor a CMASK model exists on our path. Treating the pass as a no-op on Metal would hide a missing CMASK clear, so the pass still goes through the existing unsupported-mode path.MultisampleTargetchain (boykopovar#4820, boykopovar#2132) that our tree does not have. Metal already serves such a texture from a zero-cleared resident surface (documented inMetalDraw::residentTexture) and does not model the guest MSAA footprint that reading the memory would need.StorageTexturepending-slice tracking. Metal presentation is separate. Not measured.TechnicalDebt (
docs/dev/TechnicalDebt.md): boykopovar#5149 and #454 each add a Metal entry. #454's upstream entry describes Vulkan behaviour, so it is replaced by the Metal one. The fourth commit,docs(agc): record the Metal depth-plane views and storage writes not modeled, records the gap behind the skipped boykopovar#4722, boykopovar#4873, boykopovar#5054, boykopovar#5113 and boykopovar#5446. A resident Metal depth surface is served only as a single-level 2D view or a 2D array of its extent. In a dispatch, a storage image over its plane is bound to guest memory, and a new surface starts cleared. The upstreamdocs/dev/debt/*hunks of boykopovar#5446, boykopovar#5039, boykopovar#4603 and boykopovar#4790 belong to skipped PRs and are not carried.Evidence
metal_utility_tests(arm64,-DANYPS5_METAL_SHADER_BRIDGE=ON -DANYPS5_METAL_GUEST_REPLAY=ON) passes. It has new cases:DepthResource.mmsharedPlanes, for D16S8 and D32S8: second stencil pairing, the same pairing again, back to the first, the stencil plane with another depth plane, a pairing of another extent. Without the fix(agc): share a depth plane's contents across its stencil pairings boykopovar/AnyPS5#4683 change it fails with "Sampled depth plane returned incorrect GPU values".DepthResource.mmretiredDepth: overlap edges, the surface being bound is kept, the cache keeps a retired surface, a surface of another extent at the address.TextureResource.mm: formats 51 and 55 resolve, are 4 bytes and not block compressed, and a native texture refuses them by name.unsupported guest texture format 51.takePlanesdisabled givesSampled depth plane returned incorrect GPU values.RetireDepthSurfacesdisabled givesA color target inside a depth plane did not retire its surface.-DANYPS5_METAL_UTILITIES_ONLY=ON -DANYPS5_METAL_SHADER_BRIDGE=ON -DANYPS5_BUILD_METAL_SHADER_BRIDGE_TESTS=ON -DANYPS5_METAL_GUEST_REPLAY=ON):nice -n 10 ctest --parallel 4passed all 50 tests on e7af80f. The branch is up to date withorigin/main2b4606b.anyps5_metal_guest_draw_replay,anyps5_metal_msaa_depth,anyps5_metal_color_{format_view,eqaa,msaa_color,msaa_read,cb_resolve}.MetalDrawwiring for Upstream sync 7: kernel, libc and small libs #454 (bind order, color and depth retirement) has no draw-level test. Only the rule itself (RetireDepthSurfaces) is tested directly.rosetta_libs rosetta_engine) passesrosetta_direct_graphicsandmetal_utility_tests.tools/smoke.sh, 300 s with--map, on AC power). It reached the same screens as main: main menu at 162 s or earlier, leader select by 178 s, map generation by 235 s. There was no fault and no uncaught exception, and the first unimplemented import is the same as on main (an Ajm Opus initialize). The last frames were 97% non-black. No log lines mention incompatible depth views, unsupported state or exceptions, the same as main's baseline run.🤖 Generated with Claude Code