Thomas Hellström
2023-Oct-31 10:32 UTC
[Nouveau] [PATCH drm-misc-next v7 4/7] drm/gpuvm: add an abstraction for a VM / BO combination
On Mon, 2023-10-23 at 22:16 +0200, 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 at amd.com> > Signed-off-by: Danilo Krummrich <dakr at redhat.com> > --- > ?drivers/gpu/drm/drm_gpuvm.c??????????? | 335 +++++++++++++++++++++-- > -- > ?drivers/gpu/drm/nouveau/nouveau_uvmm.c |? 64 +++-- > ?include/drm/drm_gem.h????????????????? |? 32 +-- > ?include/drm/drm_gpuvm.h??????????????? | 188 +++++++++++++- > ?4 files changed, 533 insertions(+), 86 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gpuvm.c > b/drivers/gpu/drm/drm_gpuvm.c > index c03332883432..7f4f5919f84c 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; > ? *?????} > ? * > @@ -774,6 +809,194 @@ drm_gpuvm_destroy(struct drm_gpuvm *gpuvm) > ?} > ?EXPORT_SYMBOL_GPL(drm_gpuvm_destroy); > ? > +/** > + * 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 onStill needs s/Returns:/Return:/g> 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 = 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_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. > + */ > +void > +drm_gpuvm_bo_put(struct drm_gpuvm_bo *vm_bo) > +{ > +???????if (vm_bo) > +???????????????kref_put(&vm_bo->kref, drm_gpuvm_bo_destroy); > +} > +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_put); > + > +static struct drm_gpuvm_bo * > +__drm_gpuvm_bo_find(struct drm_gpuvm *gpuvm, > +?????????????????? struct drm_gem_object *obj) > +{ > +???????struct drm_gpuvm_bo *vm_bo; > + > +???????drm_gem_gpuva_assert_lock_held(obj); > +???????drm_gem_for_each_gpuvm_bo(vm_bo, obj) > +???????????????if (vm_bo->vm == gpuvm) > +???????????????????????return vm_bo; > + > +???????return NULL; > +} > + > +/** > + * drm_gpuvm_bo_find() - find the &drm_gpuvm_bo for the given > + * &drm_gpuvm and &drm_gem_object > + * @gpuvm: The &drm_gpuvm the @obj is mapped in. > + * @obj: The &drm_gem_object being mapped in the @gpuvm. > + * > + * Find the &drm_gpuvm_bo representing the combination of the given > + * &drm_gpuvm and &drm_gem_object. If found, increases the reference > + * count of the &drm_gpuvm_bo accordingly. > + * > + * Returns: a pointer to the &drm_gpuvm_bo on success, NULL on > failure > + */ > +struct drm_gpuvm_bo * > +drm_gpuvm_bo_find(struct drm_gpuvm *gpuvm, > +???????????????? struct drm_gem_object *obj) > +{ > +???????struct drm_gpuvm_bo *vm_bo = __drm_gpuvm_bo_find(gpuvm, obj); > + > +???????return vm_bo ? drm_gpuvm_bo_get(vm_bo) : NULL; > +} > +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_find); > + > +/** > + * drm_gpuvm_bo_obtain() - obtains and instance of the &drm_gpuvm_bo > for the > + * given &drm_gpuvm and &drm_gem_object > + * @gpuvm: The &drm_gpuvm the @obj is mapped in. > + * @obj: The &drm_gem_object being mapped in the @gpuvm. > + * > + * Find the &drm_gpuvm_bo representing the combination of the given > + * &drm_gpuvm and &drm_gem_object. If found, increases the reference > + * count of the &drm_gpuvm_bo accordingly. If not found, allocates a > new > + * &drm_gpuvm_bo. > + * > + * A new &drm_gpuvm_bo is added to the GEMs gpuva list. > + * > + * Returns: a pointer to the &drm_gpuvm_bo on success, an ERR_PTR on > failure > + */ > +struct drm_gpuvm_bo * > +drm_gpuvm_bo_obtain(struct drm_gpuvm *gpuvm, > +?????????????????? struct drm_gem_object *obj) > +{ > +???????struct drm_gpuvm_bo *vm_bo; > + > +???????vm_bo = drm_gpuvm_bo_find(gpuvm, obj); > +???????if (vm_bo) > +???????????????return vm_bo; > + > +???????vm_bo = drm_gpuvm_bo_create(gpuvm, obj); > +???????if (!vm_bo) > +???????????????return ERR_PTR(-ENOMEM); > + > +???????drm_gem_gpuva_assert_lock_held(obj); > +???????list_add_tail(&vm_bo->list.entry.gem, &obj->gpuva.list); > + > +???????return vm_bo; > +} > +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_obtain); > + > +/** > + * drm_gpuvm_bo_obtain_prealloc() - obtains and instance of the > &drm_gpuvm_bo > + * for the given &drm_gpuvm and &drm_gem_object > + * @__vm_bo: A pre-allocated struct drm_gpuvm_bo. > + * > + * Find the &drm_gpuvm_bo representing the combination of the given > + * &drm_gpuvm and &drm_gem_object. If found, increases the reference > + * count of the found &drm_gpuvm_bo accordingly, while the @__vm_bo > reference > + * count is decreased. If not found @__vm_bo is returned without > further > + * increase of the reference count. > + * > + * A new &drm_gpuvm_bo is added to the GEMs gpuva list. > + * > + * Returns: a pointer to the found &drm_gpuvm_bo or @__vm_bo if no > existing > + * &drm_gpuvm_bo was found > + */ > +struct drm_gpuvm_bo * > +drm_gpuvm_bo_obtain_prealloc(struct drm_gpuvm_bo *__vm_bo) > +{ > +???????struct drm_gpuvm *gpuvm = __vm_bo->vm; > +???????struct drm_gem_object *obj = __vm_bo->obj; > +???????struct drm_gpuvm_bo *vm_bo; > + > +???????vm_bo = drm_gpuvm_bo_find(gpuvm, obj); > +???????if (vm_bo) { > +???????????????drm_gpuvm_bo_put(__vm_bo); > +???????????????return vm_bo; > +???????} > + > +???????drm_gem_gpuva_assert_lock_held(obj); > +???????list_add_tail(&__vm_bo->list.entry.gem, &obj->gpuva.list); > + > +???????return __vm_bo; > +} > +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_obtain_prealloc); > + > ?static int > ?__drm_gpuva_insert(struct drm_gpuvm *gpuvm, > ?????????????????? struct drm_gpuva *va) > @@ -864,24 +1087,33 @@ EXPORT_SYMBOL_GPL(drm_gpuva_remove); > ?/** > ? * drm_gpuva_link() - link a &drm_gpuva > ? * @va: the &drm_gpuva to link > + * @vm_bo: the &drm_gpuvm_bo to add the &drm_gpuva to > ? * > - * This adds the given &va to the GPU VA list of the &drm_gem_object > it is > - * associated with. > + * This adds the given &va to the GPU VA list of the &drm_gpuvm_bo > and the > + * &drm_gpuvm_bo to the &drm_gem_object it is associated with. > + * > + * For every &drm_gpuva entry added to the &drm_gpuvm_bo an > additional > + * reference of the latter is taken. > ? * > ? * This function expects the caller to protect the GEM's GPUVA list > against > - * concurrent access using the GEMs dma_resv lock. > + * concurrent access using either the GEMs dma_resv lock or a driver > specific > + * lock set through drm_gem_gpuva_set_lock(). > ? */ > ?void > -drm_gpuva_link(struct drm_gpuva *va) > +drm_gpuva_link(struct drm_gpuva *va, struct drm_gpuvm_bo *vm_bo) > ?{ > ????????struct drm_gem_object *obj = va->gem.obj; > +???????struct drm_gpuvm *gpuvm = va->vm; > ? > ????????if (unlikely(!obj)) > ????????????????return; > ? > -???????drm_gem_gpuva_assert_lock_held(obj); > +???????drm_WARN_ON(gpuvm->drm, obj != vm_bo->obj); > ? > -???????list_add_tail(&va->gem.entry, &obj->gpuva.list); > +???????va->vm_bo = drm_gpuvm_bo_get(vm_bo); > + > +???????drm_gem_gpuva_assert_lock_held(obj); > +???????list_add_tail(&va->gem.entry, &vm_bo->list.gpuva); > ?} > ?EXPORT_SYMBOL_GPL(drm_gpuva_link); > ? > @@ -892,20 +1124,31 @@ EXPORT_SYMBOL_GPL(drm_gpuva_link); > ? * This removes the given &va from the GPU VA list of the > &drm_gem_object it is > ? * associated with. > ? * > + * This removes the given &va from the GPU VA list of the > &drm_gpuvm_bo and > + * the &drm_gpuvm_bo from the &drm_gem_object it is associated with > in case > + * this call unlinks the last &drm_gpuva from the &drm_gpuvm_bo. > + * > + * For every &drm_gpuva entry removed from the &drm_gpuvm_bo a > reference of > + * the latter is dropped. > + * > ? * This function expects the caller to protect the GEM's GPUVA list > against > - * concurrent access using the GEMs dma_resv lock. > + * concurrent access using either the GEMs dma_resv lock or a driver > specific > + * lock set through drm_gem_gpuva_set_lock(). > ? */ > ?void > ?drm_gpuva_unlink(struct drm_gpuva *va) > ?{ > ????????struct drm_gem_object *obj = va->gem.obj; > +???????struct drm_gpuvm_bo *vm_bo = va->vm_bo; > ? > ????????if (unlikely(!obj)) > ????????????????return; > ? > ????????drm_gem_gpuva_assert_lock_held(obj); > - > ????????list_del_init(&va->gem.entry); > + > +???????va->vm_bo = NULL; > +???????drm_gpuvm_bo_put(vm_bo); > ?} > ?EXPORT_SYMBOL_GPL(drm_gpuva_unlink); > ? > @@ -1050,10 +1293,10 @@ drm_gpuva_remap(struct drm_gpuva *prev, > ????????????????struct drm_gpuva *next, > ????????????????struct drm_gpuva_op_remap *op) > ?{ > -???????struct drm_gpuva *curr = op->unmap->va; > -???????struct drm_gpuvm *gpuvm = curr->vm; > +???????struct drm_gpuva *va = op->unmap->va; > +???????struct drm_gpuvm *gpuvm = va->vm; > ? > -???????drm_gpuva_remove(curr); > +???????drm_gpuva_remove(va); > ? > ????????if (op->prev) { > ????????????????drm_gpuva_init_from_op(prev, op->prev); > @@ -1695,9 +1938,8 @@ drm_gpuvm_prefetch_ops_create(struct drm_gpuvm > *gpuvm, > ?EXPORT_SYMBOL_GPL(drm_gpuvm_prefetch_ops_create); > ? > ?/** > - * drm_gpuvm_gem_unmap_ops_create() - creates the &drm_gpuva_ops to > unmap a GEM > - * @gpuvm: the &drm_gpuvm representing the GPU VA space > - * @obj: the &drm_gem_object to unmap > + * drm_gpuvm_bo_unmap_ops_create() - creates the &drm_gpuva_ops to > unmap a GEM > + * @vm_bo: the &drm_gpuvm_bo abstraction > ? * > ? * This function creates a list of operations to perform unmapping > for every > ? * GPUVA attached to a GEM. > @@ -1714,15 +1956,14 @@ > EXPORT_SYMBOL_GPL(drm_gpuvm_prefetch_ops_create); > ? * Returns: a pointer to the &drm_gpuva_ops on success, an ERR_PTR > on failure > ? */ > ?struct drm_gpuva_ops * > -drm_gpuvm_gem_unmap_ops_create(struct drm_gpuvm *gpuvm, > -????????????????????????????? struct drm_gem_object *obj) > +drm_gpuvm_bo_unmap_ops_create(struct drm_gpuvm_bo *vm_bo) > ?{ > ????????struct drm_gpuva_ops *ops; > ????????struct drm_gpuva_op *op; > ????????struct drm_gpuva *va; > ????????int ret; > ? > -???????drm_gem_gpuva_assert_lock_held(obj); > +???????drm_gem_gpuva_assert_lock_held(vm_bo->obj); > ? > ????????ops = kzalloc(sizeof(*ops), GFP_KERNEL); > ????????if (!ops) > @@ -1730,8 +1971,8 @@ drm_gpuvm_gem_unmap_ops_create(struct drm_gpuvm > *gpuvm, > ? > ????????INIT_LIST_HEAD(&ops->list); > ? > -???????drm_gem_for_each_gpuva(va, obj) { > -???????????????op = gpuva_op_alloc(gpuvm); > +???????drm_gpuvm_bo_for_each_va(va, vm_bo) { > +???????????????op = gpuva_op_alloc(vm_bo->vm); > ????????????????if (!op) { > ????????????????????????ret = -ENOMEM; > ????????????????????????goto err_free_ops; > @@ -1745,10 +1986,10 @@ drm_gpuvm_gem_unmap_ops_create(struct > drm_gpuvm *gpuvm, > ????????return ops; > ? > ?err_free_ops: > -???????drm_gpuva_ops_free(gpuvm, ops); > +???????drm_gpuva_ops_free(vm_bo->vm, ops); > ????????return ERR_PTR(ret); > ?} > -EXPORT_SYMBOL_GPL(drm_gpuvm_gem_unmap_ops_create); > +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_unmap_ops_create); > ? > ?/** > ? * drm_gpuva_ops_free() - free the given &drm_gpuva_ops > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c > b/drivers/gpu/drm/nouveau/nouveau_uvmm.c > index ed439bf4032f..1e95b0a1b047 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c > @@ -62,6 +62,8 @@ struct bind_job_op { > ????????enum vm_bind_op op; > ????????u32 flags; > ? > +???????struct drm_gpuvm_bo *vm_bo; > + > ????????struct { > ????????????????u64 addr; > ????????????????u64 range; > @@ -1113,22 +1115,28 @@ bind_validate_region(struct nouveau_job *job) > ?} > ? > ?static void > -bind_link_gpuvas(struct drm_gpuva_ops *ops, struct > nouveau_uvma_prealloc *new) > +bind_link_gpuvas(struct bind_job_op *bop) > ?{ > +???????struct nouveau_uvma_prealloc *new = &bop->new; > +???????struct drm_gpuvm_bo *vm_bo = bop->vm_bo; > +???????struct drm_gpuva_ops *ops = bop->ops; > ????????struct drm_gpuva_op *op; > ? > ????????drm_gpuva_for_each_op(op, ops) { > ????????????????switch (op->op) { > ????????????????case DRM_GPUVA_OP_MAP: > -???????????????????????drm_gpuva_link(&new->map->va); > +???????????????????????drm_gpuva_link(&new->map->va, vm_bo); > ????????????????????????break; > -???????????????case DRM_GPUVA_OP_REMAP: > +???????????????case DRM_GPUVA_OP_REMAP: { > +???????????????????????struct drm_gpuva *va = op->remap.unmap->va; > + > ????????????????????????if (op->remap.prev) > -???????????????????????????????drm_gpuva_link(&new->prev->va); > +???????????????????????????????drm_gpuva_link(&new->prev->va, va- > >vm_bo); > ????????????????????????if (op->remap.next) > -???????????????????????????????drm_gpuva_link(&new->next->va); > -???????????????????????drm_gpuva_unlink(op->remap.unmap->va); > +???????????????????????????????drm_gpuva_link(&new->next->va, va- > >vm_bo); > +???????????????????????drm_gpuva_unlink(va); > ????????????????????????break; > +???????????????} > ????????????????case DRM_GPUVA_OP_UNMAP: > ????????????????????????drm_gpuva_unlink(op->unmap.va); > ????????????????????????break; > @@ -1150,10 +1158,18 @@ nouveau_uvmm_bind_job_submit(struct > nouveau_job *job) > ? > ????????list_for_each_op(op, &bind_job->ops) { > ????????????????if (op->op == OP_MAP) { > -???????????????????????op->gem.obj = drm_gem_object_lookup(job- > >file_priv, > -?????????????????????????????????????????????????????????? op- > >gem.handle); > -???????????????????????if (!op->gem.obj) > +???????????????????????struct drm_gem_object *obj; > + > +???????????????????????obj = drm_gem_object_lookup(job->file_priv, > +?????????????????????????????????????????????????? op->gem.handle); > +???????????????????????if (!(op->gem.obj = obj)) > ????????????????????????????????return -ENOENT; > + > +???????????????????????dma_resv_lock(obj->resv, NULL); > +???????????????????????op->vm_bo = drm_gpuvm_bo_obtain(&uvmm->base, > obj); > +???????????????????????dma_resv_unlock(obj->resv); > +???????????????????????if (IS_ERR(op->vm_bo)) > +???????????????????????????????return PTR_ERR(op->vm_bo); > ????????????????} > ? > ????????????????ret = bind_validate_op(job, op); > @@ -1364,7 +1380,7 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job > *job) > ????????????????case OP_UNMAP_SPARSE: > ????????????????case OP_MAP: > ????????????????case OP_UNMAP: > -???????????????????????bind_link_gpuvas(op->ops, &op->new); > +???????????????????????bind_link_gpuvas(op); > ????????????????????????break; > ????????????????default: > ????????????????????????break; > @@ -1511,6 +1527,12 @@ nouveau_uvmm_bind_job_free_work_fn(struct > work_struct *work) > ????????????????if (!IS_ERR_OR_NULL(op->ops)) > ????????????????????????drm_gpuva_ops_free(&uvmm->base, op->ops); > ? > +???????????????if (!IS_ERR_OR_NULL(op->vm_bo)) { > +???????????????????????dma_resv_lock(obj->resv, NULL); > +???????????????????????drm_gpuvm_bo_put(op->vm_bo); > +???????????????????????dma_resv_unlock(obj->resv); > +???????????????} > + > ????????????????if (obj) > ????????????????????????drm_gem_object_put(obj); > ????????} > @@ -1776,15 +1798,18 @@ void > ?nouveau_uvmm_bo_map_all(struct nouveau_bo *nvbo, struct nouveau_mem > *mem) > ?{ > ????????struct drm_gem_object *obj = &nvbo->bo.base; > +???????struct drm_gpuvm_bo *vm_bo; > ????????struct drm_gpuva *va; > ? > ????????dma_resv_assert_held(obj->resv); > ? > -???????drm_gem_for_each_gpuva(va, obj) { > -???????????????struct nouveau_uvma *uvma = uvma_from_va(va); > +???????drm_gem_for_each_gpuvm_bo(vm_bo, obj) { > +???????????????drm_gpuvm_bo_for_each_va(va, vm_bo) { > +???????????????????????struct nouveau_uvma *uvma = uvma_from_va(va); > ? > -???????????????nouveau_uvma_map(uvma, mem); > -???????????????drm_gpuva_invalidate(va, false); > +???????????????????????nouveau_uvma_map(uvma, mem); > +???????????????????????drm_gpuva_invalidate(va, false); > +???????????????} > ????????} > ?} > ? > @@ -1792,15 +1817,18 @@ void > ?nouveau_uvmm_bo_unmap_all(struct nouveau_bo *nvbo) > ?{ > ????????struct drm_gem_object *obj = &nvbo->bo.base; > +???????struct drm_gpuvm_bo *vm_bo; > ????????struct drm_gpuva *va; > ? > ????????dma_resv_assert_held(obj->resv); > ? > -???????drm_gem_for_each_gpuva(va, obj) { > -???????????????struct nouveau_uvma *uvma = uvma_from_va(va); > +???????drm_gem_for_each_gpuvm_bo(vm_bo, obj) { > +???????????????drm_gpuvm_bo_for_each_va(va, vm_bo) { > +???????????????????????struct nouveau_uvma *uvma = uvma_from_va(va); > ? > -???????????????nouveau_uvma_unmap(uvma); > -???????????????drm_gpuva_invalidate(va, true); > +???????????????????????nouveau_uvma_unmap(uvma); > +???????????????????????drm_gpuva_invalidate(va, true); > +???????????????} > ????????} > ?} > ? > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > index 16364487fde9..369505447acd 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -580,7 +580,7 @@ int drm_gem_evict(struct drm_gem_object *obj); > ? * drm_gem_gpuva_init() - initialize the gpuva list of a GEM object > ? * @obj: the &drm_gem_object > ? * > - * This initializes the &drm_gem_object's &drm_gpuva list. > + * This initializes the &drm_gem_object's &drm_gpuvm_bo list. > ? * > ? * Calling this function is only necessary for drivers intending to > support the > ? * &drm_driver_feature DRIVER_GEM_GPUVA. > @@ -593,28 +593,28 @@ static inline void drm_gem_gpuva_init(struct > drm_gem_object *obj) > ?} > ? > ?/** > - * drm_gem_for_each_gpuva() - iternator to walk over a list of > gpuvas > - * @entry__: &drm_gpuva structure to assign to in each iteration > step > - * @obj__: the &drm_gem_object the &drm_gpuvas to walk are > associated with > + * drm_gem_for_each_gpuvm_bo() - iterator to walk over a list of > &drm_gpuvm_bo > + * @entry__: &drm_gpuvm_bo structure to assign to in each iteration > step > + * @obj__: the &drm_gem_object the &drm_gpuvm_bo to walk are > associated with > ? * > - * This iterator walks over all &drm_gpuva structures associated > with the > - * &drm_gpuva_manager. > + * This iterator walks over all &drm_gpuvm_bo structures associated > with the > + * &drm_gem_object. > ? */ > -#define drm_gem_for_each_gpuva(entry__, obj__) \ > -???????list_for_each_entry(entry__, &(obj__)->gpuva.list, gem.entry) > +#define drm_gem_for_each_gpuvm_bo(entry__, obj__) \ > +???????list_for_each_entry(entry__, &(obj__)->gpuva.list, > list.entry.gem) > ? > ?/** > - * drm_gem_for_each_gpuva_safe() - iternator to safely walk over a > list of > - * gpuvas > - * @entry__: &drm_gpuva structure to assign to in each iteration > step > - * @next__: &next &drm_gpuva to store the next step > - * @obj__: the &drm_gem_object the &drm_gpuvas to walk are > associated with > + * drm_gem_for_each_gpuvm_bo_safe() - iterator to safely walk over a > list of > + * &drm_gpuvm_bo > + * @entry__: &drm_gpuvm_bostructure to assign to in each iteration > step > + * @next__: &next &drm_gpuvm_bo to store the next step > + * @obj__: the &drm_gem_object the &drm_gpuvm_bo to walk are > associated with > ? * > - * This iterator walks over all &drm_gpuva structures associated > with the > + * This iterator walks over all &drm_gpuvm_bo structures associated > with the > ? * &drm_gem_object. It is implemented with > list_for_each_entry_safe(), hence > ? * it is save against removal of elements. > ? */ > -#define drm_gem_for_each_gpuva_safe(entry__, next__, obj__) \ > -???????list_for_each_entry_safe(entry__, next__, &(obj__)- > >gpuva.list, gem.entry) > +#define drm_gem_for_each_gpuvm_bo_safe(entry__, next__, obj__) \ > +???????list_for_each_entry_safe(entry__, next__, &(obj__)- > >gpuva.list, list.entry.gem) > ? > ?#endif /* __DRM_GEM_H__ */ > diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h > index 47cbacb244b9..466fdd76c71a 100644 > --- a/include/drm/drm_gpuvm.h > +++ b/include/drm/drm_gpuvm.h > @@ -25,6 +25,7 @@ > ? * OTHER DEALINGS IN THE SOFTWARE. > ? */ > ? > +#include <linux/dma-resv.h> > ?#include <linux/list.h> > ?#include <linux/rbtree.h> > ?#include <linux/types.h> > @@ -33,6 +34,7 @@ > ?#include <drm/drm_gem.h> > ? > ?struct drm_gpuvm; > +struct drm_gpuvm_bo; > ?struct drm_gpuvm_ops; > ? > ?/** > @@ -73,6 +75,12 @@ struct drm_gpuva { > ???????? */ > ????????struct drm_gpuvm *vm; > ? > +???????/** > +??????? * @vm_bo: the &drm_gpuvm_bo abstraction for the mapped > +??????? * &drm_gem_object > +??????? */ > +???????struct drm_gpuvm_bo *vm_bo; > + > ????????/** > ???????? * @flags: the &drm_gpuva_flags for this mapping > ???????? */ > @@ -108,7 +116,7 @@ struct drm_gpuva { > ????????????????struct drm_gem_object *obj; > ? > ????????????????/** > -??????????????? * @entry: the &list_head to attach this object to a > &drm_gem_object > +??????????????? * @entry: the &list_head to attach this object to a > &drm_gpuvm_bo > ???????????????? */ > ????????????????struct list_head entry; > ????????} gem; > @@ -141,7 +149,7 @@ struct drm_gpuva { > ?int drm_gpuva_insert(struct drm_gpuvm *gpuvm, struct drm_gpuva *va); > ?void drm_gpuva_remove(struct drm_gpuva *va); > ? > -void drm_gpuva_link(struct drm_gpuva *va); > +void drm_gpuva_link(struct drm_gpuva *va, struct drm_gpuvm_bo > *vm_bo); > ?void drm_gpuva_unlink(struct drm_gpuva *va); > ? > ?struct drm_gpuva *drm_gpuva_find(struct drm_gpuvm *gpuvm, > @@ -188,10 +196,16 @@ static inline bool drm_gpuva_invalidated(struct > drm_gpuva *va) > ? * enum drm_gpuvm_flags - flags for struct drm_gpuvm > ? */ > ?enum drm_gpuvm_flags { > +???????/** > +??????? * @DRM_GPUVM_RESV_PROTECTED: GPUVM is protected externally > by the > +??????? * GPUVM's &dma_resv lock > +??????? */ > +???????DRM_GPUVM_RESV_PROTECTED = BIT(0), > + > ????????/** > ???????? * @DRM_GPUVM_USERBITS: user defined bits > ???????? */ > -???????DRM_GPUVM_USERBITS = BIT(0), > +???????DRM_GPUVM_USERBITS = BIT(1), > ?}; > ? > ?/** > @@ -280,6 +294,19 @@ bool drm_gpuvm_interval_empty(struct drm_gpuvm > *gpuvm, u64 addr, u64 range); > ?struct drm_gem_object * > ?drm_gpuvm_resv_object_alloc(struct drm_device *drm); > ? > +/** > + * drm_gpuvm_resv_protected() - indicates whether > &DRM_GPUVM_RESV_PROTECTED is > + * set > + * @gpuvm: the &drm_gpuvm > + * > + * Returns: true if &DRM_GPUVM_RESV_PROTECTED is set, false > otherwise. > + */ > +static inline bool > +drm_gpuvm_resv_protected(struct drm_gpuvm *gpuvm) > +{ > +???????return gpuvm->flags & DRM_GPUVM_RESV_PROTECTED; > +} > + > ?/** > ? * drm_gpuvm_resv() - returns the &drm_gpuvm's &dma_resv > ? * @gpuvm__: the &drm_gpuvm > @@ -298,6 +325,12 @@ drm_gpuvm_resv_object_alloc(struct drm_device > *drm); > ? */ > ?#define drm_gpuvm_resv_obj(gpuvm__) ((gpuvm__)->r_obj) > ? > +#define drm_gpuvm_resv_held(gpuvm__) \ > +???????dma_resv_held(drm_gpuvm_resv(gpuvm__)) > + > +#define drm_gpuvm_resv_assert_held(gpuvm__) \ > +???????dma_resv_assert_held(drm_gpuvm_resv(gpuvm__)) > + > ?#define drm_gpuvm_resv_held(gpuvm__) \ > ????????dma_resv_held(drm_gpuvm_resv(gpuvm__)) > ? > @@ -382,6 +415,128 @@ __drm_gpuva_next(struct drm_gpuva *va) > ?#define drm_gpuvm_for_each_va_safe(va__, next__, gpuvm__) \ > ????????list_for_each_entry_safe(va__, next__, &(gpuvm__)->rb.list, > rb.entry) > ? > +/** > + * struct drm_gpuvm_bo - structure representing a &drm_gpuvm and > + * &drm_gem_object combination > + * > + * This structure is an abstraction representing a &drm_gpuvm and > + * &drm_gem_object combination. It serves as an indirection to > accelerate > + * iterating all &drm_gpuvas within a &drm_gpuvm backed by the same > + * &drm_gem_object. > + * > + * Furthermore it is used cache evicted GEM objects for a certain > GPU-VM to > + * accelerate validation. > + * > + * Typically, drivers want to create an instance of a struct > drm_gpuvm_bo once > + * a GEM object is mapped first in a GPU-VM and release the instance > once the > + * last mapping of the GEM object in this GPU-VM is unmapped. > + */ > +struct drm_gpuvm_bo { > +???????/** > +??????? * @vm: The &drm_gpuvm the @obj is mapped in. This pointer is > not > +??????? * reference counted. > +??????? * > +??????? * A struct drm_gpuvm_bo is not allowed to out-live its > &drm_gpuvm > +??????? * context. Implicitly, this is ensured by the fact that the > driver is > +??????? * responsible to ensure the VM doesn't contain mappings once > it's > +??????? * freed, since a struct drm_gpuvm_bo should be freed once > the last > +??????? * mapping being backed by the corresponding buffer object is > unmapped. > +??????? */I don't think the above is completely true. Let's assume in the !RESV_PROTECTED case that a reference is grabbed on the drm_gpuvm_bo during an iteration over a list. Then user-space closes the vm and all vmas are unlinked, but this reference remains but the vm pointer becomes stale. In the RESV_PROTECTED case this is ensured not to happen if by the vm->resv being grabbed during unlink, but in the !RESV_PROTECTED case, the above wording isn't sufficient. The caller needs to ensure the vm stays alive using some sort of similar rule or use kref_get_unless_zero() on the vm under the spinlock if dereferenced.> +???????struct drm_gpuvm *vm; > + > +???????/** > +??????? * @obj: The &drm_gem_object being mapped in @vm. This is a > reference > +??????? * counted pointer. > +??????? */ > +???????struct drm_gem_object *obj; > + > +???????/** > +??????? * @kref: The reference count for this &drm_gpuvm_bo. > +??????? */ > +???????struct kref kref; > + > +???????/** > +??????? * @list: Structure containing all &list_heads. > +??????? */ > +???????struct { > +???????????????/** > +??????????????? * @gpuva: The list of linked &drm_gpuvas. > +??????????????? */ > +???????????????struct list_head gpuva;Still missing doc on how the @gpuva stays alive during iteration over the list? 8<------------------------------------------------------------- Thanks, Thomas
Jani Nikula
2023-Oct-31 11:45 UTC
[Nouveau] [PATCH drm-misc-next v7 4/7] drm/gpuvm: add an abstraction for a VM / BO combination
On Tue, 31 Oct 2023, Thomas Hellstr?m <thomas.hellstrom at linux.intel.com> wrote:> On Mon, 2023-10-23 at 22:16 +0200, Danilo Krummrich wrote: >> + * Returns: a pointer to the &drm_gpuvm_bo on success, NULL on > > Still needs s/Returns:/Return:/gFWIW, both work to accommodate the variance across the kernel, although I think only the latter is documented and recommended. It's also the most popular: 10577 Return 3596 Returns 1104 RETURN 568 return 367 returns 352 RETURNS 1 RETURNs BR, Jani. -- Jani Nikula, Intel
Danilo Krummrich
2023-Oct-31 17:38 UTC
[Nouveau] [PATCH drm-misc-next v7 4/7] drm/gpuvm: add an abstraction for a VM / BO combination
On 10/31/23 11:32, Thomas Hellstr?m wrote:> On Mon, 2023-10-23 at 22:16 +0200, 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 at amd.com> >> Signed-off-by: Danilo Krummrich <dakr at redhat.com> >> --- >> ?drivers/gpu/drm/drm_gpuvm.c??????????? | 335 +++++++++++++++++++++-- >> -- >> ?drivers/gpu/drm/nouveau/nouveau_uvmm.c |? 64 +++-- >> ?include/drm/drm_gem.h????????????????? |? 32 +-- >> ?include/drm/drm_gpuvm.h??????????????? | 188 +++++++++++++- >> ?4 files changed, 533 insertions(+), 86 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gpuvm.c >> b/drivers/gpu/drm/drm_gpuvm.c >> index c03332883432..7f4f5919f84c 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; >> ? *?????} >> ? * >> @@ -774,6 +809,194 @@ drm_gpuvm_destroy(struct drm_gpuvm *gpuvm) >> ?} >> ?EXPORT_SYMBOL_GPL(drm_gpuvm_destroy); >> >> +/** >> + * 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 > > Still needs s/Returns:/Return:/g > >> 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 = 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_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. >> + */ >> +void >> +drm_gpuvm_bo_put(struct drm_gpuvm_bo *vm_bo) >> +{ >> +???????if (vm_bo) >> +???????????????kref_put(&vm_bo->kref, drm_gpuvm_bo_destroy); >> +} >> +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_put); >> + >> +static struct drm_gpuvm_bo * >> +__drm_gpuvm_bo_find(struct drm_gpuvm *gpuvm, >> +?????????????????? struct drm_gem_object *obj) >> +{ >> +???????struct drm_gpuvm_bo *vm_bo; >> + >> +???????drm_gem_gpuva_assert_lock_held(obj); >> +???????drm_gem_for_each_gpuvm_bo(vm_bo, obj) >> +???????????????if (vm_bo->vm == gpuvm) >> +???????????????????????return vm_bo; >> + >> +???????return NULL; >> +} >> + >> +/** >> + * drm_gpuvm_bo_find() - find the &drm_gpuvm_bo for the given >> + * &drm_gpuvm and &drm_gem_object >> + * @gpuvm: The &drm_gpuvm the @obj is mapped in. >> + * @obj: The &drm_gem_object being mapped in the @gpuvm. >> + * >> + * Find the &drm_gpuvm_bo representing the combination of the given >> + * &drm_gpuvm and &drm_gem_object. If found, increases the reference >> + * count of the &drm_gpuvm_bo accordingly. >> + * >> + * Returns: a pointer to the &drm_gpuvm_bo on success, NULL on >> failure >> + */ >> +struct drm_gpuvm_bo * >> +drm_gpuvm_bo_find(struct drm_gpuvm *gpuvm, >> +???????????????? struct drm_gem_object *obj) >> +{ >> +???????struct drm_gpuvm_bo *vm_bo = __drm_gpuvm_bo_find(gpuvm, obj); >> + >> +???????return vm_bo ? drm_gpuvm_bo_get(vm_bo) : NULL; >> +} >> +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_find); >> + >> +/** >> + * drm_gpuvm_bo_obtain() - obtains and instance of the &drm_gpuvm_bo >> for the >> + * given &drm_gpuvm and &drm_gem_object >> + * @gpuvm: The &drm_gpuvm the @obj is mapped in. >> + * @obj: The &drm_gem_object being mapped in the @gpuvm. >> + * >> + * Find the &drm_gpuvm_bo representing the combination of the given >> + * &drm_gpuvm and &drm_gem_object. If found, increases the reference >> + * count of the &drm_gpuvm_bo accordingly. If not found, allocates a >> new >> + * &drm_gpuvm_bo. >> + * >> + * A new &drm_gpuvm_bo is added to the GEMs gpuva list. >> + * >> + * Returns: a pointer to the &drm_gpuvm_bo on success, an ERR_PTR on >> failure >> + */ >> +struct drm_gpuvm_bo * >> +drm_gpuvm_bo_obtain(struct drm_gpuvm *gpuvm, >> +?????????????????? struct drm_gem_object *obj) >> +{ >> +???????struct drm_gpuvm_bo *vm_bo; >> + >> +???????vm_bo = drm_gpuvm_bo_find(gpuvm, obj); >> +???????if (vm_bo) >> +???????????????return vm_bo; >> + >> +???????vm_bo = drm_gpuvm_bo_create(gpuvm, obj); >> +???????if (!vm_bo) >> +???????????????return ERR_PTR(-ENOMEM); >> + >> +???????drm_gem_gpuva_assert_lock_held(obj); >> +???????list_add_tail(&vm_bo->list.entry.gem, &obj->gpuva.list); >> + >> +???????return vm_bo; >> +} >> +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_obtain); >> + >> +/** >> + * drm_gpuvm_bo_obtain_prealloc() - obtains and instance of the >> &drm_gpuvm_bo >> + * for the given &drm_gpuvm and &drm_gem_object >> + * @__vm_bo: A pre-allocated struct drm_gpuvm_bo. >> + * >> + * Find the &drm_gpuvm_bo representing the combination of the given >> + * &drm_gpuvm and &drm_gem_object. If found, increases the reference >> + * count of the found &drm_gpuvm_bo accordingly, while the @__vm_bo >> reference >> + * count is decreased. If not found @__vm_bo is returned without >> further >> + * increase of the reference count. >> + * >> + * A new &drm_gpuvm_bo is added to the GEMs gpuva list. >> + * >> + * Returns: a pointer to the found &drm_gpuvm_bo or @__vm_bo if no >> existing >> + * &drm_gpuvm_bo was found >> + */ >> +struct drm_gpuvm_bo * >> +drm_gpuvm_bo_obtain_prealloc(struct drm_gpuvm_bo *__vm_bo) >> +{ >> +???????struct drm_gpuvm *gpuvm = __vm_bo->vm; >> +???????struct drm_gem_object *obj = __vm_bo->obj; >> +???????struct drm_gpuvm_bo *vm_bo; >> + >> +???????vm_bo = drm_gpuvm_bo_find(gpuvm, obj); >> +???????if (vm_bo) { >> +???????????????drm_gpuvm_bo_put(__vm_bo); >> +???????????????return vm_bo; >> +???????} >> + >> +???????drm_gem_gpuva_assert_lock_held(obj); >> +???????list_add_tail(&__vm_bo->list.entry.gem, &obj->gpuva.list); >> + >> +???????return __vm_bo; >> +} >> +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_obtain_prealloc); >> + >> ?static int >> ?__drm_gpuva_insert(struct drm_gpuvm *gpuvm, >> ?????????????????? struct drm_gpuva *va) >> @@ -864,24 +1087,33 @@ EXPORT_SYMBOL_GPL(drm_gpuva_remove); >> ?/** >> ? * drm_gpuva_link() - link a &drm_gpuva >> ? * @va: the &drm_gpuva to link >> + * @vm_bo: the &drm_gpuvm_bo to add the &drm_gpuva to >> ? * >> - * This adds the given &va to the GPU VA list of the &drm_gem_object >> it is >> - * associated with. >> + * This adds the given &va to the GPU VA list of the &drm_gpuvm_bo >> and the >> + * &drm_gpuvm_bo to the &drm_gem_object it is associated with. >> + * >> + * For every &drm_gpuva entry added to the &drm_gpuvm_bo an >> additional >> + * reference of the latter is taken. >> ? * >> ? * This function expects the caller to protect the GEM's GPUVA list >> against >> - * concurrent access using the GEMs dma_resv lock. >> + * concurrent access using either the GEMs dma_resv lock or a driver >> specific >> + * lock set through drm_gem_gpuva_set_lock(). >> ? */ >> ?void >> -drm_gpuva_link(struct drm_gpuva *va) >> +drm_gpuva_link(struct drm_gpuva *va, struct drm_gpuvm_bo *vm_bo) >> ?{ >> ????????struct drm_gem_object *obj = va->gem.obj; >> +???????struct drm_gpuvm *gpuvm = va->vm; >> >> ????????if (unlikely(!obj)) >> ????????????????return; >> >> -???????drm_gem_gpuva_assert_lock_held(obj); >> +???????drm_WARN_ON(gpuvm->drm, obj != vm_bo->obj); >> >> -???????list_add_tail(&va->gem.entry, &obj->gpuva.list); >> +???????va->vm_bo = drm_gpuvm_bo_get(vm_bo); >> + >> +???????drm_gem_gpuva_assert_lock_held(obj); >> +???????list_add_tail(&va->gem.entry, &vm_bo->list.gpuva); >> ?} >> ?EXPORT_SYMBOL_GPL(drm_gpuva_link); >> >> @@ -892,20 +1124,31 @@ EXPORT_SYMBOL_GPL(drm_gpuva_link); >> ? * This removes the given &va from the GPU VA list of the >> &drm_gem_object it is >> ? * associated with. >> ? * >> + * This removes the given &va from the GPU VA list of the >> &drm_gpuvm_bo and >> + * the &drm_gpuvm_bo from the &drm_gem_object it is associated with >> in case >> + * this call unlinks the last &drm_gpuva from the &drm_gpuvm_bo. >> + * >> + * For every &drm_gpuva entry removed from the &drm_gpuvm_bo a >> reference of >> + * the latter is dropped. >> + * >> ? * This function expects the caller to protect the GEM's GPUVA list >> against >> - * concurrent access using the GEMs dma_resv lock. >> + * concurrent access using either the GEMs dma_resv lock or a driver >> specific >> + * lock set through drm_gem_gpuva_set_lock(). >> ? */ >> ?void >> ?drm_gpuva_unlink(struct drm_gpuva *va) >> ?{ >> ????????struct drm_gem_object *obj = va->gem.obj; >> +???????struct drm_gpuvm_bo *vm_bo = va->vm_bo; >> >> ????????if (unlikely(!obj)) >> ????????????????return; >> >> ????????drm_gem_gpuva_assert_lock_held(obj); >> - >> ????????list_del_init(&va->gem.entry); >> + >> +???????va->vm_bo = NULL; >> +???????drm_gpuvm_bo_put(vm_bo); >> ?} >> ?EXPORT_SYMBOL_GPL(drm_gpuva_unlink); >> >> @@ -1050,10 +1293,10 @@ drm_gpuva_remap(struct drm_gpuva *prev, >> ????????????????struct drm_gpuva *next, >> ????????????????struct drm_gpuva_op_remap *op) >> ?{ >> -???????struct drm_gpuva *curr = op->unmap->va; >> -???????struct drm_gpuvm *gpuvm = curr->vm; >> +???????struct drm_gpuva *va = op->unmap->va; >> +???????struct drm_gpuvm *gpuvm = va->vm; >> >> -???????drm_gpuva_remove(curr); >> +???????drm_gpuva_remove(va); >> >> ????????if (op->prev) { >> ????????????????drm_gpuva_init_from_op(prev, op->prev); >> @@ -1695,9 +1938,8 @@ drm_gpuvm_prefetch_ops_create(struct drm_gpuvm >> *gpuvm, >> ?EXPORT_SYMBOL_GPL(drm_gpuvm_prefetch_ops_create); >> >> ?/** >> - * drm_gpuvm_gem_unmap_ops_create() - creates the &drm_gpuva_ops to >> unmap a GEM >> - * @gpuvm: the &drm_gpuvm representing the GPU VA space >> - * @obj: the &drm_gem_object to unmap >> + * drm_gpuvm_bo_unmap_ops_create() - creates the &drm_gpuva_ops to >> unmap a GEM >> + * @vm_bo: the &drm_gpuvm_bo abstraction >> ? * >> ? * This function creates a list of operations to perform unmapping >> for every >> ? * GPUVA attached to a GEM. >> @@ -1714,15 +1956,14 @@ >> EXPORT_SYMBOL_GPL(drm_gpuvm_prefetch_ops_create); >> ? * Returns: a pointer to the &drm_gpuva_ops on success, an ERR_PTR >> on failure >> ? */ >> ?struct drm_gpuva_ops * >> -drm_gpuvm_gem_unmap_ops_create(struct drm_gpuvm *gpuvm, >> -????????????????????????????? struct drm_gem_object *obj) >> +drm_gpuvm_bo_unmap_ops_create(struct drm_gpuvm_bo *vm_bo) >> ?{ >> ????????struct drm_gpuva_ops *ops; >> ????????struct drm_gpuva_op *op; >> ????????struct drm_gpuva *va; >> ????????int ret; >> >> -???????drm_gem_gpuva_assert_lock_held(obj); >> +???????drm_gem_gpuva_assert_lock_held(vm_bo->obj); >> >> ????????ops = kzalloc(sizeof(*ops), GFP_KERNEL); >> ????????if (!ops) >> @@ -1730,8 +1971,8 @@ drm_gpuvm_gem_unmap_ops_create(struct drm_gpuvm >> *gpuvm, >> >> ????????INIT_LIST_HEAD(&ops->list); >> >> -???????drm_gem_for_each_gpuva(va, obj) { >> -???????????????op = gpuva_op_alloc(gpuvm); >> +???????drm_gpuvm_bo_for_each_va(va, vm_bo) { >> +???????????????op = gpuva_op_alloc(vm_bo->vm); >> ????????????????if (!op) { >> ????????????????????????ret = -ENOMEM; >> ????????????????????????goto err_free_ops; >> @@ -1745,10 +1986,10 @@ drm_gpuvm_gem_unmap_ops_create(struct >> drm_gpuvm *gpuvm, >> ????????return ops; >> >> ?err_free_ops: >> -???????drm_gpuva_ops_free(gpuvm, ops); >> +???????drm_gpuva_ops_free(vm_bo->vm, ops); >> ????????return ERR_PTR(ret); >> ?} >> -EXPORT_SYMBOL_GPL(drm_gpuvm_gem_unmap_ops_create); >> +EXPORT_SYMBOL_GPL(drm_gpuvm_bo_unmap_ops_create); >> >> ?/** >> ? * drm_gpuva_ops_free() - free the given &drm_gpuva_ops >> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c >> b/drivers/gpu/drm/nouveau/nouveau_uvmm.c >> index ed439bf4032f..1e95b0a1b047 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c >> @@ -62,6 +62,8 @@ struct bind_job_op { >> ????????enum vm_bind_op op; >> ????????u32 flags; >> >> +???????struct drm_gpuvm_bo *vm_bo; >> + >> ????????struct { >> ????????????????u64 addr; >> ????????????????u64 range; >> @@ -1113,22 +1115,28 @@ bind_validate_region(struct nouveau_job *job) >> ?} >> >> ?static void >> -bind_link_gpuvas(struct drm_gpuva_ops *ops, struct >> nouveau_uvma_prealloc *new) >> +bind_link_gpuvas(struct bind_job_op *bop) >> ?{ >> +???????struct nouveau_uvma_prealloc *new = &bop->new; >> +???????struct drm_gpuvm_bo *vm_bo = bop->vm_bo; >> +???????struct drm_gpuva_ops *ops = bop->ops; >> ????????struct drm_gpuva_op *op; >> >> ????????drm_gpuva_for_each_op(op, ops) { >> ????????????????switch (op->op) { >> ????????????????case DRM_GPUVA_OP_MAP: >> -???????????????????????drm_gpuva_link(&new->map->va); >> +???????????????????????drm_gpuva_link(&new->map->va, vm_bo); >> ????????????????????????break; >> -???????????????case DRM_GPUVA_OP_REMAP: >> +???????????????case DRM_GPUVA_OP_REMAP: { >> +???????????????????????struct drm_gpuva *va = op->remap.unmap->va; >> + >> ????????????????????????if (op->remap.prev) >> -???????????????????????????????drm_gpuva_link(&new->prev->va); >> +???????????????????????????????drm_gpuva_link(&new->prev->va, va- >>> vm_bo); >> ????????????????????????if (op->remap.next) >> -???????????????????????????????drm_gpuva_link(&new->next->va); >> -???????????????????????drm_gpuva_unlink(op->remap.unmap->va); >> +???????????????????????????????drm_gpuva_link(&new->next->va, va- >>> vm_bo); >> +???????????????????????drm_gpuva_unlink(va); >> ????????????????????????break; >> +???????????????} >> ????????????????case DRM_GPUVA_OP_UNMAP: >> ????????????????????????drm_gpuva_unlink(op->unmap.va); >> ????????????????????????break; >> @@ -1150,10 +1158,18 @@ nouveau_uvmm_bind_job_submit(struct >> nouveau_job *job) >> >> ????????list_for_each_op(op, &bind_job->ops) { >> ????????????????if (op->op == OP_MAP) { >> -???????????????????????op->gem.obj = drm_gem_object_lookup(job- >>> file_priv, >> -?????????????????????????????????????????????????????????? op- >>> gem.handle); >> -???????????????????????if (!op->gem.obj) >> +???????????????????????struct drm_gem_object *obj; >> + >> +???????????????????????obj = drm_gem_object_lookup(job->file_priv, >> +?????????????????????????????????????????????????? op->gem.handle); >> +???????????????????????if (!(op->gem.obj = obj)) >> ????????????????????????????????return -ENOENT; >> + >> +???????????????????????dma_resv_lock(obj->resv, NULL); >> +???????????????????????op->vm_bo = drm_gpuvm_bo_obtain(&uvmm->base, >> obj); >> +???????????????????????dma_resv_unlock(obj->resv); >> +???????????????????????if (IS_ERR(op->vm_bo)) >> +???????????????????????????????return PTR_ERR(op->vm_bo); >> ????????????????} >> >> ????????????????ret = bind_validate_op(job, op); >> @@ -1364,7 +1380,7 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job >> *job) >> ????????????????case OP_UNMAP_SPARSE: >> ????????????????case OP_MAP: >> ????????????????case OP_UNMAP: >> -???????????????????????bind_link_gpuvas(op->ops, &op->new); >> +???????????????????????bind_link_gpuvas(op); >> ????????????????????????break; >> ????????????????default: >> ????????????????????????break; >> @@ -1511,6 +1527,12 @@ nouveau_uvmm_bind_job_free_work_fn(struct >> work_struct *work) >> ????????????????if (!IS_ERR_OR_NULL(op->ops)) >> ????????????????????????drm_gpuva_ops_free(&uvmm->base, op->ops); >> >> +???????????????if (!IS_ERR_OR_NULL(op->vm_bo)) { >> +???????????????????????dma_resv_lock(obj->resv, NULL); >> +???????????????????????drm_gpuvm_bo_put(op->vm_bo); >> +???????????????????????dma_resv_unlock(obj->resv); >> +???????????????} >> + >> ????????????????if (obj) >> ????????????????????????drm_gem_object_put(obj); >> ????????} >> @@ -1776,15 +1798,18 @@ void >> ?nouveau_uvmm_bo_map_all(struct nouveau_bo *nvbo, struct nouveau_mem >> *mem) >> ?{ >> ????????struct drm_gem_object *obj = &nvbo->bo.base; >> +???????struct drm_gpuvm_bo *vm_bo; >> ????????struct drm_gpuva *va; >> >> ????????dma_resv_assert_held(obj->resv); >> >> -???????drm_gem_for_each_gpuva(va, obj) { >> -???????????????struct nouveau_uvma *uvma = uvma_from_va(va); >> +???????drm_gem_for_each_gpuvm_bo(vm_bo, obj) { >> +???????????????drm_gpuvm_bo_for_each_va(va, vm_bo) { >> +???????????????????????struct nouveau_uvma *uvma = uvma_from_va(va); >> >> -???????????????nouveau_uvma_map(uvma, mem); >> -???????????????drm_gpuva_invalidate(va, false); >> +???????????????????????nouveau_uvma_map(uvma, mem); >> +???????????????????????drm_gpuva_invalidate(va, false); >> +???????????????} >> ????????} >> ?} >> >> @@ -1792,15 +1817,18 @@ void >> ?nouveau_uvmm_bo_unmap_all(struct nouveau_bo *nvbo) >> ?{ >> ????????struct drm_gem_object *obj = &nvbo->bo.base; >> +???????struct drm_gpuvm_bo *vm_bo; >> ????????struct drm_gpuva *va; >> >> ????????dma_resv_assert_held(obj->resv); >> >> -???????drm_gem_for_each_gpuva(va, obj) { >> -???????????????struct nouveau_uvma *uvma = uvma_from_va(va); >> +???????drm_gem_for_each_gpuvm_bo(vm_bo, obj) { >> +???????????????drm_gpuvm_bo_for_each_va(va, vm_bo) { >> +???????????????????????struct nouveau_uvma *uvma = uvma_from_va(va); >> >> -????????????????nouveau_uvma_unmap(uvma); >> -???????????????drm_gpuva_invalidate(va, true); >> +???????????????????????nouveau_uvma_unmap(uvma); >> +???????????????????????drm_gpuva_invalidate(va, true); >> +???????????????} >> ????????} >> ?} >> >> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h >> index 16364487fde9..369505447acd 100644 >> --- a/include/drm/drm_gem.h >> +++ b/include/drm/drm_gem.h >> @@ -580,7 +580,7 @@ int drm_gem_evict(struct drm_gem_object *obj); >> ? * drm_gem_gpuva_init() - initialize the gpuva list of a GEM object >> ? * @obj: the &drm_gem_object >> ? * >> - * This initializes the &drm_gem_object's &drm_gpuva list. >> + * This initializes the &drm_gem_object's &drm_gpuvm_bo list. >> ? * >> ? * Calling this function is only necessary for drivers intending to >> support the >> ? * &drm_driver_feature DRIVER_GEM_GPUVA. >> @@ -593,28 +593,28 @@ static inline void drm_gem_gpuva_init(struct >> drm_gem_object *obj) >> ?} >> >> ?/** >> - * drm_gem_for_each_gpuva() - iternator to walk over a list of >> gpuvas >> - * @entry__: &drm_gpuva structure to assign to in each iteration >> step >> - * @obj__: the &drm_gem_object the &drm_gpuvas to walk are >> associated with >> + * drm_gem_for_each_gpuvm_bo() - iterator to walk over a list of >> &drm_gpuvm_bo >> + * @entry__: &drm_gpuvm_bo structure to assign to in each iteration >> step >> + * @obj__: the &drm_gem_object the &drm_gpuvm_bo to walk are >> associated with >> ? * >> - * This iterator walks over all &drm_gpuva structures associated >> with the >> - * &drm_gpuva_manager. >> + * This iterator walks over all &drm_gpuvm_bo structures associated >> with the >> + * &drm_gem_object. >> ? */ >> -#define drm_gem_for_each_gpuva(entry__, obj__) \ >> -???????list_for_each_entry(entry__, &(obj__)->gpuva.list, gem.entry) >> +#define drm_gem_for_each_gpuvm_bo(entry__, obj__) \ >> +???????list_for_each_entry(entry__, &(obj__)->gpuva.list, >> list.entry.gem) >> >> ?/** >> - * drm_gem_for_each_gpuva_safe() - iternator to safely walk over a >> list of >> - * gpuvas >> - * @entry__: &drm_gpuva structure to assign to in each iteration >> step >> - * @next__: &next &drm_gpuva to store the next step >> - * @obj__: the &drm_gem_object the &drm_gpuvas to walk are >> associated with >> + * drm_gem_for_each_gpuvm_bo_safe() - iterator to safely walk over a >> list of >> + * &drm_gpuvm_bo >> + * @entry__: &drm_gpuvm_bostructure to assign to in each iteration >> step >> + * @next__: &next &drm_gpuvm_bo to store the next step >> + * @obj__: the &drm_gem_object the &drm_gpuvm_bo to walk are >> associated with >> ? * >> - * This iterator walks over all &drm_gpuva structures associated >> with the >> + * This iterator walks over all &drm_gpuvm_bo structures associated >> with the >> ? * &drm_gem_object. It is implemented with >> list_for_each_entry_safe(), hence >> ? * it is save against removal of elements. >> ? */ >> -#define drm_gem_for_each_gpuva_safe(entry__, next__, obj__) \ >> -???????list_for_each_entry_safe(entry__, next__, &(obj__)- >>> gpuva.list, gem.entry) >> +#define drm_gem_for_each_gpuvm_bo_safe(entry__, next__, obj__) \ >> +???????list_for_each_entry_safe(entry__, next__, &(obj__)- >>> gpuva.list, list.entry.gem) >> >> ?#endif /* __DRM_GEM_H__ */ >> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h >> index 47cbacb244b9..466fdd76c71a 100644 >> --- a/include/drm/drm_gpuvm.h >> +++ b/include/drm/drm_gpuvm.h >> @@ -25,6 +25,7 @@ >> ? * OTHER DEALINGS IN THE SOFTWARE. >> ? */ >> >> +#include <linux/dma-resv.h> >> ?#include <linux/list.h> >> ?#include <linux/rbtree.h> >> ?#include <linux/types.h> >> @@ -33,6 +34,7 @@ >> ?#include <drm/drm_gem.h> >> >> ?struct drm_gpuvm; >> +struct drm_gpuvm_bo; >> ?struct drm_gpuvm_ops; >> >> ?/** >> @@ -73,6 +75,12 @@ struct drm_gpuva { >> ???????? */ >> ????????struct drm_gpuvm *vm; >> >> +???????/** >> +??????? * @vm_bo: the &drm_gpuvm_bo abstraction for the mapped >> +??????? * &drm_gem_object >> +??????? */ >> +???????struct drm_gpuvm_bo *vm_bo; >> + >> ????????/** >> ???????? * @flags: the &drm_gpuva_flags for this mapping >> ???????? */ >> @@ -108,7 +116,7 @@ struct drm_gpuva { >> ????????????????struct drm_gem_object *obj; >> >> ????????????????/** >> -??????????????? * @entry: the &list_head to attach this object to a >> &drm_gem_object >> +??????????????? * @entry: the &list_head to attach this object to a >> &drm_gpuvm_bo >> ???????????????? */ >> ????????????????struct list_head entry; >> ????????} gem; >> @@ -141,7 +149,7 @@ struct drm_gpuva { >> ?int drm_gpuva_insert(struct drm_gpuvm *gpuvm, struct drm_gpuva *va); >> ?void drm_gpuva_remove(struct drm_gpuva *va); >> >> -void drm_gpuva_link(struct drm_gpuva *va); >> +void drm_gpuva_link(struct drm_gpuva *va, struct drm_gpuvm_bo >> *vm_bo); >> ?void drm_gpuva_unlink(struct drm_gpuva *va); >> >> ?struct drm_gpuva *drm_gpuva_find(struct drm_gpuvm *gpuvm, >> @@ -188,10 +196,16 @@ static inline bool drm_gpuva_invalidated(struct >> drm_gpuva *va) >> ? * enum drm_gpuvm_flags - flags for struct drm_gpuvm >> ? */ >> ?enum drm_gpuvm_flags { >> +???????/** >> +??????? * @DRM_GPUVM_RESV_PROTECTED: GPUVM is protected externally >> by the >> +??????? * GPUVM's &dma_resv lock >> +??????? */ >> +???????DRM_GPUVM_RESV_PROTECTED = BIT(0), >> + >> ????????/** >> ???????? * @DRM_GPUVM_USERBITS: user defined bits >> ???????? */ >> -???????DRM_GPUVM_USERBITS = BIT(0), >> +???????DRM_GPUVM_USERBITS = BIT(1), >> ?}; >> >> ?/** >> @@ -280,6 +294,19 @@ bool drm_gpuvm_interval_empty(struct drm_gpuvm >> *gpuvm, u64 addr, u64 range); >> ?struct drm_gem_object * >> ?drm_gpuvm_resv_object_alloc(struct drm_device *drm); >> >> +/** >> + * drm_gpuvm_resv_protected() - indicates whether >> &DRM_GPUVM_RESV_PROTECTED is >> + * set >> + * @gpuvm: the &drm_gpuvm >> + * >> + * Returns: true if &DRM_GPUVM_RESV_PROTECTED is set, false >> otherwise. >> + */ >> +static inline bool >> +drm_gpuvm_resv_protected(struct drm_gpuvm *gpuvm) >> +{ >> +???????return gpuvm->flags & DRM_GPUVM_RESV_PROTECTED; >> +} >> + >> ?/** >> ? * drm_gpuvm_resv() - returns the &drm_gpuvm's &dma_resv >> ? * @gpuvm__: the &drm_gpuvm >> @@ -298,6 +325,12 @@ drm_gpuvm_resv_object_alloc(struct drm_device >> *drm); >> ? */ >> ?#define drm_gpuvm_resv_obj(gpuvm__) ((gpuvm__)->r_obj) >> >> +#define drm_gpuvm_resv_held(gpuvm__) \ >> +???????dma_resv_held(drm_gpuvm_resv(gpuvm__)) >> + >> +#define drm_gpuvm_resv_assert_held(gpuvm__) \ >> +???????dma_resv_assert_held(drm_gpuvm_resv(gpuvm__)) >> + >> ?#define drm_gpuvm_resv_held(gpuvm__) \ >> ????????dma_resv_held(drm_gpuvm_resv(gpuvm__)) >> >> @@ -382,6 +415,128 @@ __drm_gpuva_next(struct drm_gpuva *va) >> ?#define drm_gpuvm_for_each_va_safe(va__, next__, gpuvm__) \ >> ????????list_for_each_entry_safe(va__, next__, &(gpuvm__)->rb.list, >> rb.entry) >> >> +/** >> + * struct drm_gpuvm_bo - structure representing a &drm_gpuvm and >> + * &drm_gem_object combination >> + * >> + * This structure is an abstraction representing a &drm_gpuvm and >> + * &drm_gem_object combination. It serves as an indirection to >> accelerate >> + * iterating all &drm_gpuvas within a &drm_gpuvm backed by the same >> + * &drm_gem_object. >> + * >> + * Furthermore it is used cache evicted GEM objects for a certain >> GPU-VM to >> + * accelerate validation. >> + * >> + * Typically, drivers want to create an instance of a struct >> drm_gpuvm_bo once >> + * a GEM object is mapped first in a GPU-VM and release the instance >> once the >> + * last mapping of the GEM object in this GPU-VM is unmapped. >> + */ >> +struct drm_gpuvm_bo { >> +???????/** >> +??????? * @vm: The &drm_gpuvm the @obj is mapped in. This pointer is >> not >> +??????? * reference counted. >> +??????? * >> +??????? * A struct drm_gpuvm_bo is not allowed to out-live its >> &drm_gpuvm >> +??????? * context. Implicitly, this is ensured by the fact that the >> driver is >> +??????? * responsible to ensure the VM doesn't contain mappings once >> it's >> +??????? * freed, since a struct drm_gpuvm_bo should be freed once >> the last >> +??????? * mapping being backed by the corresponding buffer object is >> unmapped. >> +??????? */ > > > I don't think the above is completely true. Let's assume in the > !RESV_PROTECTED case that a reference is grabbed on the drm_gpuvm_bo > during an iteration over a list. Then user-space closes the vm and all > vmas are unlinked, but this reference remains but the vm pointer > becomes stale. In the RESV_PROTECTED case this is ensured not to happen > if by the vm->resv being grabbed during unlink, but in the > !RESV_PROTECTED case, the above wording isn't sufficient. The caller > needs to ensure the vm stays alive using some sort of similar rule or > use kref_get_unless_zero() on the vm under the spinlock if > dereferenced.The list is part of the GPUVM. Hence, the caller *must* either already hold a reference to the GPUVM or otherwise ensure it's not freed while iterating this list. All the drm_gpuvm_bo structures within this list can't have a pointer to another VM than this one by definition. Anyway, I recognize that this isn't very obvious. Hence, I think we should probably reference count GPUVMs as well. I'd think of the same way we do it with drm_gem_objects. However, I'd prefer to introduce this with a subsequent patch.> >> +???????struct drm_gpuvm *vm; >> + >> +???????/** >> +??????? * @obj: The &drm_gem_object being mapped in @vm. This is a >> reference >> +??????? * counted pointer. >> +??????? */ >> +???????struct drm_gem_object *obj; >> + >> +???????/** >> +??????? * @kref: The reference count for this &drm_gpuvm_bo. >> +??????? */ >> +???????struct kref kref; >> + >> +???????/** >> +??????? * @list: Structure containing all &list_heads. >> +??????? */ >> +???????struct { >> +???????????????/** >> +??????????????? * @gpuva: The list of linked &drm_gpuvas. >> +??????????????? */ >> +???????????????struct list_head gpuva; > > Still missing doc on how the @gpuva stays alive during iteration over > the list?Thanks for pointing this out again, I missed that one. - Danilo> > > > 8<------------------------------------------------------------- > > Thanks, > Thomas >