<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Dec 12, 2018 at 7:15 PM Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com">rafael.antognolli@intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Dec 10, 2018 at 01:49:43PM -0600, Jason Ekstrand wrote: > On Fri, Dec 7, 2018 at 6:06 PM Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com" target="_blank">rafael.antognolli@intel.com</a>> > 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-><a href="http://block_pool.bo" rel="noreferrer" target="_blank">block_pool.bo</a>, > - &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. </blockquote><div> </div><div>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.</div><div> </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > + 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 > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div>