Jason Ekstrand <ja...@jlekstrand.net> 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;
> }
>
> 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;
> +}
> +
> 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.
That makes sense. When I investigated this, I placed the BOs above the
32 bit mark with soft-pin and wasn't able to hang the GPU. I'm pretty
sure that I didn't put them high enough to cause the problematic
overflow. Thanks for finding the root-cause here.
Kristian
> + * 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)
> {
> 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
>
> _______________________________________________
> 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
