<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 31, 2018 at 2:21 PM, Scott D Phillips <<a href="mailto:scott.d.phillips@intel.com" target="_blank">scott.d.phillips@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes: > --- > src/intel/vulkan/anv_allocator.c | 54 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 53 insertions(+), 1 deletion(-) > > diff --git a/src/intel/vulkan/anv_allocator.c b/src/intel/vulkan/anv_allocator.c > index 697da5f..f18e015 100644 > --- a/src/intel/vulkan/anv_allocator.c > +++ b/src/intel/vulkan/anv_allocator.c > @@ -1240,7 +1240,8 @@ anv_bo_cache_lookup(struct anv_bo_cache *cache, uint32_t gem_handle) > #define ANV_BO_CACHE_SUPPORTED_FLAGS \ > (EXEC_OBJECT_WRITE | \ > EXEC_OBJECT_ASYNC | \ > - EXEC_OBJECT_SUPPORTS_48B_ADDRESS) > + EXEC_OBJECT_SUPPORTS_48B_ADDRESS | \ > + EXEC_OBJECT_PINNED) > > VkResult > anv_bo_cache_alloc(struct anv_device *device, > @@ -1269,6 +1270,14 @@ anv_bo_cache_alloc(struct anv_device *device, > > bo->bo.flags = bo_flags; > > + if (!anv_vma_alloc(device, &bo->bo)) { > + anv_gem_close(device, bo->bo.gem_handle); > + vk_free(&device->alloc, bo); > + return vk_errorf(device->instance, NULL, > + VK_ERROR_OUT_OF_DEVICE_MEMORY, > + "failed to allocate virtual address for BO"); > + } > + > assert(bo->bo.gem_handle); > > pthread_mutex_lock(&cache->mutex); > @@ -1310,6 +1319,38 @@ anv_bo_cache_import(struct anv_device *device, > new_flags |= (bo->bo.flags | bo_flags) & EXEC_OBJECT_WRITE; > new_flags |= (bo->bo.flags & bo_flags) & EXEC_OBJECT_ASYNC; > new_flags |= (bo->bo.flags & bo_flags) & EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > + new_flags |= (bo->bo.flags | bo_flags) & EXEC_OBJECT_PINNED; > + > + /* It's theoretically possible for a BO to get imported such that it's > + * both pinned and not pinned. The only way this can happen is if it > + * gets imported as both a semaphore and a memory object and that would > + * be an application error. Just fail out in that case. > + */ > + if ((bo->bo.flags & EXEC_OBJECT_PINNED) != > + (bo_flags & EXEC_OBJECT_PINNED)) { > + pthread_mutex_unlock(&cache->mutex); > + return vk_errorf(device->instance, NULL, > + VK_ERROR_INVALID_EXTERNAL_HANDLE, > + "The same BO was imported two different ways"); > + } > + > + /* It's also theoretically possible that someone could export a BO from > + * one heap and import it into another or to import the same BO into two > + * different heaps. If this happens, we could potentially end up both > + * allowing and disallowing 48-bit addresses. There's not much we can > + * do about it if we're pinning so we just throw a warning and hope no > + * app is actually that stupid. > + */ > + if ((new_flags & EXEC_OBJECT_PINNED) && > + (bo->bo.flags & EXEC_OBJECT_SUPPORTS_48B_ADDRESS) && > + !(bo_flags & EXEC_OBJECT_SUPPORTS_48B_ADDRESS)) { > + vk_debug_report(&device->instance->debug_report_callbacks, > + VK_DEBUG_REPORT_WARNING_BIT_EXT, > + VK_DEBUG_REPORT_OBJECT_TYPE_DEVICE_EXT, > + (uint64_t) (uintptr_t) device, > + 0, 0, "anv", > + "Import of the same BO on multiple heaps"); > + } </div></div>Hmm, it seems like we should error here too. If the BO is getting used in a way that doesn't support 48 bit addresses then at best we can hit one of our known iffy cases, and at worst we might cram a 64 bit address into a narrower offset field and then possibly get GPU hangs. Or if there is no such case like this then I think it probably needs some more explanation about why this isn't an error. </blockquote></div></div><div class="gmail_extra"> </div><div class="gmail_extra">I think the worst case is that someone will try to make a vertex buffer out of it and maybe get cache corruption. I think that case can theoretically lead to hangs (if it's used for the gpu_memcpy of surface states) but it's pretty hard to hit. I think leaving it as a warning is probably ok. </div></div>