Messages in this thread | | | Date | Mon, 9 Oct 2023 01:08:25 +0200 | Subject | Re: [PATCH drm-misc-next v5 3/6] drm/gpuvm: add an abstraction for a VM / BO combination | From | Danilo Krummrich <> |
| |
On 10/5/23 13:51, Thomas Hellström wrote: > Hi, > > On 9/28/23 21:16, Danilo Krummrich wrote: >> This patch adds an abstraction layer between the drm_gpuva mappings of > NIT: imperative: s/This patch adds/Add/ >> 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> >> Signed-off-by: Danilo Krummrich <dakr@redhat.com> >> --- >> drivers/gpu/drm/drm_gpuvm.c | 334 +++++++++++++++++++++---- >> drivers/gpu/drm/nouveau/nouveau_uvmm.c | 64 +++-- >> include/drm/drm_gem.h | 32 +-- >> include/drm/drm_gpuvm.h | 177 ++++++++++++- >> 4 files changed, 523 insertions(+), 84 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c >> index 6368dfdbe9dd..27100423154b 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_root_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; >> * } >> * >> @@ -771,6 +806,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 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; >> + >> + kref_init(&vm_bo->kref); >> + INIT_LIST_HEAD(&vm_bo->list.gpuva); >> + INIT_LIST_HEAD(&vm_bo->list.entry.gem); >> + >> + drm_gem_object_get(obj); >> + >> + 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); >> + >> + drm_gem_gpuva_assert_lock_held(obj); >> + if (!lock) >> + drm_gpuvm_resv_assert_held(gpuvm); >> + >> + list_del(&vm_bo->list.entry.gem); >> + >> + drm_gem_object_put(obj); >> + >> + if (ops && ops->vm_bo_free) >> + ops->vm_bo_free(vm_bo); >> + else >> + kfree(vm_bo); >> +} >> + >> +/** >> + * 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); >> + >> + 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; >> + } >> + >> + 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) >> @@ -860,24 +1083,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; >> if (unlikely(!obj)) >> return; >> + WARN_ON(obj != vm_bo->obj); >> drm_gem_gpuva_assert_lock_held(obj); >> - list_add_tail(&va->gem.entry, &obj->gpuva.list); >> + drm_gpuvm_bo_get(vm_bo); >> + >> + va->vm_bo = vm_bo; >> + list_add_tail(&va->gem.entry, &vm_bo->list.gpuva); >> } >> EXPORT_SYMBOL_GPL(drm_gpuva_link); >> @@ -888,13 +1120,22 @@ 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; > Can we ditch va->gem.obj now and replace it with an accessor to the vm_bo's pointer?
Theoretically, drm_gpuvm could be used to track mappings and create drm_gpuva_ops to map / unmap stuff only. Not sure if anyone ever does that though.
If we decide to drop it and use the vm_bo's pointer instead, the drm_gpuva must always carry a pointer to the vm_bo since the drm_gpuvm_sm_map() function family requires the BO pointer (if any) to work correctly. Also I'm not sure about all the corresponding lifecycle implications yet.
Either way, I'd prefer to approach this in a separate patch if we decide so.
>> + struct drm_gpuvm_bo *vm_bo = va->vm_bo; >> if (unlikely(!obj)) >> return; >> @@ -902,6 +1143,11 @@ drm_gpuva_unlink(struct drm_gpuva *va) >> drm_gem_gpuva_assert_lock_held(obj); >> list_del_init(&va->gem.entry); >> + va->vm_bo = NULL; >> + >> + drm_gem_object_get(obj); >> + drm_gpuvm_bo_put(vm_bo); >> + drm_gem_object_put(obj); > > This get->put dance is unneccesary? If the caller is required to hold a lock on obj it is also required to hold a reference on obj.
I think I had the case where the driver has a separate (external) GEM gpuva lock to protect the GEM's VM_BO list and the VM_BO's drm_gpuva list in my mind when writing this. However, in this case we also don't need to keep a reference on the GEM object I guess...
Will remove it.
> > Besides, if the vm_bo's reference on obj is otherwise the last one, it will still be freed before the function exits. > > /Thomas > >
| |