<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, May 5, 2017 at 5:09 AM, Chris Wilson <<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. </blockquote><div> </div><div>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. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Signed-off-by: Chris Wilson <<a href="mailto:chris@chris-wilson.co.uk">chris@chris-wilson.co.uk</a>> Cc: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> --- 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>