Repository navigation
fix(agc): preserve empty images in stencil feedback checks - #5917
Conversation
0272af0 to
20e14ab
Compare
20e14ab to
5cfd7bf
Compare
|
Rebased onto main On an RTX 3060 (driver 591.86, Windows Server 2025), AI-assisted: yes |
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
5cfd7bf to
da0d5b2
Compare
|
Rebased onto main On the RTX 3060 (driver 591.86, Windows Server 2025), 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.
|
|
Tested on Linux (Arch),
With I did not find which part of the pipeline state lavapipe rejects. A skip on that device, or a different fixture state, is needed for the test to run on lavapipe. AI-assisted: yes |
adriadam10
left a comment
There was a problem hiding this comment.
The Draw.cpp:2096 change looks right, but the new fixture does not run outside your NVIDIA setup:
agc_driver_stencil_null_feedbacksegfaults insidelibvulkan_radeonon RADV (Mesa), inLinkPipelineFromLibrarieson the first draw, and also withAPS5_NO_GPL=1. @nicolasfalesy confirmed it independently on lavapipe (vkCreateGraphicsPipelinesresult -13, with and without GPL). CI runs the Vulkan tests on lavapipe, so it would be red.- Likely cause:
AttachNullImages(StencilNullFeedback.cpp:92-139) splices a descriptor-array variable into the SPIR-V by hand and appends a binding toresult.bindings. NVIDIA seems to tolerate a module/layout mismatch; other drivers do not. Please make the module and the pipeline layout agree (e.g. take the image array from a real shader, or validate the module with spirv-val), and run the test on lavapipe or RADV before pushing.
Once the fixture passes on a non-NVIDIA driver (and still fails with the Draw.cpp line reverted), this is ready.
The stencil null feedback fixture splices a storage image array into the fragment module by hand. spirv-val rejects the result with "Decorate is in an invalid layout section": the decorations were inserted in front of the first OpTypeVoid, which the recompiler emits after its Input and Output variables. The binding also used set 1 with the vertex-group number of a different heap kind (the 1D array one, not the 2D image the module declares), so the set layout the driver builds from the binding metadata (binding / 65 picks the set) put it in set 0 while the module declared set 1. Insert the decorations after the existing annotations, declare the image array type the recompiler emits for a float 2D storage heap, list the variable in the SPIR-V 1.4 entry point interface like the recompiler does, and number the binding as the recompiler does for the fragment stage, in the descriptor set the target uses. When built with ANYPS5_ENABLE_SPIRV_TOOLS the fixture validates its module against the Vulkan 1.1 environment. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks both for running the fixture on RADV and lavapipe. We only have an NVIDIA GPU, so we could not reproduce the crash. What we could do without a GPU: compile the fixture's two pixel shaders with the repo's recompiler, run the old splice on them, and check the result with spirv-val built from our 3rdparty/SPIRV-Tools submodule. That showed two real problems in
What changed (commit d707066, test and CMake only, the product line in
Check: with the same 8 combinations, spirv-val accepts the new modules, the aliasing variant used by the depth feedback case, and the unmodified recompiler output, in the vulkan1.1, vulkan1.1spv1.4, vulkan1.2 and vulkan1.3 environments (72 of 72 runs; the 8 old modules fail in all three of the environments they were checked in). We ran the validation function on the dumped modules in a separate small program; we have not built the whole tree with The fixture still feeds the same descriptor words: element 0 is a valid image and element 1 is all zeros, and the product line in Could one of you rerun AI-assisted: yes |
adriadam10
left a comment
There was a problem hiding this comment.
Re-checked the new head: the Draw.cpp:2096 skip is unchanged and correct, and agc_driver_stencil_null_feedback now passes on RADV (Mesa) and on lavapipe, including with ANYPS5_ENABLE_SPIRV_TOOLS=ON validating the spliced module. With the Draw.cpp line reverted the test fails, so it covers the change. Merging once the full Build is green.
|
Re-checked against current main: the PR still merges cleanly, the |
|
Picking this up while @adriadam10 is away: we will merge it once full CI and an independent review pass on this head. |
What
Keep supported empty typed-image elements out of stencil replay's attachment-feedback scan. A null element currently reaches texture decoding and throws before an otherwise valid draw. The existing empty-image predicate skips that element while non-null depth aliases still receive the feedback refusal.
This is the separate follow-up requested in #5466, which has merged. The diff against main is now only this change: one production line in
Draw.cppand its own native fixturetests/execution/StencilNullFeedback.cpp.Tested
341613ab6(137 commits pastab9a47684, several touchingDraw.cpp) with no conflicts; headda0d5b2be, same three commits. Conventions check passes.da0d5b2be:ctest -R 'stencil|masked_select|replay'passed 3/3 runs (agc_masked_select,agc_driver_sample_mask_depth_stencil,agc_driver_stencil_null_feedback,agc_driver_stencil_ref_export_unused,agc_driver_stencil_replay,agc_driver_stencil_replay_no_gpl,agc_driver_pm4_replay,agc_driver_pm4_replay_tool).agc_driver_stencil_ref_exportskips because this GPU lacksVK_EXT_shader_stencil_export.agc_driver_stencil_null_feedbackalso passed 3/3 runs withAPS5_NO_GPL=1.Draw.cppline reverted,agc_driver_stencil_null_feedbackfails withguest texture descriptor has a null base address; with it restored, the test passes.Checklist
main; no other open PR implements the same functions.mdfiles or images added to the repositoryCo-Authored-By: Codex noreply@openai.com