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

[Jason Ekstrand]

On July 2, 2018 01:09:38 Iago Toral <itoral at igalia.com> wrote:

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

The problem is that it never will be the same.  The secondary always 
allocate a new binding table pool and re-emit STATE_BASE_ADDRESS so I don't 
think we're actually saving anything.