Repository navigation
fix(agc): drop stale depth-plane results whose guest memory was unmapped - #9406
Conversation
github-actions
Bot
added
unblocks title
|
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.
|
|
This needs a rebase: head Is the bug still on main? I applied only the PR's So the flush still tries to store the unmapped plane on main. In the mock it now fails on the storage-format check, before the Nit: the diff adds AI-assisted: yes (Claude Code); results above were run on this machine. |
ecb2220 to
8e6bbb3
Compare
|
Thanks, and for confirming the bug on main with the mock test. The PR head is now
|
|
Re-checked
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
Windows data point for
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The reported failure is reproducible in the mock regression, and the change places the unmapped-memory check before synchronization and storage. The author’s paired Vulkan validation-layer runs, title negative control, and independent Windows positive/negative runs are useful evidence. git diff --check also passes on the current head.
I’m requesting changes for the remaining repository requirements:
- The branch is still based on
0c6a5bd593, while currentmainis4a17c71757. Please rebase onto currentmain, resolve any conflicts, and refresh the focused AGC validation runs on that base. AGC changes require the same focused tests on main and the change with the validation layer and Vulkan-required environment, comparing VUIDs and naming the GPU. - The current overlap scan cannot compare this branch against #1368 (shared
DepthSurface.cppandDepthSurfaceReuse.cpp) or #1395 (sharedDepthSurface.cpp) because those branches conflict with current main. Both PRs modify the depth-surface implementation. Please manually establish whether the changed behavior overlaps, coordinate the order with their authors where it does, and updateDepends on:accordingly. The scan also identifies uncomparable shared debt-file edits; please refresh that check after rebasing and resolve any actual same-entry overlap with the authors.
The recorded Linux full-suite and title runs are on base 0c6a5bd593, not current main; the external Windows checks are valuable independent reports but do not replace the required refreshed paired validation. No local AGC test was run in this review environment: it has no /dev/dri device or existing build tree. Hosted product jobs on this PR are skipped; the conventions check, eligibility, labels, and report checks pass.
|
A second title for this fix, and it reproduces on demand: Hamster Playground (PPSA06652) 01.009.000, Unity. Linux 7.2.9, Radeon RX 6650 XT, RADV Mesa 26.2.4, GCC Release. Trigger: from a save that already has a house, Cross at 25/40 s clears the disclaimer, Down at 55 s moves to PLAY on "Select your house", Cross at 58 s loads the house. On Stack (GDB on The presenter thread only reports it afterwards ( Before/after: this PR's base
Two of the pairs come from a launch that restored the same save before every run. The third pair is from the run right after the one that created the house. With this PR the house loads: the in-game tutorial notebook over the 3D room, then the "Skip Tutorial" dialog. The log has this PR's warning in place of the stop: followed by four Logs and reports of all six runs, the two screenshots, and the GDB log with its script: 20261022-pr9406-PPSA06652.tar.gz (sha256 AI-assisted testing and report: yes, Claude Code. |
8e6bbb3 to
1855089
Compare
|
@MarcZX10 thanks. Addressed:
The title runs in the body are still from the earlier bases; Wobbly Life was not rerun on |
|
Windows Biped (PPSA24589), content version 01.000.002, 2026-10-22 retest for discussion #9376. Compared unmodified main
Both runs reused the same source assets and the same pre-run save snapshot, with isolated frozen runtime-library copies. The native game executable was identical. No game patch, metadata override or scripted controller input. Candidate minimum sampled available RAM was 5.14 GiB; available commit 21.79 GiB. Focused tests: Remaining: main-menu music is still absent; gameplay music works (verified with the PR). The earlier cutscene-audio issue was not rechecked. Save reload and full-game/long-term stability are not established by this bounded run. These mixed-scene FPS samples are not a performance comparison. The title was sitting in a menu midway through the candidate session; I then resumed gameplay. The total recording duration is not continuous gameplay duration. Near the end, I confirmed that gameplay after resuming remained responsive, without another freeze or visible graphics problem. AI-assisted: yes (Codex prepared the builds/logging and checked results; I controlled and visually tested the game). |
|
Linux data point for Head. Both tests pass 6 of 6 runs each (3 plain, 3 with Negative control. One note for anyone adding a validation-layer arm here: AI-assisted: yes (Claude Code); results above were run on this machine. |
|
Additional native crash-regression evidence: Outer Wilds (PPSA08102), Linux/RADV, same machine and the same three-file starting-save manifest in both arms.
The candidate's 330 s capture has a black “Wake Up” screen; at 510 s it shows an outdoor first-person scene with “Look Around” / “Move” prompts. That scene is very dark, so this establishes progress past the reproducible abort, not correct brightness or gameplay responsiveness. The test started from the saved checkpoint, sent Cross at 100 s and 170 s, and sent held Circle at 230 s and 330 s; the original runtime/save/configuration were restored afterward. This is one baseline and one candidate on the frozen |
|
Correction (please disregard the comparison below): the run without this PR and the runs with it were built on different Title data point (Windows, AMD Radeon RX 6400), PPSA25936, two 600 s runs per state; libs rebuilt for each state.
This is n=2 per state on one GPU, and the debt entry describes the stop as intermittent (an earlier run of this title on an RTX 4080 SUPER, run_115620, AI-assisted (Claude Code). |
|
Repeat of the PPSA25936 data point, with both states built from the same Build: A = Runs: PPSA25936, 600 s each. The stop counted is
n=2 per state and GPU shows that the stop is absent with this PR in these runs, not a rate; the debt entry describes the stop as intermittent. The earlier B run that ended in a game-side access violation (on AI-assisted (Claude Code). |
|
Tested on macOS with Planet of Lana (PPSA19450, 01.010.000, Unity), which hits this stop on every run about 8 s after choosing New Game on slot 1: A local print at the throw shows the same case the PR describes: a 4096x4096 D32 depth plane (tile mode 4, 64 MiB) at Apple M4 Pro, macOS 27.0.1, x86-64 under Rosetta 2, MoltenVK 1.4.2.
Three of the runs without the PR, and both with it, also carried a local print on the throw path (it only prints there). In the village after the intro, the run with this PR held 27.9 fps (p50 35 ms). An older macOS build from before #7154, which does not have |
1855089 to
f4c5435
Compare
|
Rebased onto main Full coherent build passes. Main and change each pass both Linux graphics mock query paths3x (6runs/arm), with expected exact-fixture negatives against the original guard in both paths. Matched RX6650XT/RADV hardware comparison passes5/5 per arm; Khronos loads in each3 hardware cases, Vulkan required/cacheoff. Both have the same existing independentBlend00605 VUID; no new VUID types, not a zero-error claim. Fresh conventions/whitespace and clean merge-tree against mainad19afc31e. #1368 and #1395 heads remain unchanged from the manual overlap review: neither edits FlushDepthPlanes; #1368’s fixture edit sets maxImageArrayLayers. Existing merge-order coordination is on #1368; no feature dependency was introduced and neither conflicted branch was integrated or claimed tested. The description retains attributed Windows and macOS reports on their original pins. Exact source pins, raw hardware/mock logs and negative controls. No fresh native-title or FPS claim. |
|
Tested on an Apple M2 Pro, macOS, x86_64 Release under Rosetta 2, MoltenVK 1.4.2, applied on top of #8658's
AI-assisted: yes (Cana) |
Dismissing a stale review: the requested changes were addressed after it (new head and/or updated description with the asked-for runs) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Re-checked |
What
A depth plane that a draw wrote is marked stale and stored to guest memory later, when something overlaps it or the device stores idle results. If the title has unmapped that memory in the meantime, the store creates a
StorageTextureover pages that are no longer committed, and the driver stops:The path is:
Wobbly Life (PPSA29413) hits this about 108 s in: it unmaps the memory of a 64 MiB 4096x4096 D32 depth target (64 KiB Z swizzle) after drawing to it, before the plane is stored.
FlushDepthPlanesnow checks for readable committed pages before storing a stale plane. A readable plane without write permission retains the existing unsupported-store failure and its pending results; it is not treated as unmapped. When none are left, it does not store the plane: the pending depth is dropped, the surface is no longer stale, and a one-time warning names the plane. The discard path is used only when the readable committed-range query is empty. The debt file records the behaviour, and what the console does with such a store is not known.Tested
Protected-memory regression correction. The former writable-only guard also discarded pending depth for a readable mapping protected against writes, and printed that it was unmapped. A CPU/mock probe using a real
PROT_READmapping confirms the difference: the pinned main rejects the store and keeps pending depth (rejected=1, pending=1); the original #9406 rebased onto that same main silently drops it (rejected=0, pending=0). The correction queries readable ranges before discarding. The new cross-platformTestReadOnlyStalePlaneFlushrequires the original rejection and pending state to survive. The unchanged extended mock fixture fails its specific rejection assertion with the original #9406 libraries and passes with the corrected depth implementation. These initial results use separately compiled objects relinked with the pinned main object set; they are supplemental CPU/mock evidence, not the coherent full-source Vulkan comparison. The coherent Release build also passes. Maina19e7f6fand corrected headf4c543528eeach passagc_driver_graphicsandagc_driver_graphics_maps_fallbackthree times (six runs per arm). The exact coherent corrected fixture fails the readable-protection assertion against the original rebased #9406 libraries in both query paths. These binaries use a mock device and do not establish Vulkan-layer coverage. Coherent paired hardware validation also passes on maina19e7f6fdc5475c538e2a9135aa45880503ef377and headf4c543528eb339d059f9472b02cae9475e5d81e8: five focused checks per arm (host-import mock, graphics mock, depth-plane partial store, depth storage and prepared exports). Vulkan required, shader cache disabled, actual Khronos insertion confirmed in each of the three hardware cases. Both arms have the same pre-existingVUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605; the change adds no VUID types. This does not claim zero validation errors. Conventions and whitespace pass; merge-tree is clean against newer mainad19afc31e, which was not the test baseline. Pinned raw hardware/mock logs, negative-control results and source review. A Windows exact-head mock check has been requested separately and is not claimed here.Independent macOS report, historical head. @imellag reports Planet of Lana PPSA19450 on Apple M4 Pro, macOS 27.0.1 under Rosetta 2 and MoltenVK 1.4.2. The tester identifies the same unmapped 64 MiB D32 depth-plane flush path with a backtrace and preceding memory-unmap trace. Four runs without this PR stopped; two with historical head
1855089a95reached gameplay and the 200-second limit, logging the drop warning. This was a macOS integration containing #8658 and several other patches, not stock main, and has not been rerun with the protected-memory correction. It is attributed external correctness evidence; no FPS gain or console equivalence is inferred.Independent same-base repeat, 2026-10-23: @xXPBSXx reports PPSA25936 on main
ba1b117cec6caa95fb6e0c46b4392baa0ce2d11cversus that exact main plus this PR head1855089a95e9ab865a4b50d952c6e72044e3f9c4(local merge1478b1c4642184969a9e5439e28f73402ece13bb). Two 600-second runs per arm on each of AMD Radeon RX 6400 and NVIDIA RTX 4080 SUPER: the storage-texture/no-committed-pages stop occurs in both main runs on each GPU and in neither change run on either GPU. The tester reports unchanged non-AGC libraries and relinker between arms. This is attributed external evidence, not a local rerun; n=2 per arm/GPU does not establish an intermittent-failure rate, other-title coverage, gameplay correctness, console equivalence or an FPS gain. The common base is deliberately older than the then-current main. This repeat replaces that tester's earlier comparison between different main revisions.Linux 7.2.9, Ryzen 9 7950X, Radeon RX 6650 XT, RADV Mesa 26.2.4, Release.
New test.
TestUnmappedStalePlaneFlushinagc_driver_graphics(DepthSurfaceReuse.cpp, mock device) draws to a mirrorable D32 plane at an unmapped address, then flushes it. It requires that the flush does not throw, stores nothing and clears the stale state.DepthSurface.cpprestored to main and the test kept: fails, because the flush throws (rechecked onb27924d851).Independent Linux regression report. On head
1855089a9merged onto main7e2ba73e4, Dextroid17 reports that bothagc_driver_graphicsandagc_driver_graphics_maps_fallbackpass 6/6 runs each; restoring main'sDepthSurface.cppwhile keeping the regression test makes both fail 6/6. This exercises both LinuxPROCMAP_QUERYand/proc/self/mapspaths. These focused binaries use a mock device and do not load the Vulkan loader or ICD; settingVK_INSTANCE_LAYERSfor them does not establish validation-layer coverage. The actual Vulkan comparison is the separate full-suite report below. These are independently reported results on CachyOS, not a local rerun (report).Full
ctest -j8. Run on mainb27924d851and on this change rebased onto it (1855089a95), withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(the loader inserted the layer; nowas not found but was requestedline),ANYPS5_REQUIRE_VULKAN=1andANYPS5_NO_SHADER_CACHE=1. Both runs have 1827 tests with the same 4 failures and the same 25 skips. The failures are:guest_named_shm_concurrency;agc_idle_publish_wait;agc_driver_image_array_window_past_layer_limit;agc_driver_tessellation(segfault).The
VUID-sets and counts are identical:VkWriteDescriptorSet-descriptorType-00337VkPipelineColorBlendStateCreateInfo-pAttachments-00605VkImageMemoryBarrier-image-03320VkImageViewCreateInfo-usage-02275vkCmdCopyBuffer-srcBuffer-00118With
DepthSurface.cpprestored tob27924d851and the test kept,agc_driver_graphicsfails under the same environment, because the flush throws. The same comparisons on the earlier bases0c6a5bd593(1813 tests) andb3c0f48bdd(1806 tests) also matched.PPSA29413, natively. Same scripted input (Cross every 15 to 20 s):
cecb540a630c6a5bd593The runs with the change reach the play menu, the tutorial prompt and the intro comic at about 120 FPS. Each printed the drop warning once for the title's 64 MiB depth plane, and none caused a GPU fault in the kernel log.
Rebase. Main's per-unit depth-plane stores (#9026) now store only the overlapped units. The check sits in that path, before
SyncFromMemory, and clears every stale unit of an unmapped plane. The PPSA29413 rows below are on the earlier bases; the title was not rerun onb27924d851.Overlap. #1368 and #1395 also change
DepthSurface.cpp, and #1368 changesDepthSurfaceReuse.cpp. Neither changesFlushDepthPlanesor handles a plane whose memory is unmapped: #1368 adds 2D-array depth storage and only setsmaxImageArrayLayersinRunDepthSurfaceReuseTests, and #1395 adds an alternative array-storage path. Both conflict with current main, so a merge with this PR was not tested; whichever lands second must keep this check and test. This PR does not depend on either.8e6bbb3a2dmerged onto current main:agc_driver_graphicspasses 3 of 3 and fails 3 of 3 with main'sDepthSurface.cpprestored. One ran on an RTX 5070 Ti and an AMD RDNA2 iGPU (comment); the other did not record its GPU (comment).Other titles (reports by others in this PR):
0c6a5bd593in 3 of 3 runs; see the comment for the runs with the PR (comment).Not verified:
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)🤖 Generated with Claude Code