Repository navigation
fix(agc): build a partially committed sampled 3D view from its committed slices - #10064
Draft
EagleBearerYS wants to merge 10 commits into
Draft
EagleBearerYS wants to merge 10 commits into
EagleBearerYS wants to merge 10 commits into
Conversation
…ted slices Ghost of Tsushima Director's Cut (PPSA02225) on Linux with an RTX 4070 Ti samples this view, as milescoviello reported in boykopovar#9318 (comment) and boykopovar#9318 (comment): [gpu] sampled view 0x1564050000 (320x1280 format 71, dimension 4, tile 7, slices 0..899) covers 4781 MiB 157 of its 900 slices fit in its guest allocation, and the run dies at the 20 GiB cap. The snapshot path read the whole 4781 MiB range into a host vector and copied it into a host-visible staging buffer of the same size, about 9.3 GiB of host memory, then detiled it through a tiled and a linear device buffer of that size again. A sampled 3D view whose range is not wholly committed now reads, stages and detiles only the slabs that hold a committed page, packed together (a slab is one slice of a Z_X or R_X volume, blockDepth slices of a thick one). Uncommitted pages inside such a slab still read as zeros. The image keeps the view's full extent, and every other slice is copied from one slice of zeros filled on the GPU, the texels its zero bytes detiled to before. The upload's host memory is about twice the uploaded bytes: about 1.6 GiB instead of 9.3 GiB if only the 157 slices inside the allocation are committed. The cache entry keeps the slab list, so a commit or decommit that changes it rebuilds the view; the write generation and the compare cover the uploaded slabs. DCC, fast-clear keys, depth comparison, sRGB expansion and storage image views keep the whole-range path. The new log line gives the uploaded slab count and the zero-filled slice count. Whether the title gets past this view is not verified. Thanks to milescoviello for the report and the logs.
github-actions
Bot
added
the
unblocks title
12 tasks done
|
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.
|
|
A hardware data point next to lavapipe: AMD iGPU on Linux (RADV), head Setup
I didn't run Ghost of Tsushima. |
Contributor
Author
|
@Lander0606 thank you, the seven |
|
Windows run on NVIDIA and Intel. Windows 11 Pro 10.0.26300, WinLibs GCC 15.2.0 r7, Release. NVIDIA GeForce RTX 3060 Ti (driver 617.42) and Intel Arc Graphics (Meteor Lake, driver 101.9150), selected with Tests: the 1278 that
All 13 new AI-assisted: yes (Claude Code) |
Contributor
Author
|
Thanks, @BKlaasWerkman. All 13 new tests on an RTX 3060 Ti and an Intel Arc, with the seven sample modes running and nothing new under the layer, covers the vendors this PR hadn't reached yet. It's in the Tested section now. |
# Conflicts: # core/libs/prx/libSceAgcDriver/Graphics/src/ShaderResources.cpp
12 tasks done
12 tasks done
12 tasks done
thuanquang
reviewed
Oct 24, 2026
thuanquang
left a comment
thuanquang
left a comment
Contributor
There was a problem hiding this comment.
A read-through at 4a90a1a1d0; I built and ran nothing. The slab mapping and the packed detile offsets look consistent with main's geometry, and the head merges cleanly into current main. A few things a reviewer will likely ask about:
Tests
PatternElement(PartiallyResidentSnapshot.cpp:499-504) depends only on the format and the slab, andCommitSlab(:516-521) repeats that one element across the whole slab. So the-sampleschecks can tell which slab a texel came from, or zero, but not where inside the slab it was read. The main new risk is the per-slab packed detile offset (slot.tiledOffset + mip.tiledOffset), and a misplaced offset inside a slab would still pass. An element value that encodes its position (for example x, y and z within the slab) would cover it.- No case has every slab uncommitted, so the branch where
uploadedis empty (Texture.cpp:420-425,:443-447; no staging, tiled or linear buffers, zero snapshot bytes on the cache side) never runs. CommittedCaseForaccepts--committed-volume-mipsand--committed-volume-bc(:474-475), butCMakeLists.txt:2546registers only their-samplesvariants. So the mip and BC byte accounting is never run by ctest, and the body's "all 15 modes" doesn't match the 13 registered tests. The-samplesmodes also pass onmain; only the memory modes are red there.
Behaviour
- Slabs are chosen by commitment (
committedSlabs,ShaderResources.cpp:713-723), not by the view's allocation. If the range past the allocation is committed by neighbouring allocations, every slab has a committed page,:723returnsnulloptand the view takes main's full-size path unchanged. Has PPSA02225 been run with this head? The body's logs come from a stack with other open PRs, and the local cut that pointed at this view kept slices by allocation. - A question rather than a finding: when the packed snapshot falls under
SnapshotCompareBytes(256 KiB,:1067), it is kept for comparison where main's full-size snapshot was parked. A later lookup with the same slab set then goes throughequalsCommittedSlabs, which compares committed pages only. So if part of a slab is decommitted while the slab keeps other pages, is the cached texture returned with the old texels instead of rebuilt with zeros?
Body
- The cost line ("only for k3D views whose range isn't wholly committed") reads narrower than the code:
committedSlabsruns wheneverslabbedholds (:924), including wholly committed k3D views with write generation 0, and before the cache lookup, so hits pay it too. Depends on:lists #9253 (merged today) and #9884 (closed without merging); the Overlap section still describes composing with #9884.- The debt entry's "the image takes 4.7 GiB" is the guest (tiled, padded) byte count, not the VkImage size.
…tern The committed-volume pattern repeated one element per slab, so a -samples check could not tell where inside a slab a texel was read from: a wrong packed offset for a slab still read that slab's value. Each element now carries its slab and its element index within the slab (BC1 blocks carry the slab and a hash of the block index). A sample's expected texel is the same sample of a fully committed copy of the view, which takes the full-size path; the checks also require that reference to hold a texel of the sampled slab.
…b is decommitted A partially committed volume's cached view was kept while the set of slabs holding committed pages was unchanged and its committed pages still matched the snapshot. The comparison only covers the pages committed now, so a page decommitted inside a kept slab was skipped: the lookup returned the old texture with that page's old texels, where a fresh build reads zeros. CommittedSlabs now also keeps the view's committed byte ranges, relative to its base, as committedSlabs already reads them, and a lookup is refused unless the ranges committed now still cover every range committed when the snapshot was taken. Pages committed since are left to the comparison, as before: the snapshot holds zeros there, so zero pages committed later keep the view. agc_driver_partially_committed_volume_page_samples commits two slabs, samples them, decommits one page inside the second, and requires the texels of that page to read zero and the slab's others to keep theirs.
No committed-volume case left every slab uncommitted, so the upload with no packed slab (no staging, tiled or linear buffer, and an empty snapshot in the cache) never ran. committedSlabs returns nullopt only when every slab is committed, so such a view takes the slab path with an empty slab list. agc_driver_partially_committed_volume_empty checks that path: the lookup reads no guest bytes, where the full-size path reads the whole surface, it is kept on the next lookup, and the upload keeps only the zero slice. agc_driver_partially_committed_volume_empty_samples samples it and requires zeros everywhere.
CommittedCaseFor accepts --committed-volume-mips and --committed-volume-bc, but only their -samples variants were registered, so the mip and BC1 byte accounting never ran under ctest. Both run now as agc_driver_partially_committed_volume_mips and agc_driver_partially_committed_volume_bc, with their neighbours' properties. Registered, the BC1 case failed with "device does not support BC compressed textures": the test's own Vulkan device, which the non-sampling cases use, did not enable BC texture compression. It now enables it when the device has it, and the BC1 case skips when it does not.
…n image size The entry said the Ghost of Tsushima Director's Cut view's image takes 4.7 GiB. That figure is the view's guest byte count, tiled and padded, not the size of the Vulkan image.
Contributor
Author
|
@thuanquang thank you for the read-through, and in particular for the decommit question: you were right. The head is now 6dd0c32, with five new commits:
On your other points:
The GitHub run for the new commits passes 20 of 20 |
12 tasks done
12 tasks done
12 tasks done
12 tasks done
Contributor
|
@EagleBearerYS about a merge order with #10845, which reads a large watched texture straight into its upload staging buffer instead of a snapshot vector. It overlaps this PR in AI-assisted: yes (Claude Code) |
Contributor
Author
|
@milescoviello Thanks, #10845 first works for this PR, merge order only. Your resolution matches this PR's code. A packed-slab snapshot holds only the committed slabs, in order, so it isn't the surface's layout and has to stay on the vector path. Here |
Contributor
|
@EagleBearerYS thanks, and for the detail on where AI-assisted: yes (Claude Code) |
Main's boykopovar#7618 adds its swept write-only staging entry at the end of the debt file, where this branch adds its partially committed 3D textures entry; the merge keeps both.
This was referenced Oct 25, 2026
This branch has not been deployed
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
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.
What
Ghost of Tsushima Director's Cut (PPSA02225) binds this sampled view about 9 minutes in, during its opening (milescoviello, Linux, RTX 4070 Ti, first comment):
rsample.Why it costs that much:
mainalready defines the texels past the committed range.ReadCommittedreads uncommitted pages as zeros. But it materializes the zeros at full size, and holds them three times on the way into the image:For this view that's about 9.3 GiB of host memory.
The change: a sampled k3D view whose range isn't wholly committed keeps its full extent, and only the slabs with committed memory go through the upload.
Uncompressedkeys and no DCC, no depth comparison, no sRGB expansion. Everything else takesmain's path unchanged.committedSlabssplits the surface into its guest slabs. That's one slice in the per-slice Z/R 64KB_X branch or a linear volume, andblockDepthslices in a thick one.main's path runs.ReadCommittedper run of adjacent slabs. Uncommitted pages inside an uploaded slab still read as zeros.vkCmdFillBuffer-zeroed device buffer withvkCmdCopyBufferToImage. That isn't a clear, so block-compressed formats take the same path.covers … MiB, rate-limited the same way, says how many slabs were uploaded and how many zero-filled.DescribeCommittedwalk per lookup of an eligible k3D view (above), before the cache lookup, so hits pay it too, wholly committed views included. There's none when the lookup already holds the view's committed ranges.main's.#9318: this sits in the snapshot and upload path, not in #9318's view bound, so the two compose, and #9318 needs no k3D case. If
BoundArrayWindowever trimmed k3D views, it would run first and change the texels (a shallower volume).Tested
Not tested: Ghost of Tsushima Director's Cut with this fix. milescoviello has it on
test/ghost-9742-9717-3d(bc32434), the base he ran plus this commit (offer). That branch passes^agc_driver_partiallyon Linux (run). On Windows, its older base fails to build a test thatmainhas since fixed. This PR's own Windows build is the run below.New modes of
tests/execution/PartiallyResidentSnapshot.cpp(agc_driver_partially_<mode>, lavapipe):Memory. Every volume is 64 deep, and each committed slab has its own pattern. The bytes read count the snapshot's guest reads; the bytes kept count what the upload holds (staging, tiled, linear and zeros):
main/ this PR)main/ this PR)committed-volumecommitted-volume-thickmain/ 3,178,496committed-volume-linearmain/ 2,293,760committed-volume-comparecommitted-volumemain/ 655,360committed-volume-whole(control)committed-volumecommitted-array(control)Rebuilds (
committed-volume):Compares (
committed-volume-compare): newly committed zero pages compare equal and keep the view, and a CPU write rebuilds it.Texels (guest compute shaders sampling at slice centres, with z = 0, 5, 21, 22, 23, 30, 40 and 63, point-filtered):
mainand with this PR in every mode:APS5_NO_TEXTURE_CACHE=1.Red/green ×10 (the same test binary against each tree's
libSceAgcDriver):main,committed-volume,-thick,-linearand-comparefail 10/10 with "read N bytes of guest memory at its first use, expected M".Existing tests:
agc_driver_partially_resident_snapshot,…_pending_storesandagc_driver_large_tracked_snapshotstill pass.GitHub's runners (run 39907685337, on this head,
main0db47b2;^agc_driver_partially|^agc_driver_large_tracked|^agc_driver_image_sample|^agc_driver_texture):main's implementation: on its one run,agc_driver_partially_committed_volume,…_thick,…_linearand…_comparefail and the other 23 of the 27 tests pass. Green with this PR: all 27 pass ×10, none skipped.APS5_SKIP_TIMING_TESTS=1.Validation layer (run 39913316066, Khronos 1.3.275 on Linux, this head;
agc_driver_guest_memory_lifetime_copy,agc_driver_large_tracked_snapshotand^agc_driver_partially): no new VUID.VUID-VkDeviceCreateInfo-pNext-pNext, at device creation. It comes from an sType (1000574000) newer than the layer, andmain's tree shows it too.main's tree doesn't have this PR's new tests, so its arm passes; the red is the run above.On a cloud Linux machine:
Windows: MinGW syntax checks of
Texture.cpp,ShaderResources.cppand the test show 0 errors.check_conventions.py: passes. No comment lines are added.AMD Radeon 860M on Linux (RADV, Mesa 26.2.5, Fedora 44), by @Lander0606 (comment): cd60c69 against its parent 0db47b2, the
partially,snapshotanddcc_keystests, three runs each with and without the Khronos layer. The base passes 8 of 8 and this PR 21 of 21 in every run. The seven-samplesmodes sample on the 860M with no skips, and the bytes read and kept match the table above. Under the layer neither side logs aVUID-line.Windows on real GPUs, by @BKlaasWerkman (comment): cd60c69 merged into
main938812d (clean), Windows 11 Pro, WinLibs GCC 15.2.0, Release,ANYPS5_NO_SHADER_CACHE=1, with the Khronos validation layer (1.4.363) loaded. The 1,278 teststools/select_tests.pypicks for the PR's files, compared test by test with a full run of thatmainon the same GPU:main(theagc_driver_sample_ordertrio, test(agc): read the sample order tests' depth through a sampled copy #9056). NoVUID-message is added or removed.main(AGC (Intel Arc, Windows): agc_storage_write_back fails; buffer-image copies of larger RGBA16F images canonicalize fp16 NaNs #10126, test(agc): skip the MSAA retirement storage promotion without shaderStorageImageMultisample #8604). NoVUID-message is added or removed.agc_driver_alias_copy_label, which is intermittent on that device (AGC (Intel Arc, Windows): agc_driver_alias_copy_label fails about 1 in 3 runs; the label lands before the alias copy's destination store, APS5_STORE_AT_LABEL=1 hides it #10199), failed in themainrun and passed with this PR.agc_driver_partially_committed_*tests pass on both GPUs, none skipped, the seven-samplesmodes,_volume_compareand_volume_thickamong them.4a90a1a, two merges of
main(4be3da2, then 990f45e). TwoShaderResources.cppconflicts, each kept both sides:createTexturelambda that holdssampledCacheReclaimable. This PR's packed upload goes through it:createTexture(*context.detiler, resource, components, entry.bytes, depthCompare, packedSlabs(entry.slabs)).snapshotBytes(slabs, bytes), which isbyteswhenever the view has no slabs.APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.After thuanquang's review: 3871b5e, 0fbea36, c36790e and 83c9480 (review):
fix(agc): a cached slab view is no longer returned after a page inside a kept slab is decommitted (the cache entry change above). The newagc_driver_partially_committed_volume_page_samplescommits slabs 21 and 22, samples, decommits one 4 KiB page of slab 22, and samples again. Red with the fix'sShaderResources.cpphunk reverted: "sample of level 0 slice 22 (thread 96) reads (23.000000, 712.000000, 1.000000, 1.000000), expected the zeros of its decommitted page". Green with it.test(agc): each element of the pattern encodes its position in the slab, and the-sampleschecks compare each texel with the same samples of a fully committed copy, which takes the full-size path. With the slab path's packed offset shifted by 0x100 in a local build, 5 of the 7-samplescases fail (the other 2 don't take the slab path), where the earlier pattern passed all 7. An error shared by both paths, such as one in the common detile offset, still passes: these tests compare the two paths, and there's no CPU tiling oracle.test(agc):agc_driver_partially_committed_volume_emptyand_empty_samplescover a volume with no committed slab. It takes the slab path with an empty slab list ("uploads the 0 of its 64 slabs"), reads 0 of its 4194304 guest bytes, and samples zeros.test(agc):agc_driver_partially_committed_volume_mipsand_volume_bcare registered with ctest. The BC1 case's own device now enables BC texture compression when it has it, and skips otherwise.^agc_driver_partially_tests on lavapipe, on a Linux machine with CI's flags.^agc_driver_partially_): Linux: withmain's implementation of these files,agc_driver_partially_committed_volume_page_samplesfails, and with this PR 20 run, 20 pass ×10, 0 skip, 0 fail. Windows: 20 run, 0 pass, 20 skip (no Vulkan device), 0 fail. The libc and libkernel suites pass on both, run withAPS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.docs(debt): the debt entry now gives the view's guest range (4781 MiB, tiled and padded) where it said the image takes 4.7 GiB.52826ee, a merge of
main2fade7e. One conflict, in the includes oftests/execution/PartiallyResidentSnapshot.cpp:main(fix(agc): keep a rewatched import's memory compared once it is shared #9159) added<algorithm>and<span>there, which this PR also adds, so the merge keeps one of each and this PR no longer adds them. The PR's other added and removed lines are unchanged. On GitHub's runners (run),^agc_driver_partially_andagc_driver_large_tracked_snapshot:main's implementation of the changed files, 7 of the 21 fail, each on the guest bytes a partly committed volume reads at first use (for example, "the partially committed volume read 4194304 bytes of guest memory at its first use, expected 1507328 of its 4194304").APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.Overlap (
tools/pr_overlap.py --branchagainstmain5255264; this branch merges cleanly withmain):Texture's constructor that this PR changes: it takes each layer's guest offset fromGuestOffset(layer, mip), and this PR detiles each committed slab from its packed offset. They compose: once both are in, the packed offset is applied per mip the same way. fix(libSceAgcDriver): pad level 0 rows of a POW2_PAD PS4 mip chain #9842 is stacked on feat(libSceAgcDriver): draw PS4 (GCN) command buffers #9431, and its overlap with this PR is feat(libSceAgcDriver): draw PS4 (GCN) command buffers #9431's lines.CMakeLists.txt). test(agc): honor ANYPS5_GPU in the fixtures that open their own Vulkan device #9253 has merged.Texture.cpplines and is much larger. Its author agreed that this one goes first (comment): oneTexture.cppconflict, distinct topics. Once this merges, we'll send the resolved conflict to feat(agc): back extra guest mip levels with storage images #7085 as a tested commit on a fork branch.maine05ba6f (run).main's perf(agc): stage written-only buffer elements a dispatch sweeps on discrete GPUs #7618 adds its swept write-only staging entry at the end of the debt file, where this PR adds its partially committed 3D textures entry; the merge keeps both, and nothing else conflicted. The tests whose names containpartiallyorsnapshot, and theagc_driver_sample_ordertests, pass 10 runs in a row. Linux: 31 run, 29 pass, 2 skip (agc_driver_sample_order_grid_reference,agc_driver_sample_order_recipe_depth_locations), 0 fail; the before arm fails seven of theagc_driver_partially_committed_tests. Windows: 34 run, 4 pass, 30 skip (no Vulkan device), 0 fail. Suites: libc and libkernel pass on Linux (144 and 207) and on Windows (171 and 223). The suites ran withAPS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.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)