[Mesa-dev] [Mesa-stable] [PATCH 1/3] anv: Stop racing relocation offsets
Andres Gomez
agomez at igalia.com
Thu Aug 9 12:49:03 UTC 2018
Jason, this patch has been stalled in the -stable ML for quite some
time without update.
Unless you say otherwise, I will just ignore it at this point and trust
that you will also Cc -stable in the future, in case you come with
another version.
On Tue, 2017-09-26 at 12:31 +0200, Juan A. Suarez Romero wrote:
> On Mon, 2017-06-26 at 22:39 +0300, Andres Gomez wrote:
> > Jason, it doesn't seem like this patch has landed in master. Are you in
> > need of review or is it that this has been superseded?
> >
>
>
> Gently ping to know what is the status for this patch.
>
> Thaks in advance.
>
>
> J.A.
>
> > Thanks!
> >
> > On Wed, 2017-05-10 at 16:08 -0700, Jason Ekstrand wrote:
> > > One of the key invariants of the relocation system is the
> > > presumed_offset field. The assumption is made that the value currently
> > > in the address to be relocated agrees with the presumed_offset field.
> > > If presumed_offset is equal to the offset of the BO, the kernel will
> > > skip the relocation assuming that the value is already correct.
> > >
> > > Our initial implementation of relocation handling had a race where we
> > > would read bo->offset once when we wrote the relocation entry and again
> > > when we filled out actual address.
> > >
> > > Found with helgrind
> > >
> > > Cc: "17.0 17.1" <mesa-stable at lists.freedesktop.org>
> > > ---
> > > src/intel/vulkan/anv_batch_chain.c | 21 +++++++++++++++++----
> > > src/intel/vulkan/anv_private.h | 2 +-
> > > src/intel/vulkan/genX_blorp_exec.c | 5 ++++-
> > > src/intel/vulkan/genX_cmd_buffer.c | 7 +++++--
> > > 4 files changed, 27 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/anv_batch_chain.c b/src/intel/vulkan/anv_batch_chain.c
> > > index 9def174..13303b1 100644
> > > --- a/src/intel/vulkan/anv_batch_chain.c
> > > +++ b/src/intel/vulkan/anv_batch_chain.c
> > > @@ -143,7 +143,8 @@ anv_reloc_list_grow(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)
> > > + uint32_t offset, struct anv_bo *target_bo, uint32_t delta,
> > > + uint64_t *bo_offset_out)
> > > {
> > > struct drm_i915_gem_relocation_entry *entry;
> > > int index;
> > > @@ -155,6 +156,14 @@ anv_reloc_list_add(struct anv_reloc_list *list,
> > > if (result != VK_SUCCESS)
> > > return result;
> > >
> > > + /* Read the BO offset once. This same value will be used in the relocation
> > > + * entry and passed back to the caller for it to use when it writes the
> > > + * actual value. This guarantees that the two values match even if there
> > > + * is a data race between now and when the caller gets around to writing
> > > + * the address into the BO.
> > > + */
> > > + uint64_t presumed_offset = target_bo->offset;
> > > +
> > > /* XXX: Can we use I915_EXEC_HANDLE_LUT? */
> > > index = list->num_relocs++;
> > > list->reloc_bos[index] = target_bo;
> > > @@ -162,11 +171,13 @@ anv_reloc_list_add(struct anv_reloc_list *list,
> > > entry->target_handle = target_bo->gem_handle;
> > > entry->delta = delta;
> > > entry->offset = offset;
> > > - entry->presumed_offset = target_bo->offset;
> > > + entry->presumed_offset = presumed_offset;
> > > entry->read_domains = domain;
> > > entry->write_domain = domain;
> > > VG(VALGRIND_CHECK_MEM_IS_DEFINED(entry, sizeof(*entry)));
> > >
> > > + *bo_offset_out = presumed_offset;
> > > +
> > > return VK_SUCCESS;
> > > }
> > >
> > > @@ -218,14 +229,16 @@ uint64_t
> > > anv_batch_emit_reloc(struct anv_batch *batch,
> > > void *location, struct anv_bo *bo, uint32_t delta)
> > > {
> > > + uint64_t bo_offset;
> > > VkResult result = anv_reloc_list_add(batch->relocs, batch->alloc,
> > > - location - batch->start, bo, delta);
> > > + location - batch->start, bo, delta,
> > > + &bo_offset);
> > > if (result != VK_SUCCESS) {
> > > anv_batch_set_error(batch, result);
> > > return 0;
> > > }
> > >
> > > - return bo->offset + delta;
> > > + return bo_offset + delta;
> > > }
> > >
> > > void
> > > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> > > index 9b0dd67..1686da8 100644
> > > --- a/src/intel/vulkan/anv_private.h
> > > +++ b/src/intel/vulkan/anv_private.h
> > > @@ -825,7 +825,7 @@ void anv_reloc_list_finish(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);
> > > + uint32_t delta, uint64_t *bo_offset_out);
> > >
> > > struct anv_batch_bo {
> > > /* Link in the anv_cmd_buffer.owned_batch_bos list */
> > > diff --git a/src/intel/vulkan/genX_blorp_exec.c b/src/intel/vulkan/genX_blorp_exec.c
> > > index 71ed707..513c269 100644
> > > --- a/src/intel/vulkan/genX_blorp_exec.c
> > > +++ b/src/intel/vulkan/genX_blorp_exec.c
> > > @@ -57,9 +57,12 @@ 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;
> > > + MAYBE_UNUSED uint64_t bo_offset;
> > > +
> > > VkResult result =
> > > anv_reloc_list_add(&cmd_buffer->surface_relocs, &cmd_buffer->pool->alloc,
> > > - ss_offset, address.buffer, address.offset + delta);
> > > + ss_offset, address.buffer, address.offset + delta,
> > > + &bo_offset);
> > > if (result != VK_SUCCESS)
> > > anv_batch_set_error(&cmd_buffer->batch, result);
> > > }
> > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
> > > index ef9b7d0..4276f59 100644
> > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > @@ -170,10 +170,12 @@ add_surface_state_reloc(struct anv_cmd_buffer *cmd_buffer,
> > > struct anv_bo *bo, uint32_t offset)
> > > {
> > > const struct isl_device *isl_dev = &cmd_buffer->device->isl_dev;
> > > + MAYBE_UNUSED uint64_t bo_offset;
> > >
> > > VkResult result =
> > > anv_reloc_list_add(&cmd_buffer->surface_relocs, &cmd_buffer->pool->alloc,
> > > - state.offset + isl_dev->ss.addr_offset, bo, offset);
> > > + state.offset + isl_dev->ss.addr_offset, bo, offset,
> > > + &bo_offset);
> > > if (result != VK_SUCCESS)
> > > anv_batch_set_error(&cmd_buffer->batch, result);
> > > }
> > > @@ -199,11 +201,12 @@ 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;
> > >
> > > + MAYBE_UNUSED uint64_t bo_offset;
> > > VkResult result =
> > > anv_reloc_list_add(&cmd_buffer->surface_relocs,
> > > &cmd_buffer->pool->alloc,
> > > state.offset + isl_dev->ss.aux_addr_offset,
> > > - iview->bo, aux_offset);
> > > + iview->bo, aux_offset, &bo_offset);
> > > if (result != VK_SUCCESS)
> > > anv_batch_set_error(&cmd_buffer->batch, result);
> > > }
>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable
--
Br,
Andres
More information about the mesa-dev
mailing list