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

[Jason Ekstrand]

On Wed, Dec 12, 2018 at 7:15 PM Rafael Antognolli <
rafael.antognolli at intel.com> wrote:

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

You're right.  We need to at least handle the BO set in the reloc data
structure.  I'm not sure what the best way to do that is.  I guess we could
just process it on the first one or something like that.  processing the BO
set does involve a qsort so we probably don't want to do it too many extra
times.

--Jason


> >
> >     +      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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181212/a3ac1c36/attachment-0001.html>