On Wed, Mar 15, 2017 at 01:03:37PM +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).
>
> v3:
> - Callers that need the offset returned by anv_reloc_list_add() can
> compute it themselves since it is extracted from the inputs to the
> function, so change the function to return a VkResult, make
> anv_batch_emit_reloc() also return a VkResult and let their callers
> do the error management (Topi)
> ---
> src/intel/vulkan/anv_batch_chain.c | 40
> +++++++++++++++++++++++++-------------
> src/intel/vulkan/anv_private.h | 17 ++++++++++++----
> src/intel/vulkan/genX_blorp_exec.c | 20 +++++++++++++++----
> src/intel/vulkan/genX_cmd_buffer.c | 27 +++++++++++++------------
> 4 files changed, 71 insertions(+), 33 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_batch_chain.c
> b/src/intel/vulkan/anv_batch_chain.c
> index c6fdfe5..95df0c9 100644
> --- a/src/intel/vulkan/anv_batch_chain.c
> +++ b/src/intel/vulkan/anv_batch_chain.c
> @@ -140,7 +140,7 @@ anv_reloc_list_grow(struct anv_reloc_list *list,
> return VK_SUCCESS;
> }
>
> -uint64_t
> +VkResult
> anv_reloc_list_add(struct anv_reloc_list *list,
> const VkAllocationCallbacks *alloc,
> uint32_t offset, struct anv_bo *target_bo, uint32_t delta)
> @@ -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 result;
>
> /* XXX: Can we use I915_EXEC_HANDLE_LUT? */
> index = list->num_relocs++;
> @@ -166,16 +167,17 @@ anv_reloc_list_add(struct anv_reloc_list *list,
> entry->write_domain = domain;
> VG(VALGRIND_CHECK_MEM_IS_DEFINED(entry, sizeof(*entry)));
>
> - return target_bo->offset + delta;
> + return VK_SUCCESS;
> }
>
> -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;
> }
>
> /*-----------------------------------------------------------------------*
> @@ -211,12 +214,19 @@ anv_batch_emit_dwords(struct anv_batch *batch, int
> num_dwords)
> return p;
> }
>
> -uint64_t
> +VkResult
> anv_batch_emit_reloc(struct anv_batch *batch,
> - void *location, struct anv_bo *bo, uint32_t delta)
> + void *location, struct anv_bo *bo, uint32_t delta,
> + uint64_t *offset)
> {
> - return anv_reloc_list_add(batch->relocs, batch->alloc,
> - location - batch->start, bo, delta);
> + VkResult result = anv_reloc_list_add(batch->relocs, batch->alloc,
> + location - batch->start, bo, delta);
> + if (result != VK_SUCCESS)
> + return result;
I'm really sorry for the endless churn, but it looks to me that only
anv_reloc_list_add() needs to report the error. I for some reason thought the
callers of anv_batch_emit_reloc() need to take individual steps upon failure.
All they really do is set the error state which we can do already here as
you had in the original.
So here just:
if (result != VK_SUCCESS) {
anv_batch_set_error(batch, result);
return 0;
}
return bo->offset + delta;
And leave _anv_combine_address() and blorp_emit_reloc() as they were. Now that
you have written it out it becomes clearer to me.
> +
> + *offset = bo->offset + delta;
> +
> + return VK_SUCCESS;
> }
>
> void
> @@ -240,8 +250,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/anv_private.h b/src/intel/vulkan/anv_private.h
> index 87538de..081bfb6 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -673,7 +673,7 @@ VkResult anv_reloc_list_init(struct anv_reloc_list *list,
> void anv_reloc_list_finish(struct anv_reloc_list *list,
> const VkAllocationCallbacks *alloc);
>
> -uint64_t anv_reloc_list_add(struct anv_reloc_list *list,
> +VkResult anv_reloc_list_add(struct anv_reloc_list *list,
> const VkAllocationCallbacks *alloc,
> uint32_t offset, struct anv_bo *target_bo,
> uint32_t delta);
> @@ -717,8 +717,9 @@ struct anv_batch {
>
> void *anv_batch_emit_dwords(struct anv_batch *batch, int num_dwords);
> void anv_batch_emit_batch(struct anv_batch *batch, struct anv_batch *other);
> -uint64_t anv_batch_emit_reloc(struct anv_batch *batch,
> - void *location, struct anv_bo *bo, uint32_t
> offset);
> +VkResult anv_batch_emit_reloc(struct anv_batch *batch,
> + void *location, struct anv_bo *bo,
> + uint32_t delta, uint64_t *offset);
> VkResult anv_device_submit_simple_batch(struct anv_device *device,
> struct anv_batch *batch);
>
> @@ -751,7 +752,15 @@ _anv_combine_address(struct anv_batch *batch, void
> *location,
> } else {
> assert(batch->start <= location && location < batch->end);
>
> - return anv_batch_emit_reloc(batch, location, address.bo,
> address.offset + delta);
> + uint64_t offset;
> + VkResult result = anv_batch_emit_reloc(batch, location, address.bo,
> + address.offset + delta,
> &offset);
> + if (result != VK_SUCCESS) {
> + anv_batch_set_error(batch, result);
> + return 0;
> + }
> +
> + return offset;
> }
> }
>
> diff --git a/src/intel/vulkan/genX_blorp_exec.c
> b/src/intel/vulkan/genX_blorp_exec.c
> index 7084735..f775ce8 100644
> --- a/src/intel/vulkan/genX_blorp_exec.c
> +++ b/src/intel/vulkan/genX_blorp_exec.c
> @@ -48,8 +48,17 @@ blorp_emit_reloc(struct blorp_batch *batch,
> struct anv_cmd_buffer *cmd_buffer = batch->driver_batch;
> assert(cmd_buffer->batch.start <= location &&
> location < cmd_buffer->batch.end);
> - return anv_batch_emit_reloc(&cmd_buffer->batch, location,
> - address.buffer, address.offset + delta);
> +
> + uint64_t offset;
> + VkResult result =
> + anv_batch_emit_reloc(&cmd_buffer->batch, location,
> + address.buffer, address.offset + delta, &offset);
> + if (result != VK_SUCCESS) {
> + anv_batch_set_error(&cmd_buffer->batch, result);
> + return 0;
> + }
> +
> + return offset;
> }
>
> static void
> @@ -57,8 +66,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);
> + VkResult result =
> + anv_reloc_list_add(&cmd_buffer->surface_relocs,
> &cmd_buffer->pool->alloc,
> + ss_offset, address.buffer, address.offset + delta);
> + if (result != VK_SUCCESS)
> + anv_batch_set_error(&cmd_buffer->batch, result);
> }
>
> static void *
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> index e3faa17..6a759f4 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);
> +
> + VkResult result =
> + anv_reloc_list_add(&cmd_buffer->surface_relocs,
> &cmd_buffer->pool->alloc,
> + total_offset, bo, offset);
> + if (result != VK_SUCCESS)
> + anv_batch_set_error(&cmd_buffer->batch, result);
> }
>
> 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);
I also think we could avoid changes to add_surface_state_reloc() altogether
if handled the special case of aux below. So here just:
VkResult result = anv_reloc_list_add(
&cmd_buffer->surface_relocs,
&cmd_buffer->pool->alloc,
state.offset + isl_dev->ss.addr_offset,
iview->bo, iview->offset);
if (result != VK_SUCCESS) {
anv_batch_set_error(&cmd_buffer->batch, result);
return;
}
>
> 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);
And here:
if (anv_reloc_list_add(&cmd_buffer->surface_relocs,
&cmd_buffer->pool->alloc,
state.offset + isl_dev->ss.aux_addr_offset,
iview->bo, aux_offset) != VK_SUCCESS)
anv_batch_set_error(&cmd_buffer->batch, result);
> + 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)
> @@ -1221,7 +1224,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;
> @@ -1248,7 +1251,7 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,
>
> anv_fill_buffer_surface_state(cmd_buffer->device, surface_state,
> format, offset, range, 1);
> - add_surface_state_reloc(cmd_buffer, surface_state,
> + add_surface_state_reloc(cmd_buffer, surface_state, false,
> desc->buffer->bo,
> desc->buffer->offset + offset);
> break;
> @@ -1259,7 +1262,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);
>
> --
> 2.7.4
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
