Repository navigation
fix(agc): bound the sampled depth-plane copies - #7705
Conversation
|
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.
|
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Apple GPU / MoltenVK run. agc_driver_depth_plane_sampling_evicting and the depth-plane tests pass on Metal. Two of the tests listed as unchanged, depth_plane_guest_layers and depth_storage, are skipped on macOS on main. With a loader patch they run and pass with this PR.
Machine: Apple M2 Max, macOS 27.2, x86-64 build under Rosetta. Head 0258a05, PR diff applied cleanly. Drivers: MoltenVK 1.4.2 (Vulkan SDK 1.4.363.0) and MoltenVK main + SPIRV-Cross #2704.
Tested: the tree is main a373e85 + mugurc's macOS series (ci/macos-series-2) + the PRs needed to build on macOS (#6667, #6668, #7524, #6969, and two files of #7332), not plain main (it doesn't build on macOS yet). Also in the same build: #7707, #7698, #7687, #7691 and #7705 (they touch different files and applied cleanly together), plus a test-only patch that lets nine AGC fixtures load the Vulkan loader on macOS (branch jotitadev:fix/agc-tests-vulkan-loader-macos). Base on that tree: 40 ^agc failures with MoltenVK 1.4.2 and 26 with MoltenVK main + #2704.
agc_driver_depth_plane_sampling_evicting(APS5_DEPTH_PLANE_COPY_MIB=1),depth_plane_sampling,depth_plane_fill,depth_plane_guest_layers,depth_storage,depth_compare_copy,depth_surface_from_storageanddepth_bits_guest_memory: all pass 3 of 3 on both drivers.depth_storage_dispatchanddepth_storage_reusealso pass in the full-suite runs.DepthPlaneGuestLayers.cppandDepthStorage.cpploadlibvulkan.so.1themselves, without portability enumeration. Without the loader patch they printskipped, no usable Vulkan deviceon macOS, so they don't cover this change there.- Full
^agcsuite with the stack: 42 of 730 fail on MoltenVK 1.4.2 and 28 of 730 on MoltenVK main + #2704. That is the base failures plusagc_driver_pooled_buffer_rangeandagc_performance_overlay, which only run because of the loader patch and fail for unrelated reasons. No new failure from this PR.
I didn't do a without-the-fix control. Without the fix, the evicting test's variable does nothing, so the test would pass anyway.
GPUs tested: Apple M2 Max (MoltenVK 1.4.2; MoltenVK main + SPIRV-Cross #2704)
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
NVIDIA data: GeForce RTX 4090, driver 617.42, Windows 11 Pro 26200, WinLibs GCC 15.2.0 r7, Release,
AI-assisted: yes (Claude Code) |
|
Windows regression check on an Intel UHD 770 and an AMD RX 9070 XT: no new failures in the full CTest results. Windows 11 Pro 26300, WinLibs GCC 15.2.0 r7, Release, every execution test built. Intel UHD Graphics 770 ( This PR adds no test, so I ran full CTest (
AI-assisted: yes (Claude Code) |
0258a05 to
f440e3b
Compare
github-actions
Bot
added
the
unblocks title
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
The full Build of this head fails on Linux (lavapipe) in the new test, which main does not have:
agc_driver_depth_plane_sampling_evicting(run 39348454151):AGC graphics: 2 depth plane copies were kept with a 1 MiB budget, expected only the newest(DepthPlaneSampling.cpp:458).evictPlaneCopiesruns right after the copy is constructed (DepthSurface.cpp:1881), beforeRefreshcreates the staging buffer (DepthSurface.cpp:1024), soHeldBytes()of the newest copy leaves the staging out and the total is under-counted until the next lookup.- The test requires exactly one copy left at the smallest budget the variable allows (1 MiB). That depends on the driver's image sizes: on llvmpipe two copies fit. Either account after the staging exists and assert something that holds on every driver (e.g. fewer copies than the 8 kept with
APS5_DEPTH_PLANE_COPY_MIB=0, and the newest one still present), or make the budget finer than 1 MiB for the test.
The rest (shared OwnedImage ownership, recorder keeping the image and staging until the copy runs, ENVIRONMENT.md entry) checked fine.
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Apple GPU / MoltenVK run of the current head. The new agc_driver_depth_plane_sampling_evicting fails here the same way adriadam10 saw on lavapipe, so the one-copy check does not hold on MoltenVK either. My earlier run (0258a057b) passed because that head did not have the count check yet.
Machine: Apple M2 Max, macOS 27.2, x86_64 under Rosetta 2. Vulkan loader + MoltenVK 1.4.2 (Vulkan SDK 1.4.363.0), and again against MoltenVK main + SPIRV-Cross #2704. Release, BUILD_TESTING=ON.
Base/method: PR #8658 feat/macos-port @ c5fa39620a (current main plus the macOS port), not main, because main does not build on macOS. This PR's diff at f440e3b238 (git diff merge-base..head -- . ':!docs') applied cleanly. The focused runs and the suite were done in one tree that also had #9443 and #9717/#9533. They touch other tests, and the evicting failure below is also there when the binary runs alone. Full rebuild first.
agc_driver_depth_plane_sampling_evicting: fails on every run, 4 per driver, on both drivers:
AGC graphics: 2 depth plane copies were kept with a 1 MiB budget, expected only the newest
Running the same binary by hand with --evicting:
APS5_DEPTH_PLANE_COPY_MIB |
MoltenVK 1.4.2 | MoltenVK main + #2704 |
|---|---|---|
0 |
8 copies kept | 8 copies kept |
1 |
2 copies kept | 2 copies kept |
So the eviction does work here, from 8 copies down to 2. This matches what adriadam10 described. The newest copy's staging is not counted yet when evictPlaneCopies runs, and at a 1 MiB budget two copies fit on this driver too. Any assertion that holds on all drivers would have to accept 2 here.
Unchanged and passing (--repeat until-fail:3, both drivers): depth_plane_sampling, depth_plane_guest_layers, depth_plane_fill, depth_plane_partial_store, depth_compare_copy, depth_storage, depth_storage_dispatch, depth_storage_reuse, depth_surface_from_storage, depth_bits_guest_memory.
Full ^agc suite (877 tests), both drivers: the failures from the #8658 base, plus depth_plane_sampling_evicting. The suite also showed two failures from #9533's tests in the same tree, which I'm reporting on #9533. Nothing else is new.
GPUs tested: Apple M2 Max (MoltenVK 1.4.2; and MoltenVK main + SPIRV-Cross #2704).
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
@AnthonyClemens a note on merge order: #10043, an opt-in FSR 1 scaler for the presented image, adds its rows next to yours in |
A depth plane copy (a depth surface sampled as several slices, an integer view or a cube) was kept until the device went away, with a staging buffer of its own size, and a staging buffer it outgrew was never freed. Every depth surface a scene ever sampled this way held its copy, so device memory grew with each new scene (0.4-1.2 GB in RE2 after a level load). The copies now share their image's ownership with the views they hand out, the recorder keeps the image and the staging buffer until the recorded copy has run, and past APS5_DEPTH_PLANE_COPY_MIB (default 256; 0 keeps every copy) the least recently used copies leave the cache. A copy made again refills from the surface and guest memory as a new one does. agc_driver_depth_plane_sampling_evicting runs the plane sampling cases with a 1 MiB budget. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…est copy agc_driver_depth_plane_sampling_evicting passes --evicting, and the test then requires a single depth plane copy left in the cache (DepthPlaneCopyCount) after the cases. With APS5_DEPTH_PLANE_COPY_MIB=0, which keeps every copy, it fails with "8 depth plane copies were kept with a 1 MiB budget, expected only the newest". The explanatory comments move to the pull request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The newest copy's staging buffer exists only after Refresh, so evicting before it under-counted the held bytes. The evicting test now asserts a count that holds on every driver: at least the newest copy and fewer than the 8 an unbounded cache keeps.
f440e3b to
c2b128b
Compare
|
The full Build failed
Rest checked earlier (shared |
|
The depth-plane copy budget costs some scenes a lot once the copies exceed it Posting here because the regression was bisected to this merge, in case it is useful to the author. On Windows 11 / RX 9070 XT, ASTRO BOT (PPSA21567) drops from about 25 fps to 0.7 fps in the intro The eviction runs after every Details, the bisect table and fix directions are in AI-assisted: yes. |
What
A depth plane copy (a depth surface sampled as several slices, an integer view or a cube) stayed in
planeCopies()until the device went away. Each copy also kept a staging buffer the size of the copy. A staging buffer it outgrew was moved intoretiredStagingand never freed. So every depth surface a scene ever sampled this way kept its copy, and device memory grew with each new scene.With this change:
OwnedImageowner of the depth-planeTextureconstructor. A copy can leave the cache while a draw or a cached resource set still samples it.APS5_DEPTH_PLANE_COPY_MIB(default 256;0keeps every copy), the least recently used ones leave the cache. A copy made again refills from the depth surface and guest memory, as a new copy does.DepthPlaneCopyCount()returns the number of cached copies, for the test.In Resident Evil 2 (PPSA04288), these were the peak
depth plane copyallocations underAPS5_VK_MEMORY_TRACK:depth plane copyThe scene rendered the same either way. Without the bound, the staging buffers of the same size added roughly as much again under
device buffer.Tested
Windows 11 Pro (10.0.26200), AMD Radeon RX 9070 XT, WinLibs GCC 15.2.0, Release, on
main85029a0.agc_driver_depth_plane_sampling_evicting. It runs the depth-plane sampling cases withAPS5_DEPTH_PLANE_COPY_MIB=1and--evicting. Every copy but the newest leaves the cache, so later cases rebuild evicted copies and must read the same values. It then requires one copy left in the cache.APS5_DEPTH_PLANE_COPY_MIB=0, which keeps every copy asmaindoes, fails:8 depth plane copies were kept with a 1 MiB budget, expected only the newest.depth_plane_sampling,depth_plane_guest_layers,depth_plane_fill,depth_compare_copy,depth_storage,depth_storage_dispatch,depth_storage_reuse,depth_surface_from_storage,depth_bits_guest_memory.main. As MaxLikesCode noted, theprepared_shaders_abi_deferred_*failures came from the old base.Thanks to MaxLikesCode, savvaniss and jotitadev for the NVIDIA, Intel/AMD and MoltenVK runs.
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)