[Mesa-dev] [PATCH v2 10/24] anv: handle failures when growing reloc lists
Iago Toral
itoral at igalia.com
Tue Mar 14 11:06:16 UTC 2017
On Tue, 2017-03-14 at 12:01 +0100, Iago Toral wrote:
> On Tue, 2017-03-14 at 11:25 +0200, Pohjolainen, Topi wrote:
> >
> > On Fri, Mar 10, 2017 at 01:38:23PM +0100, Iago Toral Quiroga wrote:
> > >
> > >
> > > Growing the reloc list happens through calling
> > > anv_reloc_list_add()
> > > or
> > > anv_reloc_list_append(). Make sure that we call these through
> > > helpers
> > > that check the result and set the batch error status if needed.
> > >
> > > v2:
> > > - Handling the crashes is not good enough, we need to keep
> > > track
> > > of
> > > the error, for that, keep track of the errors in the batch
> > > instead (Jason).
> > > - Make reloc list growth go through helpers so we can have a
> > > central
> > > place where we can do error tracking (Jason).
> > > ---
> > > src/intel/vulkan/anv_batch_chain.c | 29 ++++++++++++++++++++----
> > > -----
> > > src/intel/vulkan/genX_blorp_exec.c | 7 +++++--
> > > src/intel/vulkan/genX_cmd_buffer.c | 25 ++++++++++++++--------
> > > ---
> > > 3 files changed, 39 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/anv_batch_chain.c
> > > b/src/intel/vulkan/anv_batch_chain.c
> > > index 3774172..2add5bd 100644
> > > --- a/src/intel/vulkan/anv_batch_chain.c
> > > +++ b/src/intel/vulkan/anv_batch_chain.c
> > > @@ -151,8 +151,9 @@ anv_reloc_list_add(struct anv_reloc_list
> > > *list,
> > > const uint32_t domain =
> > > target_bo->is_winsys_bo ? I915_GEM_DOMAIN_RENDER : 0;
> > >
> > > - anv_reloc_list_grow(list, alloc, 1);
> > > - /* TODO: Handle failure */
> > > + VkResult result = anv_reloc_list_grow(list, alloc, 1);
> > > + if (result != VK_SUCCESS)
> > > + return 0;
> > None of the callers of anv_reloc_list_add() or
> > anv_reloc_list_append() are
> > currently interested of the return value.
> and_reloc_list_append() already returns a VkResult.
>
> As for the callers of anv_reloc_list_add(), anv_batch_emit_reloc()
> needs the offset computed by anv_reloc_list_add(), but that seems to
> be
> the only one, so we could do what you suggest below.
>
> >
> > And if they were, they could
> > easily calculate it themselves: both target_bo and delta are
> > inputs.
> > So instead of relying on zero offset indicating error we could
> > simply
> > change the return type to VkResult. Or did I miss something?
> I think this should be fine.
Well, except that anv_batch_emit_reloc needs to return that offset to
its callers, so even if receives a VkResult and computes the offset
itself, in the case of an error we still need to return an offset, so I
don't think there is much gain in doing this, right?
> Iago
>
> >
> > >
> > >
> > >
> > > /* XXX: Can we use I915_EXEC_HANDLE_LUT? */
> > > index = list->num_relocs++;
> > > @@ -169,13 +170,14 @@ anv_reloc_list_add(struct anv_reloc_list
> > > *list,
> > > return target_bo->offset + delta;
> > > }
> > >
> > > -static void
> > > +static VkResult
> > > anv_reloc_list_append(struct anv_reloc_list *list,
> > > const VkAllocationCallbacks *alloc,
> > > struct anv_reloc_list *other, uint32_t
> > > offset)
> > > {
> > > - anv_reloc_list_grow(list, alloc, other->num_relocs);
> > > - /* TODO: Handle failure */
> > > + VkResult result = anv_reloc_list_grow(list, alloc, other-
> > > >
> > > > num_relocs);
> > > + if (result != VK_SUCCESS)
> > > + return result;
> > >
> > > memcpy(&list->relocs[list->num_relocs], &other->relocs[0],
> > > other->num_relocs * sizeof(other->relocs[0]));
> > > @@ -186,6 +188,7 @@ anv_reloc_list_append(struct anv_reloc_list
> > > *list,
> > > list->relocs[i + list->num_relocs].offset += offset;
> > >
> > > list->num_relocs += other->num_relocs;
> > > + return VK_SUCCESS;
> > > }
> > >
> > > /*--------------------------------------------------------------
> > > ---------*
> > > @@ -215,8 +218,12 @@ uint64_t
> > > anv_batch_emit_reloc(struct anv_batch *batch,
> > > void *location, struct anv_bo *bo, uint32_t
> > > delta)
> > > {
> > > - return anv_reloc_list_add(batch->relocs, batch->alloc,
> > > - location - batch->start, bo,
> > > delta);
> > > + uint64_t offset = anv_reloc_list_add(batch->relocs, batch-
> > > >
> > > > alloc,
> > > + location - batch->start,
> > > bo, delta);
> > > + if (offset == 0)
> > > + anv_batch_set_error(batch, VK_ERROR_OUT_OF_HOST_MEMORY);
> > > +
> > > + return offset;
> > > }
> > >
> > > void
> > > @@ -239,8 +246,12 @@ anv_batch_emit_batch(struct anv_batch
> > > *batch,
> > > struct anv_batch *other)
> > > memcpy(batch->next, other->start, size);
> > >
> > > offset = batch->next - batch->start;
> > > - anv_reloc_list_append(batch->relocs, batch->alloc,
> > > - other->relocs, offset);
> > > + VkResult result = anv_reloc_list_append(batch->relocs, batch-
> > > >
> > > > alloc,
> > > + other->relocs,
> > > offset);
> > > + if (result != VK_SUCCESS) {
> > > + anv_batch_set_error(batch, result);
> > > + return;
> > > + }
> > >
> > > batch->next += size;
> > > }
> > > diff --git a/src/intel/vulkan/genX_blorp_exec.c
> > > b/src/intel/vulkan/genX_blorp_exec.c
> > > index 7084735..90adefe 100644
> > > --- a/src/intel/vulkan/genX_blorp_exec.c
> > > +++ b/src/intel/vulkan/genX_blorp_exec.c
> > > @@ -57,8 +57,11 @@ blorp_surface_reloc(struct blorp_batch *batch,
> > > uint32_t ss_offset,
> > > struct blorp_address address, uint32_t
> > > delta)
> > > {
> > > struct anv_cmd_buffer *cmd_buffer = batch->driver_batch;
> > > - anv_reloc_list_add(&cmd_buffer->surface_relocs, &cmd_buffer-
> > > >
> > > > pool->alloc,
> > > - ss_offset, address.buffer, address.offset
> > > +
> > > delta);
> > > + uint64_t offset =
> > > + anv_reloc_list_add(&cmd_buffer->surface_relocs,
> > > &cmd_buffer-
> > > >
> > > > pool->alloc,
> > > + ss_offset, address.buffer,
> > > address.offset
> > > + delta);
> > > + if (offset == 0)
> > > + anv_batch_set_error(&cmd_buffer->batch,
> > > VK_ERROR_OUT_OF_HOST_MEMORY);
> > > }
> > >
> > > static void *
> > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > > b/src/intel/vulkan/genX_cmd_buffer.c
> > > index f74109e..61066b8 100644
> > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > @@ -155,12 +155,19 @@
> > > genX(cmd_buffer_emit_state_base_address)(struct anv_cmd_buffer
> > > *cmd_buffer)
> > > static void
> > > add_surface_state_reloc(struct anv_cmd_buffer *cmd_buffer,
> > > struct anv_state state,
> > > + bool is_aux,
> > > struct anv_bo *bo, uint32_t offset)
> > > {
> > > const struct isl_device *isl_dev = &cmd_buffer->device-
> > > >
> > > > isl_dev;
> > >
> > > - anv_reloc_list_add(&cmd_buffer->surface_relocs, &cmd_buffer-
> > > >
> > > > pool->alloc,
> > > - state.offset + isl_dev->ss.addr_offset,
> > > bo,
> > > offset);
> > > + uint32_t total_offset = state.offset +
> > > + (is_aux ? isl_dev->ss.aux_addr_offset : isl_dev-
> > > >
> > > > ss.addr_offset);
> > > +
> > > + uint64_t reloc_offset =
> > > + anv_reloc_list_add(&cmd_buffer->surface_relocs,
> > > &cmd_buffer-
> > > >
> > > > pool->alloc,
> > > + total_offset, bo, offset);
> > > + if (reloc_offset == 0)
> > > + anv_batch_set_error(&cmd_buffer->batch,
> > > VK_ERROR_OUT_OF_HOST_MEMORY);
> > > }
> > >
> > > static void
> > > @@ -171,9 +178,7 @@ add_image_view_relocs(struct anv_cmd_buffer
> > > *cmd_buffer,
> > > {
> > > const struct isl_device *isl_dev = &cmd_buffer->device-
> > > >
> > > > isl_dev;
> > >
> > > - anv_reloc_list_add(&cmd_buffer->surface_relocs, &cmd_buffer-
> > > >
> > > > pool->alloc,
> > > - state.offset + isl_dev->ss.addr_offset,
> > > - iview->bo, iview->offset);
> > > + add_surface_state_reloc(cmd_buffer, state, false, iview->bo,
> > > iview->offset);
> > >
> > > if (aux_usage != ISL_AUX_USAGE_NONE) {
> > > uint32_t aux_offset = iview->offset + iview->image-
> > > >
> > > > aux_surface.offset;
> > > @@ -186,9 +191,7 @@ add_image_view_relocs(struct anv_cmd_buffer
> > > *cmd_buffer,
> > > uint32_t *aux_addr_dw = state.map + isl_dev-
> > > >
> > > > ss.aux_addr_offset;
> > > aux_offset += *aux_addr_dw & 0xfff;
> > >
> > > - anv_reloc_list_add(&cmd_buffer->surface_relocs,
> > > &cmd_buffer-
> > > >
> > > > pool->alloc,
> > > - state.offset + isl_dev-
> > > >
> > > > ss.aux_addr_offset,
> > > - iview->bo, aux_offset);
> > > + add_surface_state_reloc(cmd_buffer, state, true, iview-
> > > >bo,
> > > aux_offset);
> > > }
> > > }
> > >
> > > @@ -1120,7 +1123,7 @@ emit_binding_table(struct anv_cmd_buffer
> > > *cmd_buffer,
> > > format, bo_offset, 12, 1);
> > >
> > > bt_map[0] = surface_state.offset + state_offset;
> > > - add_surface_state_reloc(cmd_buffer, surface_state, bo,
> > > bo_offset);
> > > + add_surface_state_reloc(cmd_buffer, surface_state, false,
> > > bo, bo_offset);
> > > }
> > >
> > > if (map->surface_count == 0)
> > > @@ -1223,7 +1226,7 @@ emit_binding_table(struct anv_cmd_buffer
> > > *cmd_buffer,
> > > case VK_DESCRIPTOR_TYPE_UNIFORM_TEXEL_BUFFER:
> > > surface_state = desc->buffer_view->surface_state;
> > > assert(surface_state.alloc_size);
> > > - add_surface_state_reloc(cmd_buffer, surface_state,
> > > + add_surface_state_reloc(cmd_buffer, surface_state,
> > > false,
> > > desc->buffer_view->bo,
> > > desc->buffer_view->offset);
> > > break;
> > > @@ -1233,7 +1236,7 @@ emit_binding_table(struct anv_cmd_buffer
> > > *cmd_buffer,
> > > ? desc->buffer_view->writeonly_storage_surface_state
> > > : desc->buffer_view->storage_surface_state;
> > > assert(surface_state.alloc_size);
> > > - add_surface_state_reloc(cmd_buffer, surface_state,
> > > + add_surface_state_reloc(cmd_buffer, surface_state,
> > > false,
> > > desc->buffer_view->bo,
> > > desc->buffer_view->offset);
> > >
More information about the mesa-dev
mailing list