Skip to content

fix(agc): give pooled buffers a VkBuffer of exactly the requested size - #2233

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
TideOPL:pr/pooled-buffer-exact-size
Oct 10, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
TideOPL:pr/pooled-buffer-exact-size

Conversation

@TideOPL

@TideOPL TideOPL commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

What

Buffer and DeviceBuffer created their VkBuffer at the pool's size class (BufferPool::Capacity: power-of-two classes below 1 MiB, finer device classes above), not at the size the caller asked for. With robustBufferAccess, a shader read past the requested size therefore stayed inside the VkBuffer and returned whatever an earlier user of the pooled memory left there, instead of an out-of-range result (zero or a value inside the buffer).

Each VkBuffer now has exactly the requested size, created with the pool's usage (BufferPool::Usage) and bound to memory sized for its class, or to its slab slot, so the allocation stays reusable. BufferAllocation records the size its VkBuffer was created with (bufferBytes). A buffer taken from the pool for a different size gets a new VkBuffer over the same memory at the same offset; a same-size reuse keeps the existing one.

tests/BufferPool.cpp followed a pooled buffer by its VkBuffer handle and checked a class by the VkBuffer's size. With exact-size buffers the class is held by the memory, so those checks now follow the bound memory (MemoryOf/ClassOf); the cases are unchanged, and the first one also checks that the VkBuffer is created at the requested size.

Open PRs #1896, #1201 and #290 also change BufferPool.cpp/.hpp, but only eviction and retained-byte bookkeeping; none touches the Buffer/DeviceBuffer constructors or BufferAllocation.

Tested

Windows 11, RTX 5070 (driver 595.x), MinGW-w64 GCC, Release, BUILD_TESTING=ON, on main 5edae353.

  • New execution test agc_driver_pooled_buffer_range:
    • A buffer of the requested size's class is filled with 0xabababab and released. A 100-byte buffer then takes the same pooled memory, and a compute shader reads word 28 (byte 112) through a VK_WHOLE_SIZE storage descriptor with robustBufferAccess enabled. On main this returns 0xabababab, the earlier user's bytes; with the fix it returns zero or a value inside the buffer.
    • The test also checks the size of every vkCreateBuffer (fresh, pooled-at-another-size and same-size-reused Buffer and DeviceBuffer), with the classes taken from BufferPool::Capacity.
    • Fails on main (5edae353), passes with this change.
  • ctest -R "^agc_driver|^agc_buffer_pool": 255/255 pass. Full ctest: 710/710 pass (display-dependent video-out tests skipped).
  • python tools/check_conventions.py --base origin/main passes.
  • Minecraft Dungeons II (PPSA16064, UE5) title screen, with the other changes that title needs: same capture points with and without this change. No visible difference on the title (its visible symptoms of this bug, translucent blocks and texture noise, came together with the stride-0 fetch fixed in fix(agc): apply the raw OOB_SELECT range rules to zero-stride vertex buffers #1769), and no regression. The game run was on the earlier base, before the rebase.

Not run: Linux (tested there on the earlier revision by @Kikoarl), AMD.

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 in TechnicalDebt
  • 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
  • AI-assisted: yes. Written with AI assistance (Claude Code); I reviewed and tested it.

@github-actions github-actions Bot added the agc libSceAgc* label Oct 9, 2026
@Kikoarl

Kikoarl commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Linux run: Linux 7.2 (x86_64), Intel(R) Graphics (RPL-S) (Mesa 26.3.0-devel, Vulkan 1.4.363), GCC 16.2.1, Release, -DBUILD_TESTING=ON, #2233 (12da51f3) tested on current main (d70b8998).

  • tests/agc_driver_pooled_buffer_range_tests: passes cleanly on Vulkan (device Intel(R) Graphics (RPL-S), pooled buffer range tests passed).
  • tools/check_conventions.py: passes cleanly.
  • No regressions observed across existing agc_driver execution tests.

Tested on Linux to cover the platform noted in the description ("Not run: Linux").

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 9, 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. The test passes here on the PR head alone, and fails with the three non-test files reverted, so it pins the fix. Needs a rebase before it can land:

  • Conflicts with main in core/libs/prx/libSceAgcDriver/Graphics/src/Resources.cpp since "perf(agc): pool device staging buffers by size class" (28345b0). main now has BufferPool::Usage(usage, properties), Capacity(size, properties) and sets capacity = allocation->bytes after Take.
  • After the rebase, BindExactBuffer (Resources.cpp:18, called at :55 and :143) must receive the pool-adjusted this->usage, not the constructor argument, or a pooled buffer re-created at the exact size gets different usage flags than the pooled memory was keyed on.
  • Device buffers now use finer size classes (deviceClassBits), so ClassBytes/the "4096-byte class" expectations in PooledBufferRange.cpp (:25, :311) need rechecking against BufferPool::Capacity(size, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT); better to derive them from it than hardcode.
  • Rerun agc_driver_pooled_buffer_range and the other agc_driver tests on the rebased tree.

Buffer and DeviceBuffer created their VkBuffer at the pool's size class, so
a robust read past the requested size stayed in bounds and returned the
bytes an earlier user of the pooled memory left there instead of an
out-of-range result. Each VkBuffer now has the requested size, created with
the pool's usage, over memory of its class (or its slab slot); one taken
from the pool for a different size is recreated over the same memory at
the same offset. agc_buffer_pool follows a pooled buffer by its memory, so
its class and reuse cases hold with the exact-size buffers.
@TideOPL
TideOPL force-pushed the pr/pooled-buffer-exact-size branch from 12da51f to aa9ee1b Compare October 10, 2026 14:33
@TideOPL

TideOPL commented Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto 5edae353 and addressed each point:

  • Resolved over 28345b0: kept BufferPool::Usage, Capacity(size, properties) and capacity = allocation->bytes.
  • BindExactBuffer now gets the pool-adjusted this->usage, both for a fresh buffer and for a rebind.
  • PooledBufferRange.cpp takes its classes from BufferPool::Capacity(...) and checks that each pair of requests shares one, instead of hardcoding 256/4096.
  • Since your review, host buffers can come from slabs (765619a), so a recreated slab buffer is rebound at its own offset, and that offset is checked against the new buffer's alignment.

One more change to flag: tests/BufferPool.cpp (from 28345b0) tracked a pooled buffer by its VkBuffer handle and checked a class by the VkBuffer's size. With exact-size buffers, the class lives in the memory and a reuse at another size gets a new handle over that same memory. Those 8 checks now follow the pooled memory instead (MemoryOf/ClassOf). The cases mean the same thing, and the first one also checks that the VkBuffer is created at the requested size.

agc_driver_pooled_buffer_range still fails on main (0xabababab) and passes with the fix. ctest -R "^agc_driver|^agc_buffer_pool": 255/255. Full ctest: 710/710. check_conventions.py --base origin/main passes.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 10, 2026
@orishamir

Copy link
Copy Markdown
Contributor

Checked aa9ee1b0 merged onto current main (57f96fd2; clean merge) on Linux (Ubuntu 26.04 in WSL2, GCC 15.2.0, Release, BUILD_TESTING=ON) on llvmpipe (Mesa, LLVM 21.1.8), so a third driver after NVIDIA and Intel:

  • agc_driver_pooled_buffer_range and agc_buffer_pool pass. ctest -R '^(agc_driver|agc_buffer_pool)': 255 tests, 0 failures, 27 capability skips (mesh, DPP, DS/GDS append, conservative rasterization and the like, which llvmpipe lacks).
  • Both tests pin the fix. With BufferPool.hpp, Resources.hpp and Resources.cpp reverted to main (tests kept): agc_driver_pooled_buffer_range fails with "a robust read of word 28 of a 100-byte pooled buffer returned 0xabababab, the bytes a 256-byte earlier user left in its memory", and agc_buffer_pool with "a 10 MiB + 4 KiB device buffer was created with 11534336 bytes, not the requested size" and "a reused device buffer kept the VkBuffer of the earlier size". Restored, both pass again.
  • The review's points are in the head: BindExactBuffer receives this->usage on the fresh and the rebind path, in Buffer and DeviceBuffer; capacity = allocation->bytes is kept after Take; PooledBufferRange.cpp takes its classes from BufferPool::Capacity(...) (80/100 host, 2500/3000 device) and asserts each pair shares one; the slab rebind checks the offset against the new buffer's alignment.
  • On the overlap note in the description: perf(agc): pipeline indirect draws behind the packet worker (APS5_PIPELINE_INDIRECT) #1896 and perf(agc): pipeline queue-0 draw commits behind the packet worker #1201 change only BufferPool.cpp, which this PR does not touch. feat(agc): host import windows, least-recent eviction under the budget #290 does edit Buffer::Buffer, DeviceBuffer::DeviceBuffer and both release() paths (the vkAllocateMemory / vkFreeMemory lines next to the cache->Put lines changed here), and merging it on top of this PR conflicts in Resources.cpp and BufferPool.hpp; feat(agc): host import windows, least-recent eviction under the budget #290 already needs a rebase against main on its own.

Not checked here: a game, or a hardware driver.

AI-assisted: yes (Claude Code).

@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
#290 core/libs/prx/libSceAgcDriver/Graphics/src/Resources.cpp L5-6, L9

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

Checked the PR merged onto current main on Linux : agc_buffer_pool and agc_driver_pooled_buffer_range pass, and both fail with the three non-test files reverted, so they pin the fix. Slab slots are bounded by the slot size and the rebind checks size and alignment; no conflicts with main. Also confirmed by @Kikoarl (Intel), @orishamir (llvmpipe) and @Mouhand-Kaddo (Windows). Merging.

@adriadam10
adriadam10 merged commit 015beb3 into boykopovar:main Oct 10, 2026
15 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