[RFC,09/14] anv: Validate the list of BOs from the block pool.

Submitted by Rafael Antognolli on Dec. 8, 2018, 12:05 a.m.

Details

Message ID 20181208000553.29501-10-rafael.antognolli@intel.com
State New
Headers show
Series "Do not use userptr in anv if softpin is available." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Rafael Antognolli Dec. 8, 2018, 12:05 a.m.
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(-)

Patch hide | download patch | download mbox

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);
+   }
+}
+
 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);
+      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.

Comments

On Fri, Dec 7, 2018 at 6:06 PM Rafael Antognolli <
rafael.antognolli@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.


> +      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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
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@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@lists.freedesktop.org
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 

> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Wed, Dec 12, 2018 at 7:15 PM Rafael Antognolli <
rafael.antognolli@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@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@lists.freedesktop.org
> >     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>