layers: Fix AlaphToCoverage with Dual Source

spencer-lunarg · Jun 5, 2024 · 7001473 · 7001473
1 parent 15380fc
commit 7001473
Show file tree

Hide file tree

Showing 10 changed files with 64 additions and 41 deletions.
diff --git a/layers/core_checks/cc_cmd_buffer_dynamic.cpp b/layers/core_checks/cc_cmd_buffer_dynamic.cpp
@@ -1570,7 +1570,7 @@ bool CoreChecks::ValidateDrawDynamicStateShaderObject(const LastBound& last_boun
             const LogObjectList frag_objlist(cb_state.Handle(), fragment_shader_stage->Handle());
             skip |= LogError(vuid.alpha_component_word_08920, frag_objlist, loc,
                              "alphaToCoverageEnable is set, but fragment shader doesn't declare a variable that covers "
-                             "Location 0, Component 0.");
+                             "Location 0, Component 3 (alpha channel).");
         }
     }
 

diff --git a/layers/core_checks/cc_pipeline_graphics.cpp b/layers/core_checks/cc_pipeline_graphics.cpp
@@ -3175,14 +3175,11 @@ bool CoreChecks::ValidateDrawPipeline(const LastBound &last_bound_state, const v
     if (pipeline.IsDynamic(CB_DYNAMIC_STATE_ALPHA_TO_COVERAGE_ENABLE_EXT) &&
         cb_state.dynamic_state_value.alpha_to_coverage_enable) {
         if (pipeline.fragment_shader_state && pipeline.fragment_shader_state->fragment_entry_point) {
-            // TODO - DualSource blend has two outputs at location zero, so Index == 0 is the one that's required.
-            // Currently lack support to test each index.
-            if (!pipeline.fragment_shader_state->fragment_entry_point->has_alpha_to_coverage_variable &&
-                !pipeline.DualSourceBlending()) {
+            if (!pipeline.fragment_shader_state->fragment_entry_point->has_alpha_to_coverage_variable) {
                 const LogObjectList objlist(cb_state.Handle(), pipeline.Handle());
                 skip |= LogError(vuid.dynamic_alpha_to_coverage_component_08919, objlist, vuid.loc(),
                                  "vkCmdSetAlphaToCoverageEnableEXT set alphaToCoverageEnable to true but the bound pipeline "
-                                 "fragment shader doesn't declare a variable that covers Location 0, Component 3.");
+                                 "fragment shader doesn't declare a variable that covers Location 0, Component 3 (alpha channel).");
             }
         }
     }

diff --git a/layers/core_checks/cc_shader_interface.cpp b/layers/core_checks/cc_shader_interface.cpp
@@ -166,13 +166,11 @@ bool CoreChecks::ValidateInterfaceFragmentOutput(const vvl::Pipeline &pipeline,
     bool skip = false;
     const auto *ms_state = pipeline.MultisampleState();
     if (!pipeline.IsDynamic(CB_DYNAMIC_STATE_ALPHA_TO_COVERAGE_ENABLE_EXT) && ms_state && ms_state->alphaToCoverageEnable) {
-        // TODO - DualSource blend has two outputs at location zero, so Index == 0 is the one that's required.
-        // Currently lack support to test each index.
-        if (!entrypoint.has_alpha_to_coverage_variable && !pipeline.DualSourceBlending()) {
+        if (!entrypoint.has_alpha_to_coverage_variable) {
             skip |= LogError("VUID-VkGraphicsPipelineCreateInfo-alphaToCoverageEnable-08891", module_state.handle(),
                              create_info_loc.dot(Field::pMultisampleState).dot(Field::alphaToCoverageEnable),
                              "is VK_TRUE, but the fragment shader doesn't declare a variable that covers "
-                             "Location 0, Component 3.");
+                             "Location 0, Component 3 (alpha channel).");
         }
     }
     return skip;

diff --git a/layers/state_tracker/pipeline_state.h b/layers/state_tracker/pipeline_state.h
@@ -296,13 +296,6 @@ class Pipeline : public StateObject {
         return false;
     }
 
-    bool DualSourceBlending() const {
-        if (fragment_output_state) {
-            return fragment_output_state->dual_source_blending == VK_TRUE;
-        }
-        return false;
-    }
-
     const vku::safe_VkPipelineViewportStateCreateInfo *ViewportState() const {
         // TODO A render pass object is required for all of these sub-states. Which one should be used for an "executable pipeline"?
         if (pre_raster_state) {

diff --git a/layers/state_tracker/pipeline_sub_state.cpp b/layers/state_tracker/pipeline_sub_state.cpp
@@ -257,22 +257,3 @@ bool FragmentOutputState::IsBlendConstantsEnabled(const AttachmentStateVector &a
     }
     return result;
 }
-
-// static
-bool FragmentOutputState::GetDualSourceBlending(const vku::safe_VkPipelineColorBlendStateCreateInfo *color_blend_state) {
-    if (!color_blend_state) {
-        return false;
-    }
-    for (uint32_t i = 0; i < color_blend_state->attachmentCount; ++i) {
-        const auto &attachment = color_blend_state->pAttachments[i];
-        if (attachment.blendEnable) {
-            if (IsSecondaryColorInputBlendFactor(attachment.srcColorBlendFactor) ||
-                IsSecondaryColorInputBlendFactor(attachment.dstColorBlendFactor) ||
-                IsSecondaryColorInputBlendFactor(attachment.srcAlphaBlendFactor) ||
-                IsSecondaryColorInputBlendFactor(attachment.dstAlphaBlendFactor)) {
-                return true;
-            }
-        }
-    }
-    return false;
-}
diff --git a/layers/state_tracker/pipeline_sub_state.h b/layers/state_tracker/pipeline_sub_state.h
@@ -176,7 +176,6 @@ struct FragmentOutputState : public PipelineSubState {
             color_blend_state = ToSafeColorBlendState(cbci);
             // In case of being dynamic state
             if (cbci.pAttachments) {
-                dual_source_blending = GetDualSourceBlending(color_blend_state.get());
                 if (cbci.attachmentCount) {
                     attachment_states.reserve(cbci.attachmentCount);
                     std::copy(cbci.pAttachments, cbci.pAttachments + cbci.attachmentCount, std::back_inserter(attachment_states));
@@ -195,7 +194,6 @@ struct FragmentOutputState : public PipelineSubState {
     }
 
     static bool IsBlendConstantsEnabled(const AttachmentStateVector &attachment_states);
-    static bool GetDualSourceBlending(const vku::safe_VkPipelineColorBlendStateCreateInfo *color_blend_state);
 
     std::shared_ptr<const vvl::RenderPass> rp_state;
     uint32_t subpass = 0;
@@ -207,5 +205,4 @@ struct FragmentOutputState : public PipelineSubState {
 
     bool blend_constants_enabled = false;  // Blend constants enabled for any attachments
     bool sample_location_enabled = false;
-    bool dual_source_blending = false;
 };
diff --git a/layers/state_tracker/shader_module.cpp b/layers/state_tracker/shader_module.cpp
@@ -43,6 +43,9 @@ void DecorationBase::Add(uint32_t decoration, uint32_t value) {
         case spv::DecorationComponent:
             component = value;
             break;
+        case spv::DecorationIndex:
+            index = value;
+            break;
         case spv::DecorationNonWritable:
             flags |= nonwritable_bit;
             break;
@@ -811,7 +814,8 @@ EntryPoint::EntryPoint(const Module& module_state, const Instruction& entrypoint
                         max_output_slot = &slot;
                         max_output_slot_variable = &variable;
                     }
-                    if (slot.Location() == 0 && slot.Component() == 3) {
+                    // Dual source blending can use a non-index of zero here
+                    if (slot.Location() == 0 && slot.Component() == 3 && variable.decorations.index == 0) {
                         has_alpha_to_coverage_variable = true;
                     }
                 }

diff --git a/layers/state_tracker/shader_module.h b/layers/state_tracker/shader_module.h
@@ -60,8 +60,9 @@ struct DecorationBase {
 
     // When being used as an User-defined Variable (input, output, rtx)
     uint32_t location = kInvalidValue;
-    // Component is optional and spec says it is 0 if not defined
+    // Component and Index are optional and spec says it is 0 if not defined
     uint32_t component = 0;
+    uint32_t index = 0;
 
     uint32_t offset = 0;
 

diff --git a/tests/unit/shader_interface.cpp b/tests/unit/shader_interface.cpp
@@ -1316,6 +1316,32 @@ TEST_F(NegativeShaderInterface, AlphaToCoverageOutputLocation0) {
     CreatePipelineHelper::OneshotTest(*this, set_info, kErrorBit, "VUID-VkGraphicsPipelineCreateInfo-alphaToCoverageEnable-08891");
 }
 
+TEST_F(PositiveShaderInterface, AlphaToCoverageOutputIndex1) {
+    TEST_DESCRIPTION("DualSource blend has two outputs at location zero, so Index 0 is the one that's required");
+    AddRequiredFeature(vkt::Feature::dualSrcBlend);
+    RETURN_IF_SKIP(Init());
+    InitRenderTarget(0u);
+
+    const char *fs_src = R"glsl(
+        #version 460
+        layout(location = 0, index = 1) out vec4 c0;
+        void main() {
+		    c0 = vec4(0.0f);
+        }
+    )glsl";
+    VkShaderObj fs(this, fs_src, VK_SHADER_STAGE_FRAGMENT_BIT);
+
+    VkPipelineMultisampleStateCreateInfo ms_state_ci = vku::InitStructHelper();
+    ms_state_ci.rasterizationSamples = VK_SAMPLE_COUNT_1_BIT;
+    ms_state_ci.alphaToCoverageEnable = VK_TRUE;
+
+    const auto set_info = [&](CreatePipelineHelper &helper) {
+        helper.shader_stages_ = {helper.vs_->GetStageCreateInfo(), fs.GetStageCreateInfo()};
+        helper.ms_ci_ = ms_state_ci;
+    };
+    CreatePipelineHelper::OneshotTest(*this, set_info, kErrorBit, "VUID-VkGraphicsPipelineCreateInfo-alphaToCoverageEnable-08891");
+}
+
 TEST_F(NegativeShaderInterface, AlphaToCoverageOutputNoAlpha) {
     TEST_DESCRIPTION(
         "Test that an error is produced when alpha to coverage is enabled but output at location 0 doesn't have alpha component.");

diff --git a/tests/unit/shader_interface_positive.cpp b/tests/unit/shader_interface_positive.cpp
@@ -735,6 +735,32 @@ TEST_F(PositiveShaderInterface, FragmentOutputNotConsumedButAlphaToCoverageEnabl
     CreatePipelineHelper::OneshotTest(*this, set_info, kErrorBit);
 }
 
+TEST_F(PositiveShaderInterface, AlphaToCoverageOutputIndex0) {
+    TEST_DESCRIPTION("DualSource blend has two outputs at location zero, so Index 0 is the one that's required");
+    AddRequiredFeature(vkt::Feature::dualSrcBlend);
+    RETURN_IF_SKIP(Init());
+    InitRenderTarget(0u);
+
+    const char *fs_src = R"glsl(
+        #version 460
+        layout(location = 0, index = 0) out vec4 c0;
+        void main() {
+		    c0 = vec4(0.0f);
+        }
+    )glsl";
+    VkShaderObj fs(this, fs_src, VK_SHADER_STAGE_FRAGMENT_BIT);
+
+    VkPipelineMultisampleStateCreateInfo ms_state_ci = vku::InitStructHelper();
+    ms_state_ci.rasterizationSamples = VK_SAMPLE_COUNT_1_BIT;
+    ms_state_ci.alphaToCoverageEnable = VK_TRUE;
+
+    const auto set_info = [&](CreatePipelineHelper &helper) {
+        helper.shader_stages_ = {helper.vs_->GetStageCreateInfo(), fs.GetStageCreateInfo()};
+        helper.ms_ci_ = ms_state_ci;
+    };
+    CreatePipelineHelper::OneshotTest(*this, set_info, kErrorBit);
+}
+
 // Spec doesn't clarify if this is valid or not
 // https://gitlab.khronos.org/vulkan/vulkan/-/issues/3445
 TEST_F(PositiveShaderInterface, DISABLED_InputOutputMatch2) {