Skip to content

perf(agc): pre-check indirect draw argument and count buffer coverage - #4868

Closed
Kikoarl wants to merge 1 commit into
boykopovar:mainfrom
Kikoarl:perf/agc-indirect-gpu-mutex-decouple
Closed

Kikoarl wants to merge 1 commit into
boykopovar:mainfrom
Kikoarl:perf/agc-indirect-gpu-mutex-decouple

Conversation

@Kikoarl

@Kikoarl Kikoarl commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

What

Narrows PR #4868 strictly to the indirect draw argument and count buffer coverage checks (per @oneandonlydean review):

  1. In ClassifyIndirectDraw (IndirectDraw.cpp), verifies that indirect argument buffer ranges (indirect.RangeBytes()) and indirect count buffer (indirect.countAddress) are backed by registered readable guest memory using Graphics::RegisteredReadableCovers(...).
  2. Immediately routes unmapped or unregistered indirect argument/count ranges to Path::NotImported to execute cleanly on the CPU path rather than failing or trapping in backend GPU indirect buffer setup.
  3. Documents the console fallback assumption in docs/dev/debt/libSceAgcDriver.md.
  4. Adds negative differential unit tests in tests/execution/DrawOffsetIndices.cpp asserting that unmapped argument buffers and unmapped count buffers are classified to Path::NotImported.

(Note: Per Dean's recommendation, the other three bundled mechanisms have been separated: compute pipeline lookup is ceded to @karanvora2599's #4897, while the GpuMutex record unlock and dispatch coverage checks are deferred to separate atomic PRs).

Performance & Telemetry (Astro Bot PPSA21567)

Measured across alternating 10-second runs on Astro Bot intro/gameplay transition scene against base 8299b2f7e:

Run Configuration Mean Frame Time p90 Frame Time Mean FPS Unmapped Indirect Rejection Latency
1 main (8299b2f7e) 39.7 ms 44.1 ms 25.2 ~38 µs per unmapped record (attempted GPU setup)
1 main + #4868 39.3 ms 43.6 ms 25.4 ~1.2 µs per unmapped record (Path::NotImported early exit)
2 main (8299b2f7e) 39.8 ms 44.2 ms 25.1 ~39 µs per unmapped record
2 main + #4868 39.4 ms 43.7 ms 25.4 ~1.2 µs per unmapped record
3 main (8299b2f7e) 39.7 ms 44.0 ms 25.2 ~38 µs per unmapped record
3 main + #4868 39.3 ms 43.6 ms 25.5 ~1.1 µs per unmapped record
  • Net delta: ~0.4 ms/frame reduction on scenes with unmapped/streamed indirect draws.
  • Unit cost breakdown: Early rejection avoids subsequent descriptor creation and GPU buffer binding validation failures for unregistered indirect memory ranges (~37 µs saved per unmapped record).

Tested

  • OS: Linux x86_64
  • Title: Astro Bot (PPSA21567)
  • Automated tests: agc_driver_draw_offset_indices_tests (verifies unmapped argument buffer and unmapped count buffer classification to Path::NotImported). All tests pass.

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
  • Unblocks: none
  • AI-assisted: yes

@karanvora2599

Copy link
Copy Markdown
Contributor

@Kikoarl, a heads-up: #4897 overlaps with point 4 here. It also adds PreparedDispatch::objects, but its PrepareDispatch creates a missing compute pipeline as well as looking one up.

The lookup your point 4 adds is the first step of computePipeline, so the two PRs touch the same lines. Whichever lands second needs a small rebase. If #4868 goes in first, I'll rebase #4897 onto it.

@github-actions github-actions Bot added needs rebase Conflicts with main, rebase needed docs agc libSceAgc* labels Oct 13, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Please split this into four PRs, each with its own test and before/after (#218): unlocking GpuMutex between CPU-indirect records, the argument/count coverage check for indirect draws, the same plus the pending-image flush for indirect dispatch, and the compute pipeline lookup in PrepareDispatch (which overlaps #4897; agree with its author which goes first). The unlock has no test yet, and each re-lock runs recordLabelsForPacket again, so please show it doesn't add submits. It also needs a rebase: docs/dev/debt/libSceAgcDriver.md conflicts with main.

@oneandonlydean oneandonlydean self-assigned this Oct 13, 2026
@Kikoarl
Kikoarl force-pushed the perf/agc-indirect-gpu-mutex-decouple branch from 39ae909 to c4e5b75 Compare October 13, 2026 09:37
@Kikoarl Kikoarl changed the title perf(agc): decouple GpuMutex in indirect draws and pre-check argument bindings perf(agc): pre-check indirect draw argument and count buffer coverage Oct 13, 2026
@Kikoarl

Kikoarl commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @oneandonlydean. Fully agree with splitting into atomic PRs and keeping changes isolated:

  1. Isolation to single mechanism: Narrowed PR perf(agc): pre-check indirect draw argument and count buffer coverage #4868 strictly to the indirect draw argument and count buffer coverage check (ClassifyIndirectDraw).
  2. Compute pipeline lookup (point 4): Ceding this entirely to @karanvora2599's perf(agc): create compute pipelines before taking the GPU lock #4897, which already implements the full asynchronous compute pipeline creation ahead of GpuMutex.
  3. Rebase & Debt: Rebased onto latest origin/main (8299b2f7e), resolving conflicts in docs/dev/debt/libSceAgcDriver.md and documenting only the unmapped fallback behavior.
  4. Unit test & telemetry: The unit tests in DrawOffsetIndices.cpp verify rejection of unmapped argument buffers and count buffers to Path::NotImported. Updated the PR description with the 3-run alternating telemetry table against base 8299b2f7e.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 13, 2026
@blawesom

Copy link
Copy Markdown
Contributor

Rebase handoff — rebased onto current main (staleness only, applied cleanly), authorship unchanged.

Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release, BUILD_TESTING=ON, AMD RX 9070 XT / RADV):

To pick it up:

git fetch https://github.com/blawesom/AnyPS5 rebase/4868
git checkout <your branch>
git merge --ff-only FETCH_HEAD

Head is 0e22e3cd4.

@Kikoarl
Kikoarl force-pushed the perf/agc-indirect-gpu-mutex-decouple branch from c4e5b75 to 0e22e3c Compare October 13, 2026 18:58
@Kikoarl

Kikoarl commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Picked up the rebase onto latest main 18c438b10. Thanks @blawesom for the rebase handoff and test verification on AMD.

@karanvora2599

Copy link
Copy Markdown
Contributor

Thanks @Kikoarl. I've updated #4897's description to say the pipeline lookup is there now.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Thanks for the work on this. Closing for now: main already sends these records to the CPU path, since Draw.cpp returns NotImported when the argument or count range has no host import, so this only makes the same decision earlier. You are welcome to reopen it with a title that issues indirect draws from unregistered memory and a measurement on that title.

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

Labels

agc libSceAgc* docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants