From 0dbf04086341574e5f2f5e72aab6c94a07609cfe Mon Sep 17 00:00:00 2001 From: Iago Toral Quiroga Date: Tue, 23 Jun 2020 12:46:49 +0200 Subject: [PATCH] v3dv: pipeline initialization fixes for disabled rasterization When rasterization is disabled there are a number of CreateInfo structs that should be ignored. We were managing this correctly for some cases, but not all of them. Specifically, viewport state must be ignored and we weren't doing that. Fixes: dEQP-VK.api.descriptor_set.descriptor_set_layout_lifetime.graphics Part-of: --- src/broadcom/vulkan/v3dv_cmd_buffer.c | 31 +++---- src/broadcom/vulkan/v3dv_pipeline.c | 114 ++++++++++++-------------- 2 files changed, 63 insertions(+), 82 deletions(-) diff --git a/src/broadcom/vulkan/v3dv_cmd_buffer.c b/src/broadcom/vulkan/v3dv_cmd_buffer.c index 7c5b626259b..0a9d790f280 100644 --- a/src/broadcom/vulkan/v3dv_cmd_buffer.c +++ b/src/broadcom/vulkan/v3dv_cmd_buffer.c @@ -2439,13 +2439,8 @@ cmd_buffer_bind_pipeline_static_state(struct v3dv_cmd_buffer *cmd_buffer, uint32_t dynamic_mask = src->mask; uint32_t dirty = 0; - /* See note on SetViewport. We follow radv approach to only allow to set - * the number of viewports/scissors at pipeline creation time. - */ - dest->viewport.count = src->viewport.count; - dest->scissor.count = src->scissor.count; - if (!(dynamic_mask & V3DV_DYNAMIC_VIEWPORT)) { + dest->viewport.count = src->viewport.count; if (memcmp(&dest->viewport.viewports, &src->viewport.viewports, src->viewport.count * sizeof(VkViewport))) { typed_memcpy(dest->viewport.viewports, @@ -2460,6 +2455,7 @@ cmd_buffer_bind_pipeline_static_state(struct v3dv_cmd_buffer *cmd_buffer, } if (!(dynamic_mask & V3DV_DYNAMIC_SCISSOR)) { + dest->scissor.count = src->scissor.count; if (memcmp(&dest->scissor.scissors, &src->scissor.scissors, src->scissor.count * sizeof(VkRect2D))) { typed_memcpy(dest->scissor.scissors, @@ -2879,15 +2875,9 @@ v3dv_CmdSetViewport(VkCommandBuffer commandBuffer, assert(firstViewport < MAX_VIEWPORTS); assert(total_count >= 1 && total_count <= MAX_VIEWPORTS); - /* anv allows CmdSetViewPort to change how many viewports are being used, - * while radv not, using the value set on the pipeline creation. spec - * doesn't specify, but radv approach makes more sense, as CmdSetViewport - * is intended to set dynamically a specific viewport, increasing the - * number of viewport used seems like a non-defined collateral - * effect. Would make sense to open a spec issue to clarify. For now, as we - * only support one, it is not really important, but we follow radv - * approach. - */ + if (state->dynamic.viewport.count < total_count) + state->dynamic.viewport.count = total_count; + if (!memcmp(state->dynamic.viewport.viewports + firstViewport, pViewports, viewportCount * sizeof(*pViewports))) { return; @@ -2918,9 +2908,9 @@ v3dv_CmdSetScissor(VkCommandBuffer commandBuffer, assert(firstScissor + scissorCount >= 1 && firstScissor + scissorCount <= MAX_SCISSORS); - /* See note on CmdSetViewport related to anv/radv differences about setting - * total viewports used. Also applies to scissor. - */ + if (state->dynamic.scissor.count < firstScissor + scissorCount) + state->dynamic.scissor.count = firstScissor + scissorCount; + if (!memcmp(state->dynamic.scissor.scissors + firstScissor, pScissors, scissorCount * sizeof(*pScissors))) { return; @@ -2935,6 +2925,9 @@ v3dv_CmdSetScissor(VkCommandBuffer commandBuffer, static void emit_scissor(struct v3dv_cmd_buffer *cmd_buffer) { + if (cmd_buffer->state.dynamic.viewport.count == 0) + return; + struct v3dv_dynamic_state *dynamic = &cmd_buffer->state.dynamic; /* FIXME: right now we only support one viewport. viewporst[0] would work @@ -3803,8 +3796,6 @@ cmd_buffer_emit_pre_draw(struct v3dv_cmd_buffer *cmd_buffer) } if (*dirty & (V3DV_CMD_DIRTY_VIEWPORT | V3DV_CMD_DIRTY_SCISSOR)) { - assert(cmd_buffer->state.dynamic.scissor.count > 0 || - cmd_buffer->state.dynamic.viewport.count > 0); emit_scissor(cmd_buffer); } diff --git a/src/broadcom/vulkan/v3dv_pipeline.c b/src/broadcom/vulkan/v3dv_pipeline.c index d6d69f624f7..f1076b29f9f 100644 --- a/src/broadcom/vulkan/v3dv_pipeline.c +++ b/src/broadcom/vulkan/v3dv_pipeline.c @@ -1623,91 +1623,84 @@ v3dv_dynamic_state_mask(VkDynamicState state) } static void -pipeline_init_dynamic_state(struct v3dv_pipeline *pipeline, - const VkGraphicsPipelineCreateInfo *pCreateInfo) +pipeline_init_dynamic_state( + struct v3dv_pipeline *pipeline, + const VkPipelineDynamicStateCreateInfo *pDynamicState, + const VkPipelineViewportStateCreateInfo *pViewportState, + const VkPipelineDepthStencilStateCreateInfo *pDepthStencilState, + const VkPipelineColorBlendStateCreateInfo *pColorBlendState, + const VkPipelineRasterizationStateCreateInfo *pRasterizationState) { pipeline->dynamic_state = default_dynamic_state; struct v3dv_dynamic_state *dynamic = &pipeline->dynamic_state; /* Create a mask of enabled dynamic states */ uint32_t dynamic_states = 0; - if (pCreateInfo->pDynamicState) { - uint32_t count = pCreateInfo->pDynamicState->dynamicStateCount; + if (pDynamicState) { + uint32_t count = pDynamicState->dynamicStateCount; for (uint32_t s = 0; s < count; s++) { dynamic_states |= - v3dv_dynamic_state_mask(pCreateInfo->pDynamicState->pDynamicStates[s]); + v3dv_dynamic_state_mask(pDynamicState->pDynamicStates[s]); } } /* For any pipeline states that are not dynamic, set the dynamic state * from the static pipeline state. - * - * Notice that we don't let the number of viewports and scissort rects to - * be set dynamically, so these are always copied from the pipeline state. */ - dynamic->viewport.count = pCreateInfo->pViewportState->viewportCount; - if (!(dynamic_states & V3DV_DYNAMIC_VIEWPORT)) { - assert(pCreateInfo->pViewportState); + if (pViewportState) { + if (!(dynamic_states & V3DV_DYNAMIC_VIEWPORT)) { + dynamic->viewport.count = pViewportState->viewportCount; + typed_memcpy(dynamic->viewport.viewports, pViewportState->pViewports, + pViewportState->viewportCount); - typed_memcpy(dynamic->viewport.viewports, - pCreateInfo->pViewportState->pViewports, - pCreateInfo->pViewportState->viewportCount); + for (uint32_t i = 0; i < dynamic->viewport.count; i++) { + v3dv_viewport_compute_xform(&dynamic->viewport.viewports[i], + dynamic->viewport.scale[i], + dynamic->viewport.translate[i]); + } + } - for (uint32_t i = 0; i < dynamic->viewport.count; i++) { - v3dv_viewport_compute_xform(&dynamic->viewport.viewports[i], - dynamic->viewport.scale[i], - dynamic->viewport.translate[i]); + if (!(dynamic_states & V3DV_DYNAMIC_SCISSOR)) { + dynamic->scissor.count = pViewportState->scissorCount; + typed_memcpy(dynamic->scissor.scissors, pViewportState->pScissors, + pViewportState->scissorCount); } } - dynamic->scissor.count = pCreateInfo->pViewportState->scissorCount; - if (!(dynamic_states & V3DV_DYNAMIC_SCISSOR)) { - typed_memcpy(dynamic->scissor.scissors, - pCreateInfo->pViewportState->pScissors, - pCreateInfo->pViewportState->scissorCount); - } - - if (pCreateInfo->pDepthStencilState != NULL) { + if (pDepthStencilState) { if (!(dynamic_states & V3DV_DYNAMIC_STENCIL_COMPARE_MASK)) { dynamic->stencil_compare_mask.front = - pCreateInfo->pDepthStencilState->front.compareMask; + pDepthStencilState->front.compareMask; dynamic->stencil_compare_mask.back = - pCreateInfo->pDepthStencilState->back.compareMask; + pDepthStencilState->back.compareMask; } if (!(dynamic_states & V3DV_DYNAMIC_STENCIL_WRITE_MASK)) { - dynamic->stencil_write_mask.front = - pCreateInfo->pDepthStencilState->front.writeMask; - dynamic->stencil_write_mask.back = - pCreateInfo->pDepthStencilState->back.writeMask; + dynamic->stencil_write_mask.front = pDepthStencilState->front.writeMask; + dynamic->stencil_write_mask.back = pDepthStencilState->back.writeMask; } if (!(dynamic_states & V3DV_DYNAMIC_STENCIL_REFERENCE)) { - dynamic->stencil_reference.front = - pCreateInfo->pDepthStencilState->front.reference; - dynamic->stencil_reference.back = - pCreateInfo->pDepthStencilState->back.reference; + dynamic->stencil_reference.front = pDepthStencilState->front.reference; + dynamic->stencil_reference.back = pDepthStencilState->back.reference; } } - if (pCreateInfo->pColorBlendState && - !(dynamic_states & V3DV_DYNAMIC_BLEND_CONSTANTS)) { - memcpy(dynamic->blend_constants, - pCreateInfo->pColorBlendState->blendConstants, + if (pColorBlendState && !(dynamic_states & V3DV_DYNAMIC_BLEND_CONSTANTS)) { + memcpy(dynamic->blend_constants, pColorBlendState->blendConstants, sizeof(dynamic->blend_constants)); } - if (pCreateInfo->pRasterizationState && - !pCreateInfo->pRasterizationState->rasterizerDiscardEnable) { - if (pCreateInfo->pRasterizationState->depthBiasEnable && + if (pRasterizationState) { + if (pRasterizationState->depthBiasEnable && !(dynamic_states & V3DV_DYNAMIC_DEPTH_BIAS)) { dynamic->depth_bias.constant_factor = - pCreateInfo->pRasterizationState->depthBiasConstantFactor; + pRasterizationState->depthBiasConstantFactor; dynamic->depth_bias.slope_factor = - pCreateInfo->pRasterizationState->depthBiasSlopeFactor; + pRasterizationState->depthBiasSlopeFactor; } if (!(dynamic_states & V3DV_DYNAMIC_LINE_WIDTH)) - dynamic->line_width = pCreateInfo->pRasterizationState->lineWidth; + dynamic->line_width = pRasterizationState->lineWidth; } pipeline->dynamic_state.mask = dynamic_states; @@ -2014,28 +2007,20 @@ stencil_op_is_no_op(const VkStencilOpState *stencil) static void enable_depth_bias(struct v3dv_pipeline *pipeline, - const VkGraphicsPipelineCreateInfo *pInfo) + const VkPipelineRasterizationStateCreateInfo *rs_info) { - assert(pInfo); - pipeline->depth_bias.enabled = false; pipeline->depth_bias.is_z16 = false; - if (!pInfo->pRasterizationState || - !pInfo->pRasterizationState->depthBiasEnable || - pInfo->pRasterizationState->rasterizerDiscardEnable) + if (!rs_info || !rs_info->depthBiasEnable) return; /* Check the depth/stencil attachment description for the subpass used with * this pipeline. */ - struct v3dv_render_pass *pass = - v3dv_render_pass_from_handle(pInfo->renderPass); - assert(pass); - - assert(pInfo->subpass < pass->subpass_count); - struct v3dv_subpass *subpass = &pass->subpasses[pInfo->subpass]; - assert(subpass); + assert(pipeline->pass && pipeline->subpass); + struct v3dv_render_pass *pass = pipeline->pass; + struct v3dv_subpass *subpass = pipeline->subpass; if (subpass->ds_attachment.attachment == VK_ATTACHMENT_UNUSED) return; @@ -2367,14 +2352,15 @@ pipeline_init(struct v3dv_pipeline *pipeline, pipeline->pass = render_pass; pipeline->subpass = &render_pass->subpasses[pCreateInfo->subpass]; - pipeline_init_dynamic_state(pipeline, pCreateInfo); - /* If rasterization is not enabled, various CreateInfo structs must be * ignored. */ const bool raster_enabled = !pCreateInfo->pRasterizationState->rasterizerDiscardEnable; + const VkPipelineViewportStateCreateInfo *vp_info = + raster_enabled ? pCreateInfo->pViewportState : NULL; + const VkPipelineDepthStencilStateCreateInfo *ds_info = raster_enabled ? pCreateInfo->pDepthStencilState : NULL; @@ -2384,6 +2370,10 @@ pipeline_init(struct v3dv_pipeline *pipeline, const VkPipelineColorBlendStateCreateInfo *cb_info = raster_enabled ? pCreateInfo->pColorBlendState : NULL; + pipeline_init_dynamic_state(pipeline, + pCreateInfo->pDynamicState, + vp_info, ds_info, cb_info, rs_info); + /* V3D 4.2 doesn't support depth bounds testing so we don't advertise that * feature and it shouldn't be used by any pipeline. */ @@ -2393,7 +2383,7 @@ pipeline_init(struct v3dv_pipeline *pipeline, pack_cfg_bits(pipeline, ds_info, rs_info); pack_stencil_cfg(pipeline, ds_info); pipeline_set_ez_state(pipeline, ds_info); - enable_depth_bias(pipeline, pCreateInfo); + enable_depth_bias(pipeline, rs_info); pipeline->primitive_restart = pCreateInfo->pInputAssemblyState->primitiveRestartEnable;