Re: [Nouveau] [PATCH drm-next v4 14/14] drm/nouveau: debugfs: implement DRM GPU VA debugfs

[Liam R. Howlett]

* Danilo Krummrich  [230606 18:32]:
> Provide the driver indirection iterating over all DRM GPU VA spaces to
> enable the common 'gpuvas' debugfs file for dumping DRM GPU VA spaces.
> 
> Signed-off-by: Danilo Krummrich 
> ---
>  drivers/gpu/drm/nouveau/nouveau_debugfs.c | 39 +++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_debugfs.c 
> b/drivers/gpu/drm/nouveau/nouveau_debugfs.c
> index 99d022a91afc..053f703f2f68 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_debugfs.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_debugfs.c
> @@ -203,6 +203,44 @@ nouveau_debugfs_pstate_open(struct inode *inode, struct 
> file *file)
>   return single_open(file, nouveau_debugfs_pstate_get, inode->i_private);
>  }
>  
> +static void
> +nouveau_debugfs_gpuva_regions(struct seq_file *m, struct nouveau_uvmm *uvmm)
> +{
> + MA_STATE(mas, >region_mt, 0, 0);
> + struct nouveau_uvma_region *reg;
> +
> + seq_puts  (m, " VA regions  | start  | range  | 
> end\n");
> + seq_puts  (m, 
> "\n");

rcu_read_lock();

> + mas_for_each(, reg, ULONG_MAX)
> + seq_printf(m, " | 0x%016llx | 0x%016llx | 
> 0x%016llx\n",
> +reg->va.addr, reg->va.range, reg->va.addr + 
> reg->va.range);

rcu_read_unlock();

> +}
> +
> +static int
> +nouveau_debugfs_gpuva(struct seq_file *m, void *data)
> +{
> + struct drm_info_node *node = (struct drm_info_node *) m->private;
> + struct nouveau_drm *drm = nouveau_drm(node->minor->dev);
> + struct nouveau_cli *cli;
> +
> + mutex_lock(>clients_lock);
> + list_for_each_entry(cli, >clients, head) {
> + struct nouveau_uvmm *uvmm = nouveau_cli_uvmm(cli);
> +
> + if (!uvmm)
> + continue;
> +
> + nouveau_uvmm_lock(uvmm);
> + drm_debugfs_gpuva_info(m, >umgr);
> + seq_puts(m, "\n");
> + nouveau_debugfs_gpuva_regions(m, uvmm);
> + nouveau_uvmm_unlock(uvmm);
> + }
> + mutex_unlock(>clients_lock);
> +
> + return 0;
> +}
> +
>  static const struct file_operations nouveau_pstate_fops = {
>   .owner = THIS_MODULE,
>   .open = nouveau_debugfs_pstate_open,
> @@ -214,6 +252,7 @@ static const struct file_operations nouveau_pstate_fops = 
> {
>  static struct drm_info_list nouveau_debugfs_list[] = {
>   { "vbios.rom",  nouveau_debugfs_vbios_image, 0, NULL },
>   { "strap_peek", nouveau_debugfs_strap_peek, 0, NULL },
> + DRM_DEBUGFS_GPUVA_INFO(nouveau_debugfs_gpuva, NULL),
>  };
>  #define NOUVEAU_DEBUGFS_ENTRIES ARRAY_SIZE(nouveau_debugfs_list)
>  
> -- 
> 2.40.1
> 

Re: [Nouveau] [PATCH drm-next v4 03/14] drm: manager to keep track of GPUs VA mappings

[Liam R. Howlett]

* Danilo Krummrich  [230606 18:32]:
> 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 
> Signed-off-by: Danilo Krummrich 
> ---
>  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 | 1687 +++
>  include/drm/drm_drv.h   |6 +
>  include/drm/drm_gem.h   |   75 ++
>  include/drm/drm_gpuva_mgr.h |  681 +
>  7 files changed, 2484 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 GPU VA Manager
> +==
> +
> +Overview
> +
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gpuva_mgr.c
> +   :doc: Overview
> +
> +Split and Merge
> +---
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gpuva_mgr.c
> +   :doc: Split and Merge
> +
> +Locking
> +---
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gpuva_mgr.c
> +   :doc: Locking
> +
> +
> +DRM GPU VA Manager Function References
> +--
> +
> +.. kernel-doc:: include/drm/drm_gpuva_mgr.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gpuva_mgr.c
> +   :export:
> +
>  DRM Buddy Allocator
>  ===
>  
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9c6446eb3c83..8eeed446a078 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -45,6 +45,7 @@ drm-y := \
>   drm_vblank.o \
>   drm_vblank_work.o \
>   drm_vma_manager.o \
> + drm_gpuva_mgr.o \
>   drm_writeback.o
>  drm-$(CONFIG_DRM_LEGACY) += \
>   drm_agpsupport.o \
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 1a5a2cd0d4ec..cd878ebddbd0 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -164,6 +164,9 @@ void drm_gem_private_object_init(struct drm_device *dev,
>   if (!obj->resv)
>   obj->resv = >_resv;
>  
> + if (drm_core_check_feature(dev, DRIVER_GEM_GPUVA))
> + drm_gem_gpuva_init(obj);
> +
>   drm_vma_node_reset(>vma_node);
>   INIT_LIST_HEAD(>lru_node);
>  }
> diff --git a/drivers/gpu/drm/drm_gpuva_mgr.c b/drivers/gpu/drm/drm_gpuva_mgr.c
> new file mode 100644
> index ..dd8dd7fef14b
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gpuva_mgr.c
> @@ -0,0 +1,1687 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Red Hat.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Danilo Krummrich 
> + *
> + */
> +
> +#include 
> +#include 
> +
> +/**
> + * DOC: Overview
> + *
> + * The DRM GPU VA Manager, represented by struct drm_gpuva_manager keeps 
> track
> + * of 

Re: [Nouveau] [PATCH drm-next v4 02/14] maple_tree: split up MA_STATE() macro

[Liam R. Howlett]

* Danilo Krummrich  [230606 18:31]:
> Split up the MA_STATE() macro such that components using the maple tree
> can easily inherit from struct ma_state and build custom tree walk
> macros to hide their internals from users.
> 
> Example:
> 
> struct sample_iterator {
>   struct ma_state mas;
>   struct sample_mgr *mgr;
> };
> 
> \#define SAMPLE_ITERATOR(name, __mgr, start)  \
>   struct sample_iterator name = { \
>   .mas = MA_STATE_INIT(&(__mgr)->mt, start, 0),   \
>   .mgr = __mgr,   \
>   }
> 
> \#define sample_iter_for_each_range(it__, entry__, end__) \
>   mas_for_each(&(it__).mas, entry__, end__)
> 
> --
> 
> struct sample *sample;
> SAMPLE_ITERATOR(si, min);
> 
> sample_iter_for_each_range(, sample, max) {
>   frob(mgr, sample);
> }
> 
> Signed-off-by: Danilo Krummrich 

Reviewed-by: Liam R. Howlett 

> ---
>  include/linux/maple_tree.h | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h
> index 1fadb5f5978b..87d55334f1c2 100644
> --- a/include/linux/maple_tree.h
> +++ b/include/linux/maple_tree.h
> @@ -423,8 +423,8 @@ struct ma_wr_state {
>  #define MA_ERROR(err) \
>   ((struct maple_enode *)(((unsigned long)err << 2) | 2UL))
>  
> -#define MA_STATE(name, mt, first, end)   
> \
> - struct ma_state name = {\
> +#define MA_STATE_INIT(mt, first, end)
> \
> + {   \
>   .tree = mt, \
>   .index = first, \
>   .last = end,\
> @@ -435,6 +435,9 @@ struct ma_wr_state {
>   .mas_flags = 0, \
>   }
>  
> +#define MA_STATE(name, mt, first, end)   
> \
> + struct ma_state name = MA_STATE_INIT(mt, first, end)
> +
>  #define MA_WR_STATE(name, ma_state, wr_entry)
> \
>   struct ma_wr_state name = { \
>   .mas = ma_state,\
> -- 
> 2.40.1
> 
> 

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

[Liam R. Howlett]

* Danilo Krummrich  [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 
> Signed-off-by: Danilo Krummrich 
> ---
>  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 _gpuva_iterator
> + *
> + * This removes the element the iterator currently points to.
> + */
> +void
> +drm_gpuva_iter_remove(struct drm_gpuva_iterator *it)
> +{
> + mas_erase(>mas);
> +}
> +EXPORT_SYMBOL(drm_gpuva_iter_remove);
> +
> +/**
> + * drm_gpuva_insert - insert a _gpuva
> + * @mgr: the _gpuva_manager to insert the _gpuva in
> + * @va: the _gpuva to insert
> + * @addr: the start address of the GPU VA
> + * @range: the range of the GPU VA
> + *
> + * Insert a _gpuva with a given address and range into a
> + * _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, >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(, 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())
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(, 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 _gpuva
> + * @va: the _gpuva to remove
> + *
> + * This removes the given  from the underlaying tree.
> + */
> +void
> +drm_gpuva_remove(struct drm_gpuva *va)
> +{
> + MA_STATE(mas, >mgr->va_mt, va->va.addr, 0);
> +
> + mas_erase();
> +}
> +EXPORT_SYMBOL(drm_gpuva_remove);
> +
...

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

Re: [Nouveau] [PATCH drm-next v2 03/16] maple_tree: split up MA_STATE() macro

[Liam R. Howlett]

* Danilo Krummrich  [230217 08:44]:
> Split up the MA_STATE() macro such that components using the maple tree
> can easily inherit from struct ma_state and build custom tree walk
> macros to hide their internals from users.
> 
> Example:
> 
> struct sample_iter {
>   struct ma_state mas;
>   struct sample_mgr *mgr;
>   struct sample_entry *entry;
> };
> 
> \#define SAMPLE_ITER(name, __mgr) \
>   struct sample_iter name = { \
>   .mas = __MA_STATE(&(__mgr)->mt, 0, 0),
>   .mgr = __mgr,
>   .entry = NULL,
>   }

I see this patch is to allow for anonymous maple states, this looks
good.

I've a lengthy comment about the iterator that I'm adding here to head
off anyone that may copy your example below.

> 
> \#define sample_iter_for_each_range(it__, start__, end__) \
>   for ((it__).mas.index = start__, (it__).entry = mas_find(&(it__).mas, 
> end__ - 1); \
>(it__).entry; (it__).entry = mas_find(&(it__).mas, end__ - 1))

I see you've added something like the above in your patch set as well.
I'd like to point out that the index isn't the only state information
that needs to be altered here, and in fact, this could go very wrong.

The maple state has a node and an offset within that node.  If you set
the index to lower than the current position of your iterator and call
mas_find() then what happens is somewhat undefined.  I expect you will
get the wrong value (most likely either the current value or the very
next one that the iterator is already pointing to).  I believe you have
been using a fresh maple state for each iterator in your patches, but I
haven't had a deep look into your code yet.

We have methods of resetting the iterator and set the range (mas_set()
and mas_set_range()) which are safe for what you are doing, but they
will start the walk from the root node to the index again.

So, if you know what you are doing is safe, then the way you have
written it will work, but it's worth mentioning that this could occur.

It is also worth pointing out that it would be much safer to use a
function to do the above so you get type safety.. and I was asked to add
this to the VMA interface by Linus [1], which is on its way upstream [2].

1. 
https://lore.kernel.org/linux-mm/CAHk-=wg9wqxbgkndkd2bqocnn73rdswuwsavbb7t-tekyke...@mail.gmail.com/
2. 
https://lore.kernel.org/linux-mm/20230120162650.984577-1-liam.howl...@oracle.com/

> 
> Signed-off-by: Danilo Krummrich 
> ---
>  include/linux/maple_tree.h | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h
> index e594db58a0f1..ca04c900e51a 100644
> --- a/include/linux/maple_tree.h
> +++ b/include/linux/maple_tree.h
> @@ -424,8 +424,8 @@ struct ma_wr_state {
>  #define MA_ERROR(err) \
>   ((struct maple_enode *)(((unsigned long)err << 2) | 2UL))
>  
> -#define MA_STATE(name, mt, first, end)   
> \
> - struct ma_state name = {\
> +#define __MA_STATE(mt, first, end)   \
> + {   \
>   .tree = mt, \
>   .index = first, \
>   .last = end,\
> @@ -435,6 +435,9 @@ struct ma_wr_state {
>   .alloc = NULL,  \
>   }
>  
> +#define MA_STATE(name, mt, first, end)   
> \
> + struct ma_state name = __MA_STATE(mt, first, end)
> +
>  #define MA_WR_STATE(name, ma_state, wr_entry)
> \
>   struct ma_wr_state name = { \
>   .mas = ma_state,\
> -- 
> 2.39.1
> 

Re: [Nouveau] [PATCH drm-next v2 04/16] maple_tree: add flag MT_FLAGS_LOCK_NONE

[Liam R. Howlett]

* Danilo Krummrich  [230217 08:44]:
> Generic components making use of the maple tree (such as the
> DRM GPUVA Manager) delegate the responsibility of ensuring mutual
> exclusion to their users.
> 
> While such components could inherit the concept of an external lock,
> some users might just serialize the access to the component and hence to
> the internal maple tree.
> 
> In order to allow such use cases, add a new flag MT_FLAGS_LOCK_NONE to
> indicate not to do any internal lockdep checks.
> 
> Signed-off-by: Danilo Krummrich 
> ---
>  include/linux/maple_tree.h | 20 +++-
>  lib/maple_tree.c   |  7 ---
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h
> index ca04c900e51a..f795e5def8d0 100644
> --- a/include/linux/maple_tree.h
> +++ b/include/linux/maple_tree.h
> @@ -170,10 +170,11 @@ enum maple_type {
>  #define MT_FLAGS_USE_RCU 0x02
>  #define MT_FLAGS_HEIGHT_OFFSET   0x02
>  #define MT_FLAGS_HEIGHT_MASK 0x7C
> -#define MT_FLAGS_LOCK_MASK   0x300
> +#define MT_FLAGS_LOCK_MASK   0x700
>  #define MT_FLAGS_LOCK_IRQ0x100
>  #define MT_FLAGS_LOCK_BH 0x200
>  #define MT_FLAGS_LOCK_EXTERN 0x300
> +#define MT_FLAGS_LOCK_NONE   0x400

Please add this to the documentation above the flags as well.  We should
probably add enough context so that users don't just set this and then
use multiple writers.

>  
>  #define MAPLE_HEIGHT_MAX 31
>  
> @@ -559,11 +560,16 @@ static inline void mas_set(struct ma_state *mas, 
> unsigned long index)
>   mas_set_range(mas, index, index);
>  }
>  
> -static inline bool mt_external_lock(const struct maple_tree *mt)
> +static inline bool mt_lock_external(const struct maple_tree *mt)
>  {
>   return (mt->ma_flags & MT_FLAGS_LOCK_MASK) == MT_FLAGS_LOCK_EXTERN;
>  }
>  
> +static inline bool mt_lock_none(const struct maple_tree *mt)
> +{
> + return (mt->ma_flags & MT_FLAGS_LOCK_MASK) == MT_FLAGS_LOCK_NONE;
> +}
> +
>  /**
>   * mt_init_flags() - Initialise an empty maple tree with flags.
>   * @mt: Maple Tree
> @@ -577,7 +583,7 @@ static inline bool mt_external_lock(const struct 
> maple_tree *mt)
>  static inline void mt_init_flags(struct maple_tree *mt, unsigned int flags)
>  {
>   mt->ma_flags = flags;
> - if (!mt_external_lock(mt))
> + if (!mt_lock_external(mt) && !mt_lock_none(mt))
>   spin_lock_init(>ma_lock);
>   rcu_assign_pointer(mt->ma_root, NULL);
>  }
> @@ -612,9 +618,11 @@ static inline void mt_clear_in_rcu(struct maple_tree *mt)
>   if (!mt_in_rcu(mt))
>   return;
>  
> - if (mt_external_lock(mt)) {
> + if (mt_lock_external(mt)) {
>   BUG_ON(!mt_lock_is_held(mt));
>   mt->ma_flags &= ~MT_FLAGS_USE_RCU;
> + } else if (mt_lock_none(mt)) {
> + mt->ma_flags &= ~MT_FLAGS_USE_RCU;
>   } else {
>   mtree_lock(mt);
>   mt->ma_flags &= ~MT_FLAGS_USE_RCU;
> @@ -631,9 +639,11 @@ static inline void mt_set_in_rcu(struct maple_tree *mt)
>   if (mt_in_rcu(mt))
>   return;
>  
> - if (mt_external_lock(mt)) {
> + if (mt_lock_external(mt)) {
>   BUG_ON(!mt_lock_is_held(mt));
>   mt->ma_flags |= MT_FLAGS_USE_RCU;
> + } else if (mt_lock_none(mt)) {
> + mt->ma_flags |= MT_FLAGS_USE_RCU;
>   } else {
>   mtree_lock(mt);
>   mt->ma_flags |= MT_FLAGS_USE_RCU;
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> index 26e2045d3cda..f51c0fd4eaad 100644
> --- a/lib/maple_tree.c
> +++ b/lib/maple_tree.c
> @@ -802,8 +802,8 @@ static inline void __rcu **ma_slots(struct maple_node 
> *mn, enum maple_type mt)
>  
>  static inline bool mt_locked(const struct maple_tree *mt)
>  {
> - return mt_external_lock(mt) ? mt_lock_is_held(mt) :
> - lockdep_is_held(>ma_lock);
> + return mt_lock_external(mt) ? mt_lock_is_held(mt) :
> + mt_lock_none(mt) ? true : lockdep_is_held(>ma_lock);

It might be better to just make this two return statements for clarity.

>  }
>  
>  static inline void *mt_slot(const struct maple_tree *mt,
> @@ -6120,7 +6120,8 @@ bool mas_nomem(struct ma_state *mas, gfp_t gfp)
>   return false;
>   }
>  
> - if (gfpflags_allow_blocking(gfp) && !mt_external_lock(mas->tree)) {
> + if (gfpflags_allow_blocking(gfp) &&
> + !mt_lock_external(mas->tree) && !mt_lock_none(mas->tree)) {
>   mtree_unlock(mas->tree);
>   mas_alloc_nodes(mas, gfp);
>   mtree_lock(mas->tree);
> -- 
> 2.39.1
> 

Re: [Nouveau] [PATCH drm-next v2 03/16] maple_tree: split up MA_STATE() macro

[Liam R. Howlett]

* Danilo Krummrich  [230220 09:38]:
> On 2/17/23 19:34, Liam R. Howlett wrote:
> > * Danilo Krummrich  [230217 08:44]:
> > > Split up the MA_STATE() macro such that components using the maple tree
> > > can easily inherit from struct ma_state and build custom tree walk
> > > macros to hide their internals from users.
> > > 
> > > Example:
> > > 
> > > struct sample_iter {
> > >   struct ma_state mas;
> > >   struct sample_mgr *mgr;
> > >   struct sample_entry *entry;
> > > };
> > > 
> > > \#define SAMPLE_ITER(name, __mgr) \
> > >   struct sample_iter name = { \
> > >   .mas = __MA_STATE(&(__mgr)->mt, 0, 0),
> > >   .mgr = __mgr,
> > >   .entry = NULL,
> > >   }
> > 
> > I see this patch is to allow for anonymous maple states, this looks
> > good.
> > 
> > I've a lengthy comment about the iterator that I'm adding here to head
> > off anyone that may copy your example below.
> > 
> > > 
> > > \#define sample_iter_for_each_range(it__, start__, end__) \
> > >   for ((it__).mas.index = start__, (it__).entry = mas_find(&(it__).mas, 
> > > end__ - 1); \
> > >(it__).entry; (it__).entry = mas_find(&(it__).mas, end__ - 1))
> > 
> > I see you've added something like the above in your patch set as well.
> > I'd like to point out that the index isn't the only state information
> > that needs to be altered here, and in fact, this could go very wrong.
> > 
> > The maple state has a node and an offset within that node.  If you set
> > the index to lower than the current position of your iterator and call
> > mas_find() then what happens is somewhat undefined.  I expect you will
> > get the wrong value (most likely either the current value or the very
> > next one that the iterator is already pointing to).  I believe you have
> > been using a fresh maple state for each iterator in your patches, but I
> > haven't had a deep look into your code yet.
> 
> Yes, I'm aware that I'd need to reset the whole iterator in order to re-use
> it.

Okay, good.  The way you have it written makes it unsafe to just call
without knowledge of the state and that will probably end poorly over
the long run.  If it's always starting from MAS_START then it's probably
safer to just initialize when you want to use it to the correct start
address.

> 
> Regarding the other considerations of the iterator design please see my
> answer to Matthew.
> 
> > 
> > We have methods of resetting the iterator and set the range (mas_set()
> > and mas_set_range()) which are safe for what you are doing, but they
> > will start the walk from the root node to the index again.
> > 
> > So, if you know what you are doing is safe, then the way you have
> > written it will work, but it's worth mentioning that this could occur.
> > 
> > It is also worth pointing out that it would be much safer to use a
> > function to do the above so you get type safety.. and I was asked to add
> > this to the VMA interface by Linus [1], which is on its way upstream [2].
> > 
> > 1. 
> > https://lore.kernel.org/linux-mm/CAHk-=wg9wqxbgkndkd2bqocnn73rdswuwsavbb7t-tekyke...@mail.gmail.com/
> > 2. 
> > https://lore.kernel.org/linux-mm/20230120162650.984577-1-liam.howl...@oracle.com/
> 
> You mean having wrappers like sample_find() instead of directly using
> mas_find()?

I'm not sure you need to go that low level, but I would ensure I have a
store/load function that ensures the correct type being put in/read from
are correct on compile - especially since you seem to have two trees to
track two different sets of things.  That iterator is probably safe
since the type is defined within itself.

> 
> > 
> > > 
> > > Signed-off-by: Danilo Krummrich 
> > > ---
> > >   include/linux/maple_tree.h | 7 +--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h
> > > index e594db58a0f1..ca04c900e51a 100644
> > > --- a/include/linux/maple_tree.h
> > > +++ b/include/linux/maple_tree.h
> > > @@ -424,8 +424,8 @@ struct ma_wr_state {
> > >   #define MA_ERROR(err) \
> > >   ((struct maple_enode *)(((unsigned long)err << 2) | 
> > > 2UL))
> > > -#define MA_STATE(name, mt, first, end)   
> > > \
> > > - struct ma_state name = {\
> > > +#define __MA_STATE(mt, first, en

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

[Liam R. Howlett]

* Danilo Krummrich  [230227 08:17]:

...
> > > Would this variant be significantly more efficient?
> > 
> > Well, what you are doing is walking the tree to see if there's anything
> > there... then re-walking the tree to store it.  So, yes, it's much more
> > efficient..  However, writing is heavier.  How much of the time is spent
> > walking vs writing depends on the size of the tree, but it's rather easy
> > to do this in a single walk of the tree so why wouldn't you?
> 
> I will, I was just curious about how much of an impact it has.
> 
> > 
> > > 
> > > Also, would this also work while already walking the tree?
> > 
> > Yes, to an extent.  If you are at the correct location in the tree, you
> > can write to that location.  If you are not in the correct location and
> > try to write to the tree then things will go poorly..  In this scenario,
> > we are very much walking the tree and writing to it in two steps.
> > 
> > > 
> > > To remove an entry while walking the tree I have a separate function
> > > drm_gpuva_iter_remove(). Would I need something similar for inserting
> > > entries?
> > 
> > I saw that.  Your remove function uses the erase operation which is
> > implemented as a walk to that location and a store of a null over the
> > range that is returned.  You do not need a function to insert an entry
> > if the maple state is at the correct location, and that doesn't just
> > mean setting mas.index/mas.last to the correct value.  There is a node &
> > offset saved in the maple state that needs to be in the correct
> > location.  If you store to that node then the node may be replaced, so
> > other iterators that you have may become stale, but the one you used
> > execute the store operation will now point to the new node with the new
> > entry.
> > 
> > > 
> > > I already provided this example in a separate mail thread, but it may 
> > > makes
> > > sense to move this to the mailing list:
> > > 
> > > In __drm_gpuva_sm_map() we're iterating a given range of the tree, where 
> > > the
> > > given range is the size of the newly requested mapping. 
> > > __drm_gpuva_sm_map()
> > > invokes a callback for each sub-operation that needs to be taken in order 
> > > to
> > > fulfill this mapping request. In most cases such a callback just creates a
> > > drm_gpuva_op object and stores it in a list.
> > > 
> > > However, drivers can also implement the callback, such that they directly
> > > execute this operation within the callback.
> > > 
> > > Let's have a look at the following example:
> > > 
> > >   0 a 2
> > > old: |---|   (bo_offset=n)
> > > 
> > > 1 b 3
> > > req:   |---| (bo_offset=m)
> > > 
> > >   0  a' 1 b 3
> > > new: |-|---| (a.bo_offset=n,b.bo_offset=m)
> > > 
> > > This would result in the following operations.
> > > 
> > > __drm_gpuva_sm_map() finds entry "a" and calls back into the driver
> > > suggesting to re-map "a" with the new size. The driver removes entry "a"
> > > from the tree and adds "a'"
> > 
> > What you have here won't work.  The driver will cause your iterators
> > maple state to point to memory that is freed.  You will either need to
> > pass through your iterator so that the modifications can occur with that
> > maple state so it remains valid, or you will need to invalidate the
> > iterator on every modification by the driver.
> > 
> > I'm sure the first idea you have will be to invalidate the iterator, but
> > that is probably not the way to proceed.  Even ignoring the unclear
> > locking of two maple states trying to modify the tree, this is rather
> > inefficient - each invalidation means a re-walk of the tree.  You may as
> > well not use an iterator in this case.
> > 
> > Depending on how/when the lookups occur, you could still iterate over
> > the tree and let the driver modify the ending of "a", but leave the tree
> > alone and just store b over whatever - but the failure scenarios may
> > cause you grief.
> > 
> > If you pass the iterator through, then you can just use it to do your
> > writes and keep iterating as if nothing changed.
> 
> Passing through the iterater clearly seems to be the way to go.
> 
> I assume that if the entry to insert isn't at the location of the iterator
> (as in the following example) we can just keep walking to this location my
> changing the index of the mas and calling mas_walk()?

no.  You have to mas_set() to the value and walk from the top of the
tree.  mas_walk() walks down, not from side to side - well, it does go
forward within a node (increasing offset), but if you hit the node limit
then you have gotten yourself in trouble.

> This would also imply
> that the "outer" tree walk continues after the entry we just inserted,
> right?

I don't understand the "outer" tree walk statement.

> 
>1 a 3
> old:   |---| (bo_offset=n)
> 
>  0 b 2
> req: |---|   (bo_offset=m)
> 
>  0 b 2  a' 3
> 

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

[Liam R. Howlett]

* Danilo Krummrich  [230313 19:46]:
> On 3/7/23 23:43, Liam R. Howlett wrote:
> > * Danilo Krummrich  [230306 10:46]:
> > > On 3/2/23 03:38, Liam R. Howlett wrote:
> > > > * Danilo Krummrich  [230227 08:17]:
> > > > 
> > > > ...
> > > > > > > Would this variant be significantly more efficient?
> > > > > > 
> > > > > > Well, what you are doing is walking the tree to see if there's 
> > > > > > anything
> > > > > > there... then re-walking the tree to store it.  So, yes, it's much 
> > > > > > more
> > > > > > efficient..  However, writing is heavier.  How much of the time is 
> > > > > > spent
> > > > > > walking vs writing depends on the size of the tree, but it's rather 
> > > > > > easy
> > > > > > to do this in a single walk of the tree so why wouldn't you?
> > > > > 
> > > > > I will, I was just curious about how much of an impact it has.
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Also, would this also work while already walking the tree?
> > > > > > 
> > > > > > Yes, to an extent.  If you are at the correct location in the tree, 
> > > > > > you
> > > > > > can write to that location.  If you are not in the correct location 
> > > > > > and
> > > > > > try to write to the tree then things will go poorly..  In this 
> > > > > > scenario,
> > > > > > we are very much walking the tree and writing to it in two steps.
> > > > > > 
> > > > > > > 
> > > > > > > To remove an entry while walking the tree I have a separate 
> > > > > > > function
> > > > > > > drm_gpuva_iter_remove(). Would I need something similar for 
> > > > > > > inserting
> > > > > > > entries?
> > > > > > 
> > > > > > I saw that.  Your remove function uses the erase operation which is
> > > > > > implemented as a walk to that location and a store of a null over 
> > > > > > the
> > > > > > range that is returned.  You do not need a function to insert an 
> > > > > > entry
> > > > > > if the maple state is at the correct location, and that doesn't just
> > > > > > mean setting mas.index/mas.last to the correct value.  There is a 
> > > > > > node &
> > > > > > offset saved in the maple state that needs to be in the correct
> > > > > > location.  If you store to that node then the node may be replaced, 
> > > > > > so
> > > > > > other iterators that you have may become stale, but the one you used
> > > > > > execute the store operation will now point to the new node with the 
> > > > > > new
> > > > > > entry.
> > > > > > 
> > > > > > > 
> > > > > > > I already provided this example in a separate mail thread, but it 
> > > > > > > may makes
> > > > > > > sense to move this to the mailing list:
> > > > > > > 
> > > > > > > In __drm_gpuva_sm_map() we're iterating a given range of the 
> > > > > > > tree, where the
> > > > > > > given range is the size of the newly requested mapping. 
> > > > > > > __drm_gpuva_sm_map()
> > > > > > > invokes a callback for each sub-operation that needs to be taken 
> > > > > > > in order to
> > > > > > > fulfill this mapping request. In most cases such a callback just 
> > > > > > > creates a
> > > > > > > drm_gpuva_op object and stores it in a list.
> > > > > > > 
> > > > > > > However, drivers can also implement the callback, such that they 
> > > > > > > directly
> > > > > > > execute this operation within the callback.
> > > > > > > 
> > > > > > > Let's have a look at the following example:
> > > > > > > 
> > > > > > > 0 a 2
> > > > > > > old: |---|   (bo_offset=n)
> > > > > > > 
> > > > > > >   

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

[Liam R. Howlett]

* Danilo Krummrich  [230306 10:46]:
> On 3/2/23 03:38, Liam R. Howlett wrote:
> > * Danilo Krummrich  [230227 08:17]:
> > 
> > ...
> > > > > Would this variant be significantly more efficient?
> > > > 
> > > > Well, what you are doing is walking the tree to see if there's anything
> > > > there... then re-walking the tree to store it.  So, yes, it's much more
> > > > efficient..  However, writing is heavier.  How much of the time is spent
> > > > walking vs writing depends on the size of the tree, but it's rather easy
> > > > to do this in a single walk of the tree so why wouldn't you?
> > > 
> > > I will, I was just curious about how much of an impact it has.
> > > 
> > > > 
> > > > > 
> > > > > Also, would this also work while already walking the tree?
> > > > 
> > > > Yes, to an extent.  If you are at the correct location in the tree, you
> > > > can write to that location.  If you are not in the correct location and
> > > > try to write to the tree then things will go poorly..  In this scenario,
> > > > we are very much walking the tree and writing to it in two steps.
> > > > 
> > > > > 
> > > > > To remove an entry while walking the tree I have a separate function
> > > > > drm_gpuva_iter_remove(). Would I need something similar for inserting
> > > > > entries?
> > > > 
> > > > I saw that.  Your remove function uses the erase operation which is
> > > > implemented as a walk to that location and a store of a null over the
> > > > range that is returned.  You do not need a function to insert an entry
> > > > if the maple state is at the correct location, and that doesn't just
> > > > mean setting mas.index/mas.last to the correct value.  There is a node &
> > > > offset saved in the maple state that needs to be in the correct
> > > > location.  If you store to that node then the node may be replaced, so
> > > > other iterators that you have may become stale, but the one you used
> > > > execute the store operation will now point to the new node with the new
> > > > entry.
> > > > 
> > > > > 
> > > > > I already provided this example in a separate mail thread, but it may 
> > > > > makes
> > > > > sense to move this to the mailing list:
> > > > > 
> > > > > In __drm_gpuva_sm_map() we're iterating a given range of the tree, 
> > > > > where the
> > > > > given range is the size of the newly requested mapping. 
> > > > > __drm_gpuva_sm_map()
> > > > > invokes a callback for each sub-operation that needs to be taken in 
> > > > > order to
> > > > > fulfill this mapping request. In most cases such a callback just 
> > > > > creates a
> > > > > drm_gpuva_op object and stores it in a list.
> > > > > 
> > > > > However, drivers can also implement the callback, such that they 
> > > > > directly
> > > > > execute this operation within the callback.
> > > > > 
> > > > > Let's have a look at the following example:
> > > > > 
> > > > >0 a 2
> > > > > old: |---|   (bo_offset=n)
> > > > > 
> > > > >  1 b 3
> > > > > req:   |---| (bo_offset=m)
> > > > > 
> > > > >0  a' 1 b 3
> > > > > new: |-|---| (a.bo_offset=n,b.bo_offset=m)
> > > > > 
> > > > > This would result in the following operations.
> > > > > 
> > > > > __drm_gpuva_sm_map() finds entry "a" and calls back into the driver
> > > > > suggesting to re-map "a" with the new size. The driver removes entry 
> > > > > "a"
> > > > > from the tree and adds "a'"
> > > > 
> > > > What you have here won't work.  The driver will cause your iterators
> > > > maple state to point to memory that is freed.  You will either need to
> > > > pass through your iterator so that the modifications can occur with that
> > > > maple state so it remains valid, or you will need to invalidate the
> > > > iterator on every modification by the driver.
> > > > 
> > > > I'm sure the

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

[Liam R. Howlett]

* Danilo Krummrich  [230222 13:13]:
> On 2/21/23 19:20, Liam R. Howlett wrote:
> > * Danilo Krummrich  [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 
> > > Signed-off-by: Danilo Krummrich 
> > > ---
> > >   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 _gpuva_iterator
> > > + *
> > > + * This removes the element the iterator currently points to.
> > > + */
> > > +void
> > > +drm_gpuva_iter_remove(struct drm_gpuva_iterator *it)
> > > +{
> > > + mas_erase(>mas);
> > > +}
> > > +EXPORT_SYMBOL(drm_gpuva_iter_remove);
> > > +
> > > +/**
> > > + * drm_gpuva_insert - insert a _gpuva
> > > + * @mgr: the _gpuva_manager to insert the _gpuva in
> > > + * @va: the _gpuva to insert
> > > + * @addr: the start address of the GPU VA
> > > + * @range: the range of the GPU VA
> > > + *
> > > + * Insert a _gpuva with a given address and range into a
> > > + * _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, >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(, 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 st

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

[Liam R. Howlett]

* Danilo Krummrich  [230227 21:17]:
> On Tue, Feb 21, 2023 at 01:20:50PM -0500, Liam R. Howlett wrote:
> > * Danilo Krummrich  [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 
> > > Signed-off-by: Danilo Krummrich 
> > > ---
> > >  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 _gpuva_iterator
> > > + *
> > > + * This removes the element the iterator currently points to.
> > > + */
> > > +void
> > > +drm_gpuva_iter_remove(struct drm_gpuva_iterator *it)
> > > +{
> > > + mas_erase(>mas);
> > > +}
> > > +EXPORT_SYMBOL(drm_gpuva_iter_remove);
> > > +
> > > +/**
> > > + * drm_gpuva_insert - insert a _gpuva
> > > + * @mgr: the _gpuva_manager to insert the _gpuva in
> > > + * @va: the _gpuva to insert
> > > + * @addr: the start address of the GPU VA
> > > + * @range: the range of the GPU VA
> > > + *
> > > + * Insert a _gpuva with a given address and range into a
> > > + * _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, >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(, 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 locat