[Mesa-dev] [PATCH 12/16] anv: ensure that we don't ever try to adjust relocations more than once

[Iago Toral]

On Tue, 2017-03-07 at 13:22 -0800, Jason Ekstrand wrote:
> On Mon, Mar 6, 2017 at 11:15 PM, Iago Toral Quiroga <itoral at igalia.co
> m> wrote:
> > There is a CTS tests that creates that situation by forcing OOM
> > during a queue
> > submision and then trying again without the OOM enforcement. The
> > driver returns
> > the appropriate out of memory error on the first try, but the
> > relocations had
> > already been computed and stored, so in the second attempt, we
> > duplicate the
> > deltas and end up with offsets outside the BO limits.
> This is something that has come up internally within Khronos.  The
> correct result (once the spec change lands) will be that basically
> all vkQueueSubmit errors will actually result in VK_ERROR_DEVICE_LOST
> precisely because of these sorts of issues.  I've been meaning to
> send out a patch that makes vkQueueSubmit turn all errors into
> DEVICE_LOST but haven't gotten around to it yet.

In that case I understand that once we have
reported VK_ERROR_DEVICE_LOST on the first attempt any subsequent
attempts have undefined behavior so the current test doesn't make sense
and we really don't need to do anything about this. That's good to
know, thanks Jason. I'll drop this patch then.

Iago
 
> > Fixes:
> > dEQP-VK.api.out_of_host_memory.complex
> > ---
> >  src/intel/vulkan/anv_batch_chain.c | 12 ++++++++++--
> >  src/intel/vulkan/anv_cmd_buffer.c  |  2 ++
> >  src/intel/vulkan/anv_private.h     |  6 ++++++
> >  3 files changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/intel/vulkan/anv_batch_chain.c
> > b/src/intel/vulkan/anv_batch_chain.c
> > index aae5f78..7df4ef2 100644
> > --- a/src/intel/vulkan/anv_batch_chain.c
> > +++ b/src/intel/vulkan/anv_batch_chain.c
> > @@ -1242,8 +1242,16 @@ anv_cmd_buffer_execbuf(struct anv_device
> > *device,
> >     struct anv_execbuf execbuf;
> >     anv_execbuf_init(&execbuf);
> > 
> > -   adjust_relocations_from_state_pool(ss_pool, &cmd_buffer-
> > >surface_relocs,
> > -                                      cmd_buffer-
> > >last_ss_pool_center);
> > +   /* Remember if we have adjusted the relocation offsets so we
> > only ever do
> > +    * this once
> > +    */
> > +   struct anv_cmd_state *state = &cmd_buffer->state;
> > +   if (!state->adjusted_relocation_offsets) {
> > +       adjust_relocations_from_state_pool(ss_pool, &cmd_buffer-
> > >surface_relocs,
> > +                                          cmd_buffer-
> > >last_ss_pool_center);
> > +      state->adjusted_relocation_offsets = true;
> > +   }
> > +
> >     VkResult result =
> >        anv_execbuf_add_bo(&execbuf, &ss_pool->bo, &cmd_buffer-
> > >surface_relocs,
> >                           &cmd_buffer->pool->alloc);
> > diff --git a/src/intel/vulkan/anv_cmd_buffer.c
> > b/src/intel/vulkan/anv_cmd_buffer.c
> > index dd5e6c8..adc7c19 100644
> > --- a/src/intel/vulkan/anv_cmd_buffer.c
> > +++ b/src/intel/vulkan/anv_cmd_buffer.c
> > @@ -142,6 +142,7 @@ anv_cmd_state_reset(struct anv_cmd_buffer
> > *cmd_buffer)
> >     state->need_query_wa = true;
> >     state->pma_fix_enabled = false;
> >     state->hiz_enabled = false;
> > +   state->adjusted_relocation_offsets = false;
> > 
> >     if (state->attachments != NULL) {
> >        vk_free(&cmd_buffer->pool->alloc, state->attachments);
> > @@ -198,6 +199,7 @@ static VkResult anv_create_cmd_buffer(
> >     cmd_buffer->pool = pool;
> >     cmd_buffer->level = level;
> >     cmd_buffer->state.attachments = NULL;
> > +   cmd_buffer->state.adjusted_relocation_offsets = false;
> > 
> >     result = anv_cmd_buffer_init_batch_bo_chain(cmd_buffer);
> >     if (result != VK_SUCCESS)
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > index 46b51a9..15d1401 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1309,6 +1309,12 @@ struct anv_cmd_state {
> >      */
> >     struct anv_state                           
> >  null_surface_state;
> > 
> > +   /**
> > +    * A flag indicating whether we have adjusted relation offsets
> > for this
> > +    * command buffer.
> > +    */
> > +   bool                                       
> >  adjusted_relocation_offsets;
> > +
> >     struct {
> >        struct anv_buffer *                       index_buffer;
> >        uint32_t                                  index_type; /**<
> > 3DSTATE_INDEX_BUFFER.IndexFormat */
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >