Repository navigation
fix(agc): track CPU writes in partly watched depth planes - #11692
Conversation
github-actions
Bot
added
the
unblocks title
|
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.
|
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Linux run on NVIDIA and lavapipe, since the description covers RADV only. Head 949306303c on main b986c11a34 (patch applies cleanly). Ubuntu 26.04, GCC 15.2, Release. ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1, -j1. lavapipe selected with VK_ICD_FILENAMES=.../lvp_icd.json.
The four tests from the description, in the same three arms:
| RTX 3070 (NVIDIA 595.99.02) | lavapipe (Mesa 26.0.3) | |
|---|---|---|
main + the two new tests only |
2/4: cpu_write_after_memo and cpu_write_after_partial_unwatch fail (a depth flush overwrote CPU descriptors written after a memoized collect) |
2/4, same two |
+ #11690's DepthSurface.cpp line |
3/4: cpu_write_after_partial_unwatch still fails |
3/4, same |
| this head | 4/4 | 4/4 |
This matches the RADV result: the per-unit fallback is what fixes the partial-watch case, and #11690 alone does not.
Full ^agc suite (without agc_display_buffer_dcc, agc_driver_flip, agc_driver_lds_atomics64, --timeout 30) with #11692 and #11691 applied together on main: 1041/1042 on both devices. The one failure on each also fails on unmodified main on this machine, with the same message: agc_driver_pipeline_journal on the 3070 (the oversized record was not counted as a write failure; it passes when only the NVIDIA ICD is visible), and agc_driver_multisample_retirement on lavapipe (MSAA storage promotion did not retain its source image).
GPUs tested: NVIDIA GeForce RTX 3070 (proprietary 595.99.02, Linux), lavapipe (Mesa 26.0.3, LLVM 21).
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed 949306303cd7b7ed30e1ff62678d552dea941e4a. Changes requested for one remaining partial-coverage case.
[P2] Preserve writes in the watched pages of a partly watched unit — DepthSurface.cpp, DepthSurface::WrittenUnits. The fallback retries each unitBytes region as one collection. Write-watch coverage is page-granular: GuestMemory::Unwatch accepts page-sized ranges, and GuestWriteWatch::Watch::Collect rejects a range unless its entire span is covered (takeFresh(..., whole=true))). If a 4 KiB page is unwatched inside a 64 KiB depth unit, the plane scan fails and the retry for that unit also returns 0, even when the CPU-written descriptor is in an adjacent page that remains watched. Because the fallback only calls WrittenSince` when the retry returns nonzero, that CPU write is not marked changed and a subsequent stale depth store can overwrite it.
Please scan the covered page/runs within each unit (preserving the existing behavior for genuinely unwatched ranges) and add a regression that unwatches one page within a unit, writes CPU descriptors into a still-watched page in that same unit, then verifies both descriptor preservation and retained GPU depth. The current test excludes a whole unit, so it does not exercise this case.
This finding is based on the current write-watch coverage/collection implementation and the reviewed fallback path; I did not independently run the GPU reproduction in this environment.
|
Addressed the partial-page coverage case in f415222 and f3e7f47, appended to the reviewed head without rebasing or merge commits.
The new
Selection: Paired logs, results and follow-up patch, SHA256 Latest upstream refresh (2026-10-27): private integration Fresh-main source composition, focused results, trace/digest and reviewed X11 screenshot, SHA-256 |
f3e7f47 to
1646ca2
Compare
|
@MarcZX10 The page-coverage follow-up is retained on the synchronized branch and now has the qualification below. Please re-review the updated head and clear the earlier request if this satisfies it. Current-main refresh, 2026-10-27 10:18 UTC pass: exact head Linux Release, RX 6650 XT/RADV Mesa 26.2.4, required Vulkan and shader caches disabled. Matched main passes all three existing controls three times (9 passes). Candidate passes six groups × three repetitions (18 passes), then 18 after restoration. The exact within-unit page-hole case additionally passes three times with APS5_NO_DIRECT_DEPTH_COPY=1. Replacing only production with its pre-page-correction parent fails exactly that page-hole case; the other five controls pass. That negative control uses normal mode; the forced-buffer variant is a positive qualification, not a separate negative control. The within-unit regression places a 4 KiB unwatched hole and CPU descriptors in an adjacent watched page of the same 64 KiB unit, checking descriptor preservation and retained GPU depth. Every individual physical execution proves RX 6650 XT selection and actual Khronos instance/device-layer insertion; zero skips, VUIDs or missing-layer failures. Restored production source matches the candidate, and the rebuilt AGC driver is byte-identical to its initial candidate. The forced-buffer setting is recorded in the executed harness/process environment and linked to the startup flag; CTest does not print every inherited variable. Conventions and whitespace pass. This is synthetic GPU regression evidence, without a new title, console, Windows or performance run. Earlier contributor results retain their original exact pins. Exact source/dependency pins, initial/negative/restored raw logs and JUnit, forced-buffer receipt, independent reviews, hashes and service-owned FIFO/PTY receipts. SHA-256: AI-assisted: yes. |
github-actions
Bot
added
build
agc
|
Windows run of
The five that pass are
An untested idea, if the case should run here: make the hole by unwatching the whole unit and rewatching the pages around it with Negative control: with AI-assisted: yes (Claude Code) |
|
Windows + NVIDIA result for head Host: Windows 11 26300, Intel i7-12700K, NVIDIA GeForce RTX 2070 SUPER (vendor 0x10de, device 0x1e84; Vulkan loader 1.4.357 from the NVIDIA driver), Khronos Validation Layers nightly 1.4.366 through The check is So this is the test's assumption rather than the library change: on Windows the page-unwatch mode should either skip (as the modes already do when Validation: same focused run on AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
…larity On Windows, GuestMemory::Unwatch excludes every page of each 64 KiB block it touches, so unwatching one page of the descriptor unit left the whole unit unwatched and the "descriptor page lost write watching" check failed. Unwatch the unit and return only its descriptor page with Rewatch there, which is page-granular, so the unit is partly watched on every platform. Linux keeps the single-page Unwatch. Diagnosed by maskym on Windows 11 26300 with an RTX 2070 SUPER. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the diagnosis, it's right. On Windows
Checked here (Windows 11 Enterprise 10.0.22621, WinLibs GCC 15.2.0 UCRT r7, Release,
Not run here: the depth-plane Vulkan tests. Their GPU runs on this machine are paused by our shared GPU queue. Could you rerun AI-assisted: yes (Claude Code). |
|
Windows rerun of the new head
Negative control, now a real red: with AI-assisted: yes (Claude Code) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
New head AI-assisted: yes (Claude Code). |
|
Retested head Host: Windows 11 26300, Intel i7-12700K, NVIDIA GeForce RTX 2070 SUPER (vendor 0x10de, device 0x1e84; Vulkan loader 1.4.357 from the NVIDIA driver 621.01), Khronos Validation Layers nightly 1.4.366 through
AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Linux run at this PR's exact head Arch Linux, kernel 7.2.9, Ryzen 9 7950X, AMD Radeon RX 6650 XT (RADV NAVI23, Mesa 26.2.4), g++ (GCC) 16.2.1, Release,
I didn't run the new test against the base to show it failing there. |
|
Checked |
What
A depth plane can contain fully watched units, partly watched units and pages excluded from write tracking. A failed whole-plane scan previously left CPU-repurposed bytes vulnerable to a stale depth store.
WrittenUnitsnow retries whole units first, then page-aligned fragments inside partly watched units. Writes in covered pages invalidate that depth unit; genuinely unwatched pages keep the existing behavior. Initial covered pages are collected before recording their generation, including for single-unit surfaces.Regressions cover both a wholly unwatched neighboring unit and a 4 KiB hole inside the descriptor's own 64 KiB unit. They write CPU descriptors after a depth draw and check descriptor preservation plus GPU depth retained in an untouched unit. On Windows, where
Unwatchexcludes whole 64 KiB blocks, the within-unit case instead unwatches the unit and returns only the descriptor page with the page-granularRewatch, so the unit is partly watched there too (c581723557, after maskym's Windows run).Built on #11690; merge it first. Shared-fixture overlap with #11685 is the same argument/include addition discussed there; retain its
--buffer-copymode alongside all CPU-write modes. No dependency on its direct-copy implementation.Tested
Current-main refresh, 2026-10-27 10:18 UTC pass: exact head
1646ca2842119411060c4db6304e47dd18a21fd8on main3fea2a937ba9fbab2f06c51691fe0c5763659aa3. Five child topic commits remain stacked on refreshed #11690. The existing within-unit page-coverage correction and its regression are preserved exactly; no genuinely unwatched page is falsely marked written. All submodule pointers match that main.Linux Release, RX 6650 XT/RADV Mesa 26.2.4, required Vulkan and shader caches disabled. Matched main passes all three existing controls three times (9 passes). Candidate passes six groups × three repetitions (18 passes), then 18 after restoration. The exact within-unit page-hole case additionally passes three times with APS5_NO_DIRECT_DEPTH_COPY=1. Replacing only production with its pre-page-correction parent fails exactly that page-hole case; the other five controls pass. That negative control uses normal mode; the forced-buffer variant is a positive qualification, not a separate negative control. The within-unit regression places a 4 KiB unwatched hole and CPU descriptors in an adjacent watched page of the same 64 KiB unit, checking descriptor preservation and retained GPU depth.
Every individual physical execution proves RX 6650 XT selection and actual Khronos instance/device-layer insertion; zero skips, VUIDs or missing-layer failures. Restored production source matches the candidate, and the rebuilt AGC driver is byte-identical to its initial candidate. The forced-buffer setting is recorded in the executed harness/process environment and linked to the startup flag; CTest does not print every inherited variable. Conventions and whitespace pass. This is synthetic GPU regression evidence, without a new title, console, Windows or performance run. Earlier contributor results retain their original exact pins.
Exact source/dependency pins, initial/negative/restored raw logs and JUnit, forced-buffer receipt, independent reviews, hashes and service-owned FIFO/PTY receipts. SHA-256:
4787a341e92b9f1764de6e6bc11f7ae5af8d8e21ff5c75f8f71483c60742d798; downloaded copy verified.Independent validation by jotitadev of the earlier head
949306303con mainb986c11a34: RTX 3070 / NVIDIA 595.99.02 and lavapipe / Mesa 26.0.3. The four-test main/test-only arm passes 2/4, adding #11690 passes 3/4, and the full earlier head passes 4/4 on both devices. This confirms the original per-unit fallback; it does not validate the subsequent within-unit page-hole revisionf3e7f47391c, whose author-run RADV evidence is reported above.Latest reviewer follow-up: exact final head
f3e7f47391cpasses all five depth-plane tests on Linux / Radeon RX6650XT / RADV Mesa26.2.4 with real Khronos validation in every process, zero VUIDs, no missing layer and no skips. Before the fix, the new within-unit page-hole regression failed while the four existing controls passed; that baseline's productionDepthSurface.cppis identical to reviewed head949306303cd. Paired logs, full commit IDs and follow-up patch. Native follow-up: mainfe2b52c7e43plus this page fix and the published Bendy prerequisites (#11690, #11704, #6782, #6913), private headd53a4b60cc1, passes16 focused gates and reloads the standing room with camera input into the doorway view; ordinary native exit0, no VUIDs, all writable state restored. Native evidence and reproducible composition. No FPS gain, later-gameplay or audio claim.Earlier qualification:
Arch Linux; AMD Radeon RX 6650 XT, Mesa RADV 26.2.4-1. Four identical focused GPU tests on the same GPU/build setup, with
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationandctest -V:abed0354680plus the two new regressions (e96256dade0): existing default/unimported controls pass; memo and partial-watch regressions fail (2/4 pass).5325ea8e726): memo and existing controls pass; partial-watch regression still fails (3/4 pass).949306303cd): all4 pass, including preservation of untouched GPU depth.Every arm has four actual Khronos insertions, zero VUIDs, no missing layer and no skipped tests. Selection:
ctest --test-dir build -R '^(agc_driver_depth_plane_partial_store|agc_driver_depth_plane_partial_store_unimported|agc_driver_depth_plane_cpu_write_after_memo|agc_driver_depth_plane_cpu_write_after_partial_unwatch)$' -Vtools/check_conventions.py --base origin/main --head codex/depth-partial-watch-20261027passes;tools/pr_overlap.py --branch codex/depth-partial-watch-20261027was run and shared depth-source changes were checked for duplicates. Windows/other GPU vendors not tested. No FPS or loading-speed claim.Bendy and the Dark Revival PPSA27624 v01.000.003: the frozen diagnostic integration at
6e6be0d8c4dstill exited134 after a fresh whole-plane collect returned0 and a watched unit was overwritten. With this fallback, the same integration becomese40e6a67284: the probe confirmsplaneWatch=0andrangeWatch=1, observes a newer stamp after the per-unit scan, drops that unit from stale depth stores, and preserves its bytes unchanged. The game reaches the rendered opening desk cutscene (screenshot at140s), then closes normally at150s with native exit0; native Khronos active, zero VUIDs, all saves/config/kernel state restored, no remaining processes. Four GPU prerequisite tests also pass with no skips/VUIDs. This native comparison uses the earliere2d457be345base, #6782, #6913 and bounded private diagnostics on both arms; the focused four-test comparison above uses current fetched mainabed0354680. Gameplay, audio and console-reference graphics equivalence are unverified.Paired GPU logs, patch, native source pins and cutscene screenshot, SHA256
933e6f14ac3d3249ca96c96187e3a3c0c0d3d3ff2a628af243238938900e764f. Raw memory, game requests, shaders and saves excluded.Latest-main native qualification: main
abed0354680plus #11690/#11692/#6782/#6913 (26cde7116e2), with no private source diagnostics and fresh eboot/seven-module conversion, passes8/8 GPU checks (five actual Khronos insertions, no skips/VUIDs). The native game renders the intro, shows the Square Stand prompt, accepts Square and reaches the standing first-person view. A later camera/movement input reaches a different guard,AGC graphics: multisampled DCC fast clear has no exact host value, and exits134 at459.73s. This is not a complete gameplay pass; displacement after that input was not visually verified. Native validation remains clean and all state is restored with no leftover processes.Current-main native pins, GPU log, input actions and standing screenshot, SHA256
55c85a1ae1769d7337ac0210e085218bb97d01d51f3c0baf683ac4f4108bbc8c. No FPS/audio/console-reference graphics claim.Independent current-head Windows/NVIDIA verification from maskym:
771dbdfec2plus #11948 merged onto maina13ee2ddd1, Windows 11 26300, RTX 2070 SUPER (NVIDIA 621.01), WinLibs GCC 15.2/UCRT Release. All 11 depth-plane tests passed twice, including page/partial-unwatch and memoized-collect cases; the loader confirmed Khronos validation on every run, with no missing-layer message or VUID. This is contributor-reported evidence for that composition, not a new local or PS5 run.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)a00e826a277a; included in main snapshot2227f136618)