[Mesa-dev] [PATCH v2 1/2] anv: Add support for 48-bit addresses

Kristian H. Kristensen krh at bitplanet.net
Thu Mar 30 16:25:29 UTC 2017


Jason Ekstrand <jason at 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;
>     }

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.

>     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;

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.

> +}
> +
>  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)

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.

At your discretion,

Reviewed-by: Kristian H. Kristensen <krh at bitplanet.net>

>  {
>     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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list