Skip to content

fix(agc): prefer imported prefixes before sparse heap mirrors - #8762

Merged
oneandonlydean merged 2 commits into
boykopovar:mainfrom
JJenkx:fix/sparse-heap-existing-import
Oct 21, 2026
Merged

oneandonlydean merged 2 commits into
boykopovar:mainfrom
JJenkx:fix/sparse-heap-existing-import

Conversation

@JJenkx

@JJenkx JJenkx commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What

A read-only buffer descriptor that extends past the committed prefix of a registered heap can bind a heap mirror before checking the host import that already covers that prefix. An address-based build then uses the import and releases the mirror. Alternating the two paths can recreate the same mirror repeatedly.

Try the existing import resolution and GPU-copy paths before the sparse heap mirror fallback. Keep the existing write-overlap, committed-range and descriptor-cut checks. A writable overhanging descriptor still falls back to a mirror; a view starting beyond an imported prefix is still rejected.

The regression fixture alternates descriptor and address-based builds over an imported 64 KiB prefix with an inaccessible tail, checks that no mirror is rebuilt or copied, rejects a view past the prefix, and verifies CPU/GPU stores through the writable fallback.

Overlap review: tools/pr_overlap.py reported no directly comparable overlap, but listed 22 stale/conflicting PRs for manual review. Their AGC diffs were inspected; none moves this sparse mirror fallback after import selection. #7647 changes mirror storage/allocation and keeps the original selection order; it is independent of this change. No dependency on the separate persistent heap-shadow experiment.

Tested

Linux / Arch Linux, AMD Radeon RX 6650 XT, RADV Mesa 26.2.4-arch1.1. Based on main 5b59fbc5539b6c6db7a26972f6069f1c6cc179ac; candidate 53b73b5dae17ab7005c93e766a3dc66fa2e10310.

  • Built libs, agc_driver_graphics_tests, and agc_driver_bda_device_tests.
  • Same focused test command on unchanged main and this branch: ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation VK_DRIVER_FILES=/usr/share/vulkan/icd.d/radeon_icd.json ctest --test-dir build-full -R '^agc_driver_(graphics|bda_device)$' -V.
  • Main: 100% tests passed out of 2; zero VUID- messages; no missing validation layer.
  • Change: 100% tests passed out of 2; zero VUID- messages; no missing validation layer; Sparse imported heap tests passed executed.
  • The initial regression fixture failed before the reorder on the earlier integration tree with the sparse descriptor did not bind its imported prefix, then passed after it. The standalone candidate above also passes on current main.
  • Review follow-up bcc36571bf (test only): sparseImportedHeapTests binds a read-only descriptor at a misaligned offset (base + 16, minStorageBufferOffsetAlignment 256, active recorder) that extends past the committed 64 KiB prefix into a PROT_NONE page. It must not bind the import, and its view and copied bytes stay within the committed prefix. agc_driver_graphics passes on bcc36571bf; with region.sparse removed from gpuCopyEligible (the reviewer's GPU-copy scenario) the case segfaults on the uncommitted tail. Sparse-run regions cannot take the GPU-copy or staging path because both require !region.sparse. This suite runs on the mock Vulkan device, so no validation layer is involved.
  • python3 tools/check_conventions.py --base origin/main: passed.

The new fixture is Linux-only. Independent Windows testing by @ycbuzi used candidate 53b73b5dae merged onto main b77bd58806 (merge 6301d6bb2a), Windows 11 Home 10.0.26200, MinGW-w64 GCC 15.2.0 (ucrt-posix-seh r7), Release with BUILD_TESTING=ON, and an NVIDIA GeForce RTX 4070 SUPER, driver 616.92 (ANYPS5_GPU=4070). They built libSceAgcDriver and agc_driver_graphics_tests; all 27 cases registered on Windows passed both repeats with ctest --repeat until-fail:2. This covers existing Windows cases; the Linux-only sparse-prefix fixture and a title were not run. No Windows validation-layer or VUID result was reported. No title frame-rate, loading-time, or crash-fix claim is made; Echoes of the End is still under investigation.

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 under Silent stubs in their library's technical debt file
  • 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
  • Fixes: none
  • Unblocks: none
  • GPUs tested: AMD Radeon RX 6650 XT; RADV Mesa 26.2.4-arch1.1; Arch Linux. Independent Windows existing graphics tests: NVIDIA RTX 4070 SUPER, driver 616.92; Windows 11, @ycbuzi (Linux-only fixture not run)
  • AI-assisted: yes

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the import-before-sparse-mirror reorder and the Linux regression fixture. The existing host-import and GPU-copy selection remains intact; the fallback now runs only after those paths, while the fixture checks read-only imported-prefix reuse, over-prefix rejection, writable fallback behavior, and cleanup. The reported focused validation runs and successful eligibility/convention checks are consistent with this narrow change.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 1191 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
PR 2434 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
PR 4448 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
core/libs/prx/libSceAgcDriver/tests/BdaResources.cpp
PR 4449 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
PR 5021 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
core/libs/prx/libSceAgcDriver/tests/BdaResources.cpp
PR 5372 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
core/libs/prx/libSceAgcDriver/tests/BdaResources.cpp
PR 6186 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
PR 6197 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
core/libs/prx/libSceAgcDriver/tests/BdaResources.cpp
PR 6412 core/libs/prx/libSceAgcDriver/tests/BdaResources.cpp
PR 6481 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
PR 6548 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
PR 6628 core/libs/prx/libSceAgcDriver/tests/BdaResources.cpp
PR 6918 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
core/libs/prx/libSceAgcDriver/tests/BdaResources.cpp
PR 6947 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
PR 7201 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
core/libs/prx/libSceAgcDriver/tests/BdaResources.cpp
PR 7514 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp
PR 7642 core/libs/prx/libSceAgcDriver/Graphics/src/GuestBufferMemory.cpp

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@JJenkx thanks, the reorder reads right for the aligned case your fixture covers. One path I'd like pinned before merge: now that the sparse mirror comes after the import paths, a sparse-run region whose import offset is not bindable in place (bindableInPlace false, so not a multiple of minStorageBufferOffsetAlignment when adjustedRegions is off) and that is gpuCopyEligible takes copyOnGpu. The copy loop then sizes the copy as region->end - region->begin. For a read-only region it first memcpys that whole range from guest memory (inputSnapshot), which would read the inaccessible tail, and CopyBuffer would read past the import, which only covers up to neededEnd. Before this change the sparse mirror took such a region first.

Could you either add a case to sparseImportedHeapTests with a descriptor that starts at a misaligned offset inside the committed prefix and extends past it, or keep sparse-run regions off the GPU copy path, for example else if (entry != nullptr && !runImport && recorder != nullptr && (staged || gpuCopyEligible(region)))? I haven't reproduced it; this is from reading the merged UploadFinish.

@ycbuzi

ycbuzi commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Windows run of 53b73b5 merged onto main b77bd58 (6301d6bb2a, clean), the host the description lists as not tested. Windows 11 Home 10.0.26200, MinGW-w64 GCC 15.2.0 (ucrt-posix-seh r7), Release, BUILD_TESTING=ON. GPU: NVIDIA GeForce RTX 4070 SUPER, driver 616.92, picked with ANYPS5_GPU=4070 (the machine also has a GTX 1650 SUPER).

Built libSceAgcDriver and agc_driver_graphics_tests with no errors or warnings; ctest --repeat until-fail:2 on the 27 tests agc_driver_graphics_tests registers on Windows: all pass on both repeats. The new fixture is under #ifndef _WIN32, so it did not run here; the existing cases pass with the reordered lookup. Not run: the new fixture (Linux-only), a title.

…fix without reading the tail

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JJenkx

JJenkx commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

@oneandonlydean thanks, pinned in bcc3657 (test only, no source change).

The GPU-copy fallback cannot take a sparse-run region: a sparse run requires region.sparse (sparseRun = region.sparseHeap && region.sparse), and both gpuCopyEligible and stagingEligible return false for region.sparse. A misaligned run import therefore falls through to the sparse heap mirror or the CPU copy of the committed pieces, as it did before the reorder.

sparseImportedHeapTests now has that case: minStorageBufferOffsetAlignment 256, an active recorder, and a read-only descriptor at base + 16 covering two 64 KiB pages of which only the first is committed (the second is PROT_NONE). The descriptor must not be bound to the import, its view must start with the guest's byte, and neither the view nor the copied bytes may exceed the committed 64 KiB. Here the view is the CPU copy of the committed piece (range 65536, offset 0, adjustment 0, 65536 bytes copied).

  • With this PR: agc_driver_graphics passes, including the later writable fallback and its mirror rebuild count.
  • Negative control, region.sparse removed from gpuCopyEligible (your scenario): the new case segfaults reading the uncommitted tail.
  • Base 5b59fbc with the new test fails earlier, at this PR's existing check (the sparse descriptor did not bind its imported prefix).

Linux 7.2.9, GCC 16.2.1, Release. This suite runs on the mock Vulkan device, so the validation layer does not load (no VUIDs to compare).

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@JJenkx thanks, that settles it: with region.sparse excluded from both gpuCopyEligible and stagingEligible, a misaligned sparse run can't reach the GPU copy, and the new case pins it with a negative control that segfaults on the uncommitted tail. We'll check bcc36571bf and run full CI.

@oneandonlydean
oneandonlydean merged commit 99844c7 into boykopovar:main Oct 21, 2026
12 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