Repository navigation
Conversation
…tables ReadRawShader drains queue 0 when a queued draw-pipeline item writes the shader code it is about to read, but the user-data callees and call-table descriptors and tables a shader links were read without that drain. A callee, descriptor or table written by queued work was then read stale and linked into the invocation. ReadUserDataCallee and ReadGuestBytes now drain the overlapping queued writes first, through the same helper. agc_driver_queued_shader_calls holds a queued producer that rewrites a callee, a call-table descriptor or a table entry until the capture drains it, and checks the linked invocation against one built after the drain. All three cases fail on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed head e7da862d03403e694ffa6a1f8f9d1e19d1a3fb92 against current main bf77a9e9cba4b02c358779b35295176dd9cae164. The new shared drain preserves the prior raw-shader read behavior and is applied before reading user-data callees, call-table descriptors and table bytes; each range is bounded to the bytes actually read. The regression test holds an overlapping queued producer and checks all three read paths against a post-drain invocation. The test target is picked up by the existing execution-test glob and registered by the shared CMake loop; its timeout/environment properties are set explicitly. The changed paths merge cleanly with current main, and tools/check_conventions.py --base upstream/main --head upstream/pr-11442 plus git diff --check upstream/main...upstream/pr-11442 pass.
The PR reports 66 focused Windows/Vulkan-validation tests passing and no VUIDs; I did not independently run that GPU suite. Hosted policy checks pass, while product/relinker jobs are skipped by path policy. I found no blocking issue in the change.
|
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.
Rechecked exact head e7da862d03403e694ffa6a1f8f9d1e19d1a3fb92 against live main 2dbe9b99f577f5fad8ee675932a0603439e5eb2a. The earlier 66/66 focused Vulkan run was reported against 7b415520b6; since then main changed the AGC CMake registration and tests/ShaderMemory.cpp. The merged tree is clean, and I found no change to this PR’s ShaderRegistry.cpp or QueuedShaderCalls.cpp logic in that interval, but the required main-vs-change validation does not cover the current tree yet.
Please sync the branch to current main, preserve the test registrations, and rerun the same focused suite with the validation layer on current main and the resulting head. Update the tested base/head in the PR description. Local current-main conventions and whitespace checks pass; the merge-tree preview is clean. Hosted product/build jobs are skipped by path policy, and I did not independently run the GPU tests.
|
A Windows run of
AI-assisted: yes (Claude Code) |
adriadam10
left a comment
There was a problem hiding this comment.
The change itself looks right: DrainShaderWrites is applied before each callee page, call-table descriptor and table read, bounded to the bytes read, and agc_driver_queued_shader_calls fails without it (the queued producer only releases after a drain, so the old code times out and throws).
One open point, the one @MarcZX10 requested on this head: the tested base (7b415520b6) is behind main. Please merge current main, rerun the focused suite with the validation layer on main and on the result, and update the base/head in Tested. @ycbuzi's run on main 48c160c58b (RTX 4070 SUPER, GTX 1650 SUPER, AMD iGPU, control failing on main) is good supporting evidence, but the numbers in the description should match the head you push.
|
@Sam-mon7 a question about reachability, from reading On
So
If there is a path I missed (a title, or the local-branch change the description mentions), could the test go through |
|
Merged current @MarcZX10 @adriadam10, could you take another look? AI-assisted: yes (Claude Code) |
|
@thuanquang thanks, your reachability analysis is right. I traced the packet and dispatch paths on current I've updated the description to say this is defensive ordering with no failure reachable on AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed current head d68f894df506cb7f80d48be4424dc0092fc5d599 and the latest reachability clarification.
The clarification resolves the earlier question: this is a defensive ordering change, and the author found no production route on main that reaches compute linking with overlapping queue-0 work pending. That does not by itself make the change incorrect; the bounded InvocationFor test documents the ordering contract. I would keep the change if it is useful as that defensive guarantee.
Two integration items remain before merge:
- The PR is based on
e2a51188396df488b222521ac517cae715b3652c, while livemainis4c7d275bef89d309313d16bc2bc05d37a1b50be2. The current merge preview is clean, but main has since changedShaderRegistry.cppand the AGC CMake test registrations. Please sync to current main, preserve the queued shader-call test registration/environment, and rerun the focused validation-layer suite on the exact current-main base and resulting head. The reported 65/65 and 66/66 comparison is useful evidence for the prior base, not the current integration. - The current overlap report lists this PR with #9899, #10103 and #10140 in
ShaderRegistry.cpp. Please manually confirm the interactions and record the agreed landing order/dependencies on both sides where the overlap rule requires it. #10103’s author documents a clean source-only merge with #9899, but I found no recorded agreement covering this PR and all three overlaps.
Checks I ran: current-main git merge-tree --write-tree is clean (36b1914bb6568cc2a90eb367865a706b7b225cee), git diff --check passes, and current-main conventions pass. I did not run the GPU tests locally; this environment has no Vulkan device. The author-reported hardware results remain attributed to the author.
✅ NVIDIA GPU test: passedPR head
Failing on main too (5, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 436 tests)Skipped (39)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. |
|
Thanks for the updated NVIDIA test report on exact head d68f894. The broad suite and validation-layer comparison are useful; the report says the new queued-shader-call test was validated but not compared to main. My remaining review requests are still open: update this branch from current main (now aaccd0b; this head is substantially behind), compare the focused validation-layer test on both current main and the updated head, and record the overlap/landing order with #9899, #10103 and #10140. I did not run the GPU suite locally. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@MarcZX10 I synced this branch with current Tested. Windows 11, NVIDIA RTX 4070, driver 610.88, Release. Same command on both trees, with
Overlaps. I proposed an order on #9899, #10103 and #10140: this PR first, since it only moves |
|
@Sam-mon7 Accepted your offered reverse order: #10103 -> this PR, with the exact shared-function resolution and a current-main GCN-vertex reachability note in our reply. #10103's body now records the order; please add its merge-order prerequisite here. #9899/#10140 coordination remains separate. Source review only, no combined runtime claim. AI-assisted: yes (Codex). |
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (5, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 433 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. |
|
An AMD RDNA 4 run on Windows of Windows 11 Pro 10.0.22631, AMD Radeon RX 9070 XT (
AI-assisted: yes (Claude Code) |
Conflicts: - core/libs/prx/libSceAgcDriver/CMakeLists.txt: main moved AGC test registrations into tests/cmake fragments (00dd261). Resolved with tools/agc_test_split.py resolve; agc_driver_queued_shader_calls keeps ANYPS5_NO_SHADER_CACHE=1;APS5_PIPELINED_DRAWS=8 and TIMEOUT 30 in tests/cmake/agc_driver_queued_shader_calls_tests.cmake. - ShaderRegistry.cpp: ReadUserDataCallee keeps main's boykopovar#10140 early return on an unreadable page, with DrainShaderWrites ahead of it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@MarcZX10 @adriadam10 I merged current
Tested (RTX 4070, driver 610.88, Windows 11, Release). I ran the same focused command as in the description on both trees, with
Overlaps, now recorded in the description:
I also updated the reachability note: GCN vertex stages can reach the new drains on a draw-cache miss, but GCN mode has no caller in this tree. No failure reachable on AI-assisted: yes (Claude Code) |
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, 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)Validated but not compared (1, not in the main baseline's validation pass)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
ReadRawShaderdrains queue 0 when a queued draw-pipeline item writes the shader code it is about to read. The user-data callees, call-table descriptors and tables thatLinkUserDataCalleeslinks were read without that check.ReadUserDataCalleeandReadGuestBytes(which reads call-table descriptors and tables) now apply the same overlap check and drain first. All three paths use one helper,DrainShaderWrites, which replaces the lambda that was inReadRawShader.Reachability on current
main:InvocationForalready drains queue 0 first, or runs on a thread whereDrawPipeline::Active()is false (traced in the review thread). The new drains do not fire there.LinkUserDataCalleesalso links them (as @Amoenus pointed out). On a draw-cache miss,materializeDrawStagereachesInvocationForon the queue-0 worker while earlier pipelined draws, DMA or fills can still be outstanding, and nothing before it drains the callee or table bytes. So the new drains can fire on that path. GCN mode is enabled only byAgcDriverUseGcnShaders_nid_postfix, and nothing in this tree calls it.maintoday has been demonstrated.agc_driver_queued_shader_callsexercises the linking API directly with an overlapping queued producer that rewrites a callee, a call-table descriptor or a table entry until the drain, and checks the linked invocation against one built after the drain. Its red/green result verifies that ordering contract rather than reproducing a failure through packet execution orDriver::dispatch.Noticed while working on Demon's Souls (PPSA01341) on a local branch; the change is generic.
Tested
Windows 11 Pro 10.0.26200, WinLibs GCC 15.2.0, Release, NVIDIA GeForce RTX 4070 (driver 610.88), Khronos validation layer 1.4.363. Merged current
main0d5e03be8cinto the branch as merge commit84c3b32b3a. It had two conflicts:libSceAgcDriver/CMakeLists.txt(the per-test fragment split): resolved withtools/agc_test_split.py resolve. The test keepsANYPS5_NO_SHADER_CACHE=1;APS5_PIPELINED_DRAWS=8andTIMEOUT 30, now intests/cmake/agc_driver_queued_shader_calls_tests.cmake.ReadUserDataCallee: fix(agc): read user-data callees up to the 1 MiB raw shader program limit #10140's early return on an unreadable page is kept, with the drain ahead of it.Focused tests,
ctest -R 'agc_driver_(queued|prepared_shader|user_data|callee|call_table|draw_write_ranges|pipelined|gated_dispatch|copied_writer|draw_cache|finisher_label|same_queue_label|unimported_label|write_data_written)' -V -j2withANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation:main0d5e03be8c: 69/69 pass.84c3b32b3a: 70/70 pass. That is the same tests plusagc_driver_queued_shader_calls.VUID-or a missing-layer message, and no test was skipped.VK_LOADER_DEBUG=layershows the validation layer inserted at instance and device level.Red/green on the new base: with
main'sShaderRegistry.cpp, the new test fails in each case when run one case at a time (shader invocation ignored queued callee write,... descriptor write,... table write). With this change, all three cases pass (Shader calls observed queued code, descriptor and table writes).python tools/check_conventions.py --base upstream/main,python tools/check_test_registration.py buildandgit diff --checkpass on the merged head.Overlap and landing order
The overlap report lists #9899, #10103 and #10140 for
ShaderRegistry.cpp. I checked each against the current heads (git merge-treeand reading the diffs):ReadRawShader. When fix(agc): retain raw draw decodes across snapshot eviction #10103 lands I'll mergemain, keep its(address, retained)signature and retained-snapshot selection, and replace its localdrainlambda withDrainShaderWritesahead of the cached/retained comparison and the uncached page reads. This is merge order only; neither depends on the other functionally.Depends on, since neither PR relies on the other (#9899 comment). There is a small textual conflict inReadRawShader(itsCheckRangemove next to the removed lambda). It also has one semantic interaction: perf(agc): reuse user-data callee links and check a cached raw shader's range once #9899's link reuse re-reads callee words inLinkUserDataCallees(theunchangedcompare) with a plainGuestMemory::Read. To keep this PR's guarantee, that read needsDrainShaderWrites(address, words.size() * 4)first. Whichever PR lands second adds it in its merge.673fa865f4), before this PR, so its recorded order (this PR first) no longer applies. ItsReadUserDataCalleechange conflicted with this PR and is resolved in84c3b32b3aas described above.#9899 and #10103 merge cleanly with each other and with current
main.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