Repository navigation
fix(agc): stop resolveGroupAhead on unknown writes and reopen gate after HLE - #11439
oneandonlydean merged 8 commits into
Conversation
dd3a169 to
c53c696
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the rebased head c53c696e97cb7b27c12081f9c28f877689b18160 against current main bf77a9e9cba4b02c358779b35295176dd9cae164. The code changes are consistent with the stated ahead-capture safeguards, and I found no additional functional defect in the inspected hunks. However, the PR’s reported 15-test Vulkan comparison uses main acb900c19432 and the prior PR head dd3a169b5682. Current main includes accae4b62d, which changes dispatch adoption and DispatchAhead.cpp in the same paths this PR modifies; the hosted product jobs are skipped. Please rerun the documented focused tests on current main and this rebased head with the stated Vulkan validation settings, compare the logs for new VUID- errors, and update the report before requesting approval.
c53c696 to
eaeafab
Compare
|
@MarcZX10 Rebased onto current |
|
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.
The new head and test report address my earlier request: the PR now tests against main at 9656d366be90 and reports all 16 focused Vulkan tests passing on both main and the change, with no missing-layer messages or VUID- output. I have not independently reproduced the Windows/NVIDIA run.
One merge-order item remains. I ran a three-way comparison with open #8105 from their common base; it has a content conflict in Dispatch.cpp at the declaration after htileClearCompletion: this PR adds codeSpan and #8105 adds Graphics::HtileMaskClear htileMask. CONTRIBUTING.md requires authors to agree on order and declare it with Depends on. Please coordinate with #8105's author and update this PR before merge. I manually checked #7201's Dispatch.cpp hunk (NoteRecentWork near the function start); it is separate from this conflict.
eaeafab to
0f8b37f
Compare
|
@MarcZX10 Dropped the local |
MarcZX10
left a comment
There was a problem hiding this comment.
I rechecked head 0f8b37f008e3e58d3d0307f353caf2c59481f79d. The previous Dispatch.cpp overlap with #8105 is resolved: I confirmed the merge-tree is clean in both merge orders. The hosted conventions, eligibility and report checks pass; AGC product jobs are skipped by path policy.
Two follow-ups remain before approval:
git diff --check upstream/main...upstream/pr-11439reports a new blank line at EOF incore/libs/prx/libSceAgcDriver/tests/execution/WriteDataWrittenAhead.cpp:408.- The branch's merge base and the Vulkan comparison base are
9656d366be90, while currentmainis7903201bb426. I found no intervening changes to this PR's touched source/test paths, but CONTRIBUTING requires branches to stay current. Please sync to currentmainand rerun the documented focused Vulkan-validation comparison on the same GPU, including the no-missing-layer and no-new-VUID-checks.
I did not independently run the Windows/NVIDIA suite. The previous dependency-order finding is withdrawn because the current heads merge cleanly in either order.
0f8b37f to
2c2843d
Compare
|
@MarcZX10 Removed the trailing blank line at EOF in |
1c26236 to
fb6613f
Compare
jotitadev
left a comment
There was a problem hiding this comment.
First NVIDIA run on Linux for this head fb6613fa5.
Linux run: Ubuntu 26.04, GCC 15.2, Release. RTX 3070 (NVIDIA 595.99.02) and lavapipe (Mesa 26.0.3), one at a time via VK_DRIVER_FILES. Khronos validation layer 1.4.341 with ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1; VK_LOADER_DEBUG=layer shows the layer inserted, no "was not found but was requested", 0 VUID- lines.
Base main 808a557ac; the merge is clean.
- Red:
main+ only this PR'sWriteDataWrittenAhead.cpp:agc_driver_write_data_written_aheadand_draw_pipelinefail on both devices withnormal dispatch after rejected ahead capture received the rejected stale variant from global dispatchCache (stale 3, fresh 0). - Green: 16 of the description's 19 focal tests pass 3/3 on each device. I left out the three
agc_shader_result_memo_budget*tests. 0VUID-. ^agcsuite (no flip/display/lds64,-j1): 3070 1052/1052, lavapipe 1051/1052. The one failure isagc_driver_multisample_retirement, which also fails onmain. Onmain,agc_submit_progress_no_timelinefailed once on the 3070. The GPU was shared, and that test passed with this PR.- Builds on Linux/GCC;
public_headerspasses.
GPUs tested: NVIDIA RTX 3070 (Linux), lavapipe.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Thanks for clearing the trailing blank line and reporting the 19 focused Vulkan-validation tests on both
Please sync with current |
Symptom / Log: - agc_driver_write_data_written_ahead: specialized atomic compute shaders with FaultBuffer but no BdaPagetable were classified as unbounded in DispatchWriteRanges, stopping ahead group resolution. Root Cause: - core/libs/prx/libSceAgcDriver/Execution/src/Driver/Memory/WrittenBuffers.cpp:DispatchWriteRanges:L56-L61 — FaultBuffer bindings were treated as unbounded pointer writes even when no BdaPagetable binding was present. Change: - Allow FaultBuffer bindings in DispatchWriteRanges without clearing bounded unless Gds or BdaPagetable is present. Metric / Verification: - DispatchWriteRanges returns bounded == true for descriptor-indexed atomic shaders with FaultBuffer and no BdaPagetable.
Symptom / Log: - 26102026_03_45_39.log:L2476-L2492, 26102026_03_53_05.log:L1968-L1985: VUID-vkCmdCopyBufferToImage-pRegions-00171 / VUID-vkCmdDispatch-None-08600 / EXCEPTION_ACCESS_VIOLATION (0xC0000005) on queue 0x20 after HLE copy and fill dispatches. Root Cause: - core/libs/prx/libSceAgcDriver/Execution/src/Driver/Dispatch/Dispatch.cpp:dispatch:L125-L154 — HLE buffer fill and copy matchers skipped ahead capture without reopening gateOpened() after execution, while BufferCopy.cpp:noteCopyWriter:L131 recorded GPU-backed copies without known values under the copy program address instead of foreign writer policy. Change: - Match dword-pattern, constant-dword, clamped-global, and memory-value buffer fills during resolvingAhead(), reopen gateOpened() when resolvedAhead() is empty after HLE clear/fill/copy execution, and record GPU-backed copies without known values under writer program 0. Metric / Verification: - Post-HLE dispatches synchronize against pending GPU-backed copies and reopen ahead group resolution once resolvedAhead() drains.
…sert Symptom / Log: - 26102026_03_45_39.log:L2476-L2492: resolveGroupAhead continued capturing dispatches after an unknown write (found == end or unbounded write), and dispatch() inserted ahead captures into global dispatchCache before OverlapsLabel / OverlapsRange rejection. Root Cause: - core/libs/prx/libSceAgcDriver/Execution/src/Driver/Dispatch/DispatchAhead.cpp:resolveGroupAhead:L290-L299 and Dispatch.cpp:dispatch:L399 — unknown writes only set unknownWrites = true without stopping the loop, WritesThroughPointers missed StorageImage and GDS write hazards, and insertDispatch ran before overlap checks. Change: - Stop resolveGroupAhead immediately (++stats.unknownWrites; break;) when a dispatch is not resolved or DispatchWriteRanges reports !bounded, and defer insertDispatch (plus compute pipeline prebuild) until OverlapsLabel and OverlapsRange pass. Metric / Verification: - Rejected ahead captures do not insert into global dispatchCache (inserts delta == 1), and resolveGroupAhead stops at unknown or unbounded writes.
… insert Symptom / Log: - Missing regression coverage in WriteDataWrittenAhead for post-HLE ahead gate reopening, rejected ahead capture cache pollution, StorageImage write overlap, and unbounded GDS writes. Root Cause: - core/libs/prx/libSceAgcDriver/tests/execution/WriteDataWrittenAhead.cpp only covered direct PM4 WRITE_DATA before an ahead dispatch. Change: - Extend WriteDataWrittenAhead to test rejected ahead capture cache pollution, CPU-backed and GPU-backed HLE StreamCopy / FillKernel / DwordPatternFill before direct and IndirectGpu dispatches, StorageImage write overlap, and unbounded GDS write stop. Metric / Verification: - ctest -R agc_driver_write_data_written_ahead passes with VK_LAYER_KHRONOS_validation and 0 VUID errors.
fb6613f to
c4ddc2d
Compare
|
@MarcZX10 Rebased onto current |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-check — changes requested for current-main integration\n\nThe author’s 19/19 focused Vulkan-validation result on both main and the PR head addresses the previous test-evidence request. The validation-layer load and zero-VUID reporting are also documented.\n\nThe comparison now uses main 1b580f13cf, while current upstream main is d2b15d8802. Since that base, core/libs/prx/libSceAgcDriver/CMakeLists.txt and docs/dev/debt/libSceAgcDriver.md changed, along with related AGC tests. The merge preview against current main is clean, and I independently confirmed git diff --check and check_conventions.py pass. CONTRIBUTING requires the branch to stay current; please refresh onto d2b15d8802 and rerun the focused comparison on the merge-ready head so the updated registration and debt context are covered.\n\nI did not run the Vulkan tests locally; product GPU checks are path-skipped by hosted CI.
…d-unknown-writes # Conflicts: # core/libs/prx/libSceAgcDriver/Execution/src/Driver/Memory/BufferCopy.cpp
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes requested
The branch now merges current main, but the focused Vulkan comparison still covers the prior integration, including three source files changed by the new base.
What I ran
Reviewed exact head 5cc2975a20f7beb519a072fa14a34e60999be417, whose second parent is current main 9624d8c6bb9e96b7aa92b815f55690f69b0a88d5. I compared the tested main 1b580f13cf7e with current main and inspected the overlap in Driver.hpp, BufferCopy.cpp, and WrittenBuffers.cpp. Environment: Linux x86-64, Git and GitHub CLI; no build or GPU test was run locally.
git diff --check 9624d8c6bb9e96b7aa92b815f55690f69b0a88d5 refs/review/pr-11439-head
git merge-base --is-ancestor 9624d8c6bb9e96b7aa92b815f55690f69b0a88d5 refs/review/pr-11439-head
git diff --name-only 1b580f13cf7e 9624d8c6bb9e96b7aa92b815f55690f69b0a88d5 -- core/libs/prx/libSceAgcDriver/Execution/include/Driver/Driver.hpp core/libs/prx/libSceAgcDriver/Execution/src/Driver/Memory/BufferCopy.cpp core/libs/prx/libSceAgcDriver/Execution/src/Driver/Memory/WrittenBuffers.cppResults
| Check | Result |
|---|---|
| Patch whitespace | Pass |
| Current-main ancestry | Pass; current main is the merge commit's second parent |
| Changes since the reported test base | Driver.hpp, BufferCopy.cpp, and WrittenBuffers.cpp changed on main; those are also modified by this PR |
| Focused Vulkan comparison | Author reports 19/19 tests and negative controls on main 1b580f13cf7e and head c4ddc2d40465; this is before the current-main merge and does not cover the three overlapping main updates |
| Hosted exact-head jobs | Windows in progress; conventions and labels queued; report and eligibility pass; Linux/product and other specialized jobs skipped |
| Local build/GPU tests | Not run |
Unedited local command output
diffcheck-exit=0
current-main-is-ancestor-exit=0
c4ddc2d404657c35b6822c37dc3c4311edf76872 9624d8c6bb9e96b7aa92b815f55690f69b0a88d5
Findings
Blocking
- The current-main merge changes the
WrittenBufferIndexstorage and writer-recording paths in files this PR modifies. The previous 19-test comparison used main1b580f13cf7eand headc4ddc2d40465, so it does not validate the combined behavior now submitted at5cc2975a20f7. Please rerun the focused Vulkan suite and the relevant negative controls on current main9624d8c6bb9eand this exact merged head, with validation-layer load andVUIDresults recorded.
I found no additional source-level defect in the rechecked diff.
Sources
- Current-main testing and branch requirements
- Exact current-main changes in the overlapping AGC files
- PR head
Not verified
I did not build or run the current merged head locally or on a GPU. The exact-head Windows job was still running; conventions and labels were queued. Linux product and specialized jobs were skipped.
To get this to a good state
Rerun the same 19 focused tests and issue-specific negative controls against current main and exact head 5cc2975a20f7, recording the required Vulkan layer and VUID checks. I will recheck the resulting head and hosted statuses.
|
@MarcZX10 Merged current |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes requested
The exact-head regression evidence and current-base merge are satisfactory, but the branch conflicts with two open PRs and its dependency checklist still says none.
What I ran
- PR head:
0711645eef72b5e26934a6245c417bfe52bd5b2f. - Base:
upstream/mainatd820fef1827f27247cd28da7b4eec7891904bae5; the merge preview was clean (99eca890510413d8115e076ea406fec1aa377cba). No files touched by this PR changed onmainafter the tested basec40cef843681. - Environment: Linux x86_64; no compiler, build, or GPU tests run locally.
git diff --check upstream/main...refs/review/pr-11439-head
git merge-tree --write-tree upstream/main refs/review/pr-11439-head
python tools/check_conventions.py --base upstream/main --head refs/review/pr-11439-head
git merge-tree --write-tree refs/review/pr-11439-head refs/review/pr-12147-head
git merge-tree --write-tree refs/review/pr-11439-head refs/review/pr-12657-headResults
| Check | Result |
|---|---|
| Patch whitespace | Pass; command exited 0 with no output. |
| Current-main merge preview | Pass; clean tree 99eca890510413d8115e076ea406fec1aa377cba. |
| Repository conventions | Pass. |
| Pairwise integration with #12147 | Conflict in core/libs/prx/libSceAgcDriver/Execution/src/Driver/Dispatch/Dispatch.cpp. |
| Pairwise integration with #12657 | Conflict in the same Dispatch.cpp. Both PRs remain open; #12657's description says it depends on #12147. |
| Hosted CI | Windows product job passed; eligibility, conventions, labels, and report checks passed. Linux and specialized jobs were skipped. |
| Focused Vulkan tests | The PR description reports 19/19 focused Vulkan validation tests on both main c40cef843681 and this head, with the validation layer loaded and zero VUID errors. This is author-reported; I did not run the GPU tests. |
Unedited local command output
python tools/check_conventions.py --base upstream/main --head refs/review/pr-11439-head
All conventions checks passed.
git merge-tree --write-tree upstream/main refs/review/pr-11439-head
99eca890510413d8115e076ea406fec1aa377cba
git diff --check upstream/main...refs/review/pr-11439-head exited 0 and produced no output.
Both pairwise merge previews exited 1 and reported:
CONFLICT (content): Merge conflict in core/libs/prx/libSceAgcDriver/Execution/src/Driver/Dispatch/Dispatch.cpp
Findings
Blocking
- Unresolved PR overlap and merge order: Pairwise merge previews conflict in
Dispatch.cppwith open #12147 and #12657. The PR currently listsDepends on: none.CONTRIBUTING.mdrequires authors to agree on which overlapping PR goes first and declare that order withDepends on(pinned contribution rule). Please coordinate the order with the affected authors and update the dependency line; then I can recheck the combined merge.
Non-blocking
- No additional functional issue was identified in this source-level recheck. The reported GPU test comparison is author-provided, not independently reproduced here.
Not verified
- I did not build or run tests locally; this environment has no verified Vulkan device. Linux, macOS, and specialized hosted jobs were skipped.
To get this to a good state
Please record the agreed order for the overlapping PRs in Depends on. I will rerun the merge checks and review the updated head.
|
@MarcZX10 Kept the |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The update preserves the unrelated prebuild block, and I confirmed the branch combines cleanly with current main and both cited PRs in either merge order.
What I ran
Reviewed head cb5d7fe3ef741780004cfd8e4e677d1b033e7311 against current main 5ecef095b61e7a406b279c97896f3a8a6fbd6353; current-main merge tree is 907b51b66dc5a7a17e45994202854189394b9941.
git merge-tree --write-tree refs/remotes/upstream/main refs/remotes/upstream/pr-11439
git diff --check refs/remotes/upstream/main...refs/remotes/upstream/pr-11439
git merge-tree --write-tree refs/remotes/upstream/pr-11439 refs/remotes/upstream/pr-12147
git merge-tree --write-tree refs/remotes/upstream/pr-12147 refs/remotes/upstream/pr-11439
git merge-tree --write-tree refs/remotes/upstream/pr-11439 refs/remotes/upstream/pr-12657
git merge-tree --write-tree refs/remotes/upstream/pr-12657 refs/remotes/upstream/pr-11439Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean |
| Diff check | Passed; no output |
| Merge with #12147 | Clean in both orders |
| Merge with #12657 | Clean in both orders |
| Hosted conventions and Windows | Passed |
| Focused Vulkan tests | Author reports 19/19 on the updated head, with validation enabled and zero VUIDs; not independently run |
Not verified
I did not run a local build or Vulkan tests. Product Linux and specialized hosted checks were skipped for this change.
✅ NVIDIA GPU test: passedPR head
Unstable on main (4, 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)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; 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. |
What
When
Driver::resolveGroupAheadscans a compute command buffer ahead of an in-flightWAIT_REG_MEMgate, it captures SRT/BDA memory for upcoming dispatches so their shaders and resources are ready before the gate opens. Four issues in the ahead-resolution path caused it to either capture stale pre-write descriptors across earlier GPU writes, pollute the globaldispatchCachewith rejected captures, or stop ahead resolution prematurely:FaultBuffer-only shaders bounded inDispatchWriteRanges(WrittenBuffers.cpp):DispatchWriteRangestreatedbinding.kind == BufferBindingKind::FaultBufferthe same asGdsandBdaPagetable(bounded = false). However,FaultBufferis an internal fault-reporting buffer added by the recompiler to specialized descriptor-indexed atomic shaders and does not write to arbitrary guest memory addresses. TreatingFaultBufferas bounded (unlessGdsorBdaPagetableis present) prevents specialized compute shaders from prematurely terminating ahead group resolution.resolvingAhead(), record unvalued GPU copies, and reopengateOpened()after HLE (Dispatch.cpp,BufferCopy.cpp):resolvingAhead(),tryDwordPatternFillBuffer,tryConstantDwordFillBuffer,tryClampedGlobalFillBuffer, andtryMemoryValueFillBufferwere bypassed (!resolvingAhead()), causing buffer-fill dispatches to be captured as regular compute shaders instead of stopping ahead resolution at the HLE fill boundary.resolvedAhead()was empty,closeGate()closed the ahead gate without reopening it for the dispatches that follow the HLE operation in the same group; reopeningopenGate()whenresolvedAhead_.empty()allows subsequent dispatches in the submission to be resolved ahead against the updated post-HLE state.Driver::noteCopyWriter, GPU-backed HLE copies (!cpuDone) that did not populateknownCopyValues(e.g., when the source range had a pending GPU write) only calledrecorder->notePendingWritewithout recording a writer inwrittenBuffers, soDispatchWriteRangesandpendingWritetreated the destination as having no writer. RecordingwrittenBuffers->noteWrite(destination, size, 0)marks the destination under foreign writer policy (program == 0).Driver::resolveGroupAheadon unknown writes and deferdispatchCacheinsertion (Driver.hpp,Dispatch.cpp,DispatchAhead.cpp):Driver::resolveGroupAheadfromWrittenBufferRanges(which only inspects direct SRTV#buffers and misses BDA table buffers,StorageImagewrites, and unbounded flags) toDispatchWriteRanges. When a dispatch is not resolved orDispatchWriteRangesreports!bounded,resolveGroupAheadincrementsstats.unknownWritesand breaks immediately (break;) instead of continuing to capture later dispatches that may depend on the unknown write.bool cacheInserted = falsetoDriver::CaptureResultand deferredinsertDispatchduringresolvingAhead()untilresolveGroupAheadverifies!OverlapsLabeland!OverlapsRange, preventing rejected ahead captures from polluting the globaldispatchCachewith stale pre-write snapshots.WriteDataWrittenAhead.cpp):agc_driver_write_data_written_aheadto verify that rejected ahead captures do not insert intodispatchCache, that CPU-backed and GPU-backed HLEStreamCopy,FillKernel, andDwordPatternFillupdate descriptors before subsequent direct andIndirectGpudispatches, and thatStorageImagewrite overlap and unboundedGdswrites stop ahead resolution cleanly.Tested
ctest --test-dir build-portable -R "^(agc_shader_table_snapshot_reuse|agc_shader_table_snapshot_reuse_unmasked|agc_shader_type_zero_image_materialization|agc_shader_memory|agc_shader_result_memo_budget|agc_shader_result_memo_budget_threads|agc_shader_result_memo_budget_masked|agc_driver_constant_dword_fill|agc_driver_dispatch_ahead_adopt_check|agc_driver_gated_dispatch_capture|agc_driver_gpu_indirect_arguments_written_ahead|agc_driver_indirect_arguments_written_ahead|agc_driver_pipelined_dispatch|agc_driver_stream_copy|agc_driver_write_data_written_ahead|agc_driver_write_data_written_ahead_draw_pipeline|agc_driver_gated_dispatch_capture_draw_pipeline|agc_driver_gated_dispatch_capture_short_timeout|agc_driver_pipelined_dispatch_item)$" -VwithANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1, andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationon bothmain(08a10d225306) and this branch (cb5d7fe3ef74) — all 19 tests pass, neither log containswas not found but was requested by env var VK_INSTANCE_LAYERS, and 0VUID-validation errors are logged.agc_driver_write_data_written_aheadfail deterministically onmain(rejected ahead capture polluted global dispatchCache with an extra insertandHLE StreamCopy (CPU-backed): dispatches after descriptor update used stale descriptor (stale 2, fresh 0)) and pass on this branch (Fresh[0] == 2..3, Stale[0] == 0).PPSA01341(Demon's Souls v1.07): verified thatDriver::resolveGroupAheadno longer captures stale pre-copy/pre-fill descriptors (fixingVUID-vkCmdCopyBuffer-size-00115and intermittentEXCEPTION_ACCESS_VIOLATIONcrashes inINTRO), stops on unknown writes, and reopens ahead resolution after HLE copies/fills.Before (
main):After (this PR):
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)