Skip to content

fix(agc): fragment SampleId, Layer, FragDepth and SampleMask interfaces - #375

Merged
adriadam10 merged 3 commits into
boykopovar:mainfrom
MichonGoddijn231849:fix/fragment-interface-validation
Oct 7, 2026
Merged

adriadam10 merged 3 commits into
boykopovar:mainfrom
MichonGoddijn231849:fix/fragment-interface-validation

Conversation

@MichonGoddijn231849

@MichonGoddijn231849 MichonGoddijn231849 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

What

The recompiler emits the following, but the SPIR-V validator refused them, so the draws were skipped:

  • SampleId and Layer fragment inputs (the packed pixel ancillary data);
  • a FragDepth output with the DepthReplacing execution mode (Z export);
  • a SampleMask output (coverage mask export), an int[1] or uint[1] array. Array interface types are now spelled elem[N] by the validator.

The validator now accepts them, together with the Geometry and SampleRateShading capabilities those inputs need in fragment shaders. They are accepted only when the device enabled geometryShader and sampleRateShading (#295 adds those enables), so a device without them refuses the draw. Vertex stages still refuse those capabilities.

GTA V's lighting, decal and particle shaders use these interfaces. The log showed unsupported device capability 2/35, unsupported fragment built-in, unsupported SPIR-V interface type and unsupported fragment execution mode.

Tested

Windows 11 Pro, AMD Ryzen 5 7600, NVIDIA GeForce RTX 5080 (driver 32.0.16.1074), 32 GB DDR5-5600, MinGW-w64 GCC 15.2.0.

Checklist

🤖 Generated with Claude Code

@MichonGoddijn231849 MichonGoddijn231849 changed the title fix(agc): fragment SampleId, Layer and FragDepth interfaces fix(agc): fragment SampleId, Layer, FragDepth and SampleMask interfaces Oct 2, 2026
@github-actions github-actions Bot added the agc libSceAgc* label Oct 2, 2026
@MichonGoddijn231849
MichonGoddijn231849 force-pushed the fix/fragment-interface-validation branch from 567a3fb to dd41c94 Compare October 2, 2026 19:39
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 3, 2026
@MichonGoddijn231849
MichonGoddijn231849 force-pushed the fix/fragment-interface-validation branch 2 times, most recently from e88b679 to feb284e Compare October 3, 2026 08:47
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 3, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

The validator side looks right to me, and the new cases are good. One dependency is missing: the body says the device already enables geometryShader and sampleRateShading, but on main VulkanDevice.cpp doesn't set either. Only #295 adds them. With this PR alone, a fragment module with Geometry or SampleRateShading passes validation and is created without those features enabled, which the Vulkan spec doesn't allow. Also, main's recompiler still emits ShaderLayer for a fragment Layer input (SpirvModuleSetup.cpp), and #295 is what switches it to Geometry.

Could you add Depends on #295, or move the two device enables into this PR? Ideally Geometry and SampleRateShading would only be accepted when the device enabled them, so a device without geometryShader refuses the draw.

@MichonGoddijn231849
MichonGoddijn231849 force-pushed the fix/fragment-interface-validation branch from feb284e to a40eb82 Compare October 4, 2026 08:48
@MichonGoddijn231849

Copy link
Copy Markdown
Contributor Author

Thanks, you're right: the description was wrong about main, and only #295 enables those features. I've done both:

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Thanks, both done. The validator now accepts Geometry and SampleRateShading in a fragment shader only when the device enabled geometryShader and sampleRateShading, and the test checks the refusal of each. Merged with current main it builds and passes the full ctest here. Ready to merge from my side right after #295, once both have been through a PPSA21564 run on my integration branch.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

This conflicts with main now in ShaderValidation.cpp, on the fragment execution-mode check. #752 (conservative Z export) added ExecutionModeDepthLess and ExecutionModeDepthGreater to the same Require(...) line where this PR adds ExecutionModeDepthReplacing. Accepting all of them (OriginUpperLeft, EarlyFragmentTests, DepthReplacing, DepthLess, DepthGreater) resolves it. #423 and #448 carry this commit and hit the same line.

@MichonGoddijn231849
MichonGoddijn231849 force-pushed the fix/fragment-interface-validation branch from fcc1118 to cc1ed88 Compare October 6, 2026 15:38
@MichonGoddijn231849

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (now cc1ed88). The ShaderValidation.cpp line accepts all five fragment modes as you suggested: OriginUpperLeft, EarlyFragmentTests, DepthReplacing, DepthLess and DepthGreater. #423 and #448 are rebased on top of it. Builds; the driver tests pass apart from agc_driver_scalar_sop1, which fails without this PR too.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 6, 2026

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

Checked the validator part (SampleId/Layer inputs, FragDepth with DepthReplacing, SampleMask [1] arrays, and Geometry/SampleRateShading gated on the device enables) against what the recompiler emits. Build + full ctest pass on cc1ed88 (360/360). This PR's own commits look good to me.

What blocks merging is the base: this branch carries #295's two commits, and #295 has open review requests (DrawRejection's read(cx, 0x1c4) throws when the register is missing, and the Z-export formats have no test). The same code is here at Graphics/src/State.cpp:793. Once #295 is fixed and merged, rebase this onto main so only the three validator commits remain, and I'll merge it.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 7, 2026
@Celegans12

Copy link
Copy Markdown
Contributor

I've been running Ghost of Yotei (PPSA26344 v1.008) with this PR in our integration tree, and it's what gets the game's depth-writing pixel shaders through now. The game has two depth-only draws that export Z: a DRAW_INDEX_AUTO with DB_SHADER_CONTROL 0x00000001 that runs once or twice per present from boot, and a DRAW_INDEX_2 with 0x00002021 that runs twice per present from the menus on.

In a 1,500 s run on an older main (before #295) and without this PR, 1,825 of those draws were skipped. With this PR at cc1ed88 on main 0d3c1ee, a 2,700 s run skips none of them, and nothing else rejects them instead: no unsupported fragment built-in, no execution-mode error, no FATAL and no device lost. A second 2,700 s run that got past the New Game settings had none either. Both runs were on Windows 11 with an RTX 3080 Laptop GPU.

Since #295 already brought the DB_SHADER_CONTROL part to main, I checked what main does on its own. Main's recompiler emits gl_FragDepth with DepthReplacing, but Inspect still accepts only FragCoord, FrontFacing and HelperInvocation as fragment built-ins and doesn't list DepthReplacing, so I'd expect these draws to stop there without your validator change. I haven't run main by itself to confirm that.

The PR conflicts with main at the moment. In our tree the merge was mechanical: main added imageInt64Atomics, runtimeDescriptorArray and partiallyBound at the same places in VulkanDevice, Context, ValidateShaders and Draw.cpp, so I kept those and put yours after them, and I took #295's zFormatSupported(zFormat) form of the 0x1c4 check in DrawRejection. I'm happy to run Yotei again on the rebased branch.

Details

Builds:

The skipped draws, from stderr of the run without the PR:

[gpu] skipped draw: depth export, shader coverage or ordered fragment execution is unsupported (register 0x203 = 0x00000001) [DRAW_INDEX_AUTO, color target 0x0]
[gpu] skipped draw: depth export, shader coverage or ordered fragment execution is unsupported (register 0x203 = 0x00002021) [DRAW_INDEX_2, color target 0x0]
phase (run without the PR) 0x00000001 per present 0x00002021 per present
boot, 0-680 s 1.00 0
menus, 680-1,092 s 1.96 1.95
gamma screen, 1,092-1,396 s 2.00 2.00
world, 1,396-1,600 s 2.00 2.00
total 1,343 482

With the PR: 0 and 0 in the cold 2,700 s run (1,115 presents) and in the warm 2,700 s run. Neither run's log has unsupported fragment built-in, unsupported fragment execution mode, unsupported SPIR-V interface type, depth or sample-mask export, FATAL or device lost. 0x2021 also sets CONSERVATIVE_Z_EXPORT to 1, so those shaders go through main's conservative-Z mapping as well.

gh_conflict_check of cc1ed88 onto main 68010df lists VulkanDevice.cpp, Context.hpp, Pipeline.hpp, Draw.cpp, ShaderValidation.cpp and State.cpp.

MichonGoddijn231849 and others added 3 commits October 7, 2026 18:18
The recompiler emits SampleId and Layer fragment inputs (pixel ancillary
data) and a FragDepth output with the DepthReplacing execution mode
(depth export), but the validator refused them, so those draws were
skipped. They are now accepted, as are the Geometry and SampleRateShading
capabilities those inputs need in fragment shaders. The device enables
both features when it has them.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Pixel shaders that export a coverage mask write gl_SampleMask, an
int[1] (or uint[1]) output. The validator rejected array interface
types; it now spells them elem[N] and accepts SampleMask as int[1] or
uint[1].

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e device enabled them

The validator takes the geometryShader and sampleRateShading enables from the
context (set where boykopovar#295 enables them), so a device without them refuses the
draw instead of creating a module that needs features it never enabled.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@MichonGoddijn231849
MichonGoddijn231849 force-pushed the fix/fragment-interface-validation branch from cc1ed88 to 29aa275 Compare October 7, 2026 17:01
@MichonGoddijn231849

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (now 29aa275). #295 is merged, so the branch now carries only this PR's three validator commits. Conflicts: main added imageInt64Atomics at the same places (VulkanDevice, Context, ValidateShaders/Inspect and the Draw.cpp call). Both stay, with main's parameter first, and the validator tests now pass imageInt64Atomics = false before geometryShader and sampleRateShading. Build and ctest pass.

@Celegans12 thanks for the Yotei runs. That matches what I see: main's Inspect still rejects FragDepth and DepthReplacing without this PR.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 7, 2026

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

Rechecked on 29aa275 merged with current main: build + full ctest pass (400/400), and the base issue from my previous review is gone now that #295 is merged, so I'm withdrawing that request.

  • The accepted types match what the recompiler emits (SampleMaskArrayType is int[1], Layer/SampleId are loaded as int).
  • The new array case in Signature() doesn't let arrayed Location variables through: AddLocations still unrolls them first.

Runtime coverage is good too: GTA V, Dreaming Sarah and Demon's Souls from the author, plus @Celegans12's 2,700 s Ghost of Yotei run with no skipped Z-export draws. Merging. @oneandonlydean, if your PPSA21564 run turns something up, a follow-up PR can handle it.

@adriadam10
adriadam10 merged commit db06102 into boykopovar:main Oct 7, 2026
4 checks passed
@MichonGoddijn231849
MichonGoddijn231849 deleted the fix/fragment-interface-validation branch October 9, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* build shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants