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

[Chris Wilson]

On Wed, Mar 29, 2017 at 08:36:36AM -0700, Jason Ekstrand wrote:
>    On Wed, Mar 29, 2017 at 1:51 AM, Chris Wilson
>    <[1]chris at chris-wilson.co.uk> wrote:
> 
>      On Tue, Mar 28, 2017 at 05:41:12PM -0700, Jason Ekstrand wrote:
>      > 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.
>      > ---
>      >  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;
> 
>      Don't set bo->supports_48bit_address when
>      !device->instance->physicalDevice.supports_48bit_addresses? My guess is
>      that flagging bo is a rarer task than add_bo(), and it looks like you
>      already have device available in the callers of bo_init(true).
> 
>    I thought a bout making that change right before I sent it but decided I
>    marginally liked this better.  I'm happy to change it if you'd like.

You're also the maintainer, you have to live with it so pick whichever
you find easier to read and less likely to get in the way of future
changes:)

>      > 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.
> 
>      How about just setting the field to the bo->size? You must know the bo
>      already at that point so that you can set the relocation target.
> 
>    Actually, we don't.  We have a pointer to a thing that claims to be a BO
>    but the actual GEM handle and size aren't known until execbuf time.  (Yes,
>    that's a bit weird but there are good reasons for it and it's not likely
>    to change.  When we stop doing relocations, there's a separate plan for
>    how to handle that.)

Hmm. I honestly didn't expect that. Another thing you can do is to use
execobject.size = 4GiB for those buffers. The kernel will then allocate
it 4GiB of space in the GTT, it's feels overkill though. Just limiting
them to the low 4GiB shouldn't be restrictive. I may have to check that
we do allocate those from the bottom -- iirc, we don't require any
special flags for them so they end up in whichever hole was hit first.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre