Skip to content

fix(agc): preserve all bytes in image-backed buffer copies - #6249

Merged
oneandonlydean merged 3 commits into
boykopovar:mainfrom
Ondra97:codex/fix-copy-alias-byte-coverage
Oct 27, 2026
Merged

oneandonlydean merged 3 commits into
boykopovar:mainfrom
Ondra97:codex/fix-copy-alias-byte-coverage

Conversation

@Ondra97

@Ondra97 Ondra97 commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

What

I reproduced incorrect raw DMA copy results on main d92a8dad390af1807cc91622b924252141816a5a. A completed GPU dispatch leaves a cached 128×120 RGBA16F surface occupying 131,072 guest bytes. The image-copy shortcut copies its texels but leaves 8,192 destination bytes unchanged. A subsequent DMA_DATA reader of offset 0x15400 receives 0xa5a5a5a5 instead of the source payload 0x3b3a3938. The same packets with APS5_NO_COPY_ALIAS=1 copy every byte correctly. The packet's byte count follows the existing decoder and AMD PAL's BuildDmaData.

I require proven image coverage of the entire guest byte range before selecting the image alias. The proof reuses StorageTexture::uncoveredBytes, including its handling of tile edges, row pitch and mip gaps/tails. A range without complete proof uses the existing byte-transfer path.

The coverage guard is a plan.refusal in the shared PlanAlias, immediately after the source lookup and before any destination lookup or creation. AliasCopyPossible and AliasCopy therefore use the same coverage rule. The guard remains specific to raw buffer-copy alias selection; image-copy callers keep their existing semantics. Prerequisite #5480, including its no-device test-registration fix, is now merged into main. This rebase drops those carried commits and retains this PR's two commits, followed by a final main-update merge.

The PM4 regression covers a fresh padded destination, a padded destination with older pending image results, and a fully covered 128×128 surface. It compares every source/destination byte after GPU completion and includes a subsequent 4-byte DMA payload consumer. All guest allocations remain alive throughout the cases. The no-alias control runs the same packets.

The additional preflight regression uses a cached source image without a source host import and an imported destination. A padded source must be refused before execution; registering it then enables the correct buffer transfer. A fully covered source must still support an image alias. The no-alias control verifies the buffer path for both layouts. Moving the guard back into AliasCopy alone makes this regression fail.

The original overlap review covered 62 exact shared PR implementations, including full diffs for 21 large branches. The #5480 diff (9 files, +163/-55) and the combined guard/test changes were reviewed during the original integration; #5480 is now part of main. #6197 changes import coverage, #5998 retained-image budgeting and #5761 untracked snapshots; these do not preserve the missing raw bytes.

Tested

Original before/after measurements, captured on 2026-10-15 on the same PC and GPU workload, compare main d92a8dad390af1807cc91622b924252141816a5a with the original fix 9886b3d96dc20bd068a9fe752d6275fb315cc85e (Windows 11, GCC 15.2.0, NVIDIA Windows driver 32.0.16.1714, shader disk cache disabled):

Measurement Before (main) After (original fix)
Incorrect bytes, fresh padded destination (131,072-byte copy) 8,192 0
Incorrect bytes, cached/pending padded destination 8,192 0
Follow-up DMA payload in the original fresh-destination reproduction 0xa5a5a5a5 0x3b3a3938
Image aliases for the fully covered 128×128 positive control 1 1

Validation refreshed on 2026-10-20. Tested head cf459245b8b17b3e1bdcc7efc1440e7db03ad5ae includes main d5c956a10087914b52922fd3ece6439ccbb3e741. The prerequisite #5480 and its no-device test-registration fix are already merged; the topic retains its two original fix/regression commits plus a final main-update merge. The byte-coverage regression source is unchanged from the previous published head.

Windows (native host): Windows 11 Pro 10.0.26300, WinLibs MinGW-w64 GCC 15.2.0, Release; AMD Ryzen 9 7900X, 32 GB RAM, NVIDIA GeForce RTX 4070 Ti SUPER, Windows driver 32.0.16.1742 (NVIDIA 617.42).

  • On both tested revisions, relinker, libs and all executable targets needed by the focused tests built successfully. This includes the runtime and all patched libraries; unrelated test fixtures were not all built or run.
  • The 19 shared focused CTests ran on main 329b9aa1a7a413b562263f47a6b6a5863bfc205f and the final change. Subsequent main updates were inspected: AGC, shader and libc sources stayed identical, and the only libkernel change was in the POSIX implementation excluded on Windows. The main-side results remain the comparison for these unchanged Windows paths; the final change was rebuilt and tested after the update. Both byte-copy variants additionally ran on the change. Runs used ANYPS5_REQUIRE_VULKAN=1, ANYPS5_GPU=NVIDIA, ANYPS5_NO_SHADER_CACHE=1, VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, verbose logging and serial execution on the same machine and GPU.
  • Khronos Validation Layer 1.4.335 was loaded from the process-local VK_LAYER_PATH; its DLL has a valid LunarG signature. The loader logs confirm actual layer insertion. Neither complete log contains a missing requested layer message or a VUID- validation error.
Validation Main Change
Shared focused CTests passed 19/19 19/19
Additional byte-copy CTests passed n/a 2/2
Failed or skipped tests 0 0
VUID- errors in the complete log 0 0
Missing requested validation layer messages 0 0

The padded PM4 cases reported zero incorrect bytes and payload 0x3b3a3938; the fully covered positive control retained one image alias. Direct padded preflight refused the unimported source and used buffer path 1 after registration; direct dense preflight used image path 3. The no-alias control used the buffer path for both layouts. agc_driver_small_copy_sync and upstream agc_driver_alias_copy_label passed. Patched-library validation loaded 352 libraries with zero failures.

Conventions, whitespace, main-sync and test-registration audits passed. The earlier guard-location mutation check was not repeated; the regression source remains unchanged.

After configuring the full Release build with BUILD_TESTING=ON and the supported MinGW toolchain, build the executable targets corresponding to the selected tests, plus relinker and libs. Run this shared pattern on both checkouts:

$env:ANYPS5_REQUIRE_VULKAN = "1"
$env:ANYPS5_GPU = "NVIDIA"
$env:ANYPS5_NO_SHADER_CACHE = "1"
$env:VK_LAYER_PATH = "C:\path\to\validation-layer"
$env:VK_INSTANCE_LAYERS = "VK_LAYER_KHRONOS_validation"
$env:VK_LOADER_DEBUG = "layer"
$shared = '^(agc_driver|agc_driver_small_copy_sync|agc_driver_alias_copy_label|agc_driver_copied_writer_copy|agc_driver_stream_copy|agc_space_pending_writes|agc_pending_write_snapshot|agc_pending_key_stores|agc_host_import_unmap|agc_host_import_retirement|agc_host_import_second_mapping|agc_copied_direct_ordering|agc_driver_pending_exact_surface|agc_driver_host_import_at_map|agc_driver_host_import_teardown|agc_driver_host_import_windows|agc_driver_graphics|agc_driver_pm4|patched_libraries)$'
ctest --test-dir build-windows --no-tests=error --output-on-failure -V -j 1 -R $shared

The full change-side pattern, including the two additional byte-copy cases, is:

$all = '^(agc_driver|agc_driver_small_copy_sync|agc_driver_copy_surface_bytes(_no_alias)?|agc_driver_alias_copy_label|agc_driver_copied_writer_copy|agc_driver_stream_copy|agc_space_pending_writes|agc_pending_write_snapshot|agc_pending_key_stores|agc_host_import_unmap|agc_host_import_retirement|agc_host_import_second_mapping|agc_copied_direct_ordering|agc_driver_pending_exact_surface|agc_driver_host_import_at_map|agc_driver_host_import_teardown|agc_driver_host_import_windows|agc_driver_graphics|agc_driver_pm4|patched_libraries)$'
ctest --test-dir build-windows --no-tests=error --output-on-failure -V -j 1 -R $all

Limits: no retail title, other GPU vendor or console timing run. Direct preflight and real PM4 packet tests establish byte-copy correctness for the exercised paths without a game compatibility or speed claim. Hosted full Windows jobs require collaborator dispatch under CONTRIBUTING.md; skipped jobs are not passes.

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: NVIDIA GeForce RTX 4070 Ti SUPER, Windows driver 32.0.16.1742, Windows 11 Pro 10.0.26300
  • AI-assisted: yes — Codex. I am responsible for the change.

@github-actions github-actions Bot added build agc libSceAgc* labels Oct 15, 2026
This was referenced Oct 15, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Heads-up on an overlap with #5480 (just rebased onto main): this PR's ImageCoversGuestBytes guard and #5480's PlanAlias refactor overlap. Whichever lands second moves the guard into PlanAlias after the source lookup as a plan.refusal, so AliasCopyPossible refuses the same copies AliasCopy does.

@Ondra97

Ondra97 commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

I have integrated this in c0dcc05 and declared #5480 as a prerequisite. The coverage guard is now a PlanAlias refusal immediately after the source lookup, shared by preflight and execution. The new regression fails when the guard is moved back into AliasCopy alone. Full validation results and platform limits are in the updated description. #5480 should land first.

AI-assisted: yes — Codex.

@github-actions github-actions Bot added needs rebase Conflicts with main, rebase needed and removed needs rebase Conflicts with main, rebase needed labels Oct 17, 2026
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 19, 2026
@Ondra97
Ondra97 force-pushed the codex/fix-copy-alias-byte-coverage branch from bdf6533 to 5ba5489 Compare October 19, 2026 04:32
@github-actions github-actions Bot added needs rebase Conflicts with main, rebase needed and removed needs rebase Conflicts with main, rebase needed labels Oct 19, 2026
@Ondra97
Ondra97 force-pushed the codex/fix-copy-alias-byte-coverage branch from 5ba5489 to 2d9e3dc Compare October 19, 2026 11:59
@github-actions github-actions Bot added needs rebase Conflicts with main, rebase needed and removed needs rebase Conflicts with main, rebase needed labels Oct 19, 2026
@oneandonlydean oneandonlydean self-assigned this Oct 20, 2026
@Ondra97
Ondra97 force-pushed the codex/fix-copy-alias-byte-coverage branch from 2d9e3dc to cf45924 Compare October 20, 2026 05:00
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 20, 2026
@EagleBearerYS

Copy link
Copy Markdown
Contributor

A note on merge order. Our upcoming fix for Ghost of Tsushima Director's Cut (PPSA02225), fix/agc-3d-view-committed-slices-v2, registers its new tests next to this PR's in core/libs/prx/libSceAgcDriver/CMakeLists.txt. The code doesn't overlap: ours uploads only the committed slabs of a sampled 3D view whose range is only partly committed.

Ours will go second and adapt to this one when it merges, with this PR in its Depends on.

@klosixgaming-arch

Copy link
Copy Markdown
Contributor

Test run (not a code review). I merged this PR onto current main (49d1f9b), built it on Windows (Release, winlibs), and ran Tekken Dark Resurrection (PPSA31371, PSPHD) for 112 s.

  • Merges cleanly, builds, and the game stays alive for the full run.
  • The run log differs from a baseline main run by only about 93 lines out of ~650, which is run-to-run noise, nothing new or removed in the errors.
  • I did not verify any visual change. My screenshots of this run did not capture the game window, so I cannot say whether the issue this PR targets is fixed.

So: no regression found, and no evidence of a fix either way.

@babyhuey

Copy link
Copy Markdown
Contributor

Linux run of this head cf459245b8b, merged onto main 442c43a574d (clean merge, local only), with the selection CI's skipped Linux product job uses: anyps5-build CI image (Ubuntu 24.04, GCC 13, Release, BUILD_TESTING=ON), lavapipe with ANYPS5_REQUIRE_VULKAN=1 GALLIUM_OVERRIDE_CPU_CAPS=avx. The tree was configured fresh after the merge and built in full, then select_tests.py select / ninja -f select.ninja selected / --target libs / select_tests.py ctest --timeout 120.

  • Selected 1,507 tests: all pass. (main itself passes its full suite in this container.)

AI-assisted: yes (Claude Code)

@EagleBearerYS

Copy link
Copy Markdown
Contributor

@babyhuey Thanks for the Linux run of this head on current main, with the selection the skipped Linux CI job uses. All 1,507 selected tests passing is a useful check.

@oneandonlydean
oneandonlydean merged commit 8b2ae8a into boykopovar:main Oct 27, 2026
12 checks passed
@Ondra97
Ondra97 deleted the codex/fix-copy-alias-byte-coverage branch October 27, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* build

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants