Repository navigation
fix(agc): hold a label wait while its producing chain advances - #15198
Conversation
mashingaan
left a comment
There was a problem hiding this comment.
The producer scan stops at the first worker whose unfinishedWrites contains any dword in the waited range. enqueue tracks these writes per queue, so the same guest label can be pending on more than one queue. If the first matching worker is stalled while another matching worker advances toward the value that satisfies this wait, this token never reflects that progress and the new timeout reset can still emit the false timeout this change is meant to avoid. Please either combine progress from every matching producer and follow each dependency chain, or identify the specific producer for the awaited write. A focused regression with the same label outstanding on two queues, one frozen and one advancing, would pin this down.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The producer scan can miss progress when multiple queues have outstanding writes to the same waited label.
What I ran
Reviewed all six changed files at head f626af98d13bdc437f8453ce21b0eb98aa9f8955 against base 9090972b7e91fd266819cbd77a8b058e177b5682; the exact head merges cleanly into that base. Environment: Debian Linux x86_64, Python 3.11.2, Git 2.39.5, GCC 12.2.0. I did not build or compile.
git diff --check 9090972b7e91fd266819cbd77a8b058e177b5682...f626af98d13bdc437f8453ce21b0eb98aa9f8955
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 15198
python3 tools/pr_overlap.py --repo boykopovar/AnyPS5 --base main --branch refs/review/15198-20261102-f626af9
gh pr checks 15198 -R boykopovar/AnyPS5Results
| Check | Result | Note |
|---|---|---|
| Diff and conventions | passed | No whitespace errors; repository conventions passed. |
| Base merge preview | passed | Exact head merges cleanly into its base. |
| Producer-progress review | failed | The scan observes only the first matching producer queue. |
| Precheck helper coverage | not run | Optional ABI and test-quality helpers are absent from the isolated PR snapshot. |
| Hosted checks | not run | All six applicable policy checks were queued at the final refresh. |
| Local C++ tests | not run | They require an exact-head build, which is excluded from the initial review. |
| PR overlap helper | failed | The helper could not parse the GitHub API response; relevant AGC PR paths were checked separately. |
Commands and raw check output
git diff --check 9090972b7e91fd266819cbd77a8b058e177b5682...f626af98d13bdc437f8453ce21b0eb98aa9f8955 exited 0 with empty stdout and stderr.
Precheck output:
/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-f626af98d13b/report.md
/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-f626af98d13b/report.json
Conclusion: checks failed
Relevant precheck rows:
| Check | Status | Command / reason |
|---|---|---|
| diff_check | passed | git diff --check 9090972b7e91fd266819cbd77a8b058e177b5682...f626af98d13bdc437f8453ce21b0eb98aa9f8955 |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base 9090972b7e91fd266819cbd77a8b058e177b5682 --head f626af98d13bdc437f8453ce21b0eb98aa9f8955 --title fix(agc): hold a label wait while its producing chain advances --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-f626af98d13b/pr-body.md |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 15198 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-f626af98d13b/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| changed_export_abi_audit | failed | Isolated PR snapshot lacks tools/pr_abi_audit.py; no exports changed. |
| test_quality_heuristics | failed | Isolated PR snapshot lacks tools/pr_test_quality.py; no tests changed. |
Unedited output excerpts from the precheck report:
All conventions checks passed.
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-f626af98d13b/head-f626af98d13b-cba86e27/tools/pr_abi_audit.py': [Errno 2] No such file or directory
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-f626af98d13b/head-f626af98d13b-cba86e27/tools/pr_test_quality.py': [Errno 2] No such file or directory
Precheck environment output:
{
"os": "Linux-6.1.0-54-amd64-x86_64-with-glibc2.36",
"python": "3.11.2 (main, Oct 7 2026, 12:35:07) [GCC 12.2.0]",
"git": "git version 2.39.5",
"compiler": "c++ (Debian 12.2.0-14+deb12u1) 12.2.0"
}Hosted check output at final refresh:
conventions pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41519279173/job/125530807393
eligibility pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41519279372/job/125530805627
intake pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41519279080/job/125530807569
labels pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41519279269/job/125530806993
library-portability pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41519279371/job/125530804737
report pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41519279338/job/125530804615
The overlap helper emitted:
pr_overlap: Invalid \uXXXX escape: line 1 column 636044 (char 636043)
Negative control
Not run: this change adds no test, and exercising the C++ wait assertions requires building the exact head. No assertion failure is claimed.
Findings
Blocking
findWaitProducerstops at the firstQueueWorkerwhoseunfinishedWritescontains any dword in the waited range.enqueuetracks outstanding label writes separately for each queue, so two queues can both be registered as producers for the same address. If the first matching worker is stalled while another matching worker advances toward the value that satisfies the wait, the returned token does not reflect that progress and the wait can still emit the timeout this change is intended to suppress. Track progress across every matching producer and its dependency chain, or identify the producer that can satisfy this particular wait; add a regression where one of two producers is frozen and the other advances.
Non-blocking
None found in the reviewed six-file diff.
Sources
findWaitProducerselects one matching worker and breaksenqueuerecords label writes per queueforgetUnfinishedWritesdecrements each worker's own pending writes
Not verified
I did not build or run C++ tests, CTest, Linux or console behavior, or a negative control. The PR description reports Windows 11 / NVIDIA RTX 3070 scene runs and failed-to-start focused CTest runs on both main and the change; I did not independently verify those reports. The six hosted checks were queued, and the precheck's optional ABI/test-quality scripts were unavailable in its isolated snapshot.
To get this to a good state
Observe progress from every outstanding producer for the waited address, or otherwise select the producer that can satisfy the wait, and add the two-queue regression. I will rerun the same no-build checks and inspect the new head; the focused C++ tests should also run on the updated head with the required validation environment.
f626af9 to
c208ca7
Compare
|
Pushed What changed
Regression test (
Local execution note: this machine's Vulkan test exes currently fail at load with |
✅ 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)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; 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 517 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. |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The producer scan now considers every matching queue, but XOR can hide synchronized progress and still allow a false timeout.
What I ran
Reviewed all eight changed files at head c208ca7529459d590ee64b1e4c6f63ef89b555cb, against the PR base 9090972b7e91fd266819cbd77a8b058e177b5682. The no-build merge preview against current main 4f41f4c375f5abb487dbd184fbc150f31994d762 was clean. Environment: Debian 12, Linux x86_64, Python 3.11.2, Git 2.39.5, GCC 12.2.0 (not used); no C++ compilation or build.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 15198
python3 tools/pr_test_quality.py --root /tmp/anyps5-15198-test-quality --json core/libs/prx/libSceAgcDriver/tests/Submit.cpp core/libs/prx/libSceAgcDriver/tests/cmake/agc_driver_tests.cmake
gh pr checks 15198 --repo boykopovar/AnyPS5I manually verified the test executable includes Submit.cpp and CMake registers agc_driver_wait_producer_chain; the quality helper’s unregistered-test warning is a false positive.
Results
| Check | Result | Note |
|---|---|---|
| Diff and conventions | passed | Patch and repository checks passed. |
| Current-main merge preview | passed | Clean against 4f41f4c. |
| Producer progress review | failed | XOR can cancel progress from two matching producers. |
| Hosted checks | passed | Windows and policy checks passed; Linux and specialized jobs skipped. |
| Local C++ build and CTest | not run | No build during initial review. |
Commands and unedited check output
$ python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 15198
/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-c208ca752945/report.md
/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-c208ca752945/report.json
Conclusion: checks failed
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | `The precheck deliberately does not configure, build, or compile AnyPS5.` |
| diff_check | passed | `git diff --check 9090972b7e91fd266819cbd77a8b058e177b5682...c208ca7529459d590ee64b1e4c6f63ef89b555cb` |
| conventions_and_pr_template | passed | `/usr/bin/python3 tools/check_conventions.py --base 9090972b7e91fd266819cbd77a8b058e177b5682 --head c208ca7529459d590ee64b1e4c6f63ef89b555cb --title fix(agc): hold a label wait while its producing chain advances --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-c208ca752945/pr-body.md` |
| changed_export_abi_audit | failed | `/usr/bin/python3 tools/pr_abi_audit.py --base 9090972b7e91fd266819cbd77a8b058e177b5682 --head c208ca7529459d590ee64b1e4c6f63ef89b555cb --json` |
| python_syntax | not_applicable | `No changed Python implementation files.` |
| related_python_unit_tests | not_applicable | `No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite.` |
| python_test_registration | not_applicable | `No changed test_*.py files.` |
| live_dependency_and_mergeability_check | passed | `/usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 15198 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-c208ca752945/stack` |
| build_dependent_cpp_tests | not_applicable | `Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build.` |
| test_quality_heuristics | failed | `/usr/bin/python3 /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-c208ca752945/head-c208ca752945-ab39cf31/tools/pr_test_quality.py --root /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15198-c208ca752945/head-c208ca752945-ab39cf31 --json core/libs/prx/libSceAgcDriver/Execution/include/Driver/Driver.hpp core/libs/prx/libSceAgcDriver/Execution/include/Driver/Queues/Submission.hpp core/libs/prx/libSceAgcDriver/Execution/src/Driver/Packets/PacketExecution.cpp core/libs/prx/libSceAgcDriver/Execution/src/Driver/Queues/QueueWorker.cpp core/libs/prx/libSceAgcDriver/Execution/src/Driver/Synchronization/LabelHistory.cpp core/libs/prx/libSceAgcDriver/Execution/src/Driver/Synchronization/WaitMemory.cpp core/libs/prx/libSceAgcDriver/tests/Submit.cpp core/libs/prx/libSceAgcDriver/tests/cmake/agc_driver_tests.cmake` |
| negative_controls | not_applicable | `No changed Python test module in tools/ can be run as a no-build base-source control.` |
$ gh pr checks 15198 --repo boykopovar/AnyPS5
conventions pass 18s https://github.com/boykopovar/AnyPS5/actions/runs/41522232504/job/125540884503
labels pass 10s https://github.com/boykopovar/AnyPS5/actions/runs/41522232426/job/125540891867
report pass 1m1s https://github.com/boykopovar/AnyPS5/actions/runs/41522236749/job/125540896579
windows pass 14m4s https://github.com/boykopovar/AnyPS5/actions/runs/41522236641/job/125547132549
edge-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41522236641/job/125547134683
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41522236641/job/125547134281
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41522236641/job/125547135644
relinker skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41522236641/job/125547134462
relinker-asan skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41522236641/job/125547135004
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41522236641/job/125547134653
eligibility pass 6s https://github.com/boykopovar/AnyPS5/actions/runs/41522236641/job/125540902010
library-portability pass 1m10s https://github.com/boykopovar/AnyPS5/actions/runs/41522236687/job/125540895115
Token calculation reproduced from the reviewed implementation:
same-position token: 0x10000000000
both producers advanced one identical wait packet: 0x10000000000
token_changed: False
Negative control
| Assertion | Change to code under test | Did the test fail? |
|---|---|---|
| A wait’s progress token changes when any matching producer advances. | No runtime mutation or counter-test was run; I evaluated the token expression for two producers with identical packet positions. | Not run; the calculation shows the token can remain unchanged. |
Findings
Blocking
findWaitProducer XORs each matching producer’s motion into one token. If two producer queues hold the awaited label and advance through the same packet cursor/opcode in step, their packet bits cancel; the queue bits stay constant. I reproduced the same token before and after both advance. waitMemory resets its timeout only when this token changes, so it can still report a timeout while both producer chains move. Please use progress tracking that does not cancel across producers and add a regression with two matching producers advancing through identical packet positions before either writes the awaited value.
Non-blocking
None.
Sources
- Producer token aggregation at the reviewed head
- Timeout reset depends on token changes
- Current regression covers one advancing producer and one frozen producer
Not verified
I did not build or run C++ tests, a runtime negative control, Linux tests, a game, or console behavior. The PR’s Windows check passed; the focused runtime checks were not independently reproduced here.
To get this to a good state
Make producer progress distinguishable when multiple queues advance in step, and add the two-producer regression described above. On the new head I will rerun the precheck, inspect the updated diff and hosted checks, and run the focused test if an exact-head test binary is available.
|
@MarcZX10 Fixed at The cancellation: The fix ( The regression ( Verified on Windows/MinGW: AI-assisted: yes. |
✅ AMD 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 (8, not counted against this PR)Validated but not compared (2, not in the main baseline's validation pass)Validation IDs already present on main (8 IDs in 549 tests)Skipped (60)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ NVIDIA GPU test: passedPR head
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 (13 IDs in 549 tests)Skipped (49)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. |
|
The Windows link failure on Verified locally: this PR touches only |
✅ AMD 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 (8, 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 556 tests)Skipped (67)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ 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 (9 IDs in 556 tests)Skipped (55)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. |
97ab670 to
cb06e25
Compare
|
Rebased onto current The earlier windows build failure ( Rebase notes:
|
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked |
What
A
WAIT_REG_MEMlabel wait times out on a fixed 1000 ms budget even when the write it awaits is still queued behind a busy producer. gtavc hit this constantly: 16 logged timeouts in a 620 s run, every one with the producer sitting in queue 0x0's backlog or running (#6558 has the annotated dumps).unfinishedWritesalready records which worker still owes each label dword. This change turns that map into a producer check: while a wait spins, every 100 ms it asksfindWaitProducerwho is going to write the awaited label. The answer folds the producing worker's executing packet and backlog depth into a token, and followsawaitedSlots— so if the producer is itself blocked inside anotherwaitMemory, the walk continues to that label's producer, up to 8 hops. Any change in the token means the dependency chain advanced, so the wait's timeout baseline andStallWatchprogress state reset; a frozen token means the whole chain is static, and the wait warns (and the watchdog can still fire) exactly as before.Nothing about the wait's satisfaction path changes — memory,
storedSince, the label table, GPU-write flushes, and the recorder reaps all still poll identically. The only difference is whether the 1 s warning fires while work the wait actually depends on is still moving.Tested
Windows 11, NVIDIA RTX 3070 (driver 617.42), Grand Theft Auto: Vice City (gtavc dump) relinked against a consistent current-main PRX set.
WAIT_REG_MEMtimeouts in a 620 s run, all with producers pending or running on queue 0x0.awaitedSlotschain walk.VUID-validation errors, gameplay stayed in-game at the same 2.5–3 fps the consistent PRX set produced on main.ctestwait/label tests (agc_driver_same_queue_label_wait,agc_completion_label_wait,agc_idle_publish_wait,agc_pipeline_queue_failure_wait,agc_driver_same_queue_label_wait_draw_pipeline): all exit0xC0000139at start on this machine on unmodified main as well as on this change (pre-existing local environment issue —vulkaninfoSDK --summaryshows instance 1.4.357 withVK_LAYER_KHRONOS_validationloading correctly fromD:/VulkanSDK/Bin, and every static import in the binary chain resolves). No test regressed relative to main; no log containedwas not found but was requested by env var VK_INSTANCE_LAYERS.python3 tools/check_conventions.py --base origin/main: clean.python3 tools/pr_overlap.py --branch fix-label-wait-producer-patience: flagged PR feat(agc): read frame capture deltas with the title's work in flight #15136 on 5 of 6 files; manual comparison shows it is thuanquang's frame-capture delta work (submission serial/store attribution), a different feature — no shared implementation of label-wait patience.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)