On Fri, May 5, 2017 at 5:09 AM, Chris Wilson <ch...@chris-wilson.co.uk>
wrote:
> It is a requirement, not just of using the NO_RELOC mode, that all
> relocation values in the execobjects match their reloc.presumed_offset,
> as the kernel will skip performing *any* relocation if the presumed_offset
> matches the target object. As anv is setting unknown relocations to -1
> irrespective of the actual value, if the kernel placed the target at -1
> (i.e. 0xfffffffff000 for 48bit GTT), it would happily skip the relocation.
> To prevent this, we need to always do the userspace relocations to ensure
> the values match. To improve further, set the unknown object offset to 0
> (a valid location) on the offchance it is available and the migration
> skipped.
>
Is this a real issue? I specifically chose -1 because it *wasn't*
page-aligned. I don't think the kernel whacks off the bottom 12 bits
before doing the comparison to determine whether or not the relocation
needs to happen.
--Jason
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Jason Ekstrand <jason.ekstr...@intel.com>
> ---
> src/intel/vulkan/anv_batch_chain.c | 97 ++++++++++++------------------
> --------
> src/intel/vulkan/anv_private.h | 2 +-
> 2 files changed, 32 insertions(+), 67 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_batch_chain.c
> b/src/intel/vulkan/anv_batch_chain.c
> index 0529f22b84..46c4ce6efb 100644
> --- a/src/intel/vulkan/anv_batch_chain.c
> +++ b/src/intel/vulkan/anv_batch_chain.c
> @@ -1205,28 +1205,10 @@ anv_reloc_list_apply(struct anv_device *device,
> * probably the fastest mechanism for doing relocations since the kernel
> would
> * have to make a full copy of all the relocations lists.
> */
> -static bool
> +static void
> relocate_cmd_buffer(struct anv_cmd_buffer *cmd_buffer,
> struct anv_execbuf *exec)
> {
> - static int userspace_relocs = -1;
> - if (userspace_relocs < 0)
> - userspace_relocs = env_var_as_boolean("ANV_USERSPACE_RELOCS",
> true);
> - if (!userspace_relocs)
> - return false;
> -
> - /* First, we have to check to see whether or not we can even do the
> - * relocation. New buffers which have never been submitted to the
> kernel
> - * don't have a valid offset so we need to let the kernel do
> relocations so
> - * that we can get offsets for them. On future execbuf2 calls, those
> - * buffers will have offsets and we will be able to skip relocating.
> - * Invalid offsets are indicated by anv_bo::offset == (uint64_t)-1.
> - */
> - for (uint32_t i = 0; i < exec->bo_count; i++) {
> - if (exec->bos[i]->offset == (uint64_t)-1)
> - return false;
> - }
> -
> /* Since surface states are shared between command buffers and we don't
> * know what order they will be submitted to the kernel, we don't know
> * what address is actually written in the surface state object at any
> @@ -1248,8 +1230,6 @@ relocate_cmd_buffer(struct anv_cmd_buffer
> *cmd_buffer,
>
> for (uint32_t i = 0; i < exec->bo_count; i++)
> exec->objects[i].offset = exec->bos[i]->offset;
> -
> - return true;
> }
>
> static VkResult
> @@ -1334,55 +1314,40 @@ setup_execbuf_for_cmd_buffer(struct anv_execbuf
> *execbuf,
> .buffer_count = execbuf->bo_count,
> .batch_start_offset = 0,
> .batch_len = batch->next - batch->start,
> - .cliprects_ptr = 0,
> - .num_cliprects = 0,
> - .DR1 = 0,
> - .DR4 = 0,
> - .flags = I915_EXEC_HANDLE_LUT | I915_EXEC_RENDER |
> - I915_EXEC_CONSTANTS_REL_GENERAL,
> + .flags =
> + I915_EXEC_RENDER |
> + I915_EXEC_HANDLE_LUT |
> + I915_EXEC_NO_RELOC |
> + I915_EXEC_CONSTANTS_REL_GENERAL,
> .rsvd1 = cmd_buffer->device->context_id,
> .rsvd2 = 0,
> };
>
> - if (relocate_cmd_buffer(cmd_buffer, execbuf)) {
> - /* If we were able to successfully relocate everything, tell the
> kernel
> - * that it can skip doing relocations. The requirement for using
> - * NO_RELOC is:
> - *
> - * 1) The addresses written in the objects must match the
> corresponding
> - * reloc.presumed_offset which in turn must match the
> corresponding
> - * execobject.offset.
> - *
> - * 2) To avoid stalling, execobject.offset should match the current
> - * address of that object within the active context.
> - *
> - * In order to satisfy all of the invariants that make userspace
> - * relocations to be safe (see relocate_cmd_buffer()), we need to
> - * further ensure that the addresses we use match those used by the
> - * kernel for the most recent execbuf2.
> - *
> - * The kernel may still choose to do relocations anyway if
> something has
> - * moved in the GTT. In this case, the relocation list still needs
> to be
> - * valid. All relocations on the batch buffers are already valid
> and
> - * kept up-to-date. For surface state relocations, by applying the
> - * relocations in relocate_cmd_buffer, we ensured that the address
> in
> - * the RENDER_SURFACE_STATE matches presumed_offset, so it should be
> - * safe for the kernel to relocate them as needed.
> - */
> - execbuf->execbuf.flags |= I915_EXEC_NO_RELOC;
> - } else {
> - /* In the case where we fall back to doing kernel relocations, we
> need
> - * to ensure that the relocation list is valid. All relocations on
> the
> - * batch buffers are already valid and kept up-to-date. Since
> surface
> - * states are shared between command buffers and we don't know what
> - * order they will be submitted to the kernel, we don't know what
> - * address is actually written in the surface state object at any
> given
> - * time. The only option is to set a bogus presumed offset and let
> the
> - * kernel relocate them.
> - */
> - for (size_t i = 0; i < cmd_buffer->surface_relocs.num_relocs; i++)
> - cmd_buffer->surface_relocs.relocs[i].presumed_offset = -1;
> - }
> + /* If we were able to successfully relocate everything, tell the kernel
> + * that it can skip doing relocations. The requirement for using
> + * NO_RELOC is:
> + *
> + * 1) The addresses written in the objects must match the
> corresponding
> + * reloc.presumed_offset which in turn must match the corresponding
> + * execobject.offset.
> + *
> + * 2) To avoid stalling, execobject.offset should match the current
> + * address of that object within the active context.
> + *
> + * In order to satisfy all of the invariants that make userspace
> + * relocations to be safe (see relocate_cmd_buffer()), we need to
> + * further ensure that the addresses we use match those used by the
> + * kernel for the most recent execbuf2.
> + *
> + * The kernel may still choose to do relocations anyway if something
> has
> + * moved in the GTT. In this case, the relocation list still needs to
> be
> + * valid. All relocations on the batch buffers are already valid and
> + * kept up-to-date. For surface state relocations, by applying the
> + * relocations in relocate_cmd_buffer, we ensured that the address in
> + * the RENDER_SURFACE_STATE matches presumed_offset, so it should be
> + * safe for the kernel to relocate them as needed.
> + */
> + relocate_cmd_buffer(cmd_buffer, execbuf);
>
> return VK_SUCCESS;
> }
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> index 21d0ac2122..f9381fb8eb 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -400,7 +400,7 @@ anv_bo_init(struct anv_bo *bo, uint32_t gem_handle,
> uint64_t size)
> {
> bo->gem_handle = gem_handle;
> bo->index = 0;
> - bo->offset = -1;
> + bo->offset = 0;
> bo->size = size;
> bo->map = NULL;
> bo->flags = 0;
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
