[Mesa-dev] [PATCH 4/4] anv/cmd_buffer: only emit state base address if the address changes

[Iago Toral]

On Sun, 2018-07-01 at 18:30 -0500, Jason Ekstrand wrote:
> On June 29, 2018 03:11:00 Iago Toral Quiroga <itoral at igalia.com>
> wrote:
> 
> > ---
> > src/intel/vulkan/anv_private.h     |  5 +++++
> > src/intel/vulkan/genX_cmd_buffer.c | 12 +++++++-----
> > 2 files changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > index 510471da602..1a9ab7013f2 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1989,6 +1989,11 @@ struct anv_cmd_state {
> >     * is one of the states in render_pass_states.
> >     */
> >    struct anv_state                             null_surface_state;
> > +
> > +   /**
> > +    * Current state base address.
> > +    */
> > +   struct
> > anv_address                           base_state_address;
> > };
> > 
> > struct anv_cmd_pool {
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index 611311904e6..2847e0b30c9 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -67,6 +67,12 @@ genX(cmd_buffer_emit_state_base_address)(struct 
> > anv_cmd_buffer *cmd_buffer)
> > {
> >    struct anv_device *device = cmd_buffer->device;
> > 
> > +   struct anv_address new_base_address =
> > +      anv_cmd_buffer_surface_base_address(cmd_buffer);
> > +   if (new_base_address.bo == cmd_buffer-
> > >state.base_state_address.bo &&
> > +       new_base_address.offset == cmd_buffer-
> > >state.base_state_address.offset)
> > +      return;
> > +
> >    /* If we are emitting a new state base address we probably need
> > to re-emit
> >     * binding tables.
> >     */
> > @@ -90,8 +96,7 @@ genX(cmd_buffer_emit_state_base_address)(struct 
> > anv_cmd_buffer *cmd_buffer)
> >       sba.GeneralStateMemoryObjectControlState = GENX(MOCS);
> >       sba.GeneralStateBaseAddressModifyEnable = true;
> > 
> > -      sba.SurfaceStateBaseAddress =
> > -         anv_cmd_buffer_surface_base_address(cmd_buffer);
> > +      sba.SurfaceStateBaseAddress = new_base_address;
> >       sba.SurfaceStateMemoryObjectControlState = GENX(MOCS);
> >       sba.SurfaceStateBaseAddressModifyEnable = true;
> > 
> > @@ -1521,9 +1526,6 @@ genX(CmdExecuteCommands)(
> >    /* Each of the secondary command buffers will use its own state
> > base
> >     * address.  We need to re-emit state base address for the
> > primary after
> >     * all of the secondaries are done.
> > -    *
> > -    * TODO: Maybe we want to make this a dirty bit to avoid extra
> > state base
> > -    * address calls?
> 
> I don't think this is correct.  When a secondary executes, we have
> to 
> reemit STATE_BASE_ADDRESS because the secondary used it's own and we
> need 
> to set it back for the primary. The comment above was saying that we
> can 
> probably avoid it if we have a bunch of ExecuteCommands calls back to
> back 
> or if the last thing in the batch is a call out to a secondary.
>   As is, I 
> think this patch will cause problems in the case where the client
> uses a 
> secondary followed by rendering in the primary.  Have I missed
> something?

I shouldn't remove the comment since this patche doesn't address that
TODO, we still emit the state base address for the primary below, the
only change in here is that if the base state address of the primary is
the same as the one for the secondaries we won't actually emit the
state packet, but that should be fine. Maybe you thought I was removing
the line below?

> 
> >     */
> >    genX(cmd_buffer_emit_state_base_address)(primary);
> > }
> > --
> > 2.14.1
> 
> 
> 
>