Repository navigation
fix(agc): bind a written TYPE 0 or 1 static image slot as a null storage image - #10906
oneandonlydean merged 6 commits into
Conversation
|
I checked the new TYPE 0/1 handling against the current descriptor path. The materializer identifies these words as null, and the storage binding path passes |
|
@MarcZX10 thanks for checking the descriptor path. On a host without
So a test can't show the placeholder store as unobservable on that host, because it is observable there, as recorded. What the PR relies on is that nothing reaches guest memory, and that follows from the slot never being decoded, on both hosts. |
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the exact PR head 0d41b11bc190e630d89ddb7d5b52b0bd511c33e8.
The change is scoped to the measured TYPE 0/1 storage-image case: nullImageDescriptor now recognizes read or written non-atomic descriptors without a table; draw write-range collection skips IsNullTextureResource in the same way dispatch collection already does; and the added tests keep the real neighboring range while excluding TYPE 0/1 ranges for both draw and dispatch. The debt update records the remaining limitation. On hosts without Vulkan nullDescriptor, these descriptors bind the existing shared storage placeholder, so stores can be observed later through a null storage image; this PR does not introduce that behavior, and it is explicitly documented. The binding path does not decode the TYPE 0/1 descriptor or schedule a guest-memory writeback for it.
The author reports an AMD Radeon PRO V620 measurement where TYPE 0 and 1 stores leave the guest dwords unchanged. Their hosted focused run reports 48 Linux AGC tests passing 10/10 and 47 Windows tests passing with one skip; the five full-suite Linux failures require IPv6. Hosted policy, intake, labels, conventions and report checks pass; the repository skips product jobs on this PR path. git diff --check and tools/check_conventions.py --base upstream/main --head upstream/pr-10906 also pass locally. I did not independently run the Vulkan execution tests or hardware probe.
The PR declares #10556 as a merge-order dependency and does not claim to fix the broader null-placeholder limitation. I found no blocking issue in the scoped change.
AI-assisted: yes.
|
@EagleBearerYS Verified this on AMD/RADV (Mesa 25.2.8) across three arches — RX Vega 56 (VEGA10, GCN5), RX 7900 GRE (NAVI31, RDNA3), 7800X3D iGPU (RAPHAEL, RDNA2).
The AI-assisted: yes. |
|
@tablefrogpillow thank you, that's the real-hardware run this PR was missing, on three architectures and with the reverted-files check too. I've added it to the Tested section, credited to your comment. |
Main's boykopovar#9186 extends the multisampled color target debt entry right after the type-0 and type-1 descriptor entry this branch rewrites. The merge keeps both entries.
|
NVIDIA and AMD-proprietary run on Windows. Windows 11 10.0.26300, WinLibs GCC 15.2.0, Release;
AI-assisted: yes (Claude Code); results above were run on this machine. |
|
@wachi35676 thank you, that's the first run of this on NVIDIA and on AMD's own driver, and the probe with the words from Ratchet & Clank: Rift Apart (PPSA01474) is the case this PR is for. I've put your results in the Tested section and added both GPUs to the list. Your note on the probe's path is fair: it shows both drivers take the null storage binding, while the discard itself comes from the gfx1030 measurement in #9749. |
|
@EagleBearerYS One test change: Please keep the successful written and read-write TYPE 0/1 materialization checks, the unchanged-words checks, the neighbouring real draw/dispatch range checks and the reserved TYPE 2 invalid-descriptor check. The code can keep refusing atomics until they are implemented; nothing new is needed there. |
Atomic access to these descriptors hasn't been measured, so the test shouldn't require a refusal: it would fail once atomics are handled. The materializer still refuses them. The written and read-write materialization checks, the unchanged-words checks and the reserved TYPE 2 checks stay. Asked for in oneandonlydean's review.
|
@oneandonlydean Thanks. Done in 83b7421: both TYPE 0/1 atomic cases, including the atomic-only one main's test already had, are out of the loop, so nothing requires them to be refused; the code still refuses them. The written and read-write materialization checks, the unchanged-words checks, the draw/dispatch range checks and the reserved TYPE 2 check stay. The focused tests pass 10 of 10 on Linux and Windows (run). |
✅ 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 390 tests)Skipped (40)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. |
# Conflicts: # docs/dev/debt/libSceAgcDriver.md
✅ NVIDIA GPU test: passedPR head
Intermittent validation messages (1, seen once in the parallel run, not reproduced when the test was re-run alone 2 times; not counted against this PR)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 (5 IDs in 484 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. |
What
A static image slot (no image table) that is written, and not atomic, whose T# has TYPE 0 or 1, is now bound as a null storage image. A read-only slot with such a T# has been bound that way since #4779, #5473 and #5828, and so has a written slot with a zero base. With VK_EXT_robustness2's
nullDescriptor, its stores are discarded and its loads return zero; without it, the heap's placeholder image is used, which keeps stores (already in the debt file). Atomic slots, and TYPE 2 to 7 in any slot, still throw.Ratchet & Clank: Rift Apart (PPSA01474) stops at 1,300 s on a compute shader whose image 4 is a written, never-read storage slot holding a buffer V# (
04efd000 00040011 00017a00 00005204 ..., TYPE 0). No other source reads those SRT words, and the store is reached on every path (#9749).nullImageDescriptor(ResourceMaterializer.cpp): TYPE <= 1 is null for a read or written, non-atomic slot with no table, instead of read-only slots only.DrawWriteRanges(WrittenBuffers.cpp): a written storage image whose words areIsNullTextureResourceadds no write range, asDispatchWriteRangesalready does. It skipped only a zero base, and threw on a TYPE 0 or 1 T#.IsNullTextureResourceitself already returns true for TYPE <= 1 in any slot.docs/dev/debt/libSceAgcDriver.md: a written-slot entry after the TYPE 0/1 entry gives the store measurement (second commit, split out of that entry in the merge ofmain5b5d24c).Measurement: on an AMD Radeon PRO V620 (gfx1030, RDNA2; Ubuntu 24.04.5, kernel 7.0.0-38, in-kernel amdgpu, ROCm 6.3.3), Tamalero ran
tools/hw-oracle/probe_type0_store.py(test/hw-oracle-type0-store) with PPSA01474's eight words (result):image_storechanged its 32 dwords;image_storechanged no dword from base−0xe00 to base+0xf000, with no fault or hang.Not measured: atomics, other RDNA generations, and the console's gfx1013.
Related: #9749 (the PPSA01474 diagnostics), #9633, #4139 and #5828 (the read measurements), #4779, #5473. Overlaps:
nullImageDescriptor's TYPE ≤ 1 line. The two compose; this one rebases after it.WrittenBuffers.cpphunk next toDrawWriteRanges' null check, from the GPU-heap change they carry. This one goes first: feat(agc): serve lane-keyed, pointer and runtime image-table columns #10193 already conflicts withmainand is much larger, and feat(agc): look up table images at run time through a device image table on the GPU heap (APS5_GPU_PATH) #7642 is ours. They keep both lines when they rebase.Tested
ResourceMaterializer.cppandWrittenBuffers.cpp,agc_shader_type_zero_image_materializationandagc_driver_draw_write_rangesfail.agc_shader_type_zero_image_materialization: TYPE 0 and 1 written and read-write storage slots materialize as null with their words unchanged; atomic slots and TYPE 2 still throw. Fails on main withruntime image descriptor is invalid.agc_driver_draw_write_ranges: written TYPE 0 and 1 storage images, and the reported words, add no draw or dispatch write range next to a real one. Fails on main withunsupported image type 0.agc_driver_graphics: the reported words areIsNullTextureResource, andDecodeTextureResourcerefuses them.ctestas an unprivileged user: 2,214 tests, 2,119 passed, 90 skipped and 5 failed. The five need IPv6, which that machine lacks:guest_net_buffer_validation,guest_socket_options,guest_socket_stream_options,guest_socket_address_lengthandguest_tcp.VUID-VkDeviceCreateInfo-pNext-pNextat device creation, is the same on main and on this branch. On GitHub's Linux runner (the same run), the layer is loaded on both arms and the VUIDs are the same: onlyVUID-VkDeviceCreateInfo-pNext-pNext; the fix adds none.agc_driver_graphics,agc_shader_memory,agc_driver_draw_write_rangesandagc_driver_draw_write_ranges_draw_pipelinepass 4 of 4 on all three;WrittenBuffers.cppandResourceMaterializer.cppback at the merge-base and this PR's tests kept,agc_shader_memory,agc_driver_draw_write_rangesandagc_driver_draw_write_ranges_draw_pipelinefail.maine05ba6f (run).main's fix(agc): retire an undrawn filled multisampled color target that a guest access overlaps #9186 extends the multisampled colour target debt entry right after the type-0 and type-1 descriptor entry this PR rewrites; the merge keeps both entries, and nothing else conflicted. Theagc_driver_draw_write_ranges,agc_driver_graphics,agc_shader_memoryandagc_shader_type_zero_image_materializationtests, with the other tests whose names those match, pass 10 runs in a row. Linux: 50 run, 50 pass, 0 skip, 0 fail. Windows: 49 run, 49 pass, 0 skip, 0 fail. The before arm failsagc_shader_type_zero_image_materialization,agc_shader_memoryandagc_driver_draw_write_rangeson both. 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.test/9749-null-storage(b9d0797: this PR's 38288a4 and 0d41b11 merged onto fix(shader): read a chased pointer's high word at its own offset #9749's head 52ae654), with feat(shader): raise the sampler and sampled-pair limits past the 32-bit sampler masks #10000's 12b8923 merged in locally; without feat(shader): raise the sampler and sampled-pair limits past the 32-bit sampler masks #10000 the branch stops at 214 s onsampler resource limit exceeded. It gets past the 1,300 s stop: there is noruntime image descriptor is invalidline and no other error before 2,632 s, and after the old stop it loads about 980 more textures and compiles new shaders. At 2,632 s a different compute shader, 0x116bb6ef00, stops onGetImageResource dword 0 is not a valid runtime value, with dword 0 coming from aLoadAddressU32at pc 0xd4d4 behind a tree ofPhis. That stop is outside this PR.mainaeff26b,ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1, no validation layer. Both drivers reportnullDescriptor.agc_driver_graphics,agc_shader_memory, bothagc_driver_draw_write_rangesentries,agc_shader_type_zero_image_materializationand the otheragc_shader_tests, 44 entries, pass on both GPUs.main'sResourceMaterializer.cppandWrittenBuffers.cpp,agc_shader_type_zero_image_materializationfails withruntime image descriptor is invalid, and bothagc_driver_draw_write_rangesentries withunsupported image type 0.image_storethrough a written slot holding a TYPE 0 T#, a TYPE 1 T# or the words from Ratchet & Clank: Rift Apart (04efd000 00040011 00017a00 00005204 ...) recompiles, binds and dispatches on both drivers with no device fault or error.mainrefuses all three withruntime image descriptor is invalid, and TYPE 2 is still refused. On the probe's path even a TYPE 9 control store didn't reach guest memory, so it shows that both drivers take the null storage binding, not the discard itself.agc_entries, the same ones fail with and without the change.agc_shader_type_zero_image_materializationno longer requires TYPE 0 and 1 atomic slots to be refused, since atomics on these descriptors aren't measured. Both atomic cases are out of its loop, including the atomic-only onemain's test already had; the materializer still refuses them. The written and read-write null-materialization checks, the unchanged-words checks and the reserved TYPE 2 checks stay. The run's 18 tests,agc_driver_draw_write_ranges,agc_driver_graphics,agc_shader_memory,agc_shader_type_zero_image_materializationand 14 otheragc_shader_*tests, pass 10 runs in a row. Linux: 18 run, 18 pass, 0 skip, 0 fail. Windows: 18 run, 18 pass, 0 skip, 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.main5b5d24c (run, run). DavidAmsellem's docs: measure type-0 and type-1 texture descriptors on gfx1100 #7910 rewrotemain's type-0 and type-1 texture descriptor entry indocs/dev/debt/libSceAgcDriver.mdwith gfx1100 (RDNA3) read measurements; this PR had extended that entry for written slots. The merge keepsmain's entry as it is and moves this PR's written-slot text into a new entry after it, with the same store measurement and atomic rule. Nothing else conflicted, and the code and test lines this PR changes are the same.agc_driver_frame_replayandagc_driver_frame_replay_concurrent, whoseagc_frame_replayhelper exited with 0xC0000139 (entry point not found) within 0.2 s.agc_driver_frame_replaytests to this PR's focused tests, and all of them pass 10 runs in a row. Linux: 53 run, 53 pass, 0 skip, 0 fail. Windows: 57 run, 57 pass, 0 skip, 0 fail, including all eightagc_driver_frame_replaytests. The before arm, withmain's implementation, failsagc_shader_type_zero_image_materializationon both.guest_timed_join_import_libkernel_sysandguest_timed_join_import_libkernel_webfailed in the Windows libkernel suite; they fail onmaintoo (run). The suites ran withAPS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.main2b1ae87, which brings savvaniss's fix(agc): retire a multisampled color target the title wrote over #8223 (run).docs/dev/debt/libSceAgcDriver.mdonly. This PR adds its "Written type-0 and type-1 texture descriptors" entry next to the old "Multisampled color targets … lose their guest-read guards" entry. fix(agc): retire a multisampled color target the title wrote over #8223 replaced that old entry and added "Shader buffer access to native multisample targets".main's two. This PR's copy of the old entry was the same as the merge base's, and its own diff is unchanged.agc_shader_type_zero_image_materialization, the other tests ofagc_driver_graphics_tests,agc_shader_memory_testsandagc_driver_draw_write_ranges_tests, and theagc_driver_frame_replaytests.APS5_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)