v3dv: fix the mess with dynamic state handling
authorIago Toral Quiroga <itoral@igalia.com>
Wed, 5 Feb 2020 09:53:20 +0000 (10:53 +0100)
committerMarge Bot <eric+marge@anholt.net>
Tue, 13 Oct 2020 21:21:27 +0000 (21:21 +0000)
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: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6766>

src/broadcom/vulkan/v3dv_cmd_buffer.c
src/broadcom/vulkan/v3dv_pipeline.c
src/broadcom/vulkan/v3dv_private.h

index 52ccb1c..b46733e 100644 (file)
@@ -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;
 }
index 069ca7f..ffc30e2 100644 (file)
@@ -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);
-
-         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]);
-         }
+      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]);
       }
    }
 
-   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;
 
index 9b1a607..d6cf671 100644 (file)
@@ -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_*).