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

From: Danilo Krummrich
Date: Mon Mar 06 2023 - 08:40:31 EST

On 2/28/23 17:24, Liam R. Howlett wrote:
* Danilo Krummrich <dakr@xxxxxxxxxx> [230227 21:17]:
On Tue, Feb 21, 2023 at 01:20:50PM -0500, Liam R. Howlett wrote:
* Danilo Krummrich <dakr@xxxxxxxxxx> [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@xxxxxxxxxx>
Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
---
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;


For some reason mas_walk() finds an entry and hence this function returns
-EEXIST for the following sequence of insertions.

A = [0xc0000 - 0xfffff]
B = [0x0 - 0xbffff]

Interestingly, inserting B before A works fine.

I attached a test module that reproduces the issue. I hope its just a stupid
mistake I just can't spot though.

This is probably my fault in how I explained things, I seem to have had
a bug in my code.

Let me try again.

mas_walk(&mas) will go to the range of mas.index
It will set mas.index = range_start
It will set mas.last = range_end
It will return entry in that range.

Your code is walking to addr (0xc0000, say)
You get NULL
and the range is now: mas.index = 0, mas.last = ULONG_MAX

You set mas.last = 0xc0000 + 0x40000 -1
You store your va in the range of 0 - 0xfffff - This isn't what you want
to do and this is why you are seeing it exists when done in this order.

In the reverse order, your lower limit is fine so it works out.

Try adding a check to ensure the lower range is still accurate as well:
if (mas.index < addr)
mas.index = addr;


Thanks for the clarification, this indeed fixes the issue.

If you compile with CONFIG_DEBUG_MAPLE_TREE, you can use mt_dump() to
dump the tree for debugging.

That's pretty handy.


I also have some quality of life patches I'm developing to configure the
format of the dump (hex/dec) and a mas_dump() for more information as
well.

Great, I tried it out and really missed a hex option. Are they in any git repository already, such that I could fetch them?



/* 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);