Re: [Mesa-dev] [PATCH 5/9] anv: Add vma_heap allocators in anv_device

[Jason Ekstrand]

On Mon, May 7, 2018 at 9:24 AM, Scott D Phillips  wrote:

> Jason Ekstrand  writes:
>
> > On Wed, May 2, 2018 at 9:01 AM, Scott D Phillips <
> scott.d.phill...@intel.com
> >> wrote:
> >
> >> These will be used to assign virtual addresses to soft pinned
> >> buffers in a later patch.
> >> ---
> >>  src/intel/vulkan/anv_device.c  | 75 ++
> >> 
> >>  src/intel/vulkan/anv_private.h | 11 +++
> >>  2 files changed, 86 insertions(+)
> >>
> >> diff --git a/src/intel/vulkan/anv_device.c
> b/src/intel/vulkan/anv_device.c
> >> index c0cec175826..d3d9c779d62 100644
> >> --- a/src/intel/vulkan/anv_device.c
> >> +++ b/src/intel/vulkan/anv_device.c
>
> ..snip..
>
> >> @@ -1887,6 +1909,59 @@ VkResult anv_DeviceWaitIdle(
> >> return anv_device_submit_simple_batch(device, );
> >>  }
> >>
> >> +bool
> >> +anv_vma_alloc(struct anv_device *device, struct anv_bo *bo)
> >> +{
> >> +   if (!(bo->flags & EXEC_OBJECT_PINNED))
> >> +  return true;
> >>
> >
> > When are things not pinned?
>
> At this point in the series nothing will be pinned. The state pools will
> be pinnned in patch 6 and everything else will be in patch 9. It's not
> safe to take this check away after patch 9 though, we'll still be
> supporting old gens where nothing is pinned.
>

Ok, makes sense.


> >> +
> >> +   pthread_mutex_lock(>vma_mutex);
> >> +
> >> +   bo->offset = 0;
> >> +
> >> +   if (bo->flags & EXEC_OBJECT_SUPPORTS_48B_ADDRESS &&
> >> +   device->vma_hi_available >= bo->size) {
> >> +  uint64_t addr = util_vma_heap_alloc(>vma_hi, bo->size,
> >> 4096);
> >> +  if (addr) {
> >> + bo->offset = canonical_address(addr);
> >> + device->vma_hi_available -= bo->size;
> >> +  }
> >> +   }
> >> +
> >> +   if (bo->offset == 0 && device->vma_lo_available >= bo->size) {
> >> +  uint64_t addr = util_vma_heap_alloc(>vma_lo, bo->size,
> >> 4096);
> >> +  if (addr) {
> >> + bo->offset = canonical_address(addr);
> >> + device->vma_lo_available -= bo->size;
> >> +  }
> >> +   }
> >>
> >
> > I'm not sure how I feel about using EXEC_OBJECT_SUPPORTS_48B_ADDRESS for
> > this.  I think it certainly works but it's not what I had pictured.
>
> What's the concern? It seems like this is exactly what
> SUPPORTS_48B_ADDRESS means, this object can cope with being anywhere in
> the address space.
>

I had an idea in my head that maybe the device would have an array of heaps
and the heaps would have pointers to the allocators or something like
that.  Really, it's mostly cosmetic not at all a big deal.  I'm happy to do
some experimenting of my own after this all lands.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/9] anv: Add vma_heap allocators in anv_device

[Scott D Phillips]

Jason Ekstrand  writes:

> On Wed, May 2, 2018 at 9:01 AM, Scott D Phillips > wrote:
>
>> These will be used to assign virtual addresses to soft pinned
>> buffers in a later patch.
>> ---
>>  src/intel/vulkan/anv_device.c  | 75 ++
>> 
>>  src/intel/vulkan/anv_private.h | 11 +++
>>  2 files changed, 86 insertions(+)
>>
>> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
>> index c0cec175826..d3d9c779d62 100644
>> --- a/src/intel/vulkan/anv_device.c
>> +++ b/src/intel/vulkan/anv_device.c

..snip..

>> @@ -1887,6 +1909,59 @@ VkResult anv_DeviceWaitIdle(
>> return anv_device_submit_simple_batch(device, );
>>  }
>>
>> +bool
>> +anv_vma_alloc(struct anv_device *device, struct anv_bo *bo)
>> +{
>> +   if (!(bo->flags & EXEC_OBJECT_PINNED))
>> +  return true;
>>
>
> When are things not pinned?

At this point in the series nothing will be pinned. The state pools will
be pinnned in patch 6 and everything else will be in patch 9. It's not
safe to take this check away after patch 9 though, we'll still be
supporting old gens where nothing is pinned.

>> +
>> +   pthread_mutex_lock(>vma_mutex);
>> +
>> +   bo->offset = 0;
>> +
>> +   if (bo->flags & EXEC_OBJECT_SUPPORTS_48B_ADDRESS &&
>> +   device->vma_hi_available >= bo->size) {
>> +  uint64_t addr = util_vma_heap_alloc(>vma_hi, bo->size,
>> 4096);
>> +  if (addr) {
>> + bo->offset = canonical_address(addr);
>> + device->vma_hi_available -= bo->size;
>> +  }
>> +   }
>> +
>> +   if (bo->offset == 0 && device->vma_lo_available >= bo->size) {
>> +  uint64_t addr = util_vma_heap_alloc(>vma_lo, bo->size,
>> 4096);
>> +  if (addr) {
>> + bo->offset = canonical_address(addr);
>> + device->vma_lo_available -= bo->size;
>> +  }
>> +   }
>>
>
> I'm not sure how I feel about using EXEC_OBJECT_SUPPORTS_48B_ADDRESS for
> this.  I think it certainly works but it's not what I had pictured.

What's the concern? It seems like this is exactly what
SUPPORTS_48B_ADDRESS means, this object can cope with being anywhere in
the address space.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/9] anv: Add vma_heap allocators in anv_device

[Jason Ekstrand]

On Wed, May 2, 2018 at 9:01 AM, Scott D Phillips  wrote:

> These will be used to assign virtual addresses to soft pinned
> buffers in a later patch.
> ---
>  src/intel/vulkan/anv_device.c  | 75 ++
> 
>  src/intel/vulkan/anv_private.h | 11 +++
>  2 files changed, 86 insertions(+)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index c0cec175826..d3d9c779d62 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -369,6 +369,8 @@ anv_physical_device_init(struct anv_physical_device
> *device,
> device->has_exec_async = anv_gem_get_param(fd,
> I915_PARAM_HAS_EXEC_ASYNC);
> device->has_exec_capture = anv_gem_get_param(fd,
> I915_PARAM_HAS_EXEC_CAPTURE);
> device->has_exec_fence = anv_gem_get_param(fd,
> I915_PARAM_HAS_EXEC_FENCE);
> +   device->has_exec_softpin = anv_gem_get_param(fd,
> I915_PARAM_HAS_EXEC_SOFTPIN)
> +  && device->supports_48bit_addresses;
>

I'd rather we call this something like use_softpin since it isn't just a
"does the kernel have this feature" flag.


> device->has_syncobj = anv_gem_get_param(fd, I915_PARAM_HAS_EXEC_FENCE_
> ARRAY);
> device->has_syncobj_wait = device->has_syncobj &&
>anv_gem_supports_syncobj_wait(fd);
> @@ -1527,6 +1529,26 @@ VkResult anv_CreateDevice(
>goto fail_fd;
> }
>
> +   if (physical_device->has_exec_softpin) {
> +  if (pthread_mutex_init(>vma_mutex, NULL) != 0) {
> + result = vk_error(VK_ERROR_INITIALIZATION_FAILED);
> + goto fail_fd;
> +  }
> +
> +  /* keep the page with address zero out of the allocator */
> +  util_vma_heap_init(>vma_lo, 4096, (1ull << 32) - 2 * 4096);
>

Why are you subtracting 2 * 4096?


> +  device->vma_lo_available =
> + physical_device->memory.heaps[physical_device->memory.heap_count
> - 1].size;
> +
> +  /* Leave the last 4GiB out of the high vma range, so that no state
> base
> +   * address + size can overflow 48 bits.
>

Might be good to have a more detailed comment here or at least reference
the comment in anv_allocator.c that deals with the workaround.


> +   */
> +  util_vma_heap_init(>vma_hi, (1ull << 32) + 4096,
> + (1ull << 48) - 2 * (1ull << 32) - 2 * 4096);
>

Why are you not starting at (1ull << 32)?

If neither of those have a good reason, then we should probably only drop
the bottom and top pages in the entire 48-bit range.


> +  device->vma_hi_available = physical_device->memory.heap_count == 1
> ? 0 :
> + physical_device->memory.heaps[0].size;
> +   }
> +
> /* As per spec, the driver implementation may deny requests to acquire
>  * a priority above the default priority (MEDIUM) if the caller does
> not
>  * have sufficient privileges. In this scenario
> VK_ERROR_NOT_PERMITTED_EXT
> @@ -1887,6 +1909,59 @@ VkResult anv_DeviceWaitIdle(
> return anv_device_submit_simple_batch(device, );
>  }
>
> +bool
> +anv_vma_alloc(struct anv_device *device, struct anv_bo *bo)
> +{
> +   if (!(bo->flags & EXEC_OBJECT_PINNED))
> +  return true;
>

When are things not pinned?


> +
> +   pthread_mutex_lock(>vma_mutex);
> +
> +   bo->offset = 0;
> +
> +   if (bo->flags & EXEC_OBJECT_SUPPORTS_48B_ADDRESS &&
>
+   device->vma_hi_available >= bo->size) {
> +  uint64_t addr = util_vma_heap_alloc(>vma_hi, bo->size,
> 4096);
> +  if (addr) {
> + bo->offset = canonical_address(addr);
> + device->vma_hi_available -= bo->size;
> +  }
> +   }
> +
> +   if (bo->offset == 0 && device->vma_lo_available >= bo->size) {
> +  uint64_t addr = util_vma_heap_alloc(>vma_lo, bo->size,
> 4096);
> +  if (addr) {
> + bo->offset = canonical_address(addr);
> + device->vma_lo_available -= bo->size;
> +  }
> +   }
>

I'm not sure how I feel about using EXEC_OBJECT_SUPPORTS_48B_ADDRESS for
this.  I think it certainly works but it's not what I had pictured.


> +
> +   pthread_mutex_unlock(>vma_mutex);
> +
> +   return bo->offset != 0;
> +}
> +
> +void
> +anv_vma_free(struct anv_device *device, struct anv_bo *bo)
> +{
> +   if (!(bo->flags & EXEC_OBJECT_PINNED))
> +  return;
> +
> +   pthread_mutex_lock(>vma_mutex);
> +
> +   if (bo->offset >= 1ull << 32) {
> +  util_vma_heap_free(>vma_hi, bo->offset, bo->size);
> +  device->vma_hi_available += bo->size;
> +   } else {
> +  util_vma_heap_free(>vma_lo, bo->offset, bo->size);
> +  device->vma_lo_available += bo->size;
> +   }
> +
> +   pthread_mutex_unlock(>vma_mutex);
> +
> +   bo->offset = 0;
> +}
> +
>  VkResult
>  anv_bo_init_new(struct anv_bo *bo, struct anv_device *device, uint64_t
> size)
>  {
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> index 761601d1e37..708c3a540d3 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -49,6 +49,7 @@
>  

Re: [Mesa-dev] [PATCH 5/9] anv: Add vma_heap allocators in anv_device

[Chris Wilson]

Quoting Scott D Phillips (2018-05-02 20:24:01)
> Chris Wilson  writes:
> 
> > Quoting Scott D Phillips (2018-05-02 17:01:05)
> >> +bool
> >> +anv_vma_alloc(struct anv_device *device, struct anv_bo *bo)
> >> +{
> >> +   if (!(bo->flags & EXEC_OBJECT_PINNED))
> >> +  return true;
> >> +
> >> +   pthread_mutex_lock(>vma_mutex);
> >> +
> >> +   bo->offset = 0;
> >
> > So bo are device scoped. There can only be a single vma per bo, so why
> > not store the vma node inside the bo? No extra allocations, no
> > searching in anv_vma_close() (a linear walk!!! Granted you have the
> > excuse of doing a full walk for list validation on top of that).
> 
> ah, I see what you're saying, something like:
> 
> bool util_vma_heap_alloc(struct util_vma_heap *heap, uint64_t size,
>uint64_t alignment, struct util_vma_heap_node *node);
> 
> to place the node in the vma, where node has a list_head and uint64_t
> addr. And then free becomes a few data structure twiddles.

Yes.

> > I guess you don't have much that stresses the vma manager :)
> 
> Right, Jason's advice is that the "good enough" point for vulkan is
> fairly low, where the model is that memory allocations are not happening
> dynamically and are few in number (~100s, I guess). There's lots of room
> for optimization in the allocator, for sure.

I'm just looking at it from the pov of drm_mm which this "replaces" (not
a complete replacement as the kernel still calls into drm_mm to validate
what the user tells us, but only on first binding). The main lesson,
imho, being that allocation and linear lists are terrible.

> The hope is that that's localized behind the allocator interface and
> everything else is fine in the series.

Also a clear API begs for unittests ;)

> > The decision to split low/high ranges rather than have a up/down
> > allocator wants a few words of explanation.
> 
> I suppose I saw that as a six-of-one/half-dozen-of-another type
> thing. It would save having two allocators, but low-memory allocation
> would still be funny with {mem=alloc; if (it wasn't actually low)
> {free;fail;}}. Maybe that logic could get hoisted into the low-side
> allocation function?
> 
> That being said, I don't really mind changing to a double sided
> allocator if there's a good argument in favor.

I don't mind either way, what I care about is that the rationale for
picking one over the other is recorded. Knowing what your expectations
are helps us all live up to them.
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/9] anv: Add vma_heap allocators in anv_device

[Scott D Phillips]

Chris Wilson  writes:

> Quoting Scott D Phillips (2018-05-02 17:01:05)
>> +bool
>> +anv_vma_alloc(struct anv_device *device, struct anv_bo *bo)
>> +{
>> +   if (!(bo->flags & EXEC_OBJECT_PINNED))
>> +  return true;
>> +
>> +   pthread_mutex_lock(>vma_mutex);
>> +
>> +   bo->offset = 0;
>
> So bo are device scoped. There can only be a single vma per bo, so why
> not store the vma node inside the bo? No extra allocations, no
> searching in anv_vma_close() (a linear walk!!! Granted you have the
> excuse of doing a full walk for list validation on top of that).

ah, I see what you're saying, something like:

bool util_vma_heap_alloc(struct util_vma_heap *heap, uint64_t size,
   uint64_t alignment, struct util_vma_heap_node *node);

to place the node in the vma, where node has a list_head and uint64_t
addr. And then free becomes a few data structure twiddles.

> I guess you don't have much that stresses the vma manager :)

Right, Jason's advice is that the "good enough" point for vulkan is
fairly low, where the model is that memory allocations are not happening
dynamically and are few in number (~100s, I guess). There's lots of room
for optimization in the allocator, for sure.

The hope is that that's localized behind the allocator interface and
everything else is fine in the series.

> The decision to split low/high ranges rather than have a up/down
> allocator wants a few words of explanation.

I suppose I saw that as a six-of-one/half-dozen-of-another type
thing. It would save having two allocators, but low-memory allocation
would still be funny with {mem=alloc; if (it wasn't actually low)
{free;fail;}}. Maybe that logic could get hoisted into the low-side
allocation function?

That being said, I don't really mind changing to a double sided
allocator if there's a good argument in favor.

> -Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/9] anv: Add vma_heap allocators in anv_device

[Chris Wilson]

Quoting Scott D Phillips (2018-05-02 17:01:05)
> +bool
> +anv_vma_alloc(struct anv_device *device, struct anv_bo *bo)
> +{
> +   if (!(bo->flags & EXEC_OBJECT_PINNED))
> +  return true;
> +
> +   pthread_mutex_lock(>vma_mutex);
> +
> +   bo->offset = 0;

So bo are device scoped. There can only be a single vma per bo, so why
not store the vma node inside the bo? No extra allocations, no
searching in anv_vma_close() (a linear walk!!! Granted you have the
excuse of doing a full walk for list validation on top of that).

I guess you don't have much that stresses the vma manager :)

The decision to split low/high ranges rather than have a up/down
allocator wants a few words of explanation.
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev