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

[Jason Ekstrand]

On Thu, Oct 6, 2016 at 1:33 PM, Nanley Chery  wrote:

> On Thu, Oct 06, 2016 at 12:35:53PM -0700, Jason Ekstrand wrote:
> > On Thu, Oct 6, 2016 at 12:30 PM, Nanley Chery 
> wrote:
> >
> > > 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 
> > > > ---
> > > >  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 =
> > > >   _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.
> > >
> >
> > Initially, I think we did it to ensure that binding tables got
> re-emitted.
> > However, we're now also re-emitting binding tables on pipeline changes
> and
> > you have a pipeline change at the top of every subpass, so it shouldn't
> be
> > needed either place.
> >
> >
>
> That makes sense. This series is
> Reviewed-by: Nanley Chery 
>

That wasn't 100% true... We do actually need to flag that render targets
are dirty in set_subpass.  It ends up not mattering in secondaries, but I
think it's worth doing there too.  I've pushed a version of the patches
which I think does what we want.


> > > -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

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

[Nanley Chery]

On Thu, Oct 06, 2016 at 12:35:53PM -0700, Jason Ekstrand wrote:
> On Thu, Oct 6, 2016 at 12:30 PM, Nanley Chery  wrote:
> 
> > 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 
> > > ---
> > >  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 =
> > >   _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.
> >
> 
> Initially, I think we did it to ensure that binding tables got re-emitted.
> However, we're now also re-emitting binding tables on pipeline changes and
> you have a pipeline change at the top of every subpass, so it shouldn't be
> needed either place.
> 
> 

That makes sense. This series is
Reviewed-by: Nanley Chery 

> > -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 

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

[Jason Ekstrand]

On Thu, Oct 6, 2016 at 12:30 PM, Nanley Chery  wrote:

> 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 
> > ---
> >  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 =
> >   _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.
>

Initially, I think we did it to ensure that binding tables got re-emitted.
However, we're now also re-emitting binding tables on pipeline changes and
you have a pipeline change at the top of every subpass, so it shouldn't be
needed either place.


> -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,
> > 

Re: [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 
> ---
>  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 =
>   
> _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
> +++