[Mesa-dev] [PATCH 1/2] anv/cmd_buffer: Don't call set_subpass in a secondary

[Nanley Chery]

On Wed, Oct 05, 2016 at 05:36:43PM -0700, Jason Ekstrand wrote:
> Initially, we had intended set_subpass to be an interesting function that
> did whatever (presumably a lot) setup we needed for a subpass.  In reality,
> it just sets a pointer and a dirty bit and then emits depth and stencil
> state.  When we call BeginCommandBuffer on a secondary, all of the dirty
> bits are already set and there's no point in setting depth and stencil
> state since it will already be set by the primary.  Instead, the only thing
> we need to do at the start of a secondary is set the subpass pointer.
> 
> Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> ---
>  src/intel/vulkan/anv_cmd_buffer.c  | 39 +-------------------------------------
>  src/intel/vulkan/anv_genX.h        |  3 ---
>  src/intel/vulkan/anv_private.h     |  3 ---
>  src/intel/vulkan/genX_cmd_buffer.c |  5 +----
>  4 files changed, 2 insertions(+), 48 deletions(-)
> 
> diff --git a/src/intel/vulkan/anv_cmd_buffer.c b/src/intel/vulkan/anv_cmd_buffer.c
> index 9dedde8..ef13dfc 100644
> --- a/src/intel/vulkan/anv_cmd_buffer.c
> +++ b/src/intel/vulkan/anv_cmd_buffer.c
> @@ -407,10 +407,8 @@ VkResult anv_BeginCommandBuffer(
>        cmd_buffer->state.pass =
>           anv_render_pass_from_handle(pBeginInfo->pInheritanceInfo->renderPass);
>  
> -      struct anv_subpass *subpass =
> +      cmd_buffer->state.subpass =
>           &cmd_buffer->state.pass->subpasses[pBeginInfo->pInheritanceInfo->subpass];
> -
> -      anv_cmd_buffer_set_subpass(cmd_buffer, subpass);

I'm not sure why we always set the fragment descriptor bit in
set_subpass, but it seems like we need to do it here as well to keep
the logic the same. I don't see where we set the dirty bits on a
secondary command buffer at BeginCommandBuffer. Aside from that, this
patch looks good.

-Nanley

>     }
>  
>     return VK_SUCCESS;
> @@ -1050,41 +1048,6 @@ anv_cmd_buffer_merge_dynamic(struct anv_cmd_buffer *cmd_buffer,
>     return state;
>  }
>  
> -/**
> - * @brief Setup the command buffer for recording commands inside the given
> - * subpass.
> - *
> - * This does not record all commands needed for starting the subpass.
> - * Starting the subpass may require additional commands.
> - *
> - * Note that vkCmdBeginRenderPass, vkCmdNextSubpass, and vkBeginCommandBuffer
> - * with VK_COMMAND_BUFFER_USAGE_RENDER_PASS_CONTINUE_BIT, all setup the
> - * command buffer for recording commands for some subpass.  But only the first
> - * two, vkCmdBeginRenderPass and vkCmdNextSubpass, can start a subpass.
> - */
> -void
> -anv_cmd_buffer_set_subpass(struct anv_cmd_buffer *cmd_buffer,
> -                           struct anv_subpass *subpass)
> -{
> -   switch (cmd_buffer->device->info.gen) {
> -   case 7:
> -      if (cmd_buffer->device->info.is_haswell) {
> -         gen75_cmd_buffer_set_subpass(cmd_buffer, subpass);
> -      } else {
> -         gen7_cmd_buffer_set_subpass(cmd_buffer, subpass);
> -      }
> -      break;
> -   case 8:
> -      gen8_cmd_buffer_set_subpass(cmd_buffer, subpass);
> -      break;
> -   case 9:
> -      gen9_cmd_buffer_set_subpass(cmd_buffer, subpass);
> -      break;
> -   default:
> -      unreachable("unsupported gen\n");
> -   }
> -}
> -
>  struct anv_state
>  anv_cmd_buffer_push_constants(struct anv_cmd_buffer *cmd_buffer,
>                                gl_shader_stage stage)
> diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h
> index 02e79c2..dc2dd5d 100644
> --- a/src/intel/vulkan/anv_genX.h
> +++ b/src/intel/vulkan/anv_genX.h
> @@ -36,9 +36,6 @@ struct anv_state
>  genX(cmd_buffer_alloc_null_surface_state)(struct anv_cmd_buffer *cmd_buffer,
>                                            struct anv_framebuffer *fb);
>  
> -void genX(cmd_buffer_set_subpass)(struct anv_cmd_buffer *cmd_buffer,
> -                                  struct anv_subpass *subpass);
> -
>  void genX(cmd_buffer_apply_pipe_flushes)(struct anv_cmd_buffer *cmd_buffer);
>  
>  void genX(flush_pipeline_select_3d)(struct anv_cmd_buffer *cmd_buffer);
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> index 443c31f..4fa403f 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -1394,9 +1394,6 @@ void anv_cmd_buffer_emit_state_base_address(struct anv_cmd_buffer *cmd_buffer);
>  void anv_cmd_state_setup_attachments(struct anv_cmd_buffer *cmd_buffer,
>                                       const VkRenderPassBeginInfo *info);
>  
> -void anv_cmd_buffer_set_subpass(struct anv_cmd_buffer *cmd_buffer,
> -                                  struct anv_subpass *subpass);
> -
>  struct anv_state
>  anv_cmd_buffer_push_constants(struct anv_cmd_buffer *cmd_buffer,
>                                gl_shader_stage stage);
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
> index 6a84383..1dff6a1 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -1292,10 +1292,7 @@ cmd_buffer_emit_depth_stencil(struct anv_cmd_buffer *cmd_buffer)
>     anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_CLEAR_PARAMS), cp);
>  }
>  
> -/**
> - * @see anv_cmd_buffer_set_subpass()
> - */
> -void
> +static void
>  genX(cmd_buffer_set_subpass)(struct anv_cmd_buffer *cmd_buffer,
>                               struct anv_subpass *subpass)
>  {
> -- 
> 2.5.0.400.gff86faf
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev