From 29a7d99c4aa16b5c81db9bbbf378195247539066 Mon Sep 17 00:00:00 2001 From: Iago Toral Quiroga Date: Wed, 5 Feb 2020 10:53:20 +0100 Subject: [PATCH] v3dv: fix the mess with dynamic state handling The general idea is that we always emit from our dynamic state, and when a particular piece of state is not dynamic, we just set our dynamic state from the pipeline state, however, the implementation was quite confusing: the mask of dynamic states flagged states that were not dynamic and some places woud mix dirty flags and dynamic state flags. We also were not updating the dynamic state mask in the command buffer, etc. This patch, hopefully, simplifies all this and makes it less confusing, starting by making the dynamic state mask flag dynamic states, fixing the places where we would confuse dirty state flags with dynamic state flags, making sure that our command buffer state is setup correctly and that we only emit state when it is actually dirty. Part-of: --- src/broadcom/vulkan/v3dv_cmd_buffer.c | 91 +++++++++++++++------------ src/broadcom/vulkan/v3dv_pipeline.c | 80 ++++++++++------------- src/broadcom/vulkan/v3dv_private.h | 21 +++---- 3 files changed, 92 insertions(+), 100 deletions(-) diff --git a/src/broadcom/vulkan/v3dv_cmd_buffer.c b/src/broadcom/vulkan/v3dv_cmd_buffer.c index 52ccb1c6bc8..b46733e937b 100644 --- a/src/broadcom/vulkan/v3dv_cmd_buffer.c +++ b/src/broadcom/vulkan/v3dv_cmd_buffer.c @@ -1390,13 +1390,17 @@ v3dv_EndCommandBuffer(VkCommandBuffer commandBuffer) return VK_SUCCESS; } +/* This goes though the list of possible dynamic states in the pipeline and, + * for those that are not configured as dynamic, copies relevant state into + * the command buffer. + */ static void -bind_dynamic_state(struct v3dv_cmd_buffer *cmd_buffer, - const struct v3dv_dynamic_state *src) +cmd_buffer_bind_pipeline_static_state(struct v3dv_cmd_buffer *cmd_buffer, + const struct v3dv_dynamic_state *src) { struct v3dv_dynamic_state *dest = &cmd_buffer->state.dynamic; - uint32_t copy_mask = src->mask; - uint32_t dest_mask = 0; + 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. @@ -1404,7 +1408,7 @@ bind_dynamic_state(struct v3dv_cmd_buffer *cmd_buffer, dest->viewport.count = src->viewport.count; dest->scissor.count = src->scissor.count; - if (copy_mask & V3DV_DYNAMIC_VIEWPORT) { + if (!(dynamic_mask & V3DV_DYNAMIC_VIEWPORT)) { if (memcmp(&dest->viewport.viewports, &src->viewport.viewports, src->viewport.count * sizeof(VkViewport))) { typed_memcpy(dest->viewport.viewports, @@ -1414,44 +1418,45 @@ bind_dynamic_state(struct v3dv_cmd_buffer *cmd_buffer, src->viewport.count); typed_memcpy(dest->viewport.translate, src->viewport.translate, src->viewport.count); - dest_mask |= V3DV_DYNAMIC_VIEWPORT; + dirty |= V3DV_CMD_DIRTY_VIEWPORT; } } - if (copy_mask & V3DV_DYNAMIC_SCISSOR) { + if (!(dynamic_mask & V3DV_DYNAMIC_SCISSOR)) { if (memcmp(&dest->scissor.scissors, &src->scissor.scissors, src->scissor.count * sizeof(VkRect2D))) { typed_memcpy(dest->scissor.scissors, src->scissor.scissors, src->scissor.count); - dest_mask |= V3DV_DYNAMIC_SCISSOR; + dirty |= V3DV_CMD_DIRTY_SCISSOR; } } - if (copy_mask & V3DV_DYNAMIC_STENCIL_COMPARE_MASK) { + if (!(dynamic_mask & V3DV_DYNAMIC_STENCIL_COMPARE_MASK)) { if (memcmp(&dest->stencil_compare_mask, &src->stencil_compare_mask, sizeof(src->stencil_compare_mask))) { dest->stencil_compare_mask = src->stencil_compare_mask; - dest_mask |= V3DV_DYNAMIC_STENCIL_COMPARE_MASK; + dirty |= V3DV_CMD_DIRTY_STENCIL_COMPARE_MASK; } } - if (copy_mask & V3DV_DYNAMIC_STENCIL_WRITE_MASK) { + if (!(dynamic_mask & V3DV_DYNAMIC_STENCIL_WRITE_MASK)) { if (memcmp(&dest->stencil_write_mask, &src->stencil_write_mask, sizeof(src->stencil_write_mask))) { dest->stencil_write_mask = src->stencil_write_mask; - dest_mask |= V3DV_DYNAMIC_STENCIL_WRITE_MASK; + dirty |= V3DV_CMD_DIRTY_STENCIL_WRITE_MASK; } } - if (copy_mask & V3DV_DYNAMIC_STENCIL_REFERENCE) { + if (!(dynamic_mask & V3DV_DYNAMIC_STENCIL_REFERENCE)) { if (memcmp(&dest->stencil_reference, &src->stencil_reference, sizeof(src->stencil_reference))) { dest->stencil_reference = src->stencil_reference; - dest_mask |= V3DV_DYNAMIC_STENCIL_REFERENCE; + dirty |= V3DV_CMD_DIRTY_STENCIL_REFERENCE; } } - cmd_buffer->state.dirty |= dest_mask; + cmd_buffer->state.dynamic.mask = dynamic_mask; + cmd_buffer->state.dirty |= dirty; } static void @@ -1506,8 +1511,8 @@ bind_graphics_pipeline(struct v3dv_cmd_buffer *cmd_buffer, return; cmd_buffer->state.pipeline = pipeline; - bind_dynamic_state(cmd_buffer, &pipeline->dynamic_state); + cmd_buffer_bind_pipeline_static_state(cmd_buffer, &pipeline->dynamic_state); cmd_buffer_update_ez_state(cmd_buffer, pipeline); cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_PIPELINE; @@ -1594,7 +1599,7 @@ v3dv_CmdSetViewport(VkCommandBuffer commandBuffer, state->dynamic.viewport.translate[i]); } - cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_DYNAMIC_VIEWPORT; + cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_VIEWPORT; } void @@ -1621,7 +1626,7 @@ v3dv_CmdSetScissor(VkCommandBuffer commandBuffer, memcpy(state->dynamic.scissor.scissors + firstScissor, pScissors, scissorCount * sizeof(*pScissors)); - cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_DYNAMIC_SCISSOR; + cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_SCISSOR; } static void @@ -1740,24 +1745,20 @@ emit_stencil(struct v3dv_cmd_buffer *cmd_buffer) for (uint32_t i = 0; i < 2; i++) { if (pipeline->emit_stencil_cfg[i]) { - /* Our dynamic state bits signal whether a particular state is - * taken from the static pipeline definition (so it is not dynamic). - */ - if (!(dynamic_state->mask & dynamic_stencil_states)) { - /* At least one of the stencil states is dynamic */ + if (dynamic_state->mask & dynamic_stencil_states) { cl_emit_with_prepacked(&job->bcl, STENCIL_CFG, pipeline->stencil_cfg[i], config) { - if (!(dynamic_state->mask & V3DV_DYNAMIC_STENCIL_COMPARE_MASK)) { + if (dynamic_state->mask & V3DV_DYNAMIC_STENCIL_COMPARE_MASK) { config.stencil_test_mask = i == 0 ? dynamic_state->stencil_compare_mask.front : dynamic_state->stencil_compare_mask.back; } - if (!(dynamic_state->mask & V3DV_DYNAMIC_STENCIL_WRITE_MASK)) { + if (dynamic_state->mask & V3DV_DYNAMIC_STENCIL_WRITE_MASK) { config.stencil_write_mask = i == 0 ? dynamic_state->stencil_write_mask.front : dynamic_state->stencil_write_mask.back; } - if (!(dynamic_state->mask & V3DV_DYNAMIC_STENCIL_REFERENCE)) { + if (dynamic_state->mask & V3DV_DYNAMIC_STENCIL_REFERENCE) { config.stencil_ref_value = i == 0 ? dynamic_state->stencil_reference.front : dynamic_state->stencil_reference.back; @@ -1768,10 +1769,16 @@ emit_stencil(struct v3dv_cmd_buffer *cmd_buffer) } } } + + const uint32_t dynamic_stencil_dirty_flags = + V3DV_CMD_DIRTY_STENCIL_COMPARE_MASK | + V3DV_CMD_DIRTY_STENCIL_WRITE_MASK | + V3DV_CMD_DIRTY_STENCIL_REFERENCE; + cmd_buffer->state.dirty &= ~dynamic_stencil_dirty_flags; } static void -cmd_buffer_emit_graphics_pipeline(struct v3dv_cmd_buffer *cmd_buffer) +emit_graphics_pipeline(struct v3dv_cmd_buffer *cmd_buffer) { struct v3dv_job *job = cmd_buffer->state.job; assert(job); @@ -1975,28 +1982,34 @@ cmd_buffer_draw(struct v3dv_cmd_buffer *cmd_buffer, struct v3dv_draw_info *info) { /* FIXME: likely to be filtered by really needed states */ - uint32_t states = cmd_buffer->state.dirty; + uint32_t *dirty = &cmd_buffer->state.dirty; struct v3dv_dynamic_state *dynamic = &cmd_buffer->state.dynamic; - /* vertex buffers info are emitted as part of the shader_state_record */ - if (states & (V3DV_CMD_DIRTY_PIPELINE | V3DV_CMD_DIRTY_VERTEX_BUFFER)) { - cmd_buffer_emit_graphics_pipeline(cmd_buffer); + /* vertex buffer state is emitted as part of the shader state record */ + if (*dirty & (V3DV_CMD_DIRTY_PIPELINE | V3DV_CMD_DIRTY_VERTEX_BUFFER)) { + emit_graphics_pipeline(cmd_buffer); } - /* Emit(flush) dynamic state */ - if (states & (V3DV_CMD_DIRTY_DYNAMIC_VIEWPORT | V3DV_CMD_DIRTY_DYNAMIC_SCISSOR)) { - assert(dynamic->scissor.count > 0 || dynamic->viewport.count > 0); + if (*dirty & (V3DV_CMD_DIRTY_VIEWPORT | V3DV_CMD_DIRTY_SCISSOR)) { + assert(dynamic->scissor.count > 0 || dynamic->viewport.count > 0); emit_scissor(cmd_buffer); } - if (states & (V3DV_CMD_DIRTY_DYNAMIC_VIEWPORT)) { + if (*dirty & V3DV_CMD_DIRTY_VIEWPORT) { emit_viewport(cmd_buffer); } + const uint32_t dynamic_stencil_dirty_flags = + V3DV_CMD_DIRTY_STENCIL_COMPARE_MASK | + V3DV_CMD_DIRTY_STENCIL_WRITE_MASK | + V3DV_CMD_DIRTY_STENCIL_REFERENCE; + if (*dirty & dynamic_stencil_dirty_flags) + emit_stencil(cmd_buffer); + /* FIXME: any dirty flag to filter ? */ cmd_buffer_emit_draw_packets(cmd_buffer, info); - cmd_buffer->state.dirty &= ~states; + cmd_buffer->state.dirty &= ~(*dirty); } void @@ -2073,7 +2086,7 @@ v3dv_CmdSetStencilCompareMask(VkCommandBuffer commandBuffer, if (faceMask & VK_STENCIL_FACE_BACK_BIT) cmd_buffer->state.dynamic.stencil_compare_mask.back = compareMask & 0xff; - cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_DYNAMIC_STENCIL_COMPARE_MASK; + cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_STENCIL_COMPARE_MASK; } void @@ -2088,7 +2101,7 @@ v3dv_CmdSetStencilWriteMask(VkCommandBuffer commandBuffer, if (faceMask & VK_STENCIL_FACE_BACK_BIT) cmd_buffer->state.dynamic.stencil_write_mask.back = writeMask & 0xff; - cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_DYNAMIC_STENCIL_WRITE_MASK; + cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_STENCIL_WRITE_MASK; } void @@ -2103,5 +2116,5 @@ v3dv_CmdSetStencilReference(VkCommandBuffer commandBuffer, if (faceMask & VK_STENCIL_FACE_BACK_BIT) cmd_buffer->state.dynamic.stencil_reference.back = reference & 0xff; - cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_DYNAMIC_STENCIL_REFERENCE; + cmd_buffer->state.dirty |= V3DV_CMD_DIRTY_STENCIL_REFERENCE; } diff --git a/src/broadcom/vulkan/v3dv_pipeline.c b/src/broadcom/vulkan/v3dv_pipeline.c index 069ca7f8221..ffc30e20253 100644 --- a/src/broadcom/vulkan/v3dv_pipeline.c +++ b/src/broadcom/vulkan/v3dv_pipeline.c @@ -881,89 +881,73 @@ v3dv_dynamic_state_mask(VkDynamicState state) } } -static uint32_t -pipeline_needed_dynamic_state(const VkGraphicsPipelineCreateInfo *pCreateInfo) -{ - uint32_t states = V3DV_DYNAMIC_ALL; - - /* FIXME: stub. Based on other values at pCreateInfo, we would need to - * remove flags from states - */ - - return states; -} - static void pipeline_init_dynamic_state(struct v3dv_pipeline *pipeline, const VkGraphicsPipelineCreateInfo *pCreateInfo) { - uint32_t needed_states = pipeline_needed_dynamic_state(pCreateInfo); - uint32_t states = needed_states; - 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; - for (uint32_t s = 0; s < count; s++) { - /* Remove all of the states that are marked as dynamic */ - states &= ~v3dv_dynamic_state_mask(pCreateInfo->pDynamicState->pDynamicStates[s]); + dynamic_states |= + v3dv_dynamic_state_mask(pCreateInfo->pDynamicState->pDynamicStates[s]); } } - struct v3dv_dynamic_state *dynamic = &pipeline->dynamic_state; - /* Note, as this can be counter-intuitive: although we are checking against - * _DYNAMIC flags, here we are copying the data from the pipeline that was - * not defined as dynamic. + /* 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. */ - if (needed_states & V3DV_DYNAMIC_VIEWPORT) { + dynamic->viewport.count = pCreateInfo->pViewportState->viewportCount; + if (!(dynamic_states & V3DV_DYNAMIC_VIEWPORT)) { assert(pCreateInfo->pViewportState); - dynamic->viewport.count = pCreateInfo->pViewportState->viewportCount; - if (states & V3DV_DYNAMIC_VIEWPORT) { - typed_memcpy(dynamic->viewport.viewports, - pCreateInfo->pViewportState->pViewports, - pCreateInfo->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 (needed_states & V3DV_DYNAMIC_SCISSOR) { - dynamic->scissor.count = pCreateInfo->pViewportState->scissorCount; - if (states & V3DV_DYNAMIC_SCISSOR) { - typed_memcpy(dynamic->scissor.scissors, - pCreateInfo->pViewportState->pScissors, - pCreateInfo->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 (needed_states & V3DV_DYNAMIC_STENCIL_COMPARE_MASK) { + if (!(dynamic_states & V3DV_DYNAMIC_STENCIL_COMPARE_MASK)) { dynamic->stencil_compare_mask.front = pCreateInfo->pDepthStencilState->front.compareMask; dynamic->stencil_compare_mask.back = pCreateInfo->pDepthStencilState->back.compareMask; } - if (needed_states & V3DV_DYNAMIC_STENCIL_WRITE_MASK) { + 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; } - if (needed_states & V3DV_DYNAMIC_STENCIL_REFERENCE) { + if (!(dynamic_states & V3DV_DYNAMIC_STENCIL_REFERENCE)) { dynamic->stencil_reference.front = pCreateInfo->pDepthStencilState->front.reference; dynamic->stencil_reference.back = pCreateInfo->pDepthStencilState->back.reference; } - pipeline->dynamic_state.mask = states; + pipeline->dynamic_state.mask = dynamic_states; } static void @@ -1072,15 +1056,15 @@ pack_single_stencil_cfg(struct v3dv_pipeline *pipeline, */ const uint8_t write_mask = pipeline->dynamic_state.mask & V3DV_DYNAMIC_STENCIL_WRITE_MASK ? - stencil_state->writeMask & 0xff : 0; + 0 : stencil_state->writeMask & 0xff; const uint8_t compare_mask = pipeline->dynamic_state.mask & V3DV_DYNAMIC_STENCIL_COMPARE_MASK ? - stencil_state->compareMask & 0xff : 0; + 0 : stencil_state->compareMask & 0xff; const uint8_t reference = pipeline->dynamic_state.mask & V3DV_DYNAMIC_STENCIL_COMPARE_MASK ? - stencil_state->reference & 0xff : 0; + 0 : stencil_state->reference & 0xff; v3dv_pack(stencil_cfg, STENCIL_CFG, config) { config.front_config = is_front; @@ -1113,7 +1097,7 @@ pack_stencil_cfg(struct v3dv_pipeline *pipeline, * packet for both faces. */ bool needs_front_and_back = false; - if (!(pipeline->dynamic_state.mask & dynamic_stencil_states) || + if ((pipeline->dynamic_state.mask & dynamic_stencil_states) || memcmp(&ds_info->front, &ds_info->back, sizeof(ds_info->front))) needs_front_and_back = true; diff --git a/src/broadcom/vulkan/v3dv_private.h b/src/broadcom/vulkan/v3dv_private.h index 9b1a6079697..d6cf671f6b8 100644 --- a/src/broadcom/vulkan/v3dv_private.h +++ b/src/broadcom/vulkan/v3dv_private.h @@ -440,23 +440,18 @@ enum v3dv_dynamic_state_bits { V3DV_DYNAMIC_ALL = (1 << 5) - 1, }; -/* To track which cmd buffer elements are "dirty" so would need an - * update. This includes viewport, scissor (so vk dynamic info), plus any - * other more high level resource, like pipeline, etc. +/* Flags for dirty pipeline state. */ enum v3dv_cmd_dirty_bits { - V3DV_CMD_DIRTY_DYNAMIC_VIEWPORT = 1 << 0, - V3DV_CMD_DIRTY_DYNAMIC_SCISSOR = 1 << 1, - V3DV_CMD_DIRTY_DYNAMIC_STENCIL_COMPARE_MASK = 1 << 2, - V3DV_CMD_DIRTY_DYNAMIC_STENCIL_WRITE_MASK = 1 << 3, - V3DV_CMD_DIRTY_DYNAMIC_STENCIL_REFERENCE = 1 << 4, - V3DV_CMD_DIRTY_DYNAMIC_ALL = (1 << 5) - 1, - - V3DV_CMD_DIRTY_PIPELINE = 1 << 5, - V3DV_CMD_DIRTY_VERTEX_BUFFER = 1 << 6, + V3DV_CMD_DIRTY_VIEWPORT = 1 << 0, + V3DV_CMD_DIRTY_SCISSOR = 1 << 1, + V3DV_CMD_DIRTY_STENCIL_COMPARE_MASK = 1 << 2, + V3DV_CMD_DIRTY_STENCIL_WRITE_MASK = 1 << 3, + V3DV_CMD_DIRTY_STENCIL_REFERENCE = 1 << 4, + V3DV_CMD_DIRTY_PIPELINE = 1 << 5, + V3DV_CMD_DIRTY_VERTEX_BUFFER = 1 << 6, }; - struct v3dv_dynamic_state { /** * Bitmask of (1 << VK_DYNAMIC_STATE_*).