Repository navigation
fix(agc): accept any fragment export where the draw has no color target - #6708
Conversation
|
I also found a hole in the targetless case. |
|
Correction: my previous comment was a false positive. I checked the PR diff again and its |
jotitadev
left a comment
There was a problem hiding this comment.
Static review of head d5b49fd (no build, no GPU run).
The fix looks right to me: a location with no ColorTarget maps to VK_ATTACHMENT_UNUSED (or no attachment at all) in Pipeline.cpp, so skipping the type check there matches what Vulkan does with that output.
One note on the second new test case. targetless.colors.clear(); targetless.blends.resize(1); is a state the Pipeline constructor rejects:
Require(state.blends.size() == (state.colors.empty() ? 0u : state.colors.back().exportIndex + 1u) && ...,
"blend states do not match decoded color state");With no colors, blends.size() has to be 0, so a draw never reaches ValidateShaders with that state's pipeline. The unused-slot shape a real draw can have is a target at exportIndex 1 with slot 0 empty (colors = {exportIndex 1}, blends.size() == 2), with the shader still exporting at location 0. The change covers it too (the find_if misses location 0), but neither case in the test builds it. Swapping the second case for that state would test the gap the description talks about.
Method: read the diff against main 5d9dcc4 and modelled both Requires in a small Python script; nothing was built.
Written with AI assistance (Claude Code). No builds or tests were run for this note; only the standalone snippet above.
|
Thanks @jotitadev, good catch: the pipeline would refuse my unused-slot state before validation. f663675 replaces it with your shape, a target at slot 1 with slot 0 unused and the shader exporting at location 0. It fails on AI-assisted: yes |
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
f663675 to
2819f1d
Compare
The fragment output check demanded a float4 (or uint4 for integer and packed targets) at every location below the draw's MRT count, at least one. A location with no color target, a draw without color targets or a gap between written slots, has no attachment there, and Vulkan discards the output whatever its type. Crimson Desert's first draw binds no color target and exports a uint4 at location 0, and stopped there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The unused-slot case had no colors and one blend, which the pipeline refuses before it validates shaders. A target at slot 1 with slot 0 unused is the gap a draw can have. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2819f1d to
3d5335a
Compare
|
AMD on Windows: the tests this PR names pass on an RDNA 3.5 GPU. Radeon 860M (RDNA 3.5, driver 32.0.13058.8001), Windows 11 26300, WinLibs GCC 15.2.0 r7, Release.
AI-assisted |
|
Thanks @LeandroLP23 for the AMD run on Windows. I added it to the description. AI-assisted: yes (Claude Code) |
|
Re-checked against current main: the merge is clean, |
What
Crimson Desert (PPSA25286) stops at its first draw:
A throwaway print at the refusal in
ValidateShaders(ShaderValidation.cpp) shows the draw:The draw binds no color target (no blends, no colors), probably a depth-only pass. Its pixel shader still exports a uint4 at MRT0. The check goes over every output below
max(blends, 1)and requiresf32x4, oru32x4for integer and packed targets. At location 0 it finds no target, so it falls back tof32x4and refuses the draw.A location without a color target has no attachment in the pipeline. With no blends,
colorAttachmentCountis 0, and a gap between written slots isVK_ATTACHMENT_UNUSED. Vulkan discards an output that has no attachment, whatever its type. The check now applies only where a color target exists. The location still goes into the returned output set as before, so masks and pipeline keys of draws that already ran don't change.Tested
Linux (Ubuntu 24.04, GCC 13.3, Release), RTX 4070 Ti, NVIDIA 595.99.
agc_driver_graphics: the packed-export shader there exports a uint4 at location 0. It's now validated against a state with no color targets and no blends, and against one with a target at slot 1 and slot 0 unused, the gap a draw can actually have. (@jotitadev pointed out that my first unused-slot state, no colors with one blend, is one the pipeline refuses before validation.) Both are accepted, and validating it against a float target still fails withfloat4 colors. Withmain'sShaderValidation.cppthe test fails with the title's line,fragment shader must export float4 colors to its attachments. It passes 3 of 3 with this change.^agc_driver_|^agc_vertex_barrier|recompiler|shader_cache|request_serializer|spirv, 468 tests): 467 pass.agc_driver_store_over_untrackedfails the same way onmain5d9dcc4 here; test(agc): keep waiting for the neighbour copy case's result #6385 addresses it.Crimson Desert, scripted pad input, with #6612, #6613, #6706 and #6178 (for memory), which it needs to reach this draw:
Nothing is presented by then, so I haven't seen this draw's result. The test above covers the validation.
Validated on Windows 11 with an AMD Radeon 860M (RDNA 3.5, driver 32.0.13058.8001) by @LeandroLP23 (comment), in one build of
main6efb726 with this head and 13 other pull requests:agc_driver_graphicsandagc_driver_store_over_untrackedpass 3 runs of 3. 713 of 716agc_*tests pass; the 3 that fail fail the same way onmainthere.Not checked on Intel or on a console.
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest -R '^agc_' -Vonmaine2a2914 and onmain+ this PR (3d5335a, a local merge).main: 815 of 815 pass; with this PR: 815 of 815 pass. Neither log reports the layer missing. VUIDs onmain:VUID-VkImageViewCreateInfo-usage-02275×6,VUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605×18,VUID-vkCmdCopyBuffer-srcBuffer-00118×2,VUID-vkCmdDraw-multisampledRenderToSingleSampled-07285×2,VUID-vkCmdWaitEvents-srcStageMask-01158×2,VUID-vkSetEvent-event-09543×2; with this PR:VUID-VkImageViewCreateInfo-usage-02275×6,VUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605×18,VUID-vkCmdCopyBuffer-srcBuffer-00118×2,VUID-vkCmdDraw-multisampledRenderToSingleSampled-07285×2,VUID-vkCmdWaitEvents-srcStageMask-01158×2,VUID-vkSetEvent-event-09543×2. None is new with this PR.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code