Re: [PATCH drm-next v2 05/16] drm: manager to keep track of GPUs VA mappings

Liam R. Howlett
2023-02-21T18:21:41Z

* Danilo Krummrich <dakr@redhat.com> [230217 08:45]:
> Add infrastructure to keep track of GPU virtual address (VA) mappings
> with a decicated VA space manager implementation.
>
> New UAPIs, motivated by Vulkan sparse memory bindings graphics drivers
> start implementing, allow userspace applications to request multiple and
> arbitrary GPU VA mappings of buffer objects. The DRM GPU VA manager is
> intended to serve the following purposes in this context.
>
> 1) Provide infrastructure to track GPU VA allocations and mappings,
> making use of the maple_tree.
>
> 2) Generically connect GPU VA mappings to their backing buffers, in
> particular DRM GEM objects.
>
> 3) Provide a common implementation to perform more complex mapping
> operations on the GPU VA space. In particular splitting and merging
> of GPU VA mappings, e.g. for intersecting mapping requests or partial
> unmap requests.
>
> Suggested-by: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
> Documentation/gpu/drm-mm.rst | 31 +
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/drm_gem.c | 3 +
> drivers/gpu/drm/drm_gpuva_mgr.c | 1704 +++++++++++++++++++++++++++++++
> include/drm/drm_drv.h | 6 +
> include/drm/drm_gem.h | 75 ++
> include/drm/drm_gpuva_mgr.h | 714 +++++++++++++
> 7 files changed, 2534 insertions(+)
> create mode 100644 drivers/gpu/drm/drm_gpuva_mgr.c
> create mode 100644 include/drm/drm_gpuva_mgr.h
>
> diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
> index a52e6f4117d6..c9f120cfe730 100644
> --- a/Documentation/gpu/drm-mm.rst
> +++ b/Documentation/gpu/drm-mm.rst
> @@ -466,6 +466,37 @@ DRM MM Range Allocator Function References
> .. kernel-doc:: drivers/gpu/drm/drm_mm.c
> :export:
>
...

> +
> +/**
> + * drm_gpuva_remove_iter - removes the iterators current element
> + * @it: the &drm_gpuva_iterator
> + *
> + * This removes the element the iterator currently points to.
> + */
> +void
> +drm_gpuva_iter_remove(struct drm_gpuva_iterator *it)
> +{
> + mas_erase(&it->mas);
> +}
> +EXPORT_SYMBOL(drm_gpuva_iter_remove);
> +
> +/**
> + * drm_gpuva_insert - insert a &drm_gpuva
> + * @mgr: the &drm_gpuva_manager to insert the &drm_gpuva in
> + * @va: the &drm_gpuva to insert
> + * @addr: the start address of the GPU VA
> + * @range: the range of the GPU VA
> + *
> + * Insert a &drm_gpuva with a given address and range into a
> + * &drm_gpuva_manager.
> + *
> + * Returns: 0 on success, negative error code on failure.
> + */
> +int
> +drm_gpuva_insert(struct drm_gpuva_manager *mgr,
> + struct drm_gpuva *va)
> +{
> + u64 addr = va->va.addr;
> + u64 range = va->va.range;
> + MA_STATE(mas, &mgr->va_mt, addr, addr + range - 1);
> + struct drm_gpuva_region *reg = NULL;
> + int ret;
> +
> + if (unlikely(!drm_gpuva_in_mm_range(mgr, addr, range)))
> + return -EINVAL;
> +
> + if (unlikely(drm_gpuva_in_kernel_region(mgr, addr, range)))
> + return -EINVAL;
> +
> + if (mgr->flags & DRM_GPUVA_MANAGER_REGIONS) {
> + reg = drm_gpuva_in_region(mgr, addr, range);
> + if (unlikely(!reg))
> + return -EINVAL;
> + }
> +

-----

> + if (unlikely(drm_gpuva_find_first(mgr, addr, range)))
> + return -EEXIST;
> +
> + ret = mas_store_gfp(&mas, va, GFP_KERNEL);

mas_walk() will set the internal maple state to the limits to what it
finds. So, instead of an iterator, you can use the walk function and
ensure there is a large enough area in the existing NULL:

/*
* Nothing at addr, mas now points to the location where the store would
* happen
*/
if (mas_walk(&mas))
return -EEXIST;

/* The NULL entry ends at mas.last, make sure there is room */
if (mas.last < (addr + range - 1))
return -EEXIST;

/* Limit the store size to the correct end address, and store */
mas.last = addr + range - 1;
ret = mas_store_gfp(&mas, va, GFP_KERNEL);

> + if (unlikely(ret))
> + return ret;
> +
> + va->mgr = mgr;
> + va->region = reg;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_gpuva_insert);
> +
> +/**
> + * drm_gpuva_remove - remove a &drm_gpuva
> + * @va: the &drm_gpuva to remove
> + *
> + * This removes the given &va from the underlaying tree.
> + */
> +void
> +drm_gpuva_remove(struct drm_gpuva *va)
> +{
> + MA_STATE(mas, &va->mgr->va_mt, va->va.addr, 0);
> +
> + mas_erase(&mas);
> +}
> +EXPORT_SYMBOL(drm_gpuva_remove);
> +
...

> +/**
> + * drm_gpuva_find_first - find the first &drm_gpuva in the given range
> + * @mgr: the &drm_gpuva_manager to search in
> + * @addr: the &drm_gpuvas address
> + * @range: the &drm_gpuvas range
> + *
> + * Returns: the first &drm_gpuva within the given range
> + */
> +struct drm_gpuva *
> +drm_gpuva_find_first(struct drm_gpuva_manager *mgr,
> + u64 addr, u64 range)
> +{
> + MA_STATE(mas, &mgr->va_mt, addr, 0);
> +
> + return mas_find(&mas, addr + range - 1);
> +}
> +EXPORT_SYMBOL(drm_gpuva_find_first);
> +
> +/**
> + * drm_gpuva_find - find a &drm_gpuva
> + * @mgr: the &drm_gpuva_manager to search in
> + * @addr: the &drm_gpuvas address
> + * @range: the &drm_gpuvas range
> + *
> + * Returns: the &drm_gpuva at a given &addr and with a given &range

Note that mas_find() will continue upwards in the address space if there
isn't anything at @addr. This means that &drm_gpuva may not be at
&addr. If you want to check just at &addr, use mas_walk().

> + */
> +struct drm_gpuva *
> +drm_gpuva_find(struct drm_gpuva_manager *mgr,
> + u64 addr, u64 range)
> +{
> + struct drm_gpuva *va;
> +
> + va = drm_gpuva_find_first(mgr, addr, range);
> + if (!va)
> + goto out;
> +
> + if (va->va.range != range)
> + goto out;
> +
> + return va;
> +
> +out:
> + return NULL;
> +}
> +EXPORT_SYMBOL(drm_gpuva_find);
> +
> +/**
> + * drm_gpuva_find_prev - find the &drm_gpuva before the given address
> + * @mgr: the &drm_gpuva_manager to search in
> + * @start: the given GPU VA's start address
> + *
> + * Find the adjacent &drm_gpuva before the GPU VA with given &start address.
> + *
> + * Note that if there is any free space between the GPU VA mappings no mapping
> + * is returned.
> + *
> + * Returns: a pointer to the found &drm_gpuva or NULL if none was found
> + */
> +struct drm_gpuva *
> +drm_gpuva_find_prev(struct drm_gpuva_manager *mgr, u64 start)

find_prev() usually continues beyond 1 less than the address. I found
this name confusing. You may as well use mas_walk(), it would be
faster.

> +{
> + MA_STATE(mas, &mgr->va_mt, start, 0);
> +
> + if (start <= mgr->mm_start ||
> + start > (mgr->mm_start + mgr->mm_range))
> + return NULL;
> +
> + return mas_prev(&mas, start - 1);
> +}
> +EXPORT_SYMBOL(drm_gpuva_find_prev);
> +
> +/**
> + * drm_gpuva_find_next - find the &drm_gpuva after the given address
> + * @mgr: the &drm_gpuva_manager to search in
> + * @end: the given GPU VA's end address
> + *
> + * Find the adjacent &drm_gpuva after the GPU VA with given &end address.
> + *
> + * Note that if there is any free space between the GPU VA mappings no mapping
> + * is returned.
> + *
> + * Returns: a pointer to the found &drm_gpuva or NULL if none was found
> + */
> +struct drm_gpuva *
> +drm_gpuva_find_next(struct drm_gpuva_manager *mgr, u64 end)

This name is also a bit confusing for the same reason. Again, it seems
worth just walking to end here.

> +{
> + MA_STATE(mas, &mgr->va_mt, end - 1, 0);
> +
> + if (end < mgr->mm_start ||
> + end >= (mgr->mm_start + mgr->mm_range))
> + return NULL;
> +
> + return mas_next(&mas, end);
> +}
> +EXPORT_SYMBOL(drm_gpuva_find_next);
> +
> +static int
> +__drm_gpuva_region_insert(struct drm_gpuva_manager *mgr,
> + struct drm_gpuva_region *reg)
> +{
> + u64 addr = reg->va.addr;
> + u64 range = reg->va.range;
> + MA_STATE(mas, &mgr->region_mt, addr, addr + range - 1);
> + int ret;
> +
> + if (unlikely(!drm_gpuva_in_mm_range(mgr, addr, range)))
> + return -EINVAL;
> +
> + ret = mas_store_gfp(&mas, reg, GFP_KERNEL);
> + if (unlikely(ret))
> + return ret;
> +
> + reg->mgr = mgr;
> +
> + return 0;
> +}
> +
> +/**
> + * drm_gpuva_region_insert - insert a &drm_gpuva_region
> + * @mgr: the &drm_gpuva_manager to insert the &drm_gpuva in
> + * @reg: the &drm_gpuva_region to insert
> + * @addr: the start address of the GPU VA
> + * @range: the range of the GPU VA
> + *
> + * Insert a &drm_gpuva_region with a given address and range into a
> + * &drm_gpuva_manager.
> + *
> + * Returns: 0 on success, negative error code on failure.
> + */
> +int
> +drm_gpuva_region_insert(struct drm_gpuva_manager *mgr,
> + struct drm_gpuva_region *reg)
> +{
> + if (unlikely(!(mgr->flags & DRM_GPUVA_MANAGER_REGIONS)))
> + return -EINVAL;
> +
> + return __drm_gpuva_region_insert(mgr, reg);
> +}
> +EXPORT_SYMBOL(drm_gpuva_region_insert);
> +
> +static void
> +__drm_gpuva_region_remove(struct drm_gpuva_region *reg)
> +{
> + struct drm_gpuva_manager *mgr = reg->mgr;
> + MA_STATE(mas, &mgr->region_mt, reg->va.addr, 0);
> +
> + mas_erase(&mas);
> +}
> +
> +/**
> + * drm_gpuva_region_remove - remove a &drm_gpuva_region
> + * @reg: the &drm_gpuva to remove
> + *
> + * This removes the given &reg from the underlaying tree.
> + */
> +void
> +drm_gpuva_region_remove(struct drm_gpuva_region *reg)
> +{
> + struct drm_gpuva_manager *mgr = reg->mgr;
> +
> + if (unlikely(!(mgr->flags & DRM_GPUVA_MANAGER_REGIONS)))
> + return;
> +
> + if (unlikely(reg == &mgr->kernel_alloc_region)) {
> + WARN(1, "Can't destroy kernel reserved region.\n");
> + return;
> + }
> +
> + if (unlikely(!drm_gpuva_region_empty(reg)))
> + WARN(1, "GPU VA region should be empty on destroy.\n");
> +
> + __drm_gpuva_region_remove(reg);
> +}
> +EXPORT_SYMBOL(drm_gpuva_region_remove);
> +
> +/**
> + * drm_gpuva_region_empty - indicate whether a &drm_gpuva_region is empty
> + * @reg: the &drm_gpuva to destroy
> + *
> + * Returns: true if the &drm_gpuva_region is empty, false otherwise
> + */
> +bool
> +drm_gpuva_region_empty(struct drm_gpuva_region *reg)
> +{
> + DRM_GPUVA_ITER(it, reg->mgr);
> +
> + drm_gpuva_iter_for_each_range(it, reg->va.addr,
> + reg->va.addr +
> + reg->va.range)
> + return false;
> +
> + return true;
> +}
> +EXPORT_SYMBOL(drm_gpuva_region_empty);
> +
> +/**
> + * drm_gpuva_region_find_first - find the first &drm_gpuva_region in the given
> + * range
> + * @mgr: the &drm_gpuva_manager to search in
> + * @addr: the &drm_gpuva_regions address
> + * @range: the &drm_gpuva_regions range
> + *
> + * Returns: the first &drm_gpuva_region within the given range
> + */
> +struct drm_gpuva_region *
> +drm_gpuva_region_find_first(struct drm_gpuva_manager *mgr,
> + u64 addr, u64 range)
> +{
> + MA_STATE(mas, &mgr->region_mt, addr, 0);
> +
> + return mas_find(&mas, addr + range - 1);
> +}
> +EXPORT_SYMBOL(drm_gpuva_region_find_first);
> +
> +/**
> + * drm_gpuva_region_find - find a &drm_gpuva_region
> + * @mgr: the &drm_gpuva_manager to search in
> + * @addr: the &drm_gpuva_regions address
> + * @range: the &drm_gpuva_regions range
> + *
> + * Returns: the &drm_gpuva_region at a given &addr and with a given &range

again, I'm not sure you want to find first or walk here.. It sounds like
you want exactly addr to addr + range VMA?

> + */
> +struct drm_gpuva_region *
> +drm_gpuva_region_find(struct drm_gpuva_manager *mgr,
> + u64 addr, u64 range)
> +{
> + struct drm_gpuva_region *reg;
> +
> + reg = drm_gpuva_region_find_first(mgr, addr, range);
> + if (!reg)
> + goto out;
> +
> + if (reg->va.range != range)
> + goto out;
> +
> + return reg;
> +
> +out:
> + return NULL;
> +}
> +EXPORT_SYMBOL(drm_gpuva_region_find);
> +

...