Skip to content

feat(agc): store pending storage-image results at each flip with APS5_STORE_AT_FLIP - #1995

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
lubaxe:feat/agc-store-pending-at-flip
Oct 10, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
lubaxe:feat/agc-store-pending-at-flip

Conversation

@lubaxe

@lubaxe lubaxe commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

What

Addresses #1970. Pending storage-image results are stored only when something reads their memory. PPSA12544 frees render targets after its GPU work, and Unity's allocator reuses that memory. A write trap on the image pages caught HollowKnight.exe+0x1a578fe and +0x1a57931 writing block headers at page boundaries (+0x1fe000, then +0x1fdff8) into a 1920x1080 target while its results were pending. The late store then:

  • either drops those GPU results (the CPU-written blocks win), so the backgrounds render as noise,
  • or overwrites the allocator's headers, which crashes the title in the same allocator code (+0x1a5771d, +0x1a57ad7) while tut_01 loads.

On hardware the results land as the GPU writes them, before the title can reuse the memory.

APS5_STORE_AT_FLIP=1 stores every pending result when a display buffer is flipped (PresentDisplayBuffer), through the existing StorageTexture::FlushAllPending, which had no caller. A title has to wait for GPU work before reusing its memory, so a flip is a point where the results are due at the latest. Storing them at the title's own waits (labels, fences) would be the general fix; it is recorded in TechnicalDebt.md as unimplemented. Off by default.

Tested

Conflicts with #1994 only in docs/user/USAGE.md. I'll rebase whichever merges second.

Update (main d70b899): #1994 is closed, since #2096 restores the frame rate on main. With this flag alone, PPSA12544 runs gameplay at 600 presents / 10 s with correct rendering (Windows 11, RTX 5090, with #846, #425, #1836, #1599, #1968 and #1969).

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

🤖 Generated with Claude Code

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

The regression evidence is unusually strong, especially the page-write trap and title run. One remaining attribution gap: the reported correct 60 fps run uses both APS5_STORE_AT_FLIP=1 and #1994’s APS5_WRITE_WATCH_IMPORTS=watch, while the bad run is described without the combination. Could you report the four-way result (neither, each switch alone, both) or otherwise isolate which change prevents the allocator overwrite? That will show whether this flag independently fixes #1970 and how it composes with #1994.

@lubaxe

lubaxe commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, here is the four-way result. Same build for all four: main f145139 with #846, #425, #1918, #1968, #1969, this PR and #1994. Same machine (Windows 11, RTX 5090). Each run starts the title and lets the intro play into tut_01 with no input, for up to 180 s, with APS5_PROFILE_DRAW=1 and frame dumps (APS5_DUMP_FRAMES=60 APS5_DUMP_FRAMES_EVERY=200).

APS5_STORE_AT_FLIP APS5_WRITE_WATCH_IMPORTS Result Gameplay rate
– – crashed at 23 s: AGC driver: null or misaligned address –
1 – ran 180 s; tut_01 frames render correctly 56 presents / 10 s (GPU busy ~104 ms)
– watch crashed at 55 s while tut_01 loaded: vertex fetch exceeds descriptor record count; the frames before it are black 600 / 10 s before the crash
1 watch ran 180 s; tut_01 frames render correctly 600 / 10 s (GPU busy ~3 ms)

Earlier runs without either switch on the same stack:

  • One reached gameplay with the backgrounds rendered as noise, at about 5 fps.
  • Three crashed while tut_01 loaded, in the allocator code at +0x1a5771d / +0x1a57ad7. One of those was in this matrix's first pass.

So:

One caveat: the frame dumps themselves change timing, which probably explains why the neither-switch run failed earlier here (23 s) than without dumps (in tut_01).

(AI-assisted: Claude Code; the runs are on my machine.)

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Thanks, this four-way matrix closes the attribution gap. It shows APS5_STORE_AT_FLIP=1 fixes the corruption on its own, while APS5_WRITE_WATCH_IMPORTS=watch alone is fast but can consume stale memory and crash; the combined setting is both fast and stable in this run. The updated #1994 description now makes the pairing requirement clear. I still see no upstream checks on either PR head, so I will hold formal review until Build and Conventions run.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

APS5_STORE_AT_FLIP=0 turns the store on too: VulkanDevice.cpp:2034 only checks getenv(...) != nullptr. Please accept 1 and throw on any other value, as APS5_WRITE_WATCH_IMPORTS does.

The new check in metadataPassTests passes on main as well, since FlushAllPending is already there, so it doesn't cover this change; drop it or test the flip path.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@lubaxe
lubaxe force-pushed the feat/agc-store-pending-at-flip branch from ebb28dd to 4d0042b Compare October 9, 2026 12:56
@lubaxe

lubaxe commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, both addressed in 4d0042b (on top of a rebase onto main d70b899):

  • Strict value. StorageTexture::StoreAtFlipRequested reads APS5_STORE_AT_FLIP: unset is off, 1 is on, anything else throws APS5_STORE_AT_FLIP=<value>: expected 1, at the first flip. USAGE.md says so.
  • Test. I dropped the metadataPassTests check. PresentDisplayBuffer needs a swapchain, and no test reaches it headless, so the flip call itself is covered only by the title runs above. The new storeAtFlipTests in agc_driver_graphics (ctest) covers what this PR adds: unset, 1, and that 0, empty, true and 11 are rejected. With the old != nullptr check it fails.

Also: on current main, agc_driver_recorder_tests stops in storageRefreshTests (the refresh's store of the results did not reach guest memory), with or without this PR, on Windows (RTX 5090).

(AI-assisted: Claude Code.)

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@lubaxe
lubaxe force-pushed the feat/agc-store-pending-at-flip branch from 4d0042b to 1cbf28d Compare October 10, 2026 01:21
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 10, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Same as #1969: it conflicts with main in tests/Graphics.cpp now; please rebase.

lubaxe and others added 2 commits October 10, 2026 08:56
…_STORE_AT_FLIP

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…stead of FlushAllPending

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@adriadam10
adriadam10 force-pushed the feat/agc-store-pending-at-flip branch from 1cbf28d to 3221ae8 Compare October 10, 2026 07:03
@adriadam10

Copy link
Copy Markdown
Collaborator

Rebased onto current main as requested (conflicts in tests/Graphics.cpp and TechnicalDebt.md, both adjacency: kept highestDrawIndexTests and the other new TechnicalDebt entry next to ours).
Rebuilt, libSceAgcDriver ctest 303/303. Merging on green CI.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 10, 2026
@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
#2573 core/libs/prx/libSceAgcDriver/tests/Graphics.cpp L3599

@adriadam10
adriadam10 merged commit a86ffd3 into boykopovar:main Oct 10, 2026
14 checks passed
SleglJan pushed a commit to SleglJan/AnyPS5 that referenced this pull request Oct 10, 2026
…-at-flip

feat(agc): store pending storage-image results at each flip with APS5_STORE_AT_FLIP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants