[Mesa-dev] [PATCH v2 1/2] anv: Add support for 48-bit addresses
Jason Ekstrand
jason at jlekstrand.net
Thu Mar 30 18:39:37 UTC 2017
On Thu, Mar 30, 2017 at 9:25 AM, Kristian H. Kristensen <krh at bitplanet.net>
wrote:
> 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.
>
In an earlier version of the series, I did exactly that. :-) I'm happy to
change it back.
> > 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.
>
I'll look at the kernel code. I just did what Chris told me to do and it
seems to work. :-)
> > +}
> > +
> > 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.
>
That's reasonable. I'll change it to flags and make flags default to 0.
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170330/4818fbb8/attachment-0001.html>
More information about the mesa-dev
mailing list