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-sta...@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);
}
--
2.5.0.400.gff86faf
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
