Skip to content

perf(agc): write draw descriptors from retained CPU metadata - #1334

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
TibixDev:pr/draw-descriptor-metadata
Oct 11, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
TibixDev:pr/draw-descriptor-metadata

Conversation

@TibixDev

@TibixDev TibixDev commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

What

Keep buffer and image descriptor metadata with each resource. Build each draw's descriptor set from that metadata in one update, with the draw's snapshot buffers substituted. This removes Vulkan descriptor copies from the draw path.

Guest snapshots, resource ownership, and descriptor-set lifetimes stay the same. #639/#1219 change resource ABI handling; #1202 changes snapshot allocation. Neither replaces these descriptor copies.

Tested

Fedora 44, Release: graphics tests. RX 6600 with Vulkan validation: mixed buffer and sampler-array bindings, repeated snapshot overrides, unchanged descriptors, and one update with no descriptor copies. Windows was not tested.

Isolated comparison: upstream e9846679 versus the same revision plus this PR. For eight storage-buffer descriptors, preparing a draw with one changed snapshot took 28.689 → 3.333 µs; with eight snapshots it took 37.294 → 4.935 µs. Six pinned processes per variant in ABBA order, using host RADV. These are draw-binding preparation timings, not title frame time.

Checklist

  • Current main; no duplicate open PR.
  • One topic.
  • No new code comments.
  • Unsupported paths still throw; UI stubs are documented.
  • No notes, logs, or images in the repository.
  • No new third-party code.
  • General behavior.
  • Depends on: none.
  • AI-assisted: yes.

@TibixDev

TibixDev commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on e9846679 with no production changes.

Measured upstream e9846679 against the same revision plus this PR (cc32a55f). Fedora 44, GCC 16 Release, Ryzen 5 5600, CPU 3, RX 6600. The runner explicitly selects host RADV and its matching host library directory. Supporting libc and libkernel binaries are identical between variants.

The same fixture calls production ShaderResources::PrepareDrawBindings with eight storage-buffer bindings and either one or eight snapshot overrides. Snapshot data changes on each call. Descriptor allocation, snapshot allocation/copy, and recorder retirement remain in the timing. One warmup and nine batches of 5,120 draws per process, with retirement every 32 draws. Six processes per variant in ABBA × 3 order; validation is off for timing. No game or build ran during measurement.

Snapshots per draw Upstream median (µs) PR median (µs) Less time Upstream range PR range
1 28.689 3.333 88.4% 28.302–32.750 3.272–4.064
8 37.294 4.935 86.8% 36.351–39.453 4.780–5.439

Snapshot checksums match. Both benchmark variants pass separate core and sync validation runs on RADV. The PR's mixed buffer/sampler-array, repeated snapshot override, and descriptor update-count tests pass with validation on RADV and llvmpipe. Graphics and contribution checks pass.

This measures draw-binding preparation only; no title frame time or Windows result is claimed. AI-assisted.

@EdouardSence

Copy link
Copy Markdown
Contributor

Title-level data for this change, from a retail title (Astro Bot, PPSA21567 v01.019.000), in case it helps the review.

Setup: RX 9070 XT (GFX1201), RADV / Mesa 26.2.4, Linux 7.2.9. Base is the astrobot fork at 88d7bb6 plus #943 (815d274, 00e6d07), because upstream main stops on this title (#1458). For this test, only libSceAgcDriver.prx changes between runs.

Profile before the change: perf record ran for 40 s on the title screen. The driver thread (Driver::run) was ~100% busy. With DWARF unwinding, 1321 of 3570 samples on that thread (37%) were in glibc memcpy called from libvulkan_radeon.so, from ShaderResources::PrepareDrawBindings (the vkUpdateDescriptorSets call with the VkCopyDescriptorSet array). The hot paths are the 16–31 and 32–63 byte copies, i.e. one copy per descriptor. APS5_PROFILE_DRAW reported ~5.7k draws/s on that scene.

Presents per second ([present] lines, APS5_PROFILE_DRAW=1, 200 s from boot to title screen, no input):

run logos (windows 3–8) title screen (last 6 windows)
base 21.9 26.3
base, second run 22.2 26.2
this PR (cc32a55, cherry-picked) 25.9 33.4
equivalent local patch (same approach) 27.2 34.9
  • No new errors in the log and no amdgpu faults in any run.
  • Frames dumped with APS5_DUMP_FRAMES match visually between base and patched runs. The differences are animation timing only.
  • The Recorder.cpp test hunk conflicted on the fork, so I dropped it for this build only. I didn't run this PR's tests here.
  • On upstream main 5d42b82, my own version of the same change passes 428/428 ctest on this GPU.

AI-assisted: the profiling, the measurements and this write-up were done by Claude (Claude Code) on my machine.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

This conflicts with main again (ShaderResources.hpp/.cpp, tests/Recorder.cpp). Could you rebase it?

TibixDev and others added 2 commits October 11, 2026 10:27
Replace per-draw Vulkan descriptor copies with a single write from retained buffer and image metadata. Keep per-draw guest snapshots, sets and resource ownership unchanged. Baseline RX 6600 profiling attributed 28% of graphics-worker cycles to descriptor copies.

Validation: mixed descriptor and snapshot regression passes on RADV RX 6600 and llvmpipe with Vulkan validation; graphics and guest-memory tests pass. Written with AI assistance.
Update descriptorSnapshotTests for the single-update path of this PR.
@adriadam10

Copy link
Copy Markdown
Collaborator

Rebased on current main (conflicts in ShaderResources.hpp/.cpp and tests/Recorder.cpp after #1730b29c4, which added allocationDescriptors and still copies the whole set). Resolved by dropping that member in favour of the retained descriptor arrays from this PR, and using main's snapshot index lookup for the overrides.

Added on top of your commit:

  • tests/Recorder.cpp: the test's sampler binding used 11, which perf(agc): baseline shader recompilation on registration #639's typed heaps reject; it now uses RuntimeAbi::Binding::Samplers.
  • tests/Graphics.cpp (descriptorSnapshotTests): it pinned the old copy-then-overwrite behaviour; it now expects no copies, one write per binding, and an untouched binding keeping its original descriptor.

Checked: agc_driver_graphics passes, and agc_driver_recorder_tests --draw-descriptors-only passes (that target is not registered in ctest on main; built by hand). Merging once CI is green on the new head.

@adriadam10
adriadam10 force-pushed the pr/draw-descriptor-metadata branch from cc32a55 to 1dbf976 Compare October 11, 2026 08:41
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 11, 2026
@adriadam10
adriadam10 merged commit 80ee119 into boykopovar:main Oct 11, 2026
14 checks passed
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.

4 participants