Repository navigation
Conversation
|
Thanks, @JJenkx. Could you cover a second draw on the same unwatched stencil plane after the guest write, before retiring it? At For example, use the existing full-plane Please handle the new draw's ownership of an unwatched plane and add this sequence alongside the existing draw → guest write → retire case. Keeping both cases green would distinguish a guest write after the final draw from a guest write superseded by a later draw. Please also exercise the read-flush path, since it uses the same snapshot predicate. |
|
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.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@oneandonlydean Thanks, you're right. Fixed in Change. Every draw that marks the stencil stale now refreshes the unwatched-plane snapshot when the guest bytes no longer match it ( Tests.
Results (Windows, RX 6700 XT, AMD 26.10.1, validation layer on, through the shared GPU queue):
🤖 Generated with Claude Code |
✅ NVIDIA GPU test: passedPR head
Unstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (4 IDs in 495 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Follow-up on the runs the description said were blocked: long Cobra boots and the Goat cost pair. All are on the current head Space Adventure Cobra: The Awakening (PPSA17337), 300 s boots with scripted
Goat Simulator 3 (PPSA04158) cost, 3840x2160,
All arms with a trace have 23 presents. The two arms of this change straddle
The trace of this change's arms names commit 🤖 Generated with Claude Code |
e84452d to
191a429
Compare
|
Rebased onto Conflict: main's 6625bb5 ("retain pending stencil clears across sampling") moved the stale-stencil block this PR changed into
So a stale plane marked by an HTILE clear or plane fill now also gets the snapshot that keeps later guest writes. Tested on Windows: 11 Enterprise 22621, Radeon RX 6700 XT, AMD 26.10.1, MinGW-w64 GCC 15.2.0 UCRT r7, Release,
This PR's cases in AI-assisted: yes. |
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — approved
The stale-stencil store now leaves alone a plane the guest wrote after the last stencil draw. The new cases pass on the merged tree and fail with main's DepthSurface.cpp, and nothing I found blocks the merge.
What I ran
Head 191a4291bbefac61f96624ee13c025c4d7b1ce9b, main 4f41f4c375f5abb487dbd184fbc150f31994d762, merged tree 41e494af0edcc143b7b1cdd470c58cedd6b55b73. Debian 13 VM, 32 vCPU Xeon Gold 6136, GCC 14.2.0, Vulkan through lavapipe (Mesa 26.1.6, LLVM 19), no GPU.
git diff --check 4f41f4c375f5abb487dbd184fbc150f31994d762...191a4291bbefac61f96624ee13c025c4d7b1ce9b
python3 tools/check_conventions.py --base 4f41f4c375f5abb487dbd184fbc150f31994d762
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer \
ctest --test-dir build -V -R "agc_driver_(stencil_plane_memory|native_depth_uniform_planes)$"
# negative control
git show 4f41f4c375f5abb487dbd184fbc150f31994d762:core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
ninja -C build -j32 -l32 agc_driver_native_multisample_depth_tests agc_driver_stencil_plane_memory_tests
env VK_INSTANCE_LAYERS=... ctest --test-dir build -V -R "agc_driver_(stencil_plane_memory|native_depth_uniform_planes)$"
git show 41e494af0edcc143b7b1cdd470c58cedd6b55b73:core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
git status --porcelain
ninja -C build -j32 -l32 agc_driver_native_multisample_depth_tests agc_driver_stencil_plane_memory_tests libs
ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "stencil|depth_plane|depth_surface|native_depth"
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
Configure, build, libs (script) |
ok |
| Selected tests (script): 2162 run | 0 new failures, 0 timeouts; 7 known failures that also fail on main |
agc_driver_stencil_plane_memory, validation layer on |
Passed, 0 VUID- lines. It printed "guest memory is not write-watched here", so this machine runs the unwatched (snapshot) path the PR targets |
Validation new_vuid in agc_driver_native_depth_uniform_planes (script: "test not run on main") |
Not caused by this PR: with main's DepthSurface.cpp the test reports the same 4 VUID-vkCmdDispatch-groupCountX-00386 errors |
35 tests matching stencil|depth_plane|depth_surface|native_depth on the merged tree |
35/35 pass (1 skipped: agc_driver_native_depth_bda_clear) |
Validation run on the merged tree
1/2 Test #1504: agc_driver_stencil_plane_memory .......... Passed 0.36 sec
1504: guest memory is not write-watched here: a guest write into the stencil plane is not exercised
1504: stencil plane memory tests passed
1852: Validation Error: [ VUID-vkCmdDispatch-groupCountX-00386 ] | MessageID = 0x8e927036 (x4, same count with main's DepthSurface.cpp)
2/2 Test #1852: agc_driver_native_depth_uniform_planes ... Passed 0.37 sec
Negative control
I restored only core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp from main and rebuilt. agc_driver_stencil_plane_memory then fails on the first new case:
1504: AGC graphics: retiring the surface stored its stale stencil 0x3c over the guest's later write of 0x77
After that I restored the merged file and rebuilt. git status --porcelain printed nothing, and the 35 tests above ran on the restored build.
The negative control stops at the first failing Require, so it does not show on its own that the later cases (second draw after the guest write, read path) fail without the fix. The author reports running that check on the previous head in their reply to the review comment.
Findings
Blocking
None.
Non-blocking
-
Every stencil-writing bind now compares the whole plane, while the global locks are held, and the description's cost note is out of date.
- Where:
DepthSurface.cpp:736. The call is from the bind path atDepthSurface.cpp:2657, underGpuMutexandsurfacesMutex. - What changed: once the plane is already stale, every later
MarkStencilStale()on an unwatched plane runsEqualsCommittedUnsyncedover the full plane. At 3840x2160 that is 8 MiB per stencil-writing draw. - Why it matters: the description says "the copy is taken once each time a surface's stencil goes from stored to stale". That no longer describes the cost since the refresh commit, and since the rebase added the
NoteHtileClearandNotePlaneFillcallers. The Goat Simulator 3 numbers do not exercise this path, as the description itself says. A title with many stencil draws per frame on Windows would pay one plane-sized memcmp per draw. - Suggested fix: update the cost paragraph. Ideally, measure a stencil-heavy scene, or make the compare cheaper, for example by checking only when the plane may have changed since the last check.
- Where:
-
A partial guest write drops the whole stencil store.
- Where:
DepthSurface.cpp:2365-2379. - What happens: if the guest writes any byte of the plane (or, on watched memory, any 4 KiB page that overlaps it), the stencil is not stored anywhere in the plane.
- On the console, the bytes the guest did not touch would hold the drawn stencil. Here they keep the bytes from before the draw.
- For the freed-and-reused case this does not matter. For a live plane that the CPU patches in part, it is a wrong result.
- Suggested fix: on the unwatched path, store the stencil only where the guest bytes still equal the snapshot. Otherwise, record the approximation in
docs/dev/debt/libSceAgcDriver.md.
- Where:
-
The watched branch of
stencilWrittenSinceStaleis not tested.- Where:
DepthSurface.cpp:2366. - This changes behaviour on watched memory too. Before, the store always ran. Now it is dropped when
UnchangedSinceCollectedreports a collected CPU store sincestencilStaleGeneration. - Neither the author's Windows runs nor this Linux/lavapipe machine (the plane was not watched here) cover that branch, and the PR's new cases do not force it. The NVIDIA CI run passing is the only indirect evidence.
- Where:
-
Stray blank line. The PR adds an empty line before the closing brace of the watched-memory
elseblock inStencilPlaneMemory.cpp:211. It is unrelated to the change and can be removed.
On the earlier review comment: the second-draw case it asked for is now in the test, together with both read-path cases, and those cases pass on the merged tree here.
Not verified
- The watched-memory path (Linux or macOS with the plane write-watched). On this machine the test reports that the plane is not watched.
- Hardware GPUs: only lavapipe here.
- The title results (PPSA17337 crash, PPSA04158 timings): no titles are run in review.
- Whether each later new case fails on its own without the fix: the negative control stops at the first failing
Require.
✅ AMD GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (7, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 517 tests)Skipped (58)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Validation IDs already present on main (4 IDs in 517 tests)Skipped (48)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
A draw that writes stencil marks the surface's stencil plane stale; the plane reaches guest memory later, when a GPU write retires the surface or something reads the plane. That store wrote the stencil over the plane even when the guest had written it after the draw, typically after freeing the memory and reusing it. The mirror upload inside StoreStencilPlane takes the guest bytes at store time, so the guest's write looked like original data, and on Windows the driver no longer write-watches memory it uses as a surface. When the stencil goes stale the driver now records the tracker generation and, for a plane that is not write-watched, a copy of its guest bytes. Both store paths skip the store, and clear the stale state, when the guest wrote the plane since: UnchangedSinceCollected on watched memory, a compare with the copy otherwise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s a guest write On an unwatched plane the snapshot was taken only when the stencil first went stale, so a guest write followed by another stencil draw made the store compare against the older bytes and keep the guest write over the later draw. Each stencil draw now refreshes the snapshot when the plane's bytes no longer match it. The test covers a second draw after the guest write through retirement and through a read, and a guest write after the last draw through a read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
191a429 to
811f2d9
Compare
|
Rebased without merge commits onto current main Also addressed the non-blocking review notes: corrected the description to state that subsequent stale marks compare the entire unwatched plane while graphics locks are held; recorded whole-plane dropping after a partial CPU write in Same host/GPU main and change: 8/8 each, real Vulkan validation layer loaded, zero VUIDs and zero skips. With current main production code plus the new fixture, the negative control fails at the expected assertion: Commands, logs, source patches, range-diff and negative control — SHA-256 |
XairRrR
left a comment
There was a problem hiding this comment.
Re-checked — approved
What changed: a rebase onto main 04a9eb4fa2 that combines #15466's unmappedSince check with this PR's guest-write check in both stale-stencil store paths, a new debt entry for partial writes and comparison cost, an updated cost paragraph and the removal of the stray blank line. Checks and tests pass, and the negative controls fail as expected.
What changed since my last review
-
Rebase. The new base is
main04a9eb4fa2877eb6e40bf2c83adf972e40d89b24. Both store paths now drop the store whenunmappedSince(...) || stencilWrittenSinceStale(...):storeStaleStencilatDepthSurface.cpp:2846(this goes throughretireStencil);- the read path in
FlushDepthPlanesatDepthSurface.cpp:3240-3242.
MarkStencilStale()(DepthSurface.cpp:858-872) has the same logic as in191a4291bb. -
Commit
811f2d92e4.- Adds a Functional entry to
docs/dev/debt/libSceAgcDriver.md:125. It covers dropping the whole store after a partial CPU write, the page-granular drop on watched memory, and the full-plane compare under the graphics locks. - Removes the stray blank line from
StencilPlaneMemory.cpp.
- Adds a Functional entry to
-
Description. The cost paragraph now says that each later stale mark compares the whole plane. A partial-write paragraph was added.
What I ran
Head 811f2d92e488fb8b335be520ef9bc74d87074d68, main 04a9eb4fa2877eb6e40bf2c83adf972e40d89b24, merged tree cab8c66aeb43838dda1d4067e7d9b0850437e8cd. Debian 13 VM, Xeon Gold 6136, GCC 14.2.0, lavapipe (Mesa 26.1.6), no GPU.
git diff --check 04a9eb4fa2877eb6e40bf2c83adf972e40d89b24...811f2d92e488fb8b335be520ef9bc74d87074d68
python3 tools/check_conventions.py --base 04a9eb4fa2877eb6e40bf2c83adf972e40d89b24
# negative control 1: main's production file
git show 04a9eb4fa2877eb6e40bf2c83adf972e40d89b24:core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
ninja -C build -j32 -l32 agc_driver_stencil_plane_memory_tests
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer ctest --test-dir build -V -R "^agc_driver_stencil_plane_memory$"
# negative control 2: the first commit's DepthSurface.cpp, without the snapshot refresh
git show 15ef08b3edd9:core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
ninja -C build -j32 -l32 agc_driver_stencil_plane_memory_tests
ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -V -R "^agc_driver_stencil_plane_memory$"
# restore
git show cab8c66aeb43838dda1d4067e7d9b0850437e8cd:core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp > core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp
git status --porcelain
ninja -C build -j32 -l32
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 VK_LOADER_DEBUG=error,warn,layer ctest --test-dir build -V -R "^agc_driver_stencil_plane_memory$"
ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "stencil|depth_plane|depth_surface|native_depth|image_gather_volume"
# script-flagged VUIDs: merged tree vs main's DepthSurface.cpp
env VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest --test-dir build -V -R "^agc_driver_image_gather_volume$"
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
Configure, build, libs (script) |
ok |
| Selected tests (script): 2173 run | 0 new failures, 0 timeouts. The 7 known failures also fail on main |
agc_driver_stencil_plane_memory, validation layer loaded |
Passed, 0 VUID- lines. The test reports that the plane is not write-watched, so the snapshot path runs |
41 tests matching stencil|depth_plane|depth_surface|native_depth|image_gather_volume |
40 pass, 1 skipped (agc_driver_native_depth_bda_clear) |
Script new_vuid in agc_driver_image_gather_volume (VUID-VkSamplerCreateInfo-addressMode*, "test not run on main") |
Not from this PR: main's DepthSurface.cpp gives the same 8 lines (2 each of addressModeU-01079 and of addressModeU/V/W-parameter). These are sampler-creation errors, unrelated to stencil stores |
Negative control
main'sDepthSurface.cpp:agc_driver_stencil_plane_memoryfails withretiring the surface stored its stale stencil 0x3c over the guest's later write of 0x77.- New this time:
DepthSurface.cppfrom the first commit15ef08b3ed, without the snapshot refresh: the test fails withretiring the surface kept the guest's 0x44 over the stencil 0x55 a later draw wrote. This shows on its own that the second-draw case needs the refresh commit. My previous review could not show this.
After both runs I restored the merged file and rebuilt. git status --porcelain printed nothing.
Previous findings
- Whole-plane compare per stencil-writing bind; cost note out of date. Resolved as a documentation finding.
- The description now states that every later stale mark compares the complete plane under the graphics locks, about 8 MiB at 3840x2160. The debt entry at
libSceAgcDriver.md:125records it. - The cost itself is unchanged and unmeasured on a stencil-heavy scene. The description says so plainly.
- The description now states that every later stale mark compares the complete plane under the graphics locks, about 8 MiB at 3840x2160. The debt entry at
- A partial guest write drops the whole stencil store. Resolved by recording it as debt in the same entry. Bytewise merging is listed as not implemented.
- The watched branch of
stencilWrittenSinceStaleis untested. Still open, non-blocking.DepthSurface.cpp:2837still runs only where the plane is watched.- The author's Linux/RADV refresh and this machine both report that the plane is not watched. The author states this in the description.
- Stray blank line in
StencilPlaneMemory.cpp. Resolved. The watched-memoryelseblock atStencilPlaneMemory.cpp:205-210now closes without it.
New findings
Blocking
None.
Non-blocking
None. The rebase resolution looks correct to me:
- In
storeStaleStencil, a guest-written plane now goes throughretireStencil. That clearsstencilStaleand callsforgetStaleStencil, as the earlier explicit branch did. - The read path clears
stencilStalein the same branch thatmainuses for an unmapped plane.
Not verified
- The watched-memory path (previous finding 3).
- Hardware GPUs: this machine has only lavapipe. The author's Linux RX 6650 XT run and the archive with its logs are author-reported. I did not download the archive.
- Title results (PPSA17337, PPSA04158): reviews do not run titles, and the description claims no new title result for this head.
|
Windows title check on the current head, plus an update that weakens the title evidence for this PR on current Space Adventure Cobra (PPSA17337) is the title whose heap crash led to this change. Same 300 s boots as my earlier comparison, Windows / RX 6700 XT, plain library sets with only the build changed:
So on current AI-assisted: yes. |
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (7, not counted against this PR)Validation IDs already present on main (8 IDs in 531 tests)Skipped (60)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (8 IDs in 531 tests)Skipped (50)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
@JJenkx some data for the Linux, write-watched side, which the description lists as not run. The title is Hollow Knight: Silksong (PPSA12544). Setup. Arch Linux 7.2.11, RX 9070 XT (RADV GFX1201, Mesa 26.2.6), Release. Base: The stop. It's the stale stencil store you describe. With dynamic resolution, depth targets from 1748x982 to 1920x1080 share stencil addresses in the title's 16 MiB heaps. An HTILE fill on the current target marks the older surfaces' planes stale. Their planes (0x1c0000-0x280000 bytes) are then stored over memory the allocator has reused. The title faults in its allocator (
Why the first-boot route still crashes. On watched memory the check compares against the generation of the last stale mark, and an HTILE clear or bind re-arms that mark every frame. I ran a trace build on this PR's code. In 1,478 of the stale-plane stores it logged, the plane's memory had been written since the plane was last synced from memory, but not since the last stale mark ( What worked here. A local variant compares against the plane's last sync instead: Test coverage. I also made a fixture for I'm not opening a competing PR: the fix belongs in this one. If the last-sync threshold works for you, I can post the diff or push the variant to a branch, whichever is easier. Not judged: other titles, Windows, frame time (the screens matched between arms at the same points of the route). |
|
Thanks for the paired route evidence and the distinction between the last stale mark and the last sync. Please post the variant diff or a branch link, including the shared-memfd fixture. I will review it in this PR and add a focused regression that writes the plane after its last sync, re-marks it stale, and then attempts a store. The existing fixture passing on both versions does not cover that ordering. I have not reproduced the first-boot route locally yet, so I am treating the title results above as your measurements, rather than claiming that this head fixes that route. |
…last mark A stale stencil plane is dropped instead of stored when the guest wrote its memory. The comparison was against the memory at the plane's latest stale mark, and HTILE clears and rebinds re-mark a plane without writing it, so a guest write before the latest mark was taken as the plane's own bytes and the stale plane was stored over it. The baseline is now the last store, or a mark that comes from a stencil write or a stencil plane fill. On write-watched memory the store checks WrittenSince against the generation of the last sync or store. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@hugodearaujo thanks for the trace, which showed exactly where it went wrong. Pushed The change.
Test. The new case in
The plane must still hold
Limits.
AI-assisted: yes. |
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (7, not counted against this PR)Validated but not compared (56, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 538 tests)Skipped (58)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (1, not counted against this PR)Validation IDs already present on main (13 IDs in 548 tests)Skipped (48)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Windows recheck of head Setup: Windows 11 Enterprise 22621, WinLibs GCC 15.2 UCRT, Release, Radeon RX 6700 XT, AMD driver 26.10.1, run with
AI-assisted: yes. |
|
@JJenkx thanks for folding it in. I ran both things you asked for on Linux, with your current head. Setup. Arch Linux 7.2.11, RX 9070 XT (RADV GFX1201, Mesa 26.2.6), Release. The base is the same as in my first comment: First-boot route. This is the Silksong route that crashed 3 of 3 times on
Watched fixture. I added the shared-memfd fixture ( If you want the fixture in this PR, I can post its diff (about 100 lines in |
|
@hugodearaujo Thank you, that is the result I needed: the first-boot route that crashed 3 of 3 on Yes, please post the fixture's diff here. It covers the write-watched branch on Linux, which none of my cases reach. I'll add it as its own commit with you as co-author, re-run AI-assisted: yes |
|
@JJenkx here it is. It applies cleanly to
On your head, with the validation layer, it passes on Linux with no For the trailer: diff --git a/core/libs/prx/libSceAgcDriver/tests/execution/StencilPlaneMemory.cpp b/core/libs/prx/libSceAgcDriver/tests/execution/StencilPlaneMemory.cpp
index b83c2ad95f..9af5921e84 100644
--- a/core/libs/prx/libSceAgcDriver/tests/execution/StencilPlaneMemory.cpp
+++ b/core/libs/prx/libSceAgcDriver/tests/execution/StencilPlaneMemory.cpp
@@ -13,7 +13,9 @@
#endif
#include <windows.h>
#else
+#include <fcntl.h>
#include <sys/mman.h>
+#include <unistd.h>
#endif
#include <algorithm>
#include <array>
@@ -147,6 +149,101 @@ void CheckGuestWriteBeforeRepeatedClears(AgcDriver::VulkanDevice& device, const
Require(AgcDriver::Graphics::FlushDepthPlanes(stencilBase, StencilBytes).empty(), "a stencil plane the guest wrote after its last store stayed stale");
}
+#ifdef __linux__
+struct SharedPlanes {
+ int file = -1;
+ std::uintptr_t begin = 0;
+ std::size_t bytes = 0;
+ std::uint64_t offset = 0;
+};
+SharedPlanes sharedPlanes;
+
+bool ResolveSharedPlanes(std::uintptr_t address, std::size_t bytes, int* file, std::uint64_t* offset) {
+ if (sharedPlanes.file < 0 || bytes == 0 || address < sharedPlanes.begin || address - sharedPlanes.begin >= sharedPlanes.bytes || bytes > sharedPlanes.bytes - (address - sharedPlanes.begin)) return false;
+ *file = fcntl(sharedPlanes.file, F_DUPFD_CLOEXEC, 0);
+ *offset = sharedPlanes.offset + (address - sharedPlanes.begin);
+ return *file >= 0;
+}
+#endif
+
+void* AllocateSharedBlocks(std::size_t bytes) {
+#ifdef __linux__
+ const int file = memfd_create("stencil-plane-memory", MFD_CLOEXEC | MFD_ALLOW_SEALING);
+ Require(file >= 0, "cannot create the shared depth and stencil planes");
+ Require(ftruncate(file, static_cast<off_t>(bytes + Block)) == 0 && fcntl(file, F_ADD_SEALS, F_SEAL_SHRINK) == 0, "cannot size the shared depth and stencil planes");
+ void* raw = mmap(nullptr, bytes + Block, PROT_READ | PROT_WRITE, MAP_SHARED, file, 0);
+ Require(raw != MAP_FAILED, "cannot map the shared depth and stencil planes");
+ const auto begin = reinterpret_cast<std::uintptr_t>(raw);
+ const auto aligned = (begin + Block - 1) & ~static_cast<std::uintptr_t>(Block - 1);
+ if (aligned != begin) munmap(raw, aligned - begin);
+ if (aligned + bytes != begin + bytes + Block) munmap(reinterpret_cast<void*>(aligned + bytes), begin + Block - aligned);
+ sharedPlanes = {file, aligned, bytes, aligned - begin};
+ GuestArena::GuestArenaSetSharedBacking_nid_postfix(ResolveSharedPlanes);
+ void* block = reinterpret_cast<void*>(aligned);
+ GuestWriteWatch::GuestWriteWatchRegister_nid_postfix(block, bytes);
+ GuestAllocations::Mutation().Add(block, bytes, true, true, true);
+ return block;
+#else
+ return AllocateBlocks(bytes);
+#endif
+}
+
+void CheckReusedStencilPlane(AgcDriver::VulkanDevice& device, const DepthFastClearHarness::Shaders& shaders, std::uint32_t swizzle) {
+ auto* blocks = static_cast<std::byte*>(AllocateSharedBlocks(DepthBytes + StencilBytes));
+ auto* depth = reinterpret_cast<float*>(blocks);
+ auto* stencil = reinterpret_cast<std::uint8_t*>(blocks + DepthBytes);
+ std::fill(depth, depth + DepthBytes / sizeof(float), 1.0f);
+ Fill(stencil, 0u);
+ const auto stencilBase = reinterpret_cast<std::uintptr_t>(stencil);
+ const Surface surface{reinterpret_cast<std::uintptr_t>(depth), stencilBase, 0, false, VK_FORMAT_D32_SFLOAT_S8_UINT, 0, 0, swizzle};
+ const auto draw = [&](std::uint8_t reference) {
+ Require(Draw(device, shaders, surface, {.depthTest = false, .stencilTest = true, .stencil = Stencil(VK_COMPARE_OP_ALWAYS, VK_STENCIL_OP_REPLACE, reference)}, PlaneExtent, Colors) == PlaneTexels, "a draw writing stencil over the reused plane did not cover it");
+ };
+ draw(0x18u);
+ Require(!AgcDriver::Graphics::FlushDepthPlanes(stencilBase, StencilBytes).empty(), "a read of the shared stencil plane after a draw stored nothing");
+ device.WaitIdle();
+ Require(Holding(stencil, 0x18u) == StencilBytes, "the stencil drawn into the shared plane did not reach guest memory");
+ if (!AgcDriver::GuestMemory::Watched(stencilBase, StencilBytes)) {
+ std::puts("the shared stencil plane is not write-watched here: a guest reusing the plane's memory is not exercised");
+ return;
+ }
+ constexpr std::size_t RecordBytes = 0x30;
+ constexpr std::array<std::size_t, 3> RecordOffsets{0x1000, 0x7dd0, StencilBytes - RecordBytes};
+ const auto writeRecords = [&](std::uint8_t value) {
+ for (const auto offset : RecordOffsets) std::fill(stencil + offset, stencil + offset + RecordBytes, value);
+ AgcDriver::GuestMemory::BumpCollectEpoch();
+ };
+ const auto recordsHold = [&](std::uint8_t value) {
+ return std::all_of(RecordOffsets.begin(), RecordOffsets.end(), [&](std::size_t offset) { return std::all_of(stencil + offset, stencil + offset + RecordBytes, [&](std::uint8_t byte) { return byte == value; }); });
+ };
+
+ draw(0x66u);
+ Fill(stencil, 0x77u);
+ const bool storedOverFill = !AgcDriver::Graphics::FlushDepthPlanes(stencilBase, StencilBytes).empty();
+ device.WaitIdle();
+ Require(Holding(stencil, 0x77u) == StencilBytes, "a read of a stencil plane the guest wrote over after a draw stored the stencil drawn before the write over the guest's bytes");
+ Require(!storedOverFill, "a read of a stencil plane the guest wrote over after a draw stored the stencil drawn before the write");
+
+ draw(0x2bu);
+ writeRecords(0x7au);
+ AgcDriver::Graphics::RetireDepthSurfaces(device.graphicsContext().device, surface.depth, DepthBytes);
+ device.WaitIdle();
+ Require(recordsHold(0x7au), "retiring the surface stored the stencil drawn before the guest wrote records into the plane over them");
+ Require(AgcDriver::Graphics::FlushDepthPlanes(stencilBase, StencilBytes).empty(), "a read of the stencil plane whose retired surface's stencil was dropped stored it");
+
+ draw(0x4du);
+ writeRecords(0x3cu);
+ static_cast<void>(AgcDriver::Graphics::FlushDepthPlanes(surface.depth, DepthBytes + StencilBytes, AgcDriver::Graphics::DepthPlaneScope::Whole));
+ device.WaitIdle();
+ Require(recordsHold(0x3cu), "a whole read of the planes stored the stencil drawn before the guest wrote records into the plane over them");
+
+ writeRecords(0x5eu);
+ draw(0x18u);
+ Require(!AgcDriver::Graphics::FlushDepthPlanes(stencilBase, StencilBytes).empty(), "a read of the stencil plane drawn after the guest's records stored nothing");
+ device.WaitIdle();
+ Require(Holding(stencil, 0x18u) == StencilBytes, "the stencil drawn after the guest wrote records did not reach guest memory over them");
+}
+
}
int main() {
@@ -255,6 +352,8 @@ int main() {
Require(!AgcDriver::Graphics::FlushDepthPlanes(stencilBase, StencilBytes).empty(), "a read of the stencil plane drawn after the unmap stored nothing");
device->WaitIdle();
Require(Holding(stencilPlane, 0x18u) == StencilBytes, "the stencil drawn after the unmap did not reach guest memory");
+
+ CheckReusedStencilPlane(*device, shaders, swizzle);
std::puts("stencil plane memory tests passed");
return 0;
} catch (const std::exception& error) { |
Back the planes with a shared memfd, imported as a dma-buf and write-watched, so the watched branch runs on Linux. Co-authored-by: hugodearaujo <175634979+hugodearaujo@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Added @hugodearaujo's fixture as Arch Linux 7.2.11, RX 6650 XT (RADV Mesa 26.2.6), Release: AI-assisted: yes |
✅ NVIDIA GPU test: passedPR head
Failing on main too (4, not counted against this PR)Validation IDs already present on main (9 IDs in 587 tests)Skipped (52)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Title data point for this PR: STEINS;GATE RE:BOOT (PPSA36591) on Linux (Ubuntu 26.04, RTX 4070 Ti, NVIDIA 615.71), the GPU Heap corruption I reported on #15350. Repro: a new game, Cross every ~0.12 s through
Earlier AI-assisted: yes (Claude Code) |
|
@milescoviello thank you, that's a useful confirmation that the reused-memory case occurs in a real title, with the stencil plane stores traced to the corrupted chunk. I've added your Steins;Gate runs to the description's Tested section, linking your comment. AI-assisted: yes |
What
A depth surface's stencil plane reaches guest memory only when something needs it: it is marked stale when a draw writes stencil, and stored later. One later trigger is a GPU write that retires the surface (
RetireDepthSurfaces,RetireCoveredDepthSurfaces); another is a read of the plane (FlushDepthPlanes).That store wrote the stencil over the plane even when the guest had written the plane after the draw, typically because the title had freed the memory and reused it for something else.
The mirror upload inside
StoreStencilPlanetakes the guest bytes at store time, so the guest's later write is already in the "original" bytes. The storage write-back therefore cannot tell that write apart from stale stencil data.On Windows the driver stops write-watching memory it uses as a surface, so the tracker does not see that write either.
Space Adventure Cobra: The Awakening (PPSA17337) crashes this way. Unity's graphics-thread allocator keeps stencil and depth targets in its heap. After the title frees one, the allocator's free-list links in the block are overwritten by the old stencil plane, and the next allocation dereferences them. In a diagnostic build that logged every guest write, the last write over the corrupted links before each crash was a write-back with reason
stencil plane store.How it works now:
The comparison point is the plane's last store, or the last stencil-writing draw or stencil plane fill. HTILE clears and rebinds that mark the plane stale again do not move it, so a guest write before them still counts. On an unwatched plane the guest bytes are copied at that point; a later stencil draw takes a new copy and so supersedes an intervening CPU write.
Before storing a stale stencil plane, both store paths check whether the guest has written the plane since that point. On watched memory this uses
CollectWritesUncachedandWrittenSinceagainst the generation of the last sync or store; otherwise the bytes are compared with the copy.If the guest wrote the plane, it keeps its bytes: the store is dropped and the stale state cleared. Otherwise the plane is stored as before.
#14475 also changes
DepthSurface.cpp, but only for depth-plane clears superseded by a storage write; it touches neither stencil store path.Tested
A title it unblocks, reported by @milescoviello (comment). STEINS;GATE RE:BOOT (PPSA36591), Ubuntu 26.04, RTX 4070 Ti (NVIDIA 615.71), new game with fast text skip, 900 s runs. On
mainthe title's GPU Heap is corrupted ([SceLibc] A heap error is detected, then a SIGSEGV in its allocator) in 5 of 5 runs one322066f82(333 to 413 s) and on9a4bcc3018(329 s). With this PR merged onto9a4bcc3018(6096b477de), two runs reached the 900 s limit with no heap error. In a tracedmainrun, the last two write-backs over the corrupted chunk werestencil plane stores of two depth targets at that address: the reused-memory case this PR handles.Linux re-check of the current head by @hugodearaujo (comment). Arch Linux 7.2.11, RX 9070 XT (RADV GFX1201, Mesa 26.2.6), Release,
main1c8c0efc8c+ #8932 with this PR's four commits up toc1fe457b32. Hollow Knight: Silksong's first-boot route (calibration and profile screens, 420 s, same save and inputs), which crashed 3 of 3 on811f2d92e4, crashes 0 of 3 onc1fe457b32; all three runs reached the limit at 600 presents per 10 s. A shared-memfd fixture of theirs that runs the write-watched branch passes next toCheckGuestWriteBeforeRepeatedClearswith the validation layer inserted and noVUID-line.Windows follow-up for the re-marking report (2026-11-04). @hugodearaujo found on Linux, in Hollow Knight: Silksong (PPSA12544), that an HTILE clear or bind re-marks a plane every frame, and that the previous comparison point (the last stale mark) moved with it. In 1,478 of the stores they traced, the guest had written the plane after its last sync but before its last mark, and the store overwrote it; their first-boot route crashed 3 of 3 with the previous head. Head
c1fe457b32(one commit on811f2d92e4) compares with the last store or stencil write instead. It also covers the unwatched path, where the re-mark re-took the snapshot after the guest write.agc_driver_stencil_plane_memorygets a new case: a draw writes stencil0x18and a read stores it, the guest writes0x77, two HTILE fills over the surface re-mark the plane, and a read follows. The plane must still hold0x77. Windows 11 Enterprise 10.0.22621, AMD Radeon RX 6700 XT, AMD proprietary driver 26.10.1, WinLibs GCC 15.2.0 UCRT r7, Release, through the shared GPU queue withANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1and the Khronos validation layer 1.4.363 inserted. Each arm is merged ontomainb11c7bf84f:mainb11c7bf84f(main's own fixture)811f2d92e4with the new fixtureNo test was skipped. As before on Windows, the fixture's watched-only section reports "guest memory is not write-watched here" in both the
mainand the fix arm, so the Windows run covers the snapshot path; the new case does not depend on write-watching. The watched path follows the reviewer's variant (last sync), which ran 0 of 6 first-boot crashes and 0 of 8 gameplay crashes in their Linux runs; I have not reproduced the title route. The Windows run is a single FIFO job,f878bf6e29d3483887c1799b4d02579c.Earlier Linux refresh (2026-11-03). Head
811f2d92e488fb8b335be520ef9bc74d87074d68is three commits on main04a9eb4fa2877eb6e40bf2c83adf972e40d89b24with no merge commits. The rebase preserves main's unmapped-plane serial/invalidation handling from merged #15466 and adds the guest-write predicate to both stale stencil store paths. The second-draw snapshot-refresh commit is unchanged apart from its parent; the final commit documents partial-write behavior and comparison cost and removes the stray test blank line.On the same Ryzen 9 7950X / Radeon RX 6650 XT (RADV NAVI23) Linux host, main and the change each pass all eight focused stencil/depth cases with
ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1, and the realVK_LAYER_KHRONOS_validationinserted: zero VUIDs, zero skipped tests, unchanged sources/libraries throughout each arm. Exact selection and commands are in the evidence archive. The fixture reports that these planes are not write-watched, so this is snapshot-path coverage; the watched branch remains unverified on this host.The negative control uses current main's production code with this PR's updated
StencilPlaneMemory.cppfixture. See the linked result for the expected stale-store corruption assertion. It stops at the first failing assertion, so it does not independently prove that every later new case fails on main. No new title, Windows, or frame-time result is claimed in this refresh.Current Linux commands, logs, patches, source pins and negative control (SHA-256
b03befd0d94a1cc1608146652b63eeb7e62641d2e60d576f94e6bedf6ce8f204).Earlier Windows and title measurements follow with their original revisions.
Windows 11 Enterprise 10.0.22621, AMD Ryzen 9 5950X, AMD Radeon RX 6700 XT, AMD proprietary driver 26.10.1, WinLibs GCC 15.2.0 UCRT r7, Release. All runs went through the shared GPU queue with
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1.agc_driver_stencil_plane_memorygets a new case: a draw writes stencil0x3c, the guest then writes0x77over the stencil plane, and a GPU write over the depth plane retires the surface. The plane must still hold0x77, and a later read must store nothing. The case runs whether or not the plane is write-watched.main93dbd0f82awith only this test added: fails with "retiring the surface stored its stale stencil 0x3c over the guest's later write of 0x77".This change: passes 3/3 with no VUIDs. The test's existing guest-write-over-a-stored-plane section still runs only where the plane is write-watched, and is not exercised on Windows.
Full GPU suite at
ac9b10b80a(this PR's commit), with the validation layer inserted 1084 times: 3234/3282 pass and 19 skip.All 27 stencil and depth tests pass, including
agc_driver_stencil_plane_memory,agc_driver_stencil_replay,agc_driver_depth_surface_from_storageand theagc_driver_depth_plane_*set.The VUIDs are the same three
mainreports.main0cc3899f69, run the same way, fails the same 28 tests: 26video_out_flip_*, which returnVK_ERROR_SURFACE_LOSTbecause this machine's display was off during both runs, plusagc_driver_sample_order_grid_referenceandngs2_pitchshift.The only difference was
agc_driver_pipeline_journal, which segfaulted once in a validation-layer worker thread on this change. It then passed 3/3 and 20/20 on this change and 3/3 and 20/20 onmain, and this change does not touch that code.Space Adventure Cobra: The Awakening (PPSA17337). Each boot started without a save and ran 120 s with scripted
crosspresses.A diagnostic build of
mainthat skipped every stale stencil store ran 5/5 without a crash. That build is not part of this PR.Boots of unchanged
maincrashed on the allocator corruption at 20–90 s. That includes 2 of 5 on0580e5c951and 2 of 3 withAPS5_NO_HOST_IMPORT=1.Matched boots, five each, in one session through the shared FIFO (all five fix boots ran first, then the five
mainboots):| Libraries | Boots | Crashed |
|---|---:|---|
| this change (
da1cee2240, same content as the PR commit) | 5 | none (120 s each) ||
main93dbd0f82a| 5 | 1, at 21.1 s, the allocator fault atapp.exe+0x17f7e33|Five boots per side is a small sample, but it is consistent with the diagnostic build and the test above. Longer
mainboots crashed every time (89 s and 212 s). Two 300 s boots of heade84452d263both ran to the cap without a crash (follow-up).Cost. On memory that is not write-watched, each store and each stencil-writing bind or stencil plane fill copies the complete plane, and each store compares it. These operations hold the graphics locks. At 3840x2160 each full-plane comparison reads about 8 MiB. The title timings below do not isolate or establish the cost of a stencil-heavy workload.
Partial-write limitation. A CPU write after the last draw drops the entire pending stencil store. Untouched bytes keep their earlier guest contents instead of the drawn stencil; on watched memory, a write anywhere on an overlapping tracked page can trigger that drop. Bytewise merging is not implemented; this is recorded in
docs/dev/debt/libSceAgcDriver.md.Goat Simulator 3 (PPSA04158) at 3840x2160, with
APS5_TRACE_SECONDS=600:4only and the log not redirected: this change gave p50 181.9 ms (22 presents) andmain93dbd0f82agave p50 178.7 ms (23 presents). These are one capture each; three more arms were lost, two to early title crashes before the window and the rest to the display being off. That is within this scene's run-to-run spread (178.9–185.5 ms onmain), so it is not a measured cost claim. On heade84452d263, two more arms of this change (p50 177.5 and 180.4 ms) straddle one validmainarm (178.3 ms); the othermainarm crashed before the window (follow-up). Still not a cost claim either way.A diagnostic Goat Simulator 3 run that logged every storage-image write-back recorded none, so the Goat numbers show only that nothing else regressed. They may not exercise the copy. The copy is exercised in Cobra, which was not traced.
Pins. The tests and boots ran on
93dbd0f82a, asda1cee2240, then asac9b10b80awith the same content. The PR head is that patch rebased ontomain4569fe654awithout conflicts (git range-diff: identical), and CI builds it.e84452d263then adds a refresh of the snapshot at each stencil draw, after review; its tests are in the review reply. The current head191a4291bbis both commits rebased ontomain4f41f4c375, wheremainhad moved this block intoDepthSurface::MarkStencilStale(). The logic now lives there, and the 7 stencil and depth-plane GPU tests pass 3 of 3 (see the rebase comment).Independent report: @TechLevelZero's NVIDIA CI (RTX 2000 Ada, driver 595.99.02) on head
e84452d263merged ontomain88e804b: 2,494 passed, 0 new failures.Not run: Linux and macOS, where the planes are write-watched and the existing generation check applies; other titles.
Checklist
Based on current
main; no other open PR implements the same functionsOne 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 under Silent stubs in their library's technical debt fileNo notes, investigation
.mdfiles 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
Fixes: none
Unblocks: none
GPUs tested: AMD Radeon RX 6650 XT / RADV NAVI23 on Linux for the current refresh; AMD Radeon RX 6700 XT, AMD proprietary driver 26.10.1, Windows 11 10.0.22621 for the historical runs below
AI-assisted: yes
🤖 Generated with Claude Code