Skip to content

test(agc): resubmit an SGPR pattern fill whose batch retired before the read saw it - #8715

Merged
oneandonlydean merged 2 commits into
boykopovar:mainfrom
janzezno:test/sgpr-fill-retired-batch
Oct 20, 2026
Merged

oneandonlydean merged 2 commits into
boykopovar:mainfrom
janzezno:test/sgpr-fill-retired-batch

Conversation

@janzezno

@janzezno janzezno commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What

agc_driver_sgpr_pattern_fill_keys times out under parallel ctest (#8708). ReadAfterWrite waits for Recorder::NewestWriteNote over the keys, which only scans the open and in-flight batches. When the queue worker submits the dispatch and its batch retires before the test thread takes GpuMutex, the note is gone and the loop waits out its 10 s deadline.

Submit now appends a WRITE_DATA of a sequence number after the dispatch. SubmitAndRead replaces each Submit + ReadAfterWrite pair. If the note isn't pending but the label already holds the latest sequence number, the dispatch was recorded (it comes before the label in the DCB) and has retired, so the same fill is submitted again, at most 16 times; past that the test fails with the dispatch completed N times without a pending write over the keys, so a dispatch that never notes its write fails at once instead of at the deadline. The existing assertions are unchanged.

A new case makes the race deterministic: SubmitAndRead(..., retireFirst = true) calls Settle() after the submit, so the fill has retired before the first poll. It asserts the fill was submitted again, then the same pending-0001 key assertions as the main loop.

Tested

Arch Linux, kernel 7.2.11, GCC 16.2.1, Release, BUILD_TESTING=ON, on main e48bbff9f.

  • Forced race (a 200 ms sleep after the submit, not part of this PR): main fails 3 of 3 with the submitted dispatch never became a pending write over the keys after 10.2 s; the first commit passes 3 of 3 in 2.45 s.
  • Mutants of the second commit, each restored afterwards: with the resubmit disabled, the new retireFirst case fails at the 10 s deadline; with the note looked up at the wrong address (a dispatch that never notes), the test fails in 0.09 s with the dispatch completed 17 times without a pending write over the keys.
  • Under load, ctest --test-dir build -j12 -R 'agc|shader|spirv|recomp' --output-on-failure: main failed this test in run 7 of 7; the first commit passes it in 15 of 15 runs (0.10 to 0.13 s), the second in 15 of 15 (0.08 to 0.11 s). The other failures in those runs also fail on main here: agc_driver_guest_memory_lifetime (agc_driver_guest_memory_lifetime fails on RADV since the host-import cut larger-side keep (16473dbbe, #6499 family) #7784), agc_driver_image_array_window_past_layer_limit (test(agc): match array sample expectations to the view #8361), agc_driver_tessellation (agc_driver_tessellation segfaults on RADV with Mesa 26.2+: nir_opt_varyings corrupts the TCS input offset's use list (Mesa bug) #5444), and the occasional agc_driver_pixel_interlock and agc_driver_storage_read_during_idle_store.
  • ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ctest --test-dir build -R '^agc_driver_sgpr_pattern_fill_keys$' -V --repeat until-fail:20: 20 of 20 on main, on the first commit and on the second, no VUID- in either log, no was not found but was requested.
  • python3 tools/check_conventions.py --base origin/main passes.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: Fixes agc_driver_sgpr_pattern_fill_keys times out under parallel ctest load #8708
  • Unblocks: none
  • GPUs tested: AMD Radeon RX 7900 GRE (RADV NAVI31), Mesa 26.2.5, Arch Linux
  • AI-assisted: yes

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry logic matches the reported race, but the deterministic regression described in Tested is not in this diff: the 200 ms post-submit delay was explicitly “not part of this PR.” As a result, normal CI can pass without reaching the new Finished == Submitted recovery branch. Please add a controlled test path that lets one fill retire before ReadAfterWrite begins (for example, an optional test helper that waits for the completion label and queue retirement before polling), then assert that SubmitAndRead resubmits and the original key assertions still pass. That would make the race fix a deterministic regression rather than relying on parallel scheduling.

Comment thread core/libs/prx/libSceAgcDriver/tests/execution/SgprPatternFillKeys.cpp Outdated
@MarcZX10

Copy link
Copy Markdown
Contributor

Thanks, retireFirst now makes the retired-before-read path deterministic, asserts that resubmission occurred, and MaxResubmits bounds recovery. That addresses my earlier coverage request. The current head bc49073a2bd7 has passing conventions and labels checks, but this run has no build/Vulkan job; the reported 15/15 load runs are from before this new path was added. Could you run agc_driver_sgpr_pattern_fill_keys on this exact head and update the report? I have only reviewed the new path statically.

@janzezno

Copy link
Copy Markdown
Contributor Author

Both reviews are addressed in the second commit, along with the bound from your comment on #8708:

  • Deterministic regression: SubmitAndRead(..., retireFirst = true) calls Settle() right after the submit, so the fill has retired before the first poll. The new case asserts resubmits > 0, then the same pending-0001 key assertions as the main loop. With the resubmit disabled it fails at the 10 s deadline, so normal CI now goes through the recovery branch.
  • Bounded retry: capped at 16 resubmits. A dispatch that never notes its write (mutant: note looked up at the wrong address) now fails in 0.09 s with the dispatch completed 17 times without a pending write over the keys, apart from the deadline message.
  • Label read: Finished is now alignas(8) std::uint32_t read through std::atomic_ref<std::uint32_t>(Finished).load(), as in Pm4Replay.cpp.

On the final code: 20 of 20 alone with the validation layer (no VUID-), 15 of 15 loaded -j12 runs. The description is updated.

@MarcZX10

Copy link
Copy Markdown
Contributor

Thanks, that confirms the deterministic path and mutation failure. One validation detail remains: did the unmodified agc_driver_sgpr_pattern_fill_keys pass on head bc49073a2bd7? Please include the OS/toolchain and exact invocation (and report the normal-head pass separately from the resubmit-disabled mutation); the hosted checks on this head run conventions/labels only.

@janzezno

Copy link
Copy Markdown
Contributor Author

Yes. Rerun on head bc49073a2 itself (clean tree, full rebuild):

Arch Linux, kernel 7.2.11, GCC 16.2.1, CMake/CTest 4.4.4, Release, BUILD_TESTING=ON; AMD Radeon RX 7900 GRE (RADV NAVI31), Mesa 26.2.5.

Unmodified head

Mutation (local, not pushed)

  • Resubmit disabled (if (false && retired)): fails at the new retireFirst case after 10.08 s with the submitted dispatch never became a pending write over the keys.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exact-head report resolves my remaining validation request: the unmodified bc49073a2bd7 passed 20 focused Vulkan runs and all 15 parallel selections, and disabling resubmission makes the deterministic retirement case fail as intended. The test diff now forces retirement, asserts a resubmit, and bounds retries at 16. Hosted CI on this head covers conventions/labels only, so I am relying on the author-reported Arch/RADV runtime runs for execution coverage.

@MarcZX10

Copy link
Copy Markdown
Contributor

Follow-up: the author supplied exact-head runtime evidence for bc49073a2bd7: 20/20 isolated focused CTest repetitions and 15/15 parallel-selection repetitions passed on Arch Linux with GCC 16.2.1, CMake/CTest 4.4.4, RADV RX 7900 GRE, and ANYPS5_REQUIRE_VULKAN=1. They also report that disabling resubmission makes the deterministic retireFirst case fail as intended. With the added bounded retry path and deterministic assertion reviewed, I approved the PR; hosted platform build jobs were not run.

@github-actions github-actions Bot added agc libSceAgc* fix error Closes an issue (Fixes #N) labels Oct 20, 2026
@oneandonlydean
oneandonlydean merged commit e28d386 into boykopovar:main Oct 20, 2026
4 checks passed
@janzezno
janzezno deleted the test/sgpr-fill-retired-batch branch October 20, 2026 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* fix error Closes an issue (Fixes #N)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

agc_driver_sgpr_pattern_fill_keys times out under parallel ctest load

3 participants