Skip to content

feat(libSceAgcDriver): pin draw committer thread on hybrid architectures - #7246

Closed
al3w0f205 wants to merge 1 commit into
boykopovar:mainfrom
al3w0f205:perf/agcdriver-async-flip-pipeline-and-committer-pinning
Closed

al3w0f205 wants to merge 1 commit into
boykopovar:mainfrom
al3w0f205:perf/agcdriver-async-flip-pipeline-and-committer-pinning

Conversation

@al3w0f205

@al3w0f205 al3w0f205 commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

What

Pins the background draw committer thread to worker affinity via PinWorkerThread("draw_committer") at worker start in DrawPipeline.cpp. This aligns the draw committer with hybrid CPU performance scheduling (when APS5_WORKER_AFFINITY is active) and avoids thread migration latency spikes during draw queue processing.

The flip completion latency changes in Presentation.cpp have been reverted to preserve GPU completion guarantees and keep video_out_flip_tests aftergpu passing.

Tested

tools/check_conventions.py --base origin/main passes. agc_driver_pm4_tests.exe and agc_driver_graphics_tests.exe pass on Windows.

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
  • GPUs tested: NVIDIA GeForce RTX 4070 Laptop GPU, driver 572.16, Windows 11
  • AI-assisted: yes

@github-actions

github-actions Bot commented Oct 17, 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 1896 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp
PR 6188 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp
PR 7642 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp
PR 10193 core/libs/prx/libSceAgcDriver/Execution/src/Driver/Draw/DrawPipeline.cpp

@github-actions github-actions Bot added the agc libSceAgc* label Oct 17, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Thanks for looking at flip latency. The Presentation.cpp change can't go in: the trailing FinishPresent() is the fix from d9632d5 ("complete a flip only once the frame's GPU work has finished"). Without it the flip completes as soon as the blit is queued, titles reuse the frame's memory while the GPU still reads it, and PPSA21564 produced runaway shader loops and wrong matrices that way. The wait already happens outside the GPU mutex, after QueuePresent. If flip latency is the target, please open an issue with frame-time captures (APS5_PROFILE_DRAW) and a title, and any change to when the flip completes needs to keep video_out_flip_tests aftergpu passing on a real GPU. The draw-committer pin is reasonable on its own; please submit it as a separate PR with a measurement on a hybrid CPU, where the pin actually applies (it is a no-op on Windows without APS5_WORKER_AFFINITY).

@cadenasynyster

Copy link
Copy Markdown
Contributor

Ran the VideoOut flip tests with a real window, which CI skips (offscreen SDL has no Vulkan surface): with this PR, flips complete before the GPU work submitted ahead of them.

Windows 11 10.0.26200.9711, RTX 5070 Ti (NVIDIA 617.42), WinLibs GCC 15.2.0 r7, Release. Head 700b4bc merged onto main f3263df (clean merge, no new build errors). All 20 video_out_flip_tests cases, run directly from build\core\libs\prx\libSceVideoOut, main vs this PR, one run each:

case main PR
pacing pass fail (exit 1)
aftergpu pass fail (exit 1)
other 14 non-EOP cases pass pass
eop, eop_main, eop_aftergpu, unavailable fail fail (same as main; EOP is #6616, unavailable expects no display)

aftergpu timing line:

main: GPU work landed at 171.2 ms, flip completed at 171.5 ms
PR:   GPU work landed at 171.9 ms, flip completed at 16.0 ms

So the flip is reported complete ~155 ms before the dispatch queued ahead of it finishes, which is what aftergpu's flipped + 20000 >= landed check guards against.

This looks like the removed trailing block in Driver::Present: with inFlight != 0, the frame is queued with QueuePresent() and gpuReady(context) is then called without the trailing FinishPresent(), so VideoOut counts the flip (flipStatus.count, flipPendingNum) before the frame's GPU work is done. I haven't tested the PinWorkerThread("draw_committer") part on its own.

AI-assisted: yes

@al3w0f205 al3w0f205 changed the title perf(libSceAgcDriver): unlock asynchronous in-flight flip pipeline and pin draw committer thread perf(libSceAgcDriver): pin draw committer thread on hybrid architectures Oct 17, 2026
@al3w0f205

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed verification and testing. I have reverted the Presentation.cpp changes to preserve the trailing FinishPresent() GPU completion guarantee and keep video_out_flip_tests (aftergpu and pacing) passing. This PR is now focused strictly on pinning the draw committer thread on hybrid CPU topologies.

@al3w0f205
al3w0f205 force-pushed the perf/agcdriver-async-flip-pipeline-and-committer-pinning branch from 700b4bc to 8dbd8de Compare October 18, 2026 01:27
@cadenasynyster

Copy link
Copy Markdown
Contributor

Rechecked 8dbd8de with a real window, same setup as my earlier comment (Windows 11 26200, RTX 5070 Ti, NVIDIA 617.42, WinLibs GCC 15.2.0 r7, Release), merged onto main 9f25d0b:

  • aftergpu passes: GPU work landed at 176.6 ms, flip completed at 177.0 ms. The early flip completion I reported is gone.
  • pacing fails (the next flip is not paced from the vblank this flip was released at), but it fails the same way on main 9f25d0b without this PR, 3 of 3 runs each. It passed on main f3263df in my earlier run, so it comes from main, not from this PR. I'm looking into that separately.

Not tested: the draw-committer pin itself. This machine is a Ryzen 7 9800X3D (no hybrid cores), and APS5_WORKER_AFFINITY was unset.

AI-assisted: yes

@MarcZX10

Copy link
Copy Markdown
Contributor

Coordination note from new issue #7581: the pacing case now fails 3/3 on main as well as on the PR's tested base. I inspected current testReleaseVblank and VideoOutDriver::processFlip: the test submits only one flip, while processFlip waits for the configured interval before release and updates lastFlipVblank immediately before presentation. The test then expects another three vblanks before that same flip's completion, although the configured interval is used to pace a subsequent flip. I posted the source-based reasoning and a possible second-flip regression shape on #7581. This may explain the baseline test failure independently of the pinning change; I have not run the RTX setup.

@cadenasynyster

Copy link
Copy Markdown
Contributor

Rechecked 8dbd8de with a real window, same setup as my earlier comment (Windows 11 26200, RTX 5070 Ti, NVIDIA 617.42, WinLibs GCC 15.2.0 r7, Release), merged onto main 9f25d0b:

Correction to my earlier comment: I listed pacing as passing on main f3263df. That run used a test binary left over from the older output folder; rebuilt from f3263df it fails too.

Not tested: the draw-committer pin itself. This machine is a Ryzen 7 9800X3D (no hybrid cores), and APS5_WORKER_AFFINITY was unset.

AI-assisted: yes

@al3w0f205

Copy link
Copy Markdown
Contributor Author

Thanks for verifying the aftergpu pass with the real window and for tracking down the pacing baseline failure on main via #7581 and #7583.

@github-actions github-actions Bot added perf Performance change perf numbers missing perf pull request without a before and after summary line needs rebase Conflicts with main, rebase needed labels Oct 18, 2026
@adriadam10 adriadam10 self-assigned this Oct 19, 2026

@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.

Thanks for reverting the Presentation.cpp part. Two things left:

  • This is a perf PR but has no before/after numbers. Please post them from a hybrid CPU with APS5_WORKER_AFFINITY=1 vs unset (e.g. APS5_PROFILE_DRAW frame times or draw committer stall counters, several runs, title/PPSA named). Pinning is off by default, so without data it is unclear what it improves.
  • The PR now conflicts with main: DrawPipeline::run() begins with PerformanceTrace::BindThread("draw committer"); and there is a new PerformanceTrace.hpp include. Rebase and keep both (PinWorkerThread("draw_committer") after the BindThread line, the WorkerAffinity.hpp include alongside).

@al3w0f205
al3w0f205 force-pushed the perf/agcdriver-async-flip-pipeline-and-committer-pinning branch from 8dbd8de to 35ed3b7 Compare October 19, 2026 17:30
@github-actions

Copy link
Copy Markdown

This pull request is labeled perf, so it needs the summary line of a capture on main and one with the change, from the same title, scene and machine, as the Performance report form asks. Please paste both aps5-summary.jsonl lines, attach the two traces and add the output of tools/trace_digest.py.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 19, 2026
@al3w0f205
al3w0f205 force-pushed the perf/agcdriver-async-flip-pipeline-and-committer-pinning branch from 35ed3b7 to c1e6a2d Compare October 20, 2026 02:35
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 21, 2026
@al3w0f205
al3w0f205 force-pushed the perf/agcdriver-async-flip-pipeline-and-committer-pinning branch from c1e6a2d to 3bc428f Compare October 21, 2026 12:15
@al3w0f205 al3w0f205 changed the title perf(libSceAgcDriver): pin draw committer thread on hybrid architectures feat(libSceAgcDriver): pin draw committer thread on hybrid architectures Oct 21, 2026
@github-actions github-actions Bot removed perf Performance change perf numbers missing perf pull request without a before and after summary line needs rebase Conflicts with main, rebase needed labels Oct 21, 2026

@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.

Thanks for reverting the Presentation.cpp part; the rebase onto main is clean now. One point is left, and only you can provide it:

  • This is a perf PR but has no measurements. Please post the aps5-summary.jsonl line of a capture on main and one with the change (APS5_WORKER_AFFINITY=1 vs unset), from the same title (PPSA named), scene and hybrid-CPU machine, several runs, the two traces and the tools/trace_digest.py output, as the pr-labels-perf bot asks. Pinning is off by default, so without numbers it is unclear what this improves.

We can't produce this ourselves (no hybrid CPU or title here). Without data we'll close this as stale later; feel free to reopen with it.

@al3w0f205 al3w0f205 closed this Oct 26, 2026
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