Repository navigation
refactor(agc): create, bind and free buffers through the device function table - #11920
milescoviello wants to merge 3 commits into
Conversation
|
This pull request is labeled |
|
Thanks for folding in the device-function table with credit. The pairing of member and name checks out across all the resolved call sites, and the fallback when there's no table looks right. Two things:
|
|
@oneandonlydean thanks for checking the pairs.
AI-assisted: yes (Claude Code) |
|
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.
|
|
@oneandonlydean measured CPU time in the by-name lookups, before and after this PR. Setup.
So in this title the change removes about 0.81 s of lookup time per 300 s run, about 2.7 ms of CPU per second of gameplay. That's roughly 0.3 % of one core. Before, the lookups cost about 110 ns each. The roughly 60,000 left are device setup and paths outside the table; they average about 800 ns each because most are first-time lookups. It's a small saving. Most of the lookups were on the draw-recording path ( AI-assisted: yes (Claude Code) |
b4d7456 to
dd7a4d1
Compare
|
Updated for
AI-assisted: yes (Claude Code) |
|
@milescoviello Could we agree on #12559 → #11920 for the overlapping storage-image clear calls? #12559 has an agreed first landing relative to #7085 and keeps the exact packed-float byte encoding in I inspected your current AI-assisted: yes (Codex). |
|
@Filip722 agreed: #12559 → #11920. When #12559 lands, I'll rebase this PR onto it:
The integration and revalidation are mine. (For the record, this PR was opened first, 2026-10-27, but the overlap is only mechanical on my side, so your order is simpler.) AI-assisted: yes (Claude Code) |
|
@milescoviello the added lookup CPU-time comparison addresses the earlier timing ask; thank you. Head dd7a4d1 now conflicts with main aaccd0b in TextureDetiler.cpp at two cmdPushConstants sites. Main adds push.splitStride = layout.gcnSplitStride immediately before each call, while this PR converts the call to Resolved. Once the agreed parent/landing order permits the refresh, could you rebase keeping both splitStride assignments and the matching resolved cmdPushConstants calls? Then rerun the table/fallback and detiler bars on that composition. No further performance evidence request here; frame-time qualification remains the coordinated gate. |
dd7a4d1 to
bfdcd5a
Compare
|
@oneandonlydean thanks. Rebased: #10764 is now At
The order with #12559 still holds: if it lands first, I rebase this PR onto it. AI-assisted: yes (Claude Code) |
❌ NVIDIA GPU test: build failedPR head
build: first errorsReproduceAutomated 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. |
bfdcd5a to
d8f9675
Compare
|
@TechLevelZero thanks, that build failure was real and came from #10764, which this PR sits on: AI-assisted: yes (Claude Code) |
✅ NVIDIA GPU test: passedPR head
Failing on main too (4, not counted against this PR)Validation IDs already present on main (4 IDs in 437 tests)Skipped (39)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. |
|
@CnRJay thanks for the review. On each point:
I'll post the narrowed head here with the Linux, validation-layer and Windows VM runs. AI-assisted: yes (Claude Code) |
8a3e219 to
8b7a2ae
Compare
|
@CnRJay the narrowed head is up:
Runs on this head:
AI-assisted: yes (Claude Code) |
❌ NVIDIA GPU test: build failedPR head
build: first errorsReproduceAutomated 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. |
|
On the NVIDIA CI's build failure above: the target that fails to link, The link error itself comes from #10764's commit: @TechLevelZero could the CI run this head again on current AI-assisted: yes (Claude Code) |
❌ AMD GPU test: build failedPR head
build: first errorsReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
8b7a2ae to
44504d6
Compare
|
Rebased onto
Checks on that tree, Ubuntu 26.04.1, RTX GPU, NVIDIA 615.71.09:
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 (2, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (4 IDs in 510 tests)Skipped (44)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. |
…ion table Buffer, DeviceBuffer and BindExactBuffer looked vkCreateBuffer, vkGetBufferMemoryRequirements, vkBindBufferMemory, vkDestroyBuffer and the memory calls up by name on every buffer; they now take the device's function table entries. The other call sites stay as they are. Co-authored-by: BiladeiroSupremo <128841645+BiladeiroSupremo@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
#15440 removes This PR has the same line with AI-assisted (Claude Code). |
|
@xXPBSXx confirmed: #15440 first, and I've added The PR's current head still has the link line, from the two test-link commits that reached AI-assisted: yes (Claude Code) |
44504d6 to
860450b
Compare
|
Rebased onto
One thing I hit on AI-assisted: yes (Claude Code) |
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — approved
The PR adds 8 buffer and memory entries to DeviceFunctions and resolves the hot Buffer / DeviceBuffer / BindExactBuffer calls through the table; it merges cleanly, builds, passes, and its new tests catch the regressions they target.
What I ran
Head 860450ba7f34b65cf1353d529ac8ef7b37c3dccd, main 4f41f4c375f5abb487dbd184fbc150f31994d762, merged tree 9bbbb24811667ede59329e345f4fb4613bdba579 (Debian 13 VM, GCC 14.2.0, Release, Ninja, lavapipe / Mesa 26.1.6).
git diff --check 4f41f4c375f5abb487dbd184fbc150f31994d762...860450ba7f34b65cf1353d529ac8ef7b37c3dccd
python3 tools/check_conventions.py --base 4f41f4c375f5abb487dbd184fbc150f31994d762
ctest --test-dir build -j8 --test-load 32 --output-on-failure -R "agc_buffer_pool|memory_accounting"
# negative controls (see below), each followed by
ninja -C build -j32 -l32
ctest --test-dir build --output-on-failure -R "^agc_buffer_pool$"
git status --porcelain
Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
All conventions checks passed |
| Build (script, merged tree) | ok |
| Selected tests (script) | 2157 ran, 2036 passed, 114 expected skips, 0 new failures, 0 timeouts; 7 known failures that also fail on main |
Vulkan validation (script, main vs merge) |
770 tests, layer loaded in 769, none missing, 0 tests with new VUIDs |
agc_buffer_pool*, agc_*memory_accounting*, agc_deferred_memory_accounting |
8/8 passed |
| Worktree after controls | back to the merged tree (git status --porcelain empty), agc_buffer_pool passes again |
Negative control
-
I restored
Resources.cpp,BufferPool.cppandMemoryAccounting.cppfrommainand kept the mergedContext.hppso the test still compiles.agc_buffer_poolfailed inTableFill:output
FAIL: the device function table's member for vkCreateBuffer was not filled from that name FAIL: the device function table's member for vkDestroyBuffer was not filled from that name -
I kept the merged
Resources.cpp(which has the table fill) and changed only its 4bindBufferMemorycall sites back tocontext.Function<PFN_vkBindBufferMemory>.TableLookupsfailed:output
FAIL: a host and a device buffer cycle of new sizes looked up 4 Vulkan functions by name while the device's function table holds them -
I restored only
BufferPool.cppandMemoryAccounting.cppfrommain. The test passed (see the non-blocking note below).
I then restored the merged files, rebuilt, and confirmed the worktree is clean and the test passes.
Findings
Blocking
None.
Non-blocking
- The
MemoryAccountingandBufferPool-constructor conversions have no test coverage.core/libs/prx/libSceAgcDriver/Graphics/src/MemoryAccounting.cpp:54and:74and theBufferPoolconstructor (core/libs/prx/libSceAgcDriver/Graphics/src/BufferPool.cpp:43) can be reverted without failingTableLookups(control 3). The pool's own allocations go through the members it captured once in its constructor, so the cycle never reachesMemoryAccounting::Allocate/Free(context, …)through the table. This doesn't affect correctness:TableFillchecks that the members are filled from the right names. But the description reads as ifTableLookupscovers these sites. I suggest narrowing that wording, or extending the cycle to cover a non-pooled allocate/free. IsDeferredMemoryFreestill sees the same pointer after the change. I checked this becauseMemoryAccounting::Freecompares the free function againstdeferredFreeMemory(core/libs/prx/libSceAgcDriver/Graphics/src/MemoryAccounting.cpp:60).VulkanDeviceinstallsDeferDestructionToIdle's resolver (core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp:2312) beforeFillDeviceFunctionsruns (:2372). The table therefore holds the same deferringvkFreeMemory/vkDestroyBufferthatFunctionwould return, so the pointer check behaves as onmain. No action needed. I'm noting it because a future reorder of those two lines would break this silently.- The checklist's dependency doesn't match the diff. The
Depends on: #15440line describes a link-line change, but the current diff no longer touchestests/cmake/agc_pipeline_cache_allocation_tests.cmake. The PR merges and builds onmainwithout #15440, so the dependency now looks like merge order only. Saying so would help the maintainer. The merge-order agreement with #13796 that the author mentioned in the discussion is also not on the current Depends-on line.
Not verified
- No hardware GPU: I ran nothing on NVIDIA or AMD. The validation comparison is the script's lavapipe run.
- I did not measure the lookup or CPU-time savings. The PR no longer claims a speed change.
- Windows/MinGW build.
✅ AMD 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 (7, not counted against this PR)Validation IDs already present on main (4 IDs in 517 tests)Skipped (58)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Validation IDs already present on main (4 IDs in 517 tests)Skipped (48)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. |
…device table The device function table now carries vkAllocateMemory and vkFreeMemory, so memory accounting no longer asks deviceProc for them and the image memory hooks never saw an allocation. The first sharing check then failed its allocation lookup and every slab test exited 1 instead of passing or skipping. The hooks now give the context a copy of the table without those two entries, so both resolve through the hooks while every other entry stays on the table.
✅ NVIDIA GPU test: passedPR head
Failing on main too (3, not counted against this PR)Validation IDs already present on main (13 IDs in 549 tests)Skipped (49)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. |
✅ AMD 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 (8, not counted against this PR)Validation IDs already present on main (8 IDs in 549 tests)Skipped (60)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Merge preview against a newer
AI-assisted: yes (Claude Code) |
What
Follow-up to #10764, as agreed with oneandonlydean there (comment). BiladeiroSupremo's lookup cache (BiladeiroSupremo/AnyPS5Wolverine@fccc2547a) gets a home in the resolved device-function table, not a separate per-thread cache. BiladeiroSupremo is credited with a
Co-authored-bytrailer.Graphics::Context::Functionlooks a Vulkan device function up by name, through the loader and every implicit layer, on each call. #10764 moved the texture detiler ontoContext::Resolved. That readsDeviceFunctionsand falls back toFunctiononly while the table is absent or lacks the entry.I counted lookups by name in Call of Duty: Black Ops II (PPSA34502 01.010.000, 300 s of campaign gameplay). The build was
mainplus #10764 and #9656 plus a local counter (not in this PR). It made 2.1 million lookups by name, and four buffer calls account for 1.9 million of them:vkCreateBuffervkBindBufferMemoryvkGetBufferMemoryRequirementsvkDestroyBuffervkCmdPipelineBarriervkMapMemoryvkCreateImageView/vkDestroyImageViewvkResetDescriptorPool, fence calls, submit, command buffer begin/end/reset, framebuffersThey come from
ShaderResources::addDataBuffer→Buffer::Buffer→BindExactBuffer: a pooled buffer rebound at a new exact size for a draw's data.This PR:
createBuffer,destroyBuffer,getBufferMemoryRequirements,bindBufferMemory,allocateMemory,freeMemory,mapMemoryandunmapMemorytoDeviceFunctionsand toFillDeviceFunctions;BindExactBuffer,Buffer::Buffer,Buffer::release,DeviceBuffer::DeviceBuffer,DeviceBuffer::release,BufferPool's constructor andMemoryAccounting::Allocate/Free. The onevkGetFenceStatuscall inCommandBatch::~CommandBatchalso uses the table entry it already had.Every other
Functioncall site stays as it is. An earlier version of this PR converted 366 call sites in 23 files. CnRJay pointed out that the counts above put the saving in these few, so the rest is out.How
Resolvedreturns the table entry when the device has filled its table, and otherwise callsFunctionas before. So contexts without a table (mocks,APS5_NO_PROC_TABLE, calls made before the table is filled) behave exactly as onmain. The table is filled once at device creation and only read afterwards.Tests, in
agc_buffer_pool:TableFillfills a table throughFillDeviceFunctionsfrom a mockvkGetDeviceProcAddr. It checks that each of the 8 new members holds the mock function of the name it is filled from, so a member filled from the wrong name fails.TableLookups: the mock counts lookups by name. A cycle of a host-visibleBufferand aDeviceBufferat sizes the pool hasn't seen forces the exact-size rebind path. Without a table the cycle must look something up, so the counter sees these calls. With a table holding the buffer and memory functions, a second cycle must make no lookup.Tested
At head
860450ba7f(rebased ontomainabe492bfea), same machine: a fullcmake --build buildsucceeds, and the 15 tests matchingBufferPool|buffer_pool|memory_accounting|device_functions|pipeline_cache_allocpass.Earlier, Linux (Ubuntu 26.04.1, GCC 15.2), RTX 4070 Ti (NVIDIA 615.71.09), at head
8b7a2ae407:main5bae1a4bc2(bfb5e10bbc, clean): a fullcmake --build buildof every target succeeds, and the 8 tests in the next item pass on the merge too.agc_buffer_pool(3 entries),agc_memory_accounting,agc_memory_accounting_budget_cap,agc_memory_accounting_budget_floorandagc_deferred_memory_accountingpass, 8 of 8 in each of 3 runs.main-side base52ca9f0137(perf(agc): take the texture detiler's Vulkan entry points from the device table #10764 alone) against this head: 8 of 8 pass on both, one Vulkan device created on each, no validation IDs on either.Windows 11 VM (GeForce GT 1030, WinLibs GCC, Release),
agc_buffer_pool,agc_memory_accounting*andagc_deferred_memory_accounting: 8 of 8 in each of 3 runs, on this head and onmain0e9dc7bc4c.Red control for
TableFill: this head withdestroyBufferfilled from"vkDestroyBufferView"(35e1d136a0, branchtest/11920-fill-mutanton my fork) failsagc_buffer_poolin all 3 runs on the VM:the device function table's member for vkDestroyBuffer was not filled from that name.I haven't re-measured CPU time for this narrowed commit, and I don't claim a frame-time change.
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)libSceAgcDriveroffagc_pipeline_cache_allocation_tests's link line; this PR rebases onto it)🤖 Generated with Claude Code