Repository navigation
fix(agc): drop an unused stencil test value export from draws without a stencil test - #5304
Conversation
|
Demon's Souls (PPSA01341 v1.07) stops on the same recompile error on NVIDIA, but in a draw that has the stencil test enabled, so the "no stencil test" rule here doesn't cover it. Windows 11, RTX 4070 (driver 596.21, no
Happy for this to be folded in here, or to open it as a follow-up on top of this PR. AI-assisted: yes |
CnRJay
left a comment
There was a problem hiding this comment.
The reasoning holds: without a stencil test, the exported test value has no effect.
One pipeline-count side effect to check. The draw-time change (if (!product->state.stencilTest) product->pixel.stencilRefExportEnable = false;) runs on every device, including ones that have VK_EXT_shader_stencil_export. stencilRefExportEnable decides whether the module has a FragStencilRefEXT output, so it is part of what the pixel variant (and the pipeline) is keyed on. A title that uses one stencil-exporting pixel shader both with and without a stencil test then gets two variants and two pipeline sets where main had one, on AMD and Intel, which currently work. It's an extra compile and an extra pipeline on first use for each such shader, so a small hitch, but it's paid on the devices that didn't need the change.
Applying the clear only when the device lacks the extension (the same check PlanRegistered does) keeps one variant on those devices, and gives NVIDIA the fix. If an unconditional clear is preferred because a shader without the export is cheaper (early tests could in principle be allowed), that's a separate decision and would want its own measurement.
On Sam-mon7's Demon's Souls case: widening the rule to "the stencil test can't observe the value" (both faces ALWAYS and every op KEEP, or a zero ref mask) is exact by the same argument and covers that stop too. It needs the full register set in the condition: STENCILFUNC/_BF, the fail/zfail/pass ops of both faces, and REPLACE_TEST in DB_STENCIL_CONTROL.
|
I have two verified follow-up commits based directly on this PR's current head dbd015c: compare the changes. The predicate also omits the export when enabled stencil testing cannot consume the reference. It considers both faces, effective 8-bit masks, reachable depth/stencil operations, and preserves the export for dependent comparisons or reachable REPLACE writes. Unknown states remain conservative. The existing clear path and missing-extension refusal are preserved. Commits available to cherry-pick:
Verification on this public branch: typed CPU checks cover 22 accepted and 12 dependent states, the old predicate fails the ALWAYS/KEEP regression, all four modified C++ units pass optimized syntax checks, and conventions pass against both this PR head and upstream/main. The native fixture extends the existing homebrew programs with full color/stencil readbacks and six dependent-state refusal cases; this public branch has not received a full build or GPU run. Separate private integration evidence using the literal same production predicate: 20 expected native outcomes passed across the old/new drivers, including KEEP/INVERT, dependent refusals, reference-dependent replay controls, and GPL-disabled positive cases. Demon's Souls moved past the previous pixel-stage refusal to the full character-creation menu with a rendered knight model and no fatal error during a 905.7-second run. The first level is still unverified. Dreaming Sarah remained at 59.94 FPS in the steady-state check. GPUs tested: NVIDIA GeForce RTX 3060 12 GB, driver 591.86 (32.0.15.9186), Vulkan 1.4.325, Windows Server 2025 10.0.26100. GPU/title results above are from the private integration, not a GPU execution of this public branch. AI used: yes, Codex. Source implementation, independent adversarial review, CPU verification, and root-owned native/title verification completed before publication. Co-Authored-By: Codex noreply@openai.com |
dbd015c to
cbf4968
Compare
|
Thanks all three. Rebased onto
Windows 11, MinGW GCC 15.2 Release, NVIDIA GeForce RTX 3070 (driver 617.14), no |
cbf4968 to
30f5688
Compare
|
Checked the new head: the unused-reference predicate, the device-gated drop in Added one commit on your branch: removed the code comment in |
jotitadev
left a comment
There was a problem hiding this comment.
Run on a device that has VK_EXT_shader_stencil_export, since the runs so far are on NVIDIA without it. @CnRJay's question was about that case.
GPUs tested: Apple M2 Max, MoltenVK 1.4.2 (Vulkan 1.4.357), macOS 27.2, x86-64 under Rosetta
main does not run the GPU tests on macOS yet, so I tested on macos-port at b7eb6b6b (mugurc/AnyPS5) plus the ImportWatched change from #1, with this PR's own diff (merge-base..head cbf496835) applied. Not main. The only later commit, 30f56882f, removes a code comment in DrawDecode.cpp, so these results also hold for the current head. Release build, LunarG Vulkan SDK 1.4.363.0. Suite: ctest -R "^agc" -E "agc_display_buffer_dcc|agc_driver_flip|agc_driver_lds_atomics64" -j1 --timeout 30. Without the PR, 13 tests fail on this setup (7 with the #2704 build), all known macOS failures listed in #1.
DrawDecode.cpp and tests/Graphics.cpp conflict with code that is on main and not on the branch. I kept the branch's side and added only this PR's own lines:
- the stencil block in
decodeDraw; stencilComparisonandstencilReferenceUnusedTests, with their call and the--unused-stencil-referenceswitch.
The other files apply cleanly.
agc_driver_stencil_ref_export_unusedpasses.--case keepand--case invertpass. The six dependent cases are skipped by design (skipped: a dependent reference refusal requires a device without native stencil export).agc_driver_graphicspasses (22 accepted states, 12 dependent countermodels), and so doesagc_driver_stencil_ref_export.- Suite: the same 13 failures with and without the PR. None new, none fixed.
- With only the new execution test on the unchanged branch, it also passes. That is expected: with the extension present,
mainnever refused this shader. So this run shows that the "keep the export when the extension is present" path does not break anything. It does not exercise the fix itself.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Rebased onto current |
30f5688 to
ea4378c
Compare
|
@adriadam10 thanks for rebasing. I rebased locally at the same time (onto |
ea4378c to
2e31643
Compare
|
Rebased onto current |
|
Demon's Souls (PPSA01342) reaches this refusal on Build: Our integration builds carry this PR's change (the AI-assisted: yes |
|
Prepared an author-preserving rebase onto The conflict is next to the new quad coverage assignments in Fresh CPU validation against the rebased One retained test caveat: the six manual dependent GPUs tested: none for this rebase (CPU only). Host: NVIDIA GeForce RTX 3060 12 GB, driver 591.86 (32.0.15.9186), Vulkan 1.4.325, Windows Server 2025 10.0.26100. AI-assisted: yes Co-Authored-By: Codex noreply@openai.com |
|
Additional native verification: the default unused-export test passes full color/stencil readbacks on the RTX 3060. The replay suite also passes with and without graphics pipeline libraries. All three returned exit 0, in 1.17–1.28 seconds per process. These ran in a fresh Release/O3 integration containing this PR's unchanged six commits, the earlier deferred-descriptor integration, #6259, and opt-in ownership diagnostics. This is separate from the current-main rebase branch offered above; it does not certify that branch's full driver build, the six legacy dependent GPUs tested: NVIDIA GeForce RTX 3060 12 GB, driver 591.86 (32.0.15.9186), Vulkan 1.4.325, Windows Server 2025 10.0.26100. AI-assisted: yes Co-Authored-By: Codex noreply@openai.com |
… a stencil test A pixel shader with DB_SHADER_CONTROL.STENCIL_TEST_VAL_EXPORT_ENABLE needs VK_EXT_shader_stencil_export, which NVIDIA's driver does not offer. In a draw without a stencil test the exported value has no effect, so the draw decode drops the export there. On a device without the extension, the pixel shader is no longer prepared when its ABI is resolved, where the draw's stencil test is not known yet; it is prepared at its first draw. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Codex <noreply@openai.com>
Exercise ALWAYS with KEEP and INVERT using the existing pixel program and complete color and stencil readbacks. Keep dependent comparisons and replacement operations behind the existing native extension refusal. Co-Authored-By: Codex <noreply@openai.com>
…il_export Dropping an unused stencil reference export changes the pixel variant key, so a device that has the extension would compile a second variant and pipeline for a shader drawn both with and without a stencil test. Only a device that lacks the extension needs the drop, so the draw decode now checks the device's capabilities first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
|
Rebased onto current |
2e31643 to
6f0aa97
Compare
|
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.
|
What
Since 22d662e a pixel shader with
DB_SHADER_CONTROL.STENCIL_TEST_VAL_EXPORT_ENABLEneedsVK_EXT_shader_stencil_export, which NVIDIA's driver does not offer. On NVIDIA this stops Marvel's Wolverine (PPSA03671):sceAgcLinkShadersresolves the ABI of pixel shader0x11d01d0900(DB_SHADER_CONTROL bit 1 set), the driver prepares it right there, the recompile throwsa pixel shader that exports the stencil reference needs VK_EXT_shader_stencil_export, which the device lacks, and the title aborts.The exported value is only the stencil test value: it feeds the stencil test and what
REPLACE_TESTwrites. In a draw without a stencil test it has no effect. So:Driver::decodeDrawclearsstencilRefExportEnablewhen the decoded state has no stencil test. The shader then has noFragStencilRefEXToutput, on every device. Early fragment tests stay off for it, as before.PlanRegistereddoes not prepare such a pixel shader when its ABI is resolved on a device without the extension, because the draw's stencil test is not known yet there. It is prepared at its first draw instead, like other shaders the ABI step cannot decide.Tested
Windows 11, RTX 3070 (no
VK_EXT_shader_stencil_export), GCC 15.2.0 UCRT, Release, on main0214069e3.agc_driver_stencil_ref_export_unused(GPU): registers a pixel shader that exports a stencil test value and a color, resolves its ABI withDB_SHADER_CONTROL=0x802andSPI_SHADER_Z_FORMAT=9, then draws it withDB_DEPTH_CONTROL=0and no depth target, and checks that the color target was written. On main it fails at the ABI resolve with the error above. With only thePlanRegisteredchange it fails at the draw with the same error. With both it passes. Not run on a device with the extension (none here); there only the draw-time change applies.agc_driver_stencil_ref_exportskips here: the device lacks the extension).tools/check_conventions.pypasses.682ef0c17plus feat(agc): implement image and sampler tables indexed at runtime offsets #757 (rebased), fix(agc): render a color view of several array slices into its first slice #3633, fix(agc): store D16 depth with stencil as D24 or D32 where the device lacks it #3846 and feat(agc): decode GS_FAST_LAUNCH geometry with one point per subgroup #4702 (current main does not reach this point without feat(agc): implement image and sampler tables indexed at runtime offsets #757): without this change it aborts with the error above. With it, the title gets pastsceAgcLinkShadersand stops later at a separateGetImageResource dword 0 is not a valid runtime valuein another pixel shader, before any draw with the stencil-exporting shader. So the draw-time part is covered by the test, not yet by the title.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository🤖 Generated with Claude Code