Re: [PATCH drm-misc-next v8 10/12] drm/gpuvm: add an abstraction for a VM / BO combination

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Thu, 2023-11-02 at 00:31 +0100, Danilo Krummrich wrote:
> Add an abstraction layer between the drm_gpuva mappings of a
> particular
> drm_gem_object and this GEM object itself. The abstraction represents
> a
> combination of a drm_gem_object and drm_gpuvm. The drm_gem_object
> holds
> a list of drm_gpuvm_bo structures (the structure representing this
> abstraction), while each drm_gpuvm_bo contains list of mappings of
> this
> GEM object.
> 
> This has multiple advantages:
> 
> 1) We can use the drm_gpuvm_bo structure to attach it to various
> lists
>    of the drm_gpuvm. This is useful for tracking external and evicted
>    objects per VM, which is introduced in subsequent patches.
> 
> 2) Finding mappings of a certain drm_gem_object mapped in a certain
>    drm_gpuvm becomes much cheaper.
> 
> 3) Drivers can derive and extend the structure to easily represent
>    driver specific states of a BO for a certain GPUVM.
> 
> The idea of this abstraction was taken from amdgpu, hence the credit
> for
> this idea goes to the developers of amdgpu.
> 
> Cc: Christian König <christian.koenig@amd.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  drivers/gpu/drm/drm_gpuvm.c            | 336 +++++++++++++++++++++--
> --
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c |  63 +++--
>  include/drm/drm_gem.h                  |  32 +--
>  include/drm/drm_gpuvm.h                | 185 +++++++++++++-
>  4 files changed, 530 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpuvm.c
> b/drivers/gpu/drm/drm_gpuvm.c
> index 6a88eafc5229..2c8fdefb19f0 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -70,6 +70,18 @@
>   * &drm_gem_object, such as the &drm_gem_object containing the root
> page table,
>   * but it can also be a 'dummy' object, which can be allocated with
>   * drm_gpuvm_resv_object_alloc().
> + *
> + * In order to connect a struct drm_gpuva its backing
> &drm_gem_object each
> + * &drm_gem_object maintains a list of &drm_gpuvm_bo structures, and
> each
> + * &drm_gpuvm_bo contains a list of &drm_gpuva structures.
> + *
> + * A &drm_gpuvm_bo is an abstraction that represents a combination
> of a
> + * &drm_gpuvm and a &drm_gem_object. Every such combination should
> be unique.
> + * This is ensured by the API through drm_gpuvm_bo_obtain() and
> + * drm_gpuvm_bo_obtain_prealloc() which first look into the
> corresponding
> + * &drm_gem_object list of &drm_gpuvm_bos for an existing instance
> of this
> + * particular combination. If not existent a new instance is created
> and linked
> + * to the &drm_gem_object.
>   */
>  
>  /**
> @@ -395,21 +407,28 @@
>  /**
>   * DOC: Locking
>   *
> - * Generally, the GPU VA manager does not take care of locking
> itself, it is
> - * the drivers responsibility to take care about locking. Drivers
> might want to
> - * protect the following operations: inserting, removing and
> iterating
> - * &drm_gpuva objects as well as generating all kinds of operations,
> such as
> - * split / merge or prefetch.
> - *
> - * The GPU VA manager also does not take care of the locking of the
> backing
> - * &drm_gem_object buffers GPU VA lists by itself; drivers are
> responsible to
> - * enforce mutual exclusion using either the GEMs dma_resv lock or
> alternatively
> - * a driver specific external lock. For the latter see also
> - * drm_gem_gpuva_set_lock().
> - *
> - * However, the GPU VA manager contains lockdep checks to ensure
> callers of its
> - * API hold the corresponding lock whenever the &drm_gem_objects GPU
> VA list is
> - * accessed by functions such as drm_gpuva_link() or
> drm_gpuva_unlink().
> + * In terms of managing &drm_gpuva entries DRM GPUVM does not take
> care of
> + * locking itself, it is the drivers responsibility to take care
> about locking.
> + * Drivers might want to protect the following operations:
> inserting, removing
> + * and iterating &drm_gpuva objects as well as generating all kinds
> of
> + * operations, such as split / merge or prefetch.
> + *
> + * DRM GPUVM also does not take care of the locking of the backing
> + * &drm_gem_object buffers GPU VA lists and &drm_gpuvm_bo
> abstractions by
> + * itself; drivers are responsible to enforce mutual exclusion using
> either the
> + * GEMs dma_resv lock or alternatively a driver specific external
> lock. For the
> + * latter see also drm_gem_gpuva_set_lock().
> + *
> + * However, DRM GPUVM contains lockdep checks to ensure callers of
> its API hold
> + * the corresponding lock whenever the &drm_gem_objects GPU VA list
> is accessed
> + * by functions such as drm_gpuva_link() or drm_gpuva_unlink(), but
> also
> + * drm_gpuvm_bo_obtain() and drm_gpuvm_bo_put().
> + *
> + * The latter is required since on creation and destruction of a
> &drm_gpuvm_bo
> + * the &drm_gpuvm_bo is attached / removed from the &drm_gem_objects
> gpuva list.
> + * Subsequent calls to drm_gpuvm_bo_obtain() for the same &drm_gpuvm
> and
> + * &drm_gem_object must be able to observe previous creations and
> destructions
> + * of &drm_gpuvm_bos in order to keep instances unique.
>   */
>  
>  /**
> @@ -439,6 +458,7 @@
>   *     {
>   *             struct drm_gpuva_ops *ops;
>   *             struct drm_gpuva_op *op
> + *             struct drm_gpuvm_bo *vm_bo;
>   *
>   *             driver_lock_va_space();
>   *             ops = drm_gpuvm_sm_map_ops_create(gpuvm, addr, range,
> @@ -446,6 +466,10 @@
>   *             if (IS_ERR(ops))
>   *                     return PTR_ERR(ops);
>   *
> + *             vm_bo = drm_gpuvm_bo_obtain(gpuvm, obj);
> + *             if (IS_ERR(vm_bo))
> + *                     return PTR_ERR(vm_bo);
> + *
>   *             drm_gpuva_for_each_op(op, ops) {
>   *                     struct drm_gpuva *va;
>   *
> @@ -458,7 +482,7 @@
>   *
>   *                             driver_vm_map();
>   *                             drm_gpuva_map(gpuvm, va, &op->map);
> - *                             drm_gpuva_link(va);
> + *                             drm_gpuva_link(va, vm_bo);
>   *
>   *                             break;
>   *                     case DRM_GPUVA_OP_REMAP: {
> @@ -485,11 +509,11 @@
>   *                             driver_vm_remap();
>   *                             drm_gpuva_remap(prev, next, &op-
> >remap);
>   *
> - *                             drm_gpuva_unlink(va);
>   *                             if (prev)
> - *                                     drm_gpuva_link(prev);
> + *                                     drm_gpuva_link(prev, va-
> >vm_bo);
>   *                             if (next)
> - *                                     drm_gpuva_link(next);
> + *                                     drm_gpuva_link(next, va-
> >vm_bo);
> + *                             drm_gpuva_unlink(va);
>   *
>   *                             break;
>   *                     }
> @@ -505,6 +529,7 @@
>   *                             break;
>   *                     }
>   *             }
> + *             drm_gpuvm_bo_put(vm_bo);
>   *             driver_unlock_va_space();
>   *
>   *             return 0;
> @@ -514,6 +539,7 @@
>   *
>   *     struct driver_context {
>   *             struct drm_gpuvm *gpuvm;
> + *             struct drm_gpuvm_bo *vm_bo;
>   *             struct drm_gpuva *new_va;
>   *             struct drm_gpuva *prev_va;
>   *             struct drm_gpuva *next_va;
> @@ -534,6 +560,7 @@
>   *                               struct drm_gem_object *obj, u64
> offset)
>   *     {
>   *             struct driver_context ctx;
> + *             struct drm_gpuvm_bo *vm_bo;
>   *             struct drm_gpuva_ops *ops;
>   *             struct drm_gpuva_op *op;
>   *             int ret = 0;
> @@ -543,16 +570,23 @@
>   *             ctx.new_va = kzalloc(sizeof(*ctx.new_va),
> GFP_KERNEL);
>   *             ctx.prev_va = kzalloc(sizeof(*ctx.prev_va),
> GFP_KERNEL);
>   *             ctx.next_va = kzalloc(sizeof(*ctx.next_va),
> GFP_KERNEL);
> - *             if (!ctx.new_va || !ctx.prev_va || !ctx.next_va) {
> + *             ctx.vm_bo = drm_gpuvm_bo_create(gpuvm, obj);
> + *             if (!ctx.new_va || !ctx.prev_va || !ctx.next_va ||
> !vm_bo) {
>   *                     ret = -ENOMEM;
>   *                     goto out;
>   *             }
>   *
> + *             // Typically protected with a driver specific GEM
> gpuva lock
> + *             // used in the fence signaling path for
> drm_gpuva_link() and
> + *             // drm_gpuva_unlink(), hence pre-allocate.
> + *             ctx.vm_bo = drm_gpuvm_bo_obtain_prealloc(ctx.vm_bo);
> + *
>   *             driver_lock_va_space();
>   *             ret = drm_gpuvm_sm_map(gpuvm, &ctx, addr, range, obj,
> offset);
>   *             driver_unlock_va_space();
>   *
>   *     out:
> + *             drm_gpuvm_bo_put(ctx.vm_bo);
>   *             kfree(ctx.new_va);
>   *             kfree(ctx.prev_va);
>   *             kfree(ctx.next_va);
> @@ -565,7 +599,7 @@
>   *
>   *             drm_gpuva_map(ctx->vm, ctx->new_va, &op->map);
>   *
> - *             drm_gpuva_link(ctx->new_va);
> + *             drm_gpuva_link(ctx->new_va, ctx->vm_bo);
>   *
>   *             // prevent the new GPUVA from being freed in
>   *             // driver_mapping_create()
> @@ -577,22 +611,23 @@
>   *     int driver_gpuva_remap(struct drm_gpuva_op *op, void *__ctx)
>   *     {
>   *             struct driver_context *ctx = __ctx;
> + *             struct drm_gpuva *va = op->remap.unmap->va;
>   *
>   *             drm_gpuva_remap(ctx->prev_va, ctx->next_va, &op-
> >remap);
>   *
> - *             drm_gpuva_unlink(op->remap.unmap->va);
> - *             kfree(op->remap.unmap->va);
> - *
>   *             if (op->remap.prev) {
> - *                     drm_gpuva_link(ctx->prev_va);
> + *                     drm_gpuva_link(ctx->prev_va, va->vm_bo);
>   *                     ctx->prev_va = NULL;
>   *             }
>   *
>   *             if (op->remap.next) {
> - *                     drm_gpuva_link(ctx->next_va);
> + *                     drm_gpuva_link(ctx->next_va, va->vm_bo);
>   *                     ctx->next_va = NULL;
>   *             }
>   *
> + *             drm_gpuva_unlink(va);
> + *             kfree(va);
> + *
>   *             return 0;
>   *     }
>   *
> @@ -813,6 +848,195 @@ drm_gpuvm_put(struct drm_gpuvm *gpuvm)
>  }
>  EXPORT_SYMBOL_GPL(drm_gpuvm_put);
>  
> +/**
> + * drm_gpuvm_bo_create() - create a new instance of struct
> drm_gpuvm_bo
> + * @gpuvm: The &drm_gpuvm the @obj is mapped in.
> + * @obj: The &drm_gem_object being mapped in the @gpuvm.
> + *
> + * If provided by the driver, this function uses the &drm_gpuvm_ops
> + * vm_bo_alloc() callback to allocate.
> + *
> + * Returns: a pointer to the &drm_gpuvm_bo on success, NULL on
> failure
> + */
> +struct drm_gpuvm_bo *
> +drm_gpuvm_bo_create(struct drm_gpuvm *gpuvm,
> +                   struct drm_gem_object *obj)
> +{
> +       const struct drm_gpuvm_ops *ops = gpuvm->ops;
> +       struct drm_gpuvm_bo *vm_bo;
> +
> +       if (ops && ops->vm_bo_alloc)
> +               vm_bo = ops->vm_bo_alloc();
> +       else
> +               vm_bo = kzalloc(sizeof(*vm_bo), GFP_KERNEL);
> +
> +       if (unlikely(!vm_bo))
> +               return NULL;
> +
> +       vm_bo->vm = drm_gpuvm_get(gpuvm);
> +       vm_bo->obj = obj;
> +       drm_gem_object_get(obj);
> +
> +       kref_init(&vm_bo->kref);
> +       INIT_LIST_HEAD(&vm_bo->list.gpuva);
> +       INIT_LIST_HEAD(&vm_bo->list.entry.gem);
> +
> +       return vm_bo;
> +}
> +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_create);
> +
> +static void
> +drm_gpuvm_bo_destroy(struct kref *kref)
> +{
> +       struct drm_gpuvm_bo *vm_bo = container_of(kref, struct
> drm_gpuvm_bo,
> +                                                 kref);
> +       struct drm_gpuvm *gpuvm = vm_bo->vm;
> +       const struct drm_gpuvm_ops *ops = gpuvm->ops;
> +       struct drm_gem_object *obj = vm_bo->obj;
> +       bool lock = !drm_gpuvm_resv_protected(gpuvm);
> +
> +       if (!lock)
> +               drm_gpuvm_resv_assert_held(gpuvm);
> +
> +       drm_gem_gpuva_assert_lock_held(obj);
> +       list_del(&vm_bo->list.entry.gem);
> +
> +       if (ops && ops->vm_bo_free)
> +               ops->vm_bo_free(vm_bo);
> +       else
> +               kfree(vm_bo);
> +
> +       drm_gpuvm_put(gpuvm);
> +       drm_gem_object_put(obj);
> +}
> +
> +/**
> + * drm_gpuvm_bo_put() - drop a struct drm_gpuvm_bo reference
> + * @vm_bo: the &drm_gpuvm_bo to release the reference of
> + *
> + * This releases a reference to @vm_bo.
> + *
> + * If the reference count drops to zero, the &gpuvm_bo is destroyed,
> which
> + * includes removing it from the GEMs gpuva list. Hence, if a call
> to this
> + * function can potentially let the reference count to zero the
> caller must
> + * hold the dma-resv or driver specific GEM gpuva lock.

Should Ideally document the context for this function as well, to avoid
future pitfalls and arguments, and also potentially a might_sleep().

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>