Skip to content

test(agc): account for unknown pending DCC key writes - #5883

Open
AnzouBelilita wants to merge 1 commit into
boykopovar:mainfrom
AnzouBelilita:fix/agc-recorder-pending-write-proof-test
Open

AnzouBelilita wants to merge 1 commit into
boykopovar:mainfrom
AnzouBelilita:fix/agc-recorder-pending-write-proof-test

Conversation

@AnzouBelilita

Copy link
Copy Markdown

What

Update the recorder key-proof test to match the current pending-DCC-write behavior. A generic pending write has no known key value, so ProvedClearKeys must synchronize before it can cache a stable proof. Known pending key fills are already covered by pendingKeyStoreTests.

The old assertion expected repeated unstable scans for an unknown pending write; on current main that expectation fails because PendingStoreKeys correctly waits when it cannot infer the value. This is the same baseline recorder-test failure noted in #5497, but this PR changes only the stale test expectation, not the pending-fill implementation.

Tested locally

  • cmake --build . --target agc_driver_visual_test agc_driver_recorder_tests --parallel 4 — passed on 28b6f1987917e59f3a7bbcad4d0acb8b0f58b519.
  • tests\agc_driver_visual_test.exe --verify — passed on Radeon RX 580; GPU readback verified.
  • tests\agc_driver_recorder_tests.exe — passed with default settings after this test correction (exit code 0).
  • tests\agc_driver_recorder_tests.exe --pipeline-library-only — exits successfully, but this GPU reports pipelineLibrary=0, so the positive graphics-pipeline-library path is not exercised.
  • python tools\check_conventions.py --base upstream/main — passed.
  • git diff --check — passed.

Refs #5497.

@Dextroid17

Copy link
Copy Markdown
Contributor

I ran this on Linux because my #6155 depends on it (its test run stops at your (9) check without your change).

Setup: main ab9a476 with this PR's change (one hunk in Recorder.cpp) applied; the PR head is 478 commits behind current main. Default CMake build (CMAKE_BUILD_TYPE empty, unoptimized), ANYPS5_NO_SHADER_CACHE=1, lavapipe (Mesa 26.2.4 llvmpipe, LLVM 23.1.1). agc_driver_recorder_tests with no arguments, 3 runs each:

So on lavapipe the new expectations match main's current behaviour and clear the (9) stop. Whether synchronizing before caching a proof is the intended behaviour I did not check. I did not check the RX 580 run or Windows.

The red conventions check has 4 errors. I reproduced all 4 locally with main's tools/check_conventions.py against the same base (f343489), passing this PR's title and body:

  • Recorder.cpp:1171: the new // An unknown pending write cannot supply DCC keys... line counts as a code comment, which needs a change to docs/dev/TechnicalDebt.md or docs/dev/debt/ in the same PR. With only that line deleted, the checker no longer reports it.
  • 'What', 'Tested' and 'Checklist' are reported missing because the checker only matches ### headings with those exact names, and the description uses ## and "Tested locally". With ### What and ### Tested, those two clear. The checker then wants a ### Checklist section with no unticked boxes, "Depends on" filled with #N or none, and "AI-assisted" answered yes or no; the template's checklist covers this.

The template checks only run locally when --title or --body is passed, which may be why a plain local run passed.

AI-assisted: yes

This was referenced Oct 15, 2026
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 1368 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 1395 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 5021 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 5925 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 6186 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 6187 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 6188 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 6192 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 6209 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 6747 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 6868 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 6946 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 6947 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 6971 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 7048 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 7505 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 7604 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 7642 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp
PR 8484 core/libs/prx/libSceAgcDriver/tests/Recorder.cpp

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Checked on Linux (lavapipe, PR merged onto current main): agc_driver_recorder_tests now gets past check (9) but still exits 1 at "AGC graphics: second draw observed overwritten input", so the test is not green with this PR alone (same as @Dextroid17 reported; they say it needs the MarkWritten calls from #6155). The target is EXCLUDE_FROM_ALL and not in ctest, so CI will not show it either.

  • Land the fix for the later failure together with this one (or make this PR depend on #6155 and list it under "Depends on"), so the recorder test passes end to end.
  • core/libs/prx/libSceAgcDriver/tests/Recorder.cpp:1327: the new // line counts as a code comment and the conventions check requires a TechnicalDebt change for it; drop the comment.
  • PR body: the headings must be ### What, ### Tested and ### Checklist (the checklist fully ticked, "Depends on" filled, "AI-assisted" yes/no); the conventions job is red on these.

@MarcZX10

Copy link
Copy Markdown
Contributor

One dependency-order detail from the recorder work: #6155 currently lists #5883 as its dependency, while the review here suggests waiting for #6155 to clear the later recorder failure. Reversing that edge would make the two PRs depend on each other. The exact composition of current heads (#5883 + #6155 + #6642) passed agc_driver_recorder 3/3 on both RTX 3070 and lavapipe, as documented in #6642. Please coordinate the intended merge order or combined change with maintainers; the composition result supports the combined behavior, but does not resolve the current dependency cycle.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc*

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants