<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Mar 30, 2017 at 9:25 AM, Kristian H. Kristensen <<a href="mailto:krh@bitplanet.net" target="_blank">krh@bitplanet.net</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: > This commit adds support for using the full 48-bit address space on > Broadwell and newer hardware. Thanks to certain limitations, not all > objects can be placed above the 32-bit boundary. In particular, general > and state base address need to live within 32 bits. (See also > Wa32bitGeneralStateOffset and Wa32bitInstructionBaseOffset.) In order > to handle this, we add a supports_48bit_address field to anv_bo and only > set EXEC_OBJECT_SUPPORTS_48B_ADDRESS if that bit is set. We set the bit > for all client-allocated memory objects but leave it false for > driver-allocated objects. While this is more conservative than needed, > all driver allocations should easily fit in the first 32 bits of address > space and keeps things simple because we don't have to think about > whether or not any given one of our allocation data structures will be > used in a 48-bit-unsafe way. > --- > src/intel/vulkan/anv_allocator.c | 10 ++++++++-- > src/intel/vulkan/anv_batch_chain.c | 14 ++++++++++---- > src/intel/vulkan/anv_device.c | 4 +++- > src/intel/vulkan/anv_gem.c | 18 ++++++++++++++++++ > src/intel/vulkan/anv_intel.c | 2 +- > src/intel/vulkan/anv_private.h | 29 +++++++++++++++++++++++++++-- > 6 files changed, 67 insertions(+), 10 deletions(-) > > diff --git a/src/intel/vulkan/anv_allocator.c b/src/intel/vulkan/anv_allocator.c > index 45c663b..88c9c13 100644 > --- a/src/intel/vulkan/anv_allocator.c > +++ b/src/intel/vulkan/anv_allocator.c > @@ -255,7 +255,7 @@ anv_block_pool_init(struct anv_block_pool *pool, > assert(util_is_power_of_two(block_size)); > > pool->device = device; > - anv_bo_init(&pool->bo, 0, 0); > + anv_bo_init(&pool->bo, 0, 0, false); > pool->block_size = block_size; > pool->free_list = ANV_FREE_LIST_EMPTY; > pool->back_free_list = ANV_FREE_LIST_EMPTY; > @@ -475,7 +475,13 @@ anv_block_pool_grow(struct anv_block_pool *pool, struct anv_block_state *state) > * values back into pool. */ > pool->map = map + center_bo_offset; > pool->center_bo_offset = center_bo_offset; > - anv_bo_init(&pool->bo, gem_handle, size); > + > + /* Block pool BOs are marked as not supporting 48-bit addresses because > + * they are used to back STATE_BASE_ADDRESS. > + * > + * See also anv_bo::supports_48bit_address. > + */ > + anv_bo_init(&pool->bo, gem_handle, size, false); > pool->bo.map = map; > > done: > diff --git a/src/intel/vulkan/anv_batch_chain.c b/src/intel/vulkan/anv_batch_chain.c > index 5d7abc6..b098e4b 100644 > --- a/src/intel/vulkan/anv_batch_chain.c > +++ b/src/intel/vulkan/anv_batch_chain.c > @@ -979,7 +979,8 @@ anv_execbuf_finish(struct anv_execbuf *exec, > } > > static VkResult > -anv_execbuf_add_bo(struct anv_execbuf *exec, > +anv_execbuf_add_bo(struct anv_device *device, > + struct anv_execbuf *exec, > struct anv_bo *bo, > struct anv_reloc_list *relocs, > const VkAllocationCallbacks *alloc) > @@ -1039,6 +1040,10 @@ anv_execbuf_add_bo(struct anv_execbuf *exec, > obj->flags = bo->is_winsys_bo ? EXEC_OBJECT_WRITE : 0; > obj->rsvd1 = 0; > obj->rsvd2 = 0; > + > + if (device->instance->physicalDevice.supports_48bit_addresses && > + bo->supports_48bit_address) > + obj->flags |= EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > } </div></div>Looking at the pointer chasing to get to supports_48bit_address and how you had to add an anv_device argument to this entire callchain, I'd suggest rolling bo->is_winsys_bo and bo->supports_48bit_address into a bo->flags field where you can set EXEC_OBJECT_SUPPORTS_48B_ADDRESS and EXEC_OBJECT_WRITE as needed at bo create time. All you need to do then is obj->flags = bo->flags and all these computations and conditionals move out of the hot path and you can drop the anv_device argument again. For the case where we set the render write domain for winsys buffers, you'll have to test if the EXEC_OBJECT_WRITE flag is set instead of testing if bo->is_winsys_bo is true. </blockquote><div> </div><div>In an earlier version of the series, I did exactly that. :-) I'm happy to change it back. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > if (relocs != NULL && obj->relocation_count == 0) { > @@ -1052,7 +1057,7 @@ anv_execbuf_add_bo(struct anv_execbuf *exec, > for (size_t i = 0; i < relocs->num_relocs; i++) { > /* A quick sanity check on relocations */ > assert(relocs->relocs[i].offset < bo->size); > - anv_execbuf_add_bo(exec, relocs->reloc_bos[i], NULL, alloc); > + anv_execbuf_add_bo(device, exec, relocs->reloc_bos[i], NULL, alloc); > } > } > > @@ -1264,7 +1269,8 @@ anv_cmd_buffer_execbuf(struct anv_device *device, > adjust_relocations_from_state_pool(ss_pool, &cmd_buffer->surface_relocs, > cmd_buffer->last_ss_pool_center); > VkResult result = > - anv_execbuf_add_bo(&execbuf, &ss_pool->bo, &cmd_buffer->surface_relocs, > + anv_execbuf_add_bo(device, &execbuf, &ss_pool->bo, > + &cmd_buffer->surface_relocs, > &cmd_buffer->pool->alloc); > if (result != VK_SUCCESS) > return result; > @@ -1277,7 +1283,7 @@ anv_cmd_buffer_execbuf(struct anv_device *device, > adjust_relocations_to_state_pool(ss_pool, &(*bbo)->bo, &(*bbo)->relocs, > cmd_buffer->last_ss_pool_center); > > - anv_execbuf_add_bo(&execbuf, &(*bbo)->bo, &(*bbo)->relocs, > + anv_execbuf_add_bo(device, &execbuf, &(*bbo)->bo, &(*bbo)->relocs, > &cmd_buffer->pool->alloc); > } > > diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c > index 4e4fa19..f9d04ee 100644 > --- a/src/intel/vulkan/anv_device.c > +++ b/src/intel/vulkan/anv_device.c > @@ -149,6 +149,8 @@ anv_physical_device_init(struct anv_physical_device *device, > goto fail; > } > > + device->supports_48bit_addresses = anv_gem_supports_48b_addresses(fd); > + > if (!anv_device_get_cache_uuid(device->uuid)) { > result = vk_errorf(VK_ERROR_INITIALIZATION_FAILED, > "cannot generate UUID"); > @@ -1396,7 +1398,7 @@ anv_bo_init_new(struct anv_bo *bo, struct anv_device *device, uint64_t size) > if (!gem_handle) > return vk_error(VK_ERROR_OUT_OF_DEVICE_MEMORY); > > - anv_bo_init(bo, gem_handle, size); > + anv_bo_init(bo, gem_handle, size, true); > > return VK_SUCCESS; > } > diff --git a/src/intel/vulkan/anv_gem.c b/src/intel/vulkan/anv_gem.c > index 0dde6d9..3d45243 100644 > --- a/src/intel/vulkan/anv_gem.c > +++ b/src/intel/vulkan/anv_gem.c > @@ -301,6 +301,24 @@ anv_gem_get_aperture(int fd, uint64_t *size) > return 0; > } > > +bool > +anv_gem_supports_48b_addresses(int fd) > +{ > + struct drm_i915_gem_exec_object2 obj = { > + .flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS, > + }; > + > + struct drm_i915_gem_execbuffer2 execbuf = { > + .buffers_ptr = (uintptr_t)&obj, > + .buffer_count = 1, > + .rsvd1 = 0xffffffu, > + }; > + > + int ret = anv_ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf); > + > + return ret == -1 && errno == ENOENT; </div></div>Without looking at the kernel code, I'm assuming the kernel will return ENOENT because of rsvd1 (hw context) being 0xffffffu, but return EINVAL before that if the EXEC_OBJECT_SUPPORTS_48B_ADDRESS flag isn't supported. That probably needs a comment though. </blockquote><div> </div><div>I'll look at the kernel code. I just did what Chris told me to do and it seems to work. :-) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > +} > + > int > anv_gem_handle_to_fd(struct anv_device *device, uint32_t gem_handle) > { > diff --git a/src/intel/vulkan/anv_intel.c b/src/intel/vulkan/anv_intel.c > index c356e84..d97fbf7 100644 > --- a/src/intel/vulkan/anv_intel.c > +++ b/src/intel/vulkan/anv_intel.c > @@ -57,7 +57,7 @@ VkResult anv_CreateDmaBufImageINTEL( > > uint64_t size = (uint64_t)pCreateInfo->strideInBytes * pCreateInfo->extent.height; > > - anv_bo_init(&mem->bo, gem_handle, size); > + anv_bo_init(&mem->bo, gem_handle, size, true); > > anv_image_create(_device, > &(struct anv_image_create_info) { > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h > index 27c887c..425e376 100644 > --- a/src/intel/vulkan/anv_private.h > +++ b/src/intel/vulkan/anv_private.h > @@ -299,11 +299,34 @@ struct anv_bo { > * writing to them and synchronize uses on other rings (eg if the display > * server uses the blitter ring). > */ > - bool is_winsys_bo; > + bool is_winsys_bo:1; > + > + /* Whether or not this BO supports having a 48-bit address. Not all > + * buffers support arbitrary 48-bit addresses. In particular, we need to > + * be careful with general and instruction state buffers because we set the > + * size in STATE_BASE_ADDRESS to 0xfffff (the maximum) even though the BO > + * is most likely significantly smaller. If we let the kernel place it > + * anywhere it wants, it will default to placing it as high up the address > + * space as possible, the range specified by STATE_BASE_ADDRESS will > + * over-flow the 48-bit address range, and the GPU will hang. In order to > + * avoid this problem, we tell the kernel that the buffer does not support > + * 48-bit addresses, and it places the buffer at a 32-bit address. While > + * this solution is probably overkill, it is effective. > + * > + * There are two documented workarounds for this: Wa32bitGeneralStateOffset > + * and Wa32bitInstructionBaseOffset which state that those two base > + * addresses do not support 48-bit addresses. Empirical evidence, however, > + * contradicts this and supports the explanation above. > + * > + * If the kernel or hardware does not support 48-bit addresses, this field > + * is ignored. > + */ > + bool supports_48bit_address:1; > }; > > static inline void > -anv_bo_init(struct anv_bo *bo, uint32_t gem_handle, uint64_t size) > +anv_bo_init(struct anv_bo *bo, uint32_t gem_handle, uint64_t size, > + bool supports_48bit_address) </div></div>I'd be inclined to not take supports_48bit_address as an argument here and just default it to off. Then in the two places where you need to set it, like we do for is_winsys_bo. </blockquote><div> </div><div>That's reasonable. I'll change it to flags and make flags default to 0. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> At your discretion, Reviewed-by: Kristian H. Kristensen <<a href="mailto:krh@bitplanet.net">krh@bitplanet.net</a>> > { > bo->gem_handle = gem_handle; > bo->index = 0; > @@ -311,6 +334,7 @@ anv_bo_init(struct anv_bo *bo, uint32_t gem_handle, uint64_t size) > bo->size = size; > bo->map = NULL; > bo->is_winsys_bo = false; > + bo->supports_48bit_address = supports_48bit_address; > } > > /* Represents a lock-free linked list of "free" things. This is used by > @@ -654,6 +678,7 @@ int anv_gem_destroy_context(struct anv_device *device, int context); > int anv_gem_get_param(int fd, uint32_t param); > bool anv_gem_get_bit6_swizzle(int fd, uint32_t tiling); > int anv_gem_get_aperture(int fd, uint64_t *size); > +bool anv_gem_supports_48b_addresses(int fd); > int anv_gem_handle_to_fd(struct anv_device *device, uint32_t gem_handle); > uint32_t anv_gem_fd_to_handle(struct anv_device *device, int fd); > int anv_gem_set_caching(struct anv_device *device, uint32_t gem_handle, uint32_t caching); > -- > 2.5.0.400.gff86faf > <div class="HOEnZb"><div class="h5">> _______________________________________________ > 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> </div></div></blockquote></div> </div></div>