Skip to content

fix(agc): bind a buffer descriptor past the range limit by its committed bytes, or empty - #3202

Closed
CnRJay wants to merge 1 commit into
boykopovar:mainfrom
CnRJay:fix/agc-oversized-buffer-descriptor
Closed

CnRJay wants to merge 1 commit into
boykopovar:mainfrom
CnRJay:fix/agc-oversized-buffer-descriptor

Conversation

@CnRJay

@CnRJay CnRJay commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

What

A buffer V# whose size (num_records * stride) is past maxStorageBufferRange aborted the whole dispatch with shader buffer exceeds descriptor range limit. PPSA28416 (STAR WARS: Galactic Racer) reaches this with V# words read from memory that holds no descriptor (one had base 0x55268c155511, size 0x1c9523763d77); the shader never touches the buffer on the path it takes, so on hardware nothing happens.

The descriptor is now bound by what is behind it:

  • committed guest memory at its base: it is cut at the end of the committed run (and at the limit, rounded down to 4 bytes), so accesses past it read 0 and drop stores, as for unmapped pages;
  • nothing committed at its base: it is bound as the empty placeholder buffer, like a null V#.

A descriptor inside the limit is unchanged. The two descriptor range limit rejections in Graphics.cpp are gone, since that case no longer throws; the new test covers it.

Tested

Windows 11, WinLibs GCC 15.2.0, Release, RTX 4080.

  • agc_driver_graphics (oversizedDescriptorTests in Graphics.cpp): a V# of 256 GiB over 64 KiB of committed guest-arena memory is bound at the mock limit (16 KiB); the same V# over uncommitted memory is bound as the placeholder (EmptyBufferBytes). Without the change it fails with shader buffer exceeds descriptor range limit.
  • Full suite: 759/759 pass (video-out tests skipped, as on main).
  • PPSA28416: the dispatch no longer aborts; the title runs on to its next stop.

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

@github-actions github-actions Bot added the agc libSceAgc* label Oct 11, 2026
@Silverash-i8

Copy link
Copy Markdown
Contributor

Windows run, since the PR has no Intel or Windows results yet: Windows 11, WinLibs GCC 15.2.0 r7, Release, merged onto main d31b57a in one build with #3217, #3202, #3167, #3137, #2863, #2849 and #2847, on an RTX 5060 Laptop and the Arrow Lake iGPU (ANYPS5_GPU=Intel).

  • agc_driver_graphics (Graphics.cpp) passes on both GPUs.

Full ctest of that build on both GPUs has no failure that isn't also on plain main here (guest_filesystem needs the symlink privilege, #3169; the Intel ones are listed on #2114 and #3207).

AI-assisted (Claude Code); I ran this on my machine.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Please keep the throw when the committed run reaches the limit (ShaderResources.cpp:2681). As it is, a descriptor over more committed memory than maxStorageBufferRange is cut there and the shader reads 0 from mapped memory; the test binds 16 KiB of 64 KiB committed. Cutting at the end of a committed run shorter than the limit, and the empty binding, are fine.

oversizedDescriptorTests is _WIN32 only, so with the two descriptor range limit rejections removed the Linux job checks none of this. Please make it run there too.

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

  • core/libs/prx/libSceAgcDriver/tests/Graphics.cpp:1899: oversizedDescriptorTests is wrapped in #ifdef _WIN32, so on Linux it runs nothing, and the two Linux-running expectSingleFailure(... "descriptor range limit") rows are deleted. Net result: no Linux coverage of this path, and the negative check can't fail there. Please make the test run on Linux too (e.g. a guest-arena-independent way to get committed/uncommitted memory, or mmap/VirtualAlloc through the same helper other tests in this file use), or keep a Linux-valid case.
  • ShaderResources.cpp:2674-2682: this binds a plausible but unverified result where main threw. The "accesses past it read 0 and drop stores" semantics have no cited source (hardware measurement or reference). Please state the source in the PR and add a TechnicalDebt line under "Unknown function info" (not at the end of the section) for the cut-at-committed-end and empty-placeholder choices.
  • ShaderResources.cpp:2681: only committed.back().second is used, so gaps between committed runs inside the range stay bound as if valid. Say whether that is intended, or cut at the first gap.
  • ShaderRegistry.cpp:62 already computes a committed extent with CommittedRanges; please reuse/share that rule instead of a second copy.
  • The PPSA28416 result can't be reproduced here; a Linux test with a V# like the one reported (base/size values from the issue) would cover it.

@Mouhand-Kaddo

Copy link
Copy Markdown
Contributor

Additional platform validation of standalone PR head fe8660fd372dc56272e2ec31be2fce385295cac9 (2026-10-11):

Linux: passed — Arch Linux in WSL2 (kernel 6.6.87.2), GCC 16.2.1 20260810; CMake 4.3.1 / Ninja; Release, BUILD_TESTING=ON.

1 checks, 0 failures, 0 CTest skips. Built the selected targets and dependencies; this is focused validation, not a full build or full-suite run.

Checks: agc_driver_graphics.

Command after building: ctest --test-dir <build> --verbose --no-tests=error --timeout 30 -R "^agc_driver_graphics$".

This scope checks CPU-side behavior; no GPU execution result is claimed.

Internal limitation: heap-mirror subcases returned early because the guest arena was not write-watched in WSL. Zero CTest skips does not include that early return.

The new oversizedDescriptorTests fixture is guarded by _WIN32, so this Linux run checks the shared-source build and existing graphics cases; it does not execute the new over-limit descriptor regression. That Linux regression gap remains as noted by the maintainer.

No title/gameplay validation. These results cover the standalone head, not a merge with current main.

@Tamalero

Copy link
Copy Markdown
Contributor

Tested on High on Life (PPSA15388 v01.120.000, Unreal Engine 4): this fixes its stop when loading a level. On my tree, loading the first level of a new game aborted with:

AGC graphics: shader buffer exceeds descriptor range limit

The PR's commit cherry-picks cleanly. With it, that load goes through, and the abort hasn't come back in any run since. The agc_* tests have only the failures my tree had before.

Details

Setup: CachyOS (Linux 7.2), RTX 4090 (NVIDIA 615.78), GCC, Release. main f5c0fe00 plus my local integration patches and ports of #423/#1077, #425, #454 and #2904.

Tested and written by Claude (AI assistant) on a contributor's machine.

@jweidenh

Copy link
Copy Markdown
Contributor

I prepared a tested correction for the outstanding review points in this PR: patch on my fork.

The correction keeps shader buffer exceeds descriptor range limit when the accessible prefix reaches the device limit. A shorter prefix stops at the first inaccessible page, or the first non-writable page for a writable binding; an inaccessible base uses the empty binding. ReadRawShader and descriptor binding now share GuestMemory::CommittedPrefixBytes. The unverified console behavior is recorded under Unknown function info in TechnicalDebt.

The tests now run on both Windows and Linux. They cover exact-limit acceptance, oversized rejection with committed memory at/beyond the limit, short prefixes, inaccessible and read-only gaps, and empty bindings. The reported V# base/size is checked as descriptor metadata; the runtime empty-binding case uses the same stride/record count at an unaligned address inside an owned reservation.

A real Vulkan compute test also reads the bound SSBO length and two exact uint32 values from the valid prefix. It uses a reduced 192 KiB context limit over a bounded mapping; it does not allocate a device-limit-sized buffer or perform out-of-bounds shader accesses.

Validation used current main a11c8f7f70a0ed4e71504a89de69e8cdae687817 plus this PR's unchanged fe8660fd372dc56272e2ec31be2fce385295cac9, with identical tests before and after the correction:

Platform Before correction After correction
Windows 11, WinLibs GCC 15.2.0 r7, Release, RTX 5090 / driver 617.42 All 3 focused tests fail at their intended assertions All 5 selected CTests pass, no skips or capability omissions
Linux, GCC 13.3.0, Release, Mesa 25.2.8 lavapipe All 3 focused tests fail at their intended assertions All 5 selected CTests pass; graphics control has 2 existing heap-mirror omissions

The three regressions are agc_oversized_descriptor_limit, agc_oversized_descriptor_gap, and agc_oversized_descriptor_readback. Controls are agc_driver_graphics and agc_shader_memory. Every test finished below 30 seconds. The corrected native RTX 5090 readback took 2.74 seconds. Linux's graphics control could not check heap mirrors or heap mirrors of imported ranges because the guest arena was unavailable or not write-watched; all new regression assertions ran on both platforms. Linux validation built the patched libs target too, on a locally managed self-hosted runner.

Test-only baseline: b60b1f8447951d969477fec90871e5b0c2296f04. Corrected source: 7ff3781b5b04b4b7ed035e6ec4a4548febf7ec60.

To apply both commits, first rebase this PR onto main a11c8f7f70a0ed4e71504a89de69e8cdae687817, then cherry-pick b60b1f8447951d969477fec90871e5b0c2296f04 followed by 7ff3781b5b04b4b7ed035e6ec4a4548febf7ec60. I verified that sequence produces the tested tree. Direct application to the older PR head has context conflicts in the test files.

This is focused validation, not a full-suite or retail-title run. I do not have either affected title, and have not measured console behavior, game startup time or FPS. AI-assisted.

@github-actions

github-actions Bot commented Oct 12, 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
#4040 core/libs/prx/libSceAgcDriver/tests/Graphics.cpp L3

@CnRJay CnRJay closed this Oct 12, 2026
@CnRJay
CnRJay deleted the fix/agc-oversized-buffer-descriptor branch October 12, 2026 17:40
saarthakpant added a commit to saarthakpant/AnyPS5 that referenced this pull request Oct 12, 2026
A buffer V# larger than maxStorageBufferRange aborted the draw or
dispatch. Shaders keep unrelated words in SRT slots the path they take
never reaches, so such a V# now binds what is committed at its base:
nothing committed binds it as null, a shorter committed run cuts it there,
and the first case is logged. A committed run that reaches the limit
still throws, now with the V# words. Ghostrunner 2 aborts on such a V#
after its first presentation; High on Life reached the same stop (boykopovar#3202).

AI-assisted: yes
saarthakpant added a commit to saarthakpant/AnyPS5 that referenced this pull request Oct 13, 2026
A buffer V# larger than maxStorageBufferRange aborted the draw or
dispatch. Shaders keep unrelated words in SRT slots the path they take
never reaches, so such a V# now binds what is committed at its base:
nothing committed binds it as null, a shorter committed run cuts it there,
and the first case is logged. A committed run that reaches the limit
still throws, now with the V# words. Ghostrunner 2 aborts on such a V#
after its first presentation; High on Life reached the same stop (boykopovar#3202).

AI-assisted: yes
@Tamalero

Copy link
Copy Markdown
Contributor

High on Life (PPSA15388) still needs this on current main (18c438b10), and there is no replacement for it. Walking through the home base aborts with:

AGC graphics: shader buffer exceeds descriptor range limit

This PR's commit (fe8660fd3) still cherry-picks cleanly, and with it the title runs on. @CnRJay, is a resubmission planned? @jweidenh's correction above kept the refusal for prefixes that reach the device limit. If neither of you plans to carry it, I can open it again with that correction.

Tested and written by Claude (AI assistant) on a contributor's machine.

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.

7 participants