search

KhronosGroup / Vulkan-ValidationLayers

Vulkan Validation Layers (VVL)
https://vulkan.lunarg.com/doc/sdk/latest/linux/khronos_validation_layer.html
Other
725 stars 396 forks source link

False positive about fragment output dynamic state when fragment shader is missing #7858

Open ShabbyX opened 2 months ago

ShabbyX commented 2 months ago

Take the following scenario:

VVL complains that:

[ VUID-vkCmdDraw-None-08608 ] ... vkCmdDraw():  VkPipeline 0x4b000000004b[] doesn't set up VK_DYNAMIC_STATE_BLEND_CONSTANTS, but it calls the related dynamic state setting commands.

which is not true. This seems to be due to the following code:

// In Pipeline::CreateFragmentOutputState:

    if (p.library_create_info) {
        auto ss = GetLibSubState<VK_GRAPHICS_PIPELINE_LIBRARY_FRAGMENT_OUTPUT_INTERFACE_BIT_EXT>(state, *p.library_create_info);
        // If this pipeline is linking in a library that contains FO state, check to see if the FO state is valid before creating it
        // for this pipeline
        if (ss && EnablesRasterizationStates(p.pre_raster_state)) {
            return ss;
        }

In the above, the linked fragment output library is removed from tracking. Later in GetGraphicsDynamicState, this if is false:

    if (!has_fragment_output_state && pipe_state.fragment_output_state) {
        flags |= pipe_state.fragment_output_state->parent.dynamic_state;
    }

which means VVL doesn't realize the pipeline does in fact have the dynamic state.

ShabbyX commented 2 months ago

This fixes things for the test I'm looking at (though I don't know the ramifications on everything else):

-        if (ss && EnablesRasterizationStates(p.pre_raster_state)) {
+        if (ss) {
spencer-lunarg commented 2 months ago

So I tried to reproduce it with a test but couldn't

TEST_F(PositiveGraphicsLibrary, NoFragmentShaderDynamicBlendConstant) { TEST_DESCRIPTION("https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/7858"); SetTargetApiVersion(VK_API_VERSION_1_3); RETURN_IF_SKIP(InitBasicGraphicsLibrary()); InitRenderTarget(); CreatePipelineHelper vertex_input_lib(*this); vertex_input_lib.InitVertexInputLibInfo(); vertex_input_lib.CreateGraphicsPipeline(false); CreatePipelineHelper pre_raster_lib(*this); { const auto vs_spv = GLSLToSPV(VK_SHADER_STAGE_VERTEX_BIT, kVertexMinimalGlsl); vkt::GraphicsPipelineLibraryStage vs_stage(vs_spv, VK_SHADER_STAGE_VERTEX_BIT); pre_raster_lib.rs_state_ci_.rasterizerDiscardEnable = VK_TRUE; pre_raster_lib.InitPreRasterLibInfo(&vs_stage.stage_ci); pre_raster_lib.CreateGraphicsPipeline(); } VkDynamicState dynamic_states[1] = {VK_DYNAMIC_STATE_BLEND_CONSTANTS}; VkPipelineDynamicStateCreateInfo dynamic_create_info = vku::InitStructHelper(); dynamic_create_info.pDynamicStates = dynamic_states; dynamic_create_info.dynamicStateCount = 1; CreatePipelineHelper frag_out_lib(*this); frag_out_lib.InitFragmentOutputLibInfo(); frag_out_lib.gp_ci_.pDynamicState = &dynamic_create_info; frag_out_lib.CreateGraphicsPipeline(false); VkPipeline libraries[3] = { vertex_input_lib.Handle(), pre_raster_lib.Handle(), frag_out_lib.Handle(), }; VkPipelineLibraryCreateInfoKHR link_info = vku::InitStructHelper(); link_info.libraryCount = size(libraries); link_info.pLibraries = libraries; VkGraphicsPipelineCreateInfo exe_pipe_ci = vku::InitStructHelper(&link_info); exe_pipe_ci.pDynamicState = &dynamic_create_info; exe_pipe_ci.layout = pre_raster_lib.gp_ci_.layout; vkt::Pipeline exe_pipe(*m_device, exe_pipe_ci); ASSERT_TRUE(exe_pipe.initialized()); m_commandBuffer->begin(); m_commandBuffer->BeginRenderPass(m_renderPassBeginInfo); float blends[4] = {1.0f, 1.0f, 1.0f, 1.0f}; vk::CmdSetBlendConstants(m_commandBuffer->handle(), blends); vk::CmdBindPipeline(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, exe_pipe.handle()); vk::CmdDraw(m_commandBuffer->handle(), 3, 1, 0, 0); m_commandBuffer->EndRenderPass(); m_commandBuffer->end(); }

@ShabbyX is there an ANGLE test to reproduce this issue

The underlying problem seems to be that we decide to ignore the Fragment Output state ... there are many reason in the spec it can be ignored so hard to reproduce from the description

ShabbyX commented 2 months ago

@ShabbyX is there an ANGLE test to reproduce this issue

Yes sure. In vk_renderer.cpp, comment out the lines that suppress 08608:

diff --git a/src/libANGLE/renderer/vulkan/vk_renderer.cpp b/src/libANGLE/renderer/vulkan/vk_renderer.cpp
index be2c2e4ffb..d20252ced6 100644
--- a/src/libANGLE/renderer/vulkan/vk_renderer.cpp
+++ b/src/libANGLE/renderer/vulkan/vk_renderer.cpp
@@ -248,8 +248,8 @@ constexpr const char *kSkippedMessages[] = {
     "VUID-vkCmdBlitImage-srcImage-00240",
     // https://anglebug.com/8242
     // VVL bug: https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/7858
-    "VUID-vkCmdDraw-None-08608",
-    "VUID-vkCmdDrawIndexed-None-08608",
+    //"VUID-vkCmdDraw-None-08608",
+    //"VUID-vkCmdDrawIndexed-None-08608",
     // https://anglebug.com/8242
     // Invalid feedback loop caused by the application
     "VUID-vkCmdDraw-None-09000",

and run either of these:

$ ./angle_end2end_tests --gtest_filter=StateChangeTestES3.RasterizerDiscard/ES3_Vulkan_NoSupportsExtendedDynamicState2
$ ./angle_end2end_tests --gtest_filter=StateChangeTestES3.RedefineTransformFeedbackBuffer/ES3_Vulkan_NoSupportsExtendedDynamicState2

Looks like dynamic rasterizer discard is fine, but not static (given that only the no-EDS2 variants of the tests generate this error)

spencer-lunarg commented 2 months ago

Thanks the ANGLE tests are perfect, can reproduce the issue with simple VVL test

TEST_F(PositiveDynamicState, DepthWriteFromVertexShader) {
    TEST_DESCRIPTION("https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/7858");
    RETURN_IF_SKIP(Init());
    InitRenderTarget();

    CreatePipelineHelper pipe(*this);
    pipe.AddDynamicState(VK_DYNAMIC_STATE_BLEND_CONSTANTS);
    pipe.shader_stages_ = {pipe.vs_->GetStageCreateInfo()};
    pipe.rs_state_ci_.rasterizerDiscardEnable = VK_TRUE;
    pipe.CreateGraphicsPipeline();

    m_commandBuffer->begin();
    m_commandBuffer->BeginRenderPass(m_renderPassBeginInfo);

    vk::CmdBindPipeline(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, pipe.Handle());
    float blends[4] = {1.0f, 1.0f, 1.0f, 1.0f};
    vk::CmdSetBlendConstants(m_commandBuffer->handle(), blends);
    vk::CmdDraw(m_commandBuffer->handle(), 1, 0, 0, 0);

    m_commandBuffer->EndRenderPass();
    m_commandBuffer->end();
}

looking into the issue with the fragment based dynamic state not being saved

ShabbyX commented 6 days ago

Potentially related bug: If rasterizer discard is enabled and VK_EXT_graphics_pipeline_library is used, VVL does this for the fragment output libraries:

dynamic_pipeline_rendering_create_info((pPipelineRenderingCreateInfo && rasterization_enabled)
                                           ? pPipelineRenderingCreateInfo
                                           : &VkPipelineRenderingCreateInfo_default) {

Basically when rasterizer discard is enabled, VVL considers there are no attachment formats. This leads to incorrect draw time validation error:

if (view_state && view_state->create_info.format != pipeline_rendering_ci.depthAttachmentFormat) {
    const LogObjectList objlist(cb_state.Handle(), pipeline.Handle());
    skip |= LogError(vuid.dynamic_rendering_depth_format_08914, objlist, vuid.loc(),
                     "VkRenderingInfo::pDepthAttachment->imageView format (%s) must match corresponding format "
                     "in VkPipelineRenderingCreateInfo::depthAttachmentFormat (%s)",
                     string_VkFormat(view_state->create_info.format),
                     string_VkFormat(pipeline_rendering_ci.depthAttachmentFormat));
}

It complains that the rendering pass has a depth/stencil format, but the pipeline didn't specify what format it is. It's not correct because the app did provide that information, but VVL decided to throw it away :grin:

ShabbyX commented 5 days ago

Another related bug, a pipeline with rasterizer disacard that is used with an MSAA framebuffer (no MSRTSS, no dynamic sample count, no GPL) triggers VUID-vkCmdDrawIndexed-multisampledRenderToSingleSampled-07285 (and 07286 and 07287) because the MSAA state passed to the pipeline is ignored. In that case, the following code returns nullptr:

    // the VkPipelineMultisampleStateCreateInfo need to be identically defined so can safely grab both
    const vku::safe_VkPipelineMultisampleStateCreateInfo *MultisampleState() const {
        // TODO A render pass object is required for all of these sub-states. Which one should be used for an "executable pipeline"?
        if (fragment_shader_state && fragment_shader_state->ms_state &&
            (fragment_shader_state->ms_state->rasterizationSamples >= VK_SAMPLE_COUNT_1_BIT) &&
            (fragment_shader_state->ms_state->rasterizationSamples < VK_SAMPLE_COUNT_FLAG_BITS_MAX_ENUM)) {
            return fragment_shader_state->ms_state.get();
        } else if (fragment_output_state && fragment_output_state->ms_state &&
                   (fragment_output_state->ms_state->rasterizationSamples >= VK_SAMPLE_COUNT_1_BIT) &&
                   (fragment_output_state->ms_state->rasterizationSamples < VK_SAMPLE_COUNT_FLAG_BITS_MAX_ENUM)) {
            return fragment_output_state->ms_state.get();
        }
        return nullptr;
    }

and the following code returns VK_SAMPLE_COUNT_1_BIT:

VkSampleCountFlagBits LastBound::GetRasterizationSamples() const {
    // For given pipeline, return number of MSAA samples, or one if MSAA disabled
    VkSampleCountFlagBits rasterization_samples = VK_SAMPLE_COUNT_1_BIT;
    if (!pipeline_state || pipeline_state->IsDynamic(CB_DYNAMIC_STATE_RASTERIZATION_SAMPLES_EXT)) {
        rasterization_samples = cb_state.dynamic_state_value.rasterization_samples;
    } else {
        if (auto ms_state = pipeline_state->MultisampleState()) {
            rasterization_samples = ms_state->rasterizationSamples;
        }
    }
    return rasterization_samples;
}

Causing VVL to complain that the sample count doesn't match the attachment's sample count. Somehow this only triggers with dynamic rendering. To repro, get this ANGLE change: https://chromium-review.googlesource.com/c/angle/angle/+/5637155, and run:

$ ANGLE_FEATURE_OVERRIDES_DISABLED=supportsGraphicsPipelineLibrary:warmUpPipelineCacheAtLink gdb --args ./angle_deqp_gles3_multisample_tests --use-angle=swiftshader --deqp-case=dEQP-GLES3.functional.rasterizer_discard.basic.write_depth_line_loop

(note: the above disables GPL and extra pipeline creation to simplify things. Issue happens with GPL too)