[Mesa-dev] [RFC PATCH 09/14] anv: Validate the list of BOs from the block pool.

[Rafael Antognolli]

On Mon, Dec 10, 2018 at 01:49:43PM -0600, Jason Ekstrand wrote:
> On Fri, Dec 7, 2018 at 6:06 PM Rafael Antognolli <rafael.antognolli at intel.com>
> wrote:
> 
>     We now have multiple BOs in the block pool, but sometimes we still
>     reference only the first one in some instructions, and use relative
>     offsets in others. So we must be sure to add all the BOs from the block
>     pool to the validation list when submitting commands.
>     ---
>      src/intel/vulkan/anv_batch_chain.c | 47 ++++++++++++++++++++++++++----
>      1 file changed, 42 insertions(+), 5 deletions(-)
> 
>     diff --git a/src/intel/vulkan/anv_batch_chain.c b/src/intel/vulkan/
>     anv_batch_chain.c
>     index bec4d647b7e..65df28ccb91 100644
>     --- a/src/intel/vulkan/anv_batch_chain.c
>     +++ b/src/intel/vulkan/anv_batch_chain.c
>     @@ -1356,6 +1356,36 @@ relocate_cmd_buffer(struct anv_cmd_buffer
>     *cmd_buffer,
>         return true;
>      }
> 
>     +static void
>     +anv_reloc_list_add_dep(struct anv_cmd_buffer *cmd_buffer,
>     +                       struct anv_bo_list *bo_list)
>     +{
>     +   struct anv_bo_list *iter;
>     +   struct anv_bo *bo;
>     +   struct anv_reloc_list *relocs = cmd_buffer->batch.relocs;
>     +
>     +   anv_block_pool_foreach_bo(bo_list, iter, bo) {
>     +      _mesa_set_add(relocs->deps, bo);
>     +   }
>     +}
>     +
>     +static void
>     +anv_batch_bos_add(struct anv_cmd_buffer *cmd_buffer)
>     +{
>     +   struct anv_bo_list *bo_list;
>     +
>     +   bo_list = cmd_buffer->device->dynamic_state_pool.block_pool.bos;
>     +   anv_reloc_list_add_dep(cmd_buffer, bo_list);
>     +
>     +   bo_list = cmd_buffer->device->instruction_state_pool.block_pool.bos;
>     +   anv_reloc_list_add_dep(cmd_buffer, bo_list);
>     +
>     +   if (cmd_buffer->device->instance->physicalDevice.use_softpin) {
>     +      bo_list = cmd_buffer->device->binding_table_pool.block_pool.bos;
>     +      anv_reloc_list_add_dep(cmd_buffer, bo_list);
> 
> 
> I don't think want to add things to the command buffer's dependency set this
> late (at submit time).  Instead, I think we want to just do anv_execbuf_add_bo
> for each of them directly at the top of setup_execbuf_for_cmd_buffer.
> 
> 
>     +   }
>     +}
>     +
>      static VkResult
>      setup_execbuf_for_cmd_buffer(struct anv_execbuf *execbuf,
>                                   struct anv_cmd_buffer *cmd_buffer)
>     @@ -1364,13 +1394,20 @@ setup_execbuf_for_cmd_buffer(struct anv_execbuf
>     *execbuf,
>         struct anv_state_pool *ss_pool =
>            &cmd_buffer->device->surface_state_pool;
> 
>     +   anv_batch_bos_add(cmd_buffer);
>     +
>         adjust_relocations_from_state_pool(ss_pool, &cmd_buffer->
>     surface_relocs,
>                                            cmd_buffer->last_ss_pool_center);
>     -   VkResult result = anv_execbuf_add_bo(execbuf, ss_pool->block_pool.bo,
>     -                                        &cmd_buffer->surface_relocs, 0,
>     -                                        &cmd_buffer->device->alloc);
>     -   if (result != VK_SUCCESS)
>     -      return result;
>     +   VkResult result;
>     +   struct anv_bo *bo;
>     +   struct anv_bo_list *iter;
>     +   anv_block_pool_foreach_bo(ss_pool->block_pool.bos, iter, bo) {
>     +      result = anv_execbuf_add_bo(execbuf, bo,
>     +                                  &cmd_buffer->surface_relocs, 0,
>     +                                  &cmd_buffer->device->alloc);
> 
> 
> I don't think we want to pass the relocation list on every BO.  Instead, we
> should have a softpin case where we walk the list and don't provide any
> relocations and a non-softpin case where we assert that there is only one BO
> and do provide the relocations.
>  

So, in the case of softpin, we need to pass the surface relocations at
some point, because the BO dependency is stored there. Unless we store
it somewhere else.

If we use:

   anv_execbuf_add_bo(execbuf, bo, NULL, 0, &cmd_buffer->device->alloc);

for the surface state pool, it seems like we end up missing some BOs in
the execbuf.

> 
>     +      if (result != VK_SUCCESS)
>     +         return result;
>     +   }
> 
>         /* First, we walk over all of the bos we've seen and add them and their
>          * relocations to the validate list.
>     --
>     2.17.1
> 
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 

> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev