Skip to content

perf(agc): prove sampled depth surfaces in the fast resource proof - #1430

Merged
adriadam10 merged 3 commits into
boykopovar:mainfrom
fisherjoey:fix/depth-surface-record
Oct 10, 2026
Merged

adriadam10 merged 3 commits into
boykopovar:mainfrom
fisherjoey:fix/depth-surface-record

Conversation

@fisherjoey

@fisherjoey fisherjoey commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

What

This is the fix from #1132. Sets that sample a depth surface now pass the fast resource proof instead of taking the full walk on every reuse.

cachedTexture returned DepthSurfaceTexture's object before calling logLookup, so the element had no lookup record and fastRevalidate failed with no record every time. The walk then succeeded, because the depth surface hands out the same object each time. The only cost was one full walk per dispatch.

  • The depth return now logs a record flagged as depth, and ValidatedSurface carries the flag.
  • fastRevalidate and fastRevalidateEach prove a depth element with the new DepthSurfaceHolds(context, address, texture): the newest surface at that address still hands out that object, which is what the walk's DepthSurfaceTexture lookup would return. The memory, key and pending checks are skipped for that element.
  • A new fast-fail reason, depth surface, covers a surface that no longer hands the object out.

New test: depthSurfaceProofTests in tests/Recorder.cpp (agc_driver_recorder_tests, a separate commit). It registers a D32 depth surface on a real device, builds a compute set whose only binding samples it, and requires Revalidate to take the fast path on the second and third use. The test needs a real device with the driver's Context, which the mock harness in tests/Graphics.cpp doesn't have, so it sits with the other cache tests in the recorder suite. It builds on main unchanged and fails there on behaviour:

AGC graphics: a set sampling an unchanged depth surface left the fast proof for the full walk on use 2

With APS5_PROFILE_DRAW=1, the [rescache] line on main shows the reason:

[rescache] revalidate 1 calls 0.0 ms: fast 0, full 1, failed 0; fast-fail by reason: no record 1; ...

On this branch it passes, and the same line reads:

[rescache] revalidate 1 calls 0.0 ms: fast 1, full 0, failed 0; fast-fail by reason:; ...

The idea for a dedicated test came from @emirplus in #1199, which implements the same change. Thanks to them, and to @oneandonlydean for spelling out what the test has to show.

Demon's Souls (PPSA01341 v1.07), PS Studios movie, per 10 s:

before after
revalidate calls: fast / full 72142 / 18697 101101 / 2617
indirect proofs: fast / T1 refreshed / full 2187 / not logged / 9547 3017 / 8396 / 463
indirect device call 193.9-229.6 us 145.7 us

The frame rate did not change: queue 0's wait for the GPU mutex took up the time saved.

#425 and #454 change the same lookup (per-use depth copies, retired surfaces). If one of them lands first, a depth record has to skip per-use copies and DepthSurfaceHolds has to treat a retired surface as not holding. I'll rebase onto whichever lands.

Closes #1132.

Tested

Linux 7.2 (Nobara 44), GCC 16.2, RTX 5070 Ti, driver 595.91.07.

  • agc_driver_recorder_tests (run directly; it isn't registered with ctest): fails on main with the message above and passes here. If you want it under ctest, I can register the suite here or in a follow-up.
  • Rebased onto main a29ef59. Since the typed descriptor heaps landed, a sampled image has to sit in an image heap and the heaps need nullDescriptor, so the test binds at RuntimeAbi::FirstImageBinding and the recorder suite's device now enables VK_EXT_robustness2 nullDescriptor when the GPU has it, as the driver's own device does (the test reports "not tested" without it). The new test passes. The suite then stops later at "guest allocation ... is still pinned", the same as on unmodified main.
  • ctest -R agc_ -j1 on a29ef59: 321 of 322. The one failure, agc_driver_dcc_keys_first_write, also fails on main.
  • On the earlier base, ctest -R "agc_driver|guest_agc_driver": 164 of 164 pass. agc_driver_lds_atomics64 and agc_driver_scalar_sop1 are excluded; they fail on unmodified main on this GPU too.
  • All runs used APS5_WRITE_WATCH_IMPORTS=watch. On current main, this NVIDIA driver refuses the new dma-buf import probe (vkAllocateMemory -2), and after that every allocation on the device fails, so most GPU tests fail on unmodified main without the variable. The recorder suite also stops at its own dma-buf probe test for the same reason, so the new test runs before that point. fix(agc): turn off dma-buf import on the NVIDIA proprietary driver #1380 and fix(agc): keep dma-buf import off the NVIDIA proprietary driver #1329 turn that probe off on NVIDIA's proprietary driver.
  • Demon's Souls (PPSA01341 v1.07): numbers above.

Checklist

  • Based on current main; no other open PR implements the same functions (fix(agc): record depth surface lookups and fast-prove held surfaces #1199 does; see above)
  • 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
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes

@oneandonlydean

Copy link
Copy Markdown
Collaborator

This now conflicts with main in ShaderResources.cpp, please rebase. It also replaces #1199, which was closed in its favour.

@github-actions github-actions Bot added the agc libSceAgc* label Oct 8, 2026
@fisherjoey
fisherjoey force-pushed the fix/depth-surface-record branch from 1af4eeb to 965f018 Compare October 8, 2026 14:38
@fisherjoey

Copy link
Copy Markdown
Contributor Author

Rebased onto main 9f4bd71 (head is now 965f018).

The conflict was in cachedTexture: #1113 added the Recorder::BoundKeptBytes() call right above the depth-surface return. I kept that call and put the lookup record after it.

ctest (-j1 --timeout 60, Nobara 44 / Linux 7.2, GCC 16.2.1, RTX 5070 Ti, driver 595.91.07): agc_driver + guest_agc_driver 183/183. Full run 462/463. The one failure is guest_dynamic_loader, which also fails on a build of #1133's branch on the same main and is not related to this change.

@fisherjoey

Copy link
Copy Markdown
Contributor Author

Rebased on main 29e533a (head 33bcbf0). The depth fast path now only covers plain 2D depth lookups (2D dimension, base array 0), so it composes with #1368's slice refresh: array and layered views take the full walk through DepthSurfaceTexture. depthSurfaceProofTests also checks that the same lookup as a 2D array, or with a nonzero base array, is not fast-provable.

agc_driver_recorder_tests passes the depth cases here but stops later in importWindowTests (Recorder.cpp:2138, "a cache over the pages the import reported sees no change"). Main does the same on this NVIDIA setup.

@popcoder565

Copy link
Copy Markdown
Contributor

Native Windows run, since no Windows results were posted. Setup: Windows 10 (19045), RTX 3060 Laptop (NVIDIA 616.92), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release. Head 33bcbf07, merged onto main 8b393b6 in one build with 31 other PRs that had no Windows run.

agc_driver_recorder_tests now stops in the new depthSurfaceProofTests:

Shader runtime ABI: binding is not a typed heap

The test's DescriptorBinding sets binding = 0 for a sampled image. On current main, binding 0 is Binding::Buffers, and sampled images start at FirstImageBinding (1). So HeapCapacity throws. That doesn't depend on Windows, but the recorder test isn't in ctest, so CI won't show it. I ran it with #2248 and #2299 merged, since main fails it earlier for other reasons (#2155).

The exception then unwinds through ~Recorder, whose Sync → Submit crashes in the driver and hides the message. I had to catch it inside main's scope to see it.

The full ctest passes apart from one unrelated failure from #2359 (520 of 521).

🤖 Addressed by Claude Code

@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 for the fix and the numbers. One blocker:

  • depthSurfaceProofTests sets binding.binding = 0 (tests/Recorder.cpp:2630). On current main binding 0 is Binding::Buffers, and sampled images start at RuntimeAbi::FirstImageBinding, so HeapCapacity throws Shader runtime ABI: binding is not a typed heap (RuntimeAbi.hpp:73). @popcoder565 hit exactly this on Windows with this head. Please use ShaderRecompiler::RuntimeAbi::FirstImageBinding (as tests/Graphics.cpp does) and re-check that the test still fails on main for the intended reason (no record), not on this exception.
  • agc_driver_recorder_tests isn't registered in ctest, so CI never runs the new test and didn't catch the above. Please register the suite (or move the test to a registered target) so it is covered.

The change itself in ShaderResources.cpp / DepthSurface.cpp looks fine.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 10, 2026
@fisherjoey
fisherjoey force-pushed the fix/depth-surface-record branch from 33bcbf0 to f1e4838 Compare October 10, 2026 10:44
@fisherjoey

Copy link
Copy Markdown
Contributor Author

@popcoder565 thanks, that was right. The rebase onto a29ef59 (f1e4838) binds the test's sampled image at FirstImageBinding, passes the new gpu argument to Mutation::Add, and enables nullDescriptor on the recorder suite's device when the GPU has it (the driver's own device already requires it); without that the depth cases print "not tested". The depth proof cases pass there on the RTX 5070 Ti. The PR conflicts with main again, so it gets another rebase next.

AI-assisted: yes

@adriadam10
adriadam10 force-pushed the fix/depth-surface-record branch from f1e4838 to a8c4f86 Compare October 10, 2026 22:58

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

Rebased onto current main (the only conflict was in tests/Recorder.cpp).

  • main now enables nullDescriptor on the recorder suite's device and exposes Device::NullDescriptors(), so I dropped the duplicate robustness block; depthSurfaceProofTests checks device.NullDescriptors() and sets context.nullDescriptors = true on its context copy, like the neighbouring test.
  • agc_driver_recorder_tests on RADV: depthSurfaceProofTests passes (fast 1, full 0). With the four non-test files reverted to main it fails with left the fast proof for the full walk on use 2 / no record 1. The suite still stops later at (s) results pending when their memory left the watch, as on main without this test.
  • ctest for libSceAgcDriver: 332/332.
  • Not covered by CI: the suite isn't registered in ctest and PR CI doesn't build.

Merging once CI is green.

@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
#374 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp L20
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp L122, L219
#425 core/libs/prx/libSceAgcDriver/Graphics/include/DepthSurface.hpp L20-21
core/libs/prx/libSceAgcDriver/Graphics/src/DepthSurface.cpp L122

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.

AGC: sets that sample a depth surface never pass the fast proof (no record)

4 participants