Repository navigation
test(agc): keep queued_index_read_order's image results pending past the idle store - #10750
Conversation
… their readers Queue 0's worker stores every pending storage-image result once the queues have stayed idle for 50 ms. A test that orders a pending result against a later consumer races that store: it can land before the consumer is queued, and the order under test is never exercised. APS5_NO_IDLE_STORE=1 turns the idle store off, so pending results stay pending until a reader, a label or an explicit flush stores them. The default is unchanged.
…the idle store Since boykopovar#7233, Driver::WaitIdle also syncs the recorder and waits for the device under the GPU mutex. On lavapipe that takes longer than the queue worker's 50 ms idle quiet period, so the idle store flushes the image write's results as soon as WaitIdle returns. The test then fails its setup check ("the image write left no pending result to order against") on most runs on GitHub's Linux runner. Under load the store can instead land after that check but before the draw holds its read, and the test fails at "the queued draw's commit stored the image's results over its index buffer" although the commit never stored them. The test now sets APS5_NO_IDLE_STORE=1, as CopiedWriterLabelOrder sets APS5_NO_OPPORTUNISTIC_REAP, so only the queued draw's held read decides when the results are stored. The idle store keeps its own tests (agc_idle_publish_wait, agc_driver_store_idle_results_wake).
|
@EagleBearerYS The switch removes the idle store, but leaves a busy loop: with APS5_NO_IDLE_STORE present, idleResultsPending() is always false. Once queue 0 is idle with pending image results and no completions, the retry at QueueWorker.cpp:224-225 sees !storeIdleResults && AnyPending() and continues before changed.wait(). Nothing in that path clears those deliberately retained results, so it spins unlocking/relocking the driver mutex until another consumer arrives. Please gate the pending-result retry on idle-store eligibility while preserving epoch-change/completion wake handling, and add a disabled-store idle/pending regression showing the worker parks and still wakes. Keep the existing held-read assertions and default idle-store tests. |
…lts stay pending With the idle store off, idleResultsPending() is always false, so queue 0's retry after the idle check saw no stored results and pending ones on every pass. Once every queue was idle with results pending and no completions, it went round the loop without reaching changed.wait(), unlocking and relocking the driver mutex until another submission came. The retry for pending results now applies only when the idle store is on. The retry on an idle epoch change and the wait's wake on a submission, an epoch change or a completion are unchanged.
|
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.
|
|
Linux/RADV — the Since the failure on
So on real AMD hardware the setup check holds on AI-assisted: yes. |
MarcZX10
left a comment
There was a problem hiding this comment.
I independently verified the shutdown path described in the existing inline comment at this exact head. With APS5_NO_IDLE_STORE present, idleResultsPending() is false; when queue 0 is idle and storage-image results remain pending, the retry condition after the device-lock section stays true and continues before the worker reaches changed.wait(). This can busy-spin until another consumer happens to clear those results. Please address the existing request to gate the retry on idle-store eligibility while retaining epoch/completion wake handling, and add an idle-pending regression that verifies the worker parks and wakes. I did not run the Vulkan runtime test locally.
agc_driver_store_idle_results_disabled runs the idle store fixture with APS5_NO_IDLE_STORE=1. Once a dispatch leaves a storage result pending and every queue is idle, queue 0 must wait within 2 s, and 200 ms later the result must still be pending and not in guest memory. A second dispatch, to another texture, must then run: its result is pending within 10 s, and queue 0 waits again within 2 s.
|
@oneandonlydean thanks, that was a real spin: with the switch on, the pending-result retry after the idle check never let queue 0 reach 5f0c5f0 adds @MarcZX10 this also addresses your review of the same path. Could you re-review 5f0c5f0? @tablefrogpillow thank you for the runs on three AMD GPUs; they're in Tested now. |
MarcZX10
left a comment
There was a problem hiding this comment.
The PR explains the timing-dependent failure with a concrete cause in StoreIdleResults and keeps the production default unchanged. The opt-out is used only in the order test, while the idle-store behavior remains covered independently; the follow-up also preserves queue 0 waiting when disabled. The reported repeated lavapipe runs, validation-layer comparison, and AMD hardware runs provide useful coverage across the regression and affected queue behavior. I reviewed the head and hosted checks; native jobs are path-skipped on this PR head, so I rely on the described CI/test evidence rather than claiming a local Vulkan run.
Main's boykopovar#9965 pipelines queue-0 DMA_DATA by default and drops APS5_PIPELINE_DMA, including from queued_index_read_order's setup. The merge keeps that and this branch's APS5_NO_IDLE_STORE there.
|
@EagleBearerYS This now conflicts with main: ce6865f ("perf(agc): pipeline queue-0 DMA_DATA by default") made queue-0 DMA_DATA pipelining the default and removed the |
|
@oneandonlydean done in 519e411d96: |
|
A run on real Windows hardware. The Windows runner skips these tests because it has no Vulkan device. Windows 11 Pro 26200, Intel Core i5-14600K, NVIDIA GeForce RTX 5070 Ti (driver 610.88), WinLibs GCC 15.2.0 r7, Release. Head
Full |
|
@SeaSunPT thank you, that's the run on real Windows hardware this PR was missing, with the validation layer on and no |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The updated tests keep the queued image results pending through the idle-store interval, and the author reports same-machine main/change comparisons on head 519e411d965397a509c507b4134f17ea3a0adc0d.
What I ran
Reviewed head 519e411d965397a509c507b4134f17ea3a0adc0d against PR base 63a863d3b07f2457ee1210f1abba1c11409cdff5; the merge preview against current main dc38224141e0b6096007b876b4c42fc9144d11b8 is clean. I inspected the four changed code/test files and the environment documentation. Local checks ran in Linux; no build or compiler was invoked.
git diff --check refs/remotes/upstream/main...refs/review/pr-10750-current
python3 tools/check_conventions.py --base refs/remotes/upstream/main --head refs/review/pr-10750-current
git merge-tree --write-tree refs/remotes/upstream/main refs/review/pr-10750-current
gh pr checks 10750 --repo boykopovar/AnyPS5
gh run view 40188153448 --repo boykopovar/AnyPS5 --json headSha,status,conclusion,displayTitleResults
| Check | Result |
|---|---|
| Diff check | Passed; no output |
| Repository conventions | Passed |
Merge preview with current main |
Clean; tree 8b7f242358c9bccaad60c83e3db0ea0915638d4c |
| Hosted checks | Conventions, eligibility, labels and report passed on exact head; product Linux/Windows and specialized jobs skipped |
| Focused GPU tests | Author reports seven tests pass 5/5 on an RTX 5070 Ti with Vulkan validation, and the six present on main pass 3/3 on the same machine; not independently run here |
Unedited local check output
All conventions checks passed.
8b7f242358c9bccaad60c83e3db0ea0915638d4c
Findings
Blocking
None found in this re-check. The added setup and pending-result assertions address the prior timing sensitivity without pinning the implementation to refusing otherwise valid guest operations.
Non-blocking
The GPU runs are author-provided. I did not run a local build or tests; the hosted Linux/Windows product jobs skipped because no Vulkan device is available to those runners.
Sources
- Idle-store control at this head
- Queued-index and idle-store tests at this head
- RTX 5070 Ti main/change test report
Not verified
No local tests, console behavior, or validation-layer run was performed. The GPU validation and same-machine baseline results above are from the author and tester's report.
✅ 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 (4 IDs in 421 tests)Skipped (42)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 conflict in core/libs/prx/libSceAgcDriver/CMakeLists.txt from boykopovar#12862's split is resolved with tools/agc_test_split.py resolve, which moves this PR's test registrations into tests/cmake/.
✅ 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 (4, 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 476 tests)Skipped (44)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
agc_driver_queued_index_read_order(from #6914) fails onmainon most runs on GitHub's Linux runner (lavapipe), at its setup check: "the image write left no pending result to order against". That check guards the order the test is for, so on those runs the order check never runs.Why. Queue 0's worker stores every pending storage-image result once the queues have been idle for 50 ms (
StoreIdleResults, fromQueueWorker.cpp). The test submits an image write, callsAgcDriverWaitIdle, and expects the result to be still pending. Since #7233,Driver::WaitIdlealso syncs the recorder and waits for the device under the GPU mutex. On lavapipe that outlasts the 50 ms, and the idle store flushes the result as soon asWaitIdlereturns.WaitIdleand its check.^agcrun, the store can instead land after the setup check but before the draw holds its read. The test then fails at "the queued draw's commit stored the image's results over its index buffer", although the commit never stored them. The idle store goes throughFlushPending, which stores nothing while a queued draw holds a read (DrawPipeline::HeldReads).What it does.
APS5_NO_IDLE_STORE=1turns the idle store off (f9b5cc2). The default is unchanged. It is documented indocs/dev/ENVIRONMENT.md.CopiedWriterLabelOrdersetsAPS5_NO_OPPORTUNISTIC_REAP, so only the queued draw's held read decides when the image's results are stored. The idle store keeps its own tests (agc_idle_publish_wait,agc_driver_store_idle_results_wake).docs/dev/ENVIRONMENT.mdsays queue 0 still waits.agc_driver_store_idle_results_disabledruns the idle store fixture withAPS5_NO_IDLE_STORE=1. With a result pending and every queue idle, queue 0 must wait within 2 s, and 200 ms later the result must still be pending and not in guest memory. A second dispatch, to another texture, must then run, so its result is pending within 10 s, and queue 0 must wait again within 2 s.A switch, rather than a change to the idle store: the idle store is run-time behaviour with its own tests, and its 50 ms timer is the one thing in this test it can't control. Lengthening or gating the timer to fit one test would change when every title's pending results are stored.
Tested
Where it broke (GitHub's Linux runner, lavapipe): passes 5 runs in a row at fix(agc): order deferred write-backs after queued draws' index reads #6914's merge 3efab5e (run) and at fix(agc): separate possible writes from complete coverage #7233's parent 8b3a6ba (run); fails its first run at fix(agc): separate possible writes from complete coverage #7233's merge 3ca07d0, with the setup message (run).
mainwithout this PR, Linux: the test failed on its first run, at the setup check, in 4 of 5 runs:main137a8fa (run) and c9b2660 (run);main, the test alone (run);main2fade7e passed its first run (run, the "before" arm), so the failure depends on timing and isn't certain.This head (the same trees as d57418e, on
main2fade7e; run):agc_driver_queued_index_read_order,agc_idle_publish_waitandagc_driver_store_idle_results_wake, 20 runs in a row.Validation comparison (CONTRIBUTING's AGC rule, d57418e,
VK_LAYER_KHRONOS_validationon both arms,ANYPS5_NO_SHADER_CACHE=1; run): the same 3 tests on Linux (llvmpipe, Mesa 25.2.8, Ubuntu 24.04.5), all pass on both arms. The layer loaded 5 times in each. The only VUID isVUID-VkDeviceCreateInfo-pNext-pNext, on both arms; this PR adds none.A parallel
^agcrun with this head (run):agc_driver_pipelined_metadata_pass,agc_driver_pipelined_metadata_pass_draw_pipeline,agc_submit_progressandagc_submit_progress_no_timeline. The same 4 fail in an^agcrun on that runner without this change (run).5f0c5f0, red/green against d57418e (the switch without the wait fix) on GitHub's runners (run): the three tests above,
agc_driver_store_idle_results_disabledand^agc_driver_queue_zero(agc_driver_queue_zero_idle_wait,agc_driver_queue_zero_late_interrupt).agc_driver_store_idle_results_disabledfails with "with APS5_NO_IDLE_STORE queue 0 kept polling over pending storage results with every queue idle instead of waiting". With this head all 6 pass 20 runs in a row, none skipped or failed.APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.Validation comparison at 5f0c5f0 (run),
main2fade7e against this head, the same 6 tests once on Linux (llvmpipe), withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1.main's tree ran the 5 it has, andagc_driver_queued_index_read_orderfailed there at its setup check, the failure this PR is for; this head ran all 6, and they pass. The layer was inserted 8 times onmainand 10 with this PR, neither log haswas not found but was requested, and both report onlyVUID-VkDeviceCreateInfo-pNext-pNext, so this PR adds noVUID-message.Real AMD hardware, by @tablefrogpillow (comment): Linux, RADV (Mesa 25.2.8), kernel 6.8,
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1, the merge-base 2fade7e against d57418e, 25 standalone runs each.agc_driver_queued_index_read_orderpasses 25 of 25 on both trees on an RX 7900 GRE (RDNA3), a Ryzen 7 7800X3D's integrated GPU (RDNA2) and an RX Vega 56 (GCN5). So on that hardwaremain's setup check holds, and the failure is lavapipe's timing, as they say. These are standalone runs; the parallel^agccase wasn't run there.519e411, a merge of
main63a863d (run).main's perf(agc): pipeline queue-0 DMA_DATA by default #9965 made queue-0DMA_DATApipelining the default and removedAPS5_PIPELINE_DMA, including from this test's setup; the merge keeps that and this PR'sAPS5_NO_IDLE_STOREthere, so the test no longer sets the removed variable. The PR's own lines are unchanged.agc_driver_queued_index_read_order,agc_driver_store_idle_results_disabled,agc_driver_store_idle_results_wake,agc_idle_publish_waitand theagc_driver_queue_zerotests pass 10 runs in a row. Linux: 6 run, 6 pass, 0 skip, 0 fail; the before arm failsagc_driver_store_idle_results_disabledandagc_driver_queued_index_read_order. Windows: 6 run, 0 pass, 6 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.NVIDIA GeForce RTX 5070 Ti (610.88), Windows 11 Pro, by @SeaSunPT (comment): Intel Core i5-14600K, WinLibs GCC 15.2.0 r7, Release, this PR's head 519e411 merged onto
main5a2744c and compared with thatmain, withANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1andVK_LAYER_KHRONOS_validation(SDK 1.4.363.0). With this PR,agc_driver_queue_zero_idle_wait,agc_driver_queue_zero_late_interrupt,agc_driver_queued_index_read_order,agc_driver_store_idle_results,agc_driver_store_idle_results_disabled,agc_driver_store_idle_results_wakeandagc_idle_publish_waitpass 5 runs of 5, none skipped; onmainthe six that exist there pass 3 runs of 3, andagc_driver_queued_index_read_orderdid not hit its setup failure on this GPU. NoVUID-line in any log. A fullctest -j 8passes 2380 of 2382; the two failures,agc_host_import_cut_lifetime(agc_host_import_cut_lifetime fails on Windows since the merge of #9159 #10746) andagc_driver_depth_plane_partial_store(AGC: imported depth-plane partial-store test fails refresh-proof assertion on Windows (NVIDIA and AMD) #10664), fail onmainon that machine too.314eff1, a merge of
main0836dfd (run). build(agc): register each AGC test executable in its own CMake fragment #12862 movedlibSceAgcDriver's test registrations intotests/cmake/; the conflict inCMakeLists.txtis resolved withtools/agc_test_split.py resolve, which moves this PR's registration intotests/cmake/agc_driver_store_idle_results_tests.cmake(+5 lines). Nothing else conflicted. Apart from the move, this PR's diff is unchanged. With-DBUILD_TESTING=ON,ctest -Ngives 3076 tests:main's 3075 plusagc_driver_store_idle_results_disabled, each listed once. The 6 tests of this PR's verify set pass 10 runs in a row. Linux: 6 run, 6 pass, 0 skip, 0 fail. Windows: 6 run, 0 pass, 6 skip (no Vulkan device), 0 fail. Suites: libc and libkernel pass on Linux (199 and 308) and on Windows (226 and 330). 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 functions (fix(agc): build the pending image snapshot and the queued index read order test with Apple's libc++ #10676 changes casts in the same test file, on other lines)NotImplemented_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)