Repository navigation
perf(agc): back the GDS with anonymous shared memory on integrated GPUs and keep its DMA_DATA on the GPU - #13159
Conversation
|
@Gonic0 thanks, the design reads well: moving the GDS DMA onto the GPU timeline behind the queued work is the right way to keep its ordering against shader GDS use, and the three negative controls make the test convincing. Reviewed at 5ddf02c. Four points:
|
|
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.
|
5ddf02c to
57190a6
Compare
|
Thanks for the careful review.
The tests are now in #9385's AI-assisted: yes |
|
An AMD RDNA 4 run of Windows 11 Pro 10.0.22631, AMD Radeon RX 9070 XT (
I did not check which GDS backing the default run picked on this card; the logs do not print it. AI-assisted: yes (Claude Code) |
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 427 tests)Skipped (50)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Windows GPU run of head Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, Radeon RX 6700 XT with AMD proprietary driver 26.10.1 (Vulkan 1.4.315), WinLibs GCC 15.2.0 UCRT r7, Ninja, Release,
AI-assisted: yes (Claude Code). |
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (2, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 483 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
First run of the new path on an integrated GPU outside macOS: a Radeon 860M under Linux/RADV takes it ( Setup: Radeon 860M (
Why: the shared GDS is created, so
What it costs: with the test's
Every value check passes, so RADV falls back to #9385's behaviour and nothing reads wrong data. The private mapping imports on RADV: as a second local experiment, I kept the private GDS ( So on RADV it's the other way round from MoltenVK: the private mapping imports and the shared one doesn't. Two ways that would cover both, if they fit your design:
Full AI-assisted: yes (Claude Code) |
|
Hi Gonic0, Thanks for the PR. Lander0606's RADV run (Radeon 860M, Mesa 26.2.5) matches what the code does at 25d850e. Could you make importability depend on the import actually succeeding? Either set the flag only after the first successful host import, or fall back to the private mapping when the shared one is refused. The test should then expect #9385's copy path whenever the import was refused. A run on a RADV integrated GPU, or Lander0606's results, on the new head would confirm it. |
…Us and keep its DMA_DATA on the GPU Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The shared GDS was taken as importable as soon as its shm_open mapping existed, before any Vulkan import was tried. RADV refuses host-pointer imports of memfd-backed shared mappings (VK_ERROR_INVALID_EXTERNAL_HANDLE), so the driver fell back to copies while GdsImportable() still reported the GPU path. Import the GDS when the device is created and drop the flag when the import is refused, so such devices keep boykopovar#9385's path. GdsShared() tells the shared mapping apart from a working import; the test checks the mapping against the request and expects the copy path when the import was refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
25d850e to
9e874bb
Compare
|
Thanks both, and thanks Lander0606 for the RADV run. The branch is rebased onto
Tested on macOS 27.2, Apple M5 Pro, MoltenVK 1.4.2 (Vulkan SDK 1.4.363.0).
A run on a RADV integrated GPU would confirm the real refusal. AI-assisted: yes 🤖 Generated with Claude Code |
…d-append The shared GDS mapping from boykopovar#13159 takes GdsBindingBytes, as its import check does, so the ordered-append turn and timeout count past the 64 KiB GDS stay mapped; disk cache format 52 follows main's 51. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
Builds on #9385. Until it merges, this branch carries its commits (merged at
ec471877d3); only the top commit,57190a6629, is new here.The GDS is a 64 KiB private anonymous guest mapping (
Pm4::GdsAddress). On MoltenVK the GPU cannot import that mapping: registered for the GPU as an experiment, its host import failed atvkAllocateMemory(-2) (APS5_TRACE_HOST_IMPORT=1). So DMA_DATA through the GDS stays on the CPU and waits for the GPU:GuestMemory::Read, whose flush hook waits for the recorded batches writing it (syncThroughUnlocked, which no performance timer covers).This change, on top of #9385:
VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU) withVK_EXT_external_memory_host, the Vulkan device requests an importable GDS when it is created (Pm4::RequestImportableGds).GdsAddressthen backs the GDS with anonymous shared memory:shm_open(SHM_ANON), 64 KiB, mapped shared with CPU read and write, zeroed, and the descriptor closed. This is not direct memory, so the title's direct memory pool andsceKernelAvailableDirectMemorySizeare unchanged, and the object has no name a title could open. Its host import succeeds on MoltenVK.Pm4::GdsImportable()reports the backing actually obtained. If the shared mapping fails, if the request was not made, or if the GDS was first used before the device made it, the GDS keeps the private mapping and everything behaves as with perf(agc): write DMA_DATA fills of an idle GDS without draining #9385 alone.APS5_NO_SHARED_GDS=1forces that.preparePacketMemory, a GDS destination first tries the GPU copy behind a pending source (copyBehindPendingSource, withdecodeCopydecoding GDS to memory and memory to GDS), then perf(agc): write DMA_DATA fills of an idle GDS without draining #9385's idle-GDS path, then the existing store path (ResolveStore, which perf(agc): write DMA_DATA fills of an idle GDS without draining #9385 extends to GDS destinations, thenWriteLabelOnGpu), which records the store on the GPU behind the queued work instead of draining. A GDS to memory copy behind pending GDS writes is recorded on the GPU the same way. With a private GDS, as on any discrete GPU,decodeCopyreturns nothing for the GDS and the store path is skipped for it, so those devices take perf(agc): write DMA_DATA fills of an idle GDS without draining #9385's paths unchanged.gdsWritesPendingalso counts the GPU writes this change records into the GDS (VulkanDevice::StoresPendingOver), so perf(agc): write DMA_DATA fills of an idle GDS without draining #9385's idle path does not take a GDS that a recorded copy or store still writes. In the test below, removing that check did not fail: the idle path's CPU write still landed after the recorded copy. The check keeps such a fill on the GPU instead of on that path.DecodeMemoryCopyandResolveStoreare unchanged by this commit. On an integrated GPU the shaders' GDS bindings (ShaderResources,AddWritable(gds, GdsBytes, true)) now find the import and use it in place.Devices that are neither integrated nor discrete (
CPU,VIRTUAL_GPU,OTHER) keep the private GDS. I ran only an integrated GPU (Apple M5 Pro).Tested
Apple M5 Pro, macOS 27.2, MoltenVK from Vulkan SDK 1.4.363, x86-64 Release under Rosetta 2.
mainalone does not build on macOS. I ran everything in a tree with the open macOS port PRs onmain39fc186c97with #9385 merged, and this commit applied; it applies unchanged there.agc_driver_gds_dma_fill(#9385's test), extended:APS5_NO_SHARED_GDSis not set.agc_driver_gds_dma_fill_privateruns the same test withAPS5_NO_SHARED_GDS=1: same values, perf(agc): write DMA_DATA fills of an idle GDS without draining #9385's drain, no GPU copy or store over the GDS.Negative controls, each of which fails
agc_driver_gds_dma_fill:decodeCopywithout the GDS cases: the copy out of the shared GDS is not recorded on the GPU;gdsWritesPendingwithoutStoresPendingOverdoes not fail the test, as said above.With
VK_LAYER_KHRONOS_validation,ANYPS5_REQUIRE_VULKAN=1andANYPS5_NO_SHADER_CACHE=1, these 44 CTest entries pass:agc_driver,agc_driver_gds_dma_fill,agc_driver_gds_dma_fill_private,agc_driver_gds_append_consume,agc_atomic_gds_size,agc_dma_data_tests,agc_driver_pipelined_dma,agc_driver_pipelined_dma_draw_pipeline,agc_driver_pm4,agc_guarded_memory_loadsand the 34agc_storage_,agc_driver_storage_andagc_driver_linear_target_entries. The only validation message isVUID-VkDeviceCreateInfo-pProperties-04451, from the tests' own device creation on MoltenVK.PPSA21564 01.007.000, the logos before the intro. Per frame there is one 4-byte DATA → GDS fill on queue 0x40 behind GDS writes, which drained for 3 to 6 ms on that queue (the graphics queue waits on it), and about 15 four-byte GDS → memory copies read on the CPU. The base is the tree with #9385 and without this commit (
99fffc44a1, its revert); the change isc4562034f0. Alternating, no profiling switches,APS5_TRACE_FRAMES=150:240, four pairs:FPS is 480 presents over the time between dumped frames 5 and 13; the load average is the one-minute value after each run. The host was not idle: a browser and the desktop stayed open, and the first pair ran while the load was still falling from earlier runs. Round 2 is the only one where this change was slower. Rounds 3 to 8 give 28.9 to 33.1 FPS for the base against 33.0 to 36.3 with this change, frame p50 28.7 to 32.6 ms against 25.2 to 29.1 ms. An earlier set on a busier host (load 7 to 10) moved the frame p50 from 41.6 to 43.1 ms to 36.4 to 37.2 ms, with flat FPS. No run reported an unexpected copy path, and the dumped frames show the logos intact.
Base (
99fffc44a1, round 5):{"commit":"99fffc44a17d","dirty":null,"env":{"APS5_ASYNC_PIPELINES":"1","APS5_DUMP_FRAMES":"200","APS5_DUMP_FRAMES_EVERY":"60","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_FRAMES":"150:240"},"gpu":"Apple M5 Pro","driver":"MoltenVK 1.4.2","os":"Darwin 27.2.0","cpu":"VirtualApple @ 2.50GHz","cpu_threads":18,"memory_mib":49152,"title_id":"PPSA21564","title":"ASTRO BOT","content_version":"01.007.000","resolution":"3840x2160","window":{"spec":"frames 150:240","first_present":150,"ms":7120.992},"presents":240,"frame_ms":{"p50":28.924,"p90":33.006,"p99":37.86,"max":53.888},"gpu_busy_ms_per_present":18.877,"worker_busy_pct":{"queue worker 0x0":98.859,"queue worker 0x40":99.605,"queue worker 0x48":95.681,"queue worker 0x3a":0.031},"top_phases_ms_per_frame":{"Driver.Packet":86.34,"Driver.Packet.memory_wait":53.801,"Driver.Packet.dispatch":10.166,"Driver.Dispatch":8.528,"Driver.Packet.prepare_memory":8.131,"VideoOut.Flip":6.569,"Driver.Present":5.431,"VideoOut.Flip.present":5.417},"trace_dropped":false,"trace":"aps5-trace-99fffc44a17d-76398.json.gz"}This change (
c4562034f0, round 6):{"commit":"c4562034f0dd","dirty":null,"env":{"APS5_ASYNC_PIPELINES":"1","APS5_DUMP_FRAMES":"200","APS5_DUMP_FRAMES_EVERY":"60","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_FRAMES":"150:240"},"gpu":"Apple M5 Pro","driver":"MoltenVK 1.4.2","os":"Darwin 27.2.0","cpu":"VirtualApple @ 2.50GHz","cpu_threads":18,"memory_mib":49152,"title_id":"PPSA21564","title":"ASTRO BOT","content_version":"01.007.000","resolution":"3840x2160","window":{"spec":"frames 150:240","first_present":150,"ms":6130.148},"presents":240,"frame_ms":{"p50":25.211,"p90":27.206,"p99":29.588,"max":51.432},"gpu_busy_ms_per_present":16.241,"worker_busy_pct":{"queue worker 0x0":98.659,"queue worker 0x40":99.667,"queue worker 0x48":96.778,"queue worker 0x3a":0.049},"top_phases_ms_per_frame":{"Driver.Packet":74.486,"Driver.Packet.memory_wait":45.523,"Driver.Packet.dispatch":15.598,"Driver.Dispatch":14.113,"VideoOut.Flip":6.743,"Vulkan.DispatchRecipe":6.181,"Vulkan.DispatchRecipe.command_record":6.177,"Driver.Present":5.653},"trace_dropped":false,"trace":"aps5-trace-c4562034f0dd-77169.json.gz"}Traces: base, this change
Not tested on Linux, Windows, a discrete GPU or a software Vulkan driver.
Overlap
preparePacketMemory; it should land first.GdsBindingBytes; whichever lands second sizes the shared mapping here to that.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)🤖 Generated with Claude Code