Repository navigation
perf(agc): present on a queue of its own outside the GPU mutex - #9670
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
|
A Linux run on RADV and lavapipe, which take the fallback path: both expose a single queue in their graphics family, so this PR can't take a presentation queue there. Ubuntu, kernel 7.0, Wayland. Ryzen 5 5625U iGPU (Vega/GCN 5, RADV RENOIR, Mesa 26.0.8) and lavapipe (llvmpipe, LLVM 21.1.8), GCC 15.2.0, Release. I compared
So on Mesa's single-queue graphics family, this changes nothing and breaks nothing. I didn't measure frame times, since the path is the same as |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The implementation releases GuestMemory::GpuMutex before vkQueuePresentKHR only when the device obtained a distinct queue, and keeps presentQueueMutex around both presentation and vkDeviceWaitIdle; the no-free-queue fallback stays on the main queue. The reported paired validation-layer tests, Windows GPU measurements, full fork CI result, and current hosted checks are useful. The PR is mergeable with current main.
Please resolve the remaining repository review items before merge:
- The overlap bot identifies open #7647 in the same queue-planning block. #7647 replaces the inline settle/sparse queue selection with
PlanDeviceQueues, while this PR adds a presentation index to that selection. The PR body acknowledges whichever lands second must adapt, but it does not state an agreed merge order or listDepends on: #7647(or the reverse). Please coordinate with that author and declare the order on the checklist as CONTRIBUTING requires. VulkanDevice.hppstill documentsQueuePresentas running underGuestMemory::GpuMutex(the comment aboveFlipInFlight). This PR runs it outside that mutex when using the new queue. Please update the comment and record the required explanation in technical debt so the conventions check accepts the comment change.- CONTRIBUTING requires the performance traces to be attached. The description names
hades-traces-main-da54c652e.zipandhades-traces-pr-7343b1875.zip, but I can find only those filenames in the PR body and no attachment links in the conversation. Please attach/link both captures so the reported A/B summaries can be audited.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The additional Linux RADV/lavapipe report is useful: it verifies the single-queue fallback, runs the flip/VideoOut test set with a real Wayland swapchain, and reports 36/36 passing on both drivers with the validation layer loaded and no new VUIDs. That strengthens the portability evidence beyond the Windows queue path.
The three items from the earlier review still appear unresolved:
- Open #7647 changes the same device queue selection. I could not find an author agreement or a checklist Depends/merge-order entry. Please coordinate and record which PR lands first and what the second rebase must preserve.
- VulkanDevice.hpp still describes QueuePresent as running under GuestMemory::GpuMutex. The updated implementation releases that mutex when it uses the presentation queue. Please update the stale comment and include the required technical-debt change so the conventions rule accepts it.
- The Hades body names two trace ZIP files, but I still see no attachment links to either capture. Please attach or link both so the performance summary can be audited as CONTRIBUTING requires.
I did not rebuild locally; the author and independent reviewer have supplied Windows and Linux test results, and the current hosted product jobs are skipped under repository policy.
|
@MarcZX10 thanks. The three items:
|
|
Windows run on NVIDIA (new path) and Intel (fallback), which the description lists as not run on Intel. Windows 11 Pro 10.0.26300, WinLibs GCC 15.2.0 r7, Release. NVIDIA GeForce RTX 3060 Ti (driver 617.42) and Intel Arc Graphics (Meteor Lake,
No performance measurement here. AI-assisted: yes (Claude Code) |
vkQueuePresentKHR blocks the presenter for a frame's pacing on some drivers (about 6 ms per present on NVIDIA in FIFO mode), and it ran under GuestMemory::GpuMutex because the main queue needs external synchronization with the recorder's submits. When the presentation family has a free queue, the device now takes it as a presentation queue guarded by its own mutex, and the presenter hands the image to it after releasing the GPU mutex. WaitIdle takes that mutex around vkDeviceWaitIdle. Without a free queue the present stays on the main queue under the GPU mutex as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ue fallback Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4d228cd to
008e327
Compare
|
Rebased onto
@BKlaasWerkman thanks for the runs on the RTX 3060 Ti and Intel Arc. They're cited in the description, and the debt entry now names Intel's Windows driver next to RADV and lavapipe. |
Dismissing a stale review: the requested changes were addressed after it (new head and/or updated description with the asked-for runs) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked the presentation queue in |
What
The presenter held
GuestMemory::GpuMutexacrossvkQueuePresentKHR, which on NVIDIA in FIFO mode blocks for about 6 ms per present. It held the mutex only because the mainVkQueueneeds external synchronization with the recorder's submits, and the draw committer waited behind it: about 5 ms of every frame in Hades (PPSA03355).When the presentation family has a free queue, the device now takes it as a presentation queue with its own mutex, and the presenter presents on it after releasing the GPU mutex:
DeviceQueuePlan. When the device has a window,PlanDeviceQueuestakes the next free queue of the main family after the main and settle queues (SelectPresentQueue(queueCount, mainCount)), records it aspresent/presentIndexand raisesmainCountto cover it. The sparse queue never takes an index of the main family, so it is unchanged. That family already passedvkGetPhysicalDeviceSurfaceSupportKHR, so the swapchain images need no ownership transfer.rendered[index]. The present waits on it from the other queue, and its submit comes after the blit's on the same thread, so the image is shown only once the blit and every batch before it are done.WaitIdletakes the presentation queue's mutex aroundvkDeviceWaitIdle. The presenter never takes the GPU mutex while holding it, so the lock order is fixed.main.The presenter still blocks for the same time. That block no longer stops the draw committer, and it doesn't move into the recorder's
vkQueueSubmitcalls.The header comment above
FlipInFlightinVulkanDevice.hppnow says whereQueuePresentruns, anddocs/dev/debt/libSceAgcDriver.mdrecords that a device without a free queue in the presentation family, such as RADV, lavapipe and Intel's Windows driver, still presents under the GPU mutex.#7647 landed first and moved this queue creation into
PlanDeviceQueues. This branch is rebased onto it (maine3d03fb) and adapted to its plan. One trap in that rebase: git's automatic merge kept this PR's oldpresentIndexblock, but placed it beforePlanDeviceQueues, where the main family's queue count was still 1. The presentation queue would have taken index 1, which is the settle queue's index on a main family with two or more queues, and the plan'smainCountwould then have overwritten the count it set. It would have compiled. The index is now chosen inside the plan, after the settle queue.agc_driver_device_queue_planchecks each existing case with and without a window, where the window may only add the presentation queue, plus six window cases, among them "settle on the main family, three queues, with a window" (main 3, settle 0:1, presentation 2, sparse 1:0).Tested
Windows 11 (10.0.26300), NVIDIA GeForce RTX 5070 Ti, driver 591.86, WinLibs GCC 15.2.0 r7, Release, with Overwolf's implicit Vulkan layers disabled.
Hades (PPSA03355), gameplay: 34.5–34.9 FPS on
main(da54c65), 41.8–42.8 FPS with this PR (7343b18). The committer's GPU mutex waits drop from 5.4 to 0.02 ms per frame. TwoAPS5_TRACE_SECONDS=150:10captures per side, alternated, noAPS5_PROFILE_*; the traces are inhades-traces-main-da54c652e.zipandhades-traces-pr-7343b1875.zip.Measurements
mainmainGpuMutex.wait.*per present (ms)Vulkan.QueuePresentper present (ms)Recorder.Submitper call, mean / max (ms)Scripted input in the first room, warm caches,
APS5_TRACE_EVENTS=1048576so no thread dropped events, andAPS5_TRACE_SUMMARY_EVERY=10.A swapchain of 3 images in FIFO, instead of the surface's minimum of 2, didn't help:
Vulkan.QueuePresentstayed at 6.13 ms per present, and it would add up to a frame of latency.aps5-summary.jsonl,mainfirst:tools/trace_digest.py,main:p99 ms =====@@#######%#### 0-42.0
gpu ms +#####%%%%@%#@@%%@@ 0-2.0
tools/trace_digest.py, this PR:p99 ms =====@%**##%%#### 0-38.9
gpu ms @@@#************* 0-2.0
Other titles,
mainagainst this PR: The Pathless (PPSA01825) main menu goes from 32.9–33.6 to 39.5–40.0 FPS. Balatro (PPSA21402) and ANIMAL WELL (PPSA22520) stay at 60 FPS with the same frame times, and LEGO Horizon Adventures (PPSA14632) loading goes from 5.9–6.2 to 6.8–7.3 FPS. The screenshots show no tearing or broken frames.Measurements
One run per side,
APS5_TRACE_SUMMARY_EVERY=10, last six intervals:main; 395–400, p50 21.5–21.7 ms with this PR.main, 68–73 with this PR.Validation layer: the same flip and VideoOut tests pass on
mainand on this PR, with noVUID-errors. They ran withVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1.Tests
ctest -V:agc_driver_present_mode,agc_driver_flip_compute,agc_driver_worker_failure_log, and thevideo_out_flip_*tests labels, labels_main, lifetime, reopen, decode, controls, overlay, unopened, ps4, eop, eop_main, eop_aftergpu, cpuflip and cpuflip_in_order. That is 17 tests per side.video_out_flip_tests pacing, 3 runs per side. Thepacingfailure onmainmentioned in feat(libSceAgcDriver): pin draw committer thread on hybrid architectures #7246 doesn't happen here.was not found but was requested by env var VK_INSTANCE_LAYERS. The branch logsPresentation queue family=0 index=2in each test that opens a window.agc_driver_present_modealso checksSelectPresentQueue, andagc_driver_device_queue_planchecks the plan with a window.After the rebase onto
maine3d03fb, at this head 008e327, with NVIDIA driver 610.88:ctest -V -R 'flip|present|video_out|pacing|queue_plan'with the validation layer: 35 of 35 pass onmainand on this head (2 skipped on both,guest_video_out_high_frame_rateandguest_video_out_wqhd_detection_no_vulkan), with noVUID-messages and nowas not found but was requested by env var VK_INSTANCE_LAYERS. The branch logsPresentation queue family=0 index=2in the 20 runs that open a window;mainlogs none.video_out_flip_tests pacing, 3 runs per side with the validation layer: all pass, with noVUID-messages.APS5_TRACE_SECONDS=150:10capture per side, to confirm the gain rather than remeasure it: 374 presents in the window onmainand 470 on this head. The committer'sGpuMutex.wait.*goes from 5.31 to 0.015 ms per present, andVulkan.QueuePresentstays at 6.19–6.21 ms.ctestsuite with the GPU (--parallel 4 --timeout 120): 1994 of 2003 pass and 6 are skipped. The other 3 (agc_driver_sample_order,agc_driver_sample_order_grid_referenceandagc_driver_sample_order_recipe_depth_locations) fail the same way onmain.CI on the fork, Windows only, before the rebase (run 39732177808): all 1887 tests pass. The first attempt lost
guest_raise_in_system_callto a timeout. That is libkernel, which this change doesn't touch, and it also times out on GitHub's Windows runner in other PRs (e.g. fix(agc): prepare the push-less pixel variant only without fixed push slots #9563).tools/check_conventions.pypasses. After the comment and debt commit (4d228cd), the same 17 flip and VideoOut tests and 3pacingruns pass again with the validation layer, with noVUID-errors.Checklist
main; no other open PR implements the same functionsNotImplemented_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)hades-traces-main-da54c652e.zip
hades-traces-pr-7343b1875.zip