| Message ID | 20230928191624.13703-4-dakr@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM GPUVM features | expand |
Hi Danilo, kernel test robot noticed the following build warnings: [auto build test WARNING on a4ead6e37e3290cff399e2598d75e98777b69b37] url: https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/drm-gpuvm-add-common-dma-resv-per-struct-drm_gpuvm/20230929-031831 base: a4ead6e37e3290cff399e2598d75e98777b69b37 patch link: https://lore.kernel.org/r/20230928191624.13703-4-dakr%40redhat.com patch subject: [PATCH drm-misc-next v5 3/6] drm/gpuvm: add an abstraction for a VM / BO combination reproduce: (https://download.01.org/0day-ci/archive/20231002/202310021416.3jqeZtQG-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202310021416.3jqeZtQG-lkp@intel.com/ All warnings (new ones prefixed by >>): >> ./include/drm/drm_gpuvm.h:464: warning: Function parameter or member 'vm' not described in 'drm_gpuvm_bo' vim +464 ./include/drm/drm_gpuvm.h 427 428 /** 429 * @gpuvm: The &drm_gpuvm the @obj is mapped in. 430 */ 431 struct drm_gpuvm *vm; 432 433 /** 434 * @obj: The &drm_gem_object being mapped in the @gpuvm. 435 */ 436 struct drm_gem_object *obj; 437 438 /** 439 * @kref: The reference count for this &drm_gpuvm_bo. 440 */ 441 struct kref kref; 442 443 /** 444 * @list: Structure containing all &list_heads. 445 */ 446 struct { 447 /** 448 * @gpuva: The list of linked &drm_gpuvas. 449 */ 450 struct list_head gpuva; 451 452 /** 453 * @entry: Structure containing all &list_heads serving as 454 * entry. 455 */ 456 struct { 457 /** 458 * @gem: List entry to attach to the &drm_gem_objects 459 * gpuva list. 460 */ 461 struct list_head gem; 462 } entry; 463 } list; > 464 }; 465
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? > + 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. Besides, if the vm_bo's reference on obj is otherwise the last one, it will still be freed before the function exits. /Thomas
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 > >
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; + 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); } EXPORT_SYMBOL_GPL(drm_gpuva_unlink); @@ -1046,10 +1292,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); @@ -1693,9 +1939,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. @@ -1712,15 +1957,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) @@ -1728,8 +1972,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; @@ -1743,10 +1987,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 93ad2ba7ec8b..4e46f850e65f 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 bc9f6aa2f3fe..7147978d82d8 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -571,7 +571,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. @@ -584,28 +584,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 13539f32c2e2..7ab479153a00 100644 --- a/include/drm/drm_gpuvm.h +++ b/include/drm/drm_gpuvm.h @@ -26,12 +26,14 @@ */ #include <linux/list.h> +#include <linux/dma-resv.h> #include <linux/rbtree.h> #include <linux/types.h> #include <drm/drm_gem.h> struct drm_gpuvm; +struct drm_gpuvm_bo; struct drm_gpuvm_ops; /** @@ -72,6 +74,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 */ @@ -107,7 +115,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; @@ -140,7 +148,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, @@ -187,10 +195,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 = (1 << 0), + /** * @DRM_GPUVM_USERBITS: user defined bits */ - DRM_GPUVM_USERBITS = (1 << 0), + DRM_GPUVM_USERBITS = (1 << 1), }; /** @@ -272,6 +286,19 @@ bool drm_gpuvm_interval_empty(struct drm_gpuvm *gpuvm, u64 addr, u64 range); struct drm_gem_object * drm_gpuvm_root_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 @@ -290,6 +317,12 @@ drm_gpuvm_root_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__)) @@ -374,6 +407,117 @@ __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 { + + /** + * @gpuvm: The &drm_gpuvm the @obj is mapped in. + */ + struct drm_gpuvm *vm; + + /** + * @obj: The &drm_gem_object being mapped in the @gpuvm. + */ + 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; + + /** + * @entry: Structure containing all &list_heads serving as + * entry. + */ + struct { + /** + * @gem: List entry to attach to the &drm_gem_objects + * gpuva list. + */ + struct list_head gem; + } entry; + } list; +}; + +struct drm_gpuvm_bo * +drm_gpuvm_bo_create(struct drm_gpuvm *gpuvm, + struct drm_gem_object *obj); + +struct drm_gpuvm_bo * +drm_gpuvm_bo_obtain(struct drm_gpuvm *gpuvm, + struct drm_gem_object *obj); +struct drm_gpuvm_bo * +drm_gpuvm_bo_obtain_prealloc(struct drm_gpuvm_bo *vm_bo); + +/** + * drm_gpuvm_bo_get() - acquire a struct drm_gpuvm_bo reference + * @vm_bo: the &drm_gpuvm_bo to acquire the reference of + * + * This function acquires an additional reference to @vm_bo. It is illegal to + * call this without already holding a reference. No locks required. + */ +static inline struct drm_gpuvm_bo * +drm_gpuvm_bo_get(struct drm_gpuvm_bo *vm_bo) +{ + kref_get(&vm_bo->kref); + return vm_bo; +} + +void drm_gpuvm_bo_put(struct drm_gpuvm_bo *vm_bo); + +struct drm_gpuvm_bo * +drm_gpuvm_bo_find(struct drm_gpuvm *gpuvm, + struct drm_gem_object *obj); + +/** + * drm_gpuvm_bo_for_each_va() - iterator to walk over a list of &drm_gpuva + * @va__: &drm_gpuva structure to assign to in each iteration step + * @vm_bo__: the &drm_gpuvm_bo the &drm_gpuva to walk are associated with + * + * This iterator walks over all &drm_gpuva structures associated with the + * &drm_gpuvm_bo. + */ +#define drm_gpuvm_bo_for_each_va(va__, vm_bo__) \ + list_for_each_entry(va__, &(vm_bo)->list.gpuva, gem.entry) + +/** + * drm_gpuvm_bo_for_each_va_safe() - iterator to safely walk over a list of + * &drm_gpuva + * @va__: &drm_gpuva structure to assign to in each iteration step + * @next__: &next &drm_gpuva to store the next step + * @vm_bo__: the &drm_gpuvm_bo the &drm_gpuva to walk are associated with + * + * This iterator walks over all &drm_gpuva structures associated with the + * &drm_gpuvm_bo. It is implemented with list_for_each_entry_safe(), hence + * it is save against removal of elements. + */ +#define drm_gpuvm_bo_for_each_va_safe(va__, next__, vm_bo__) \ + list_for_each_entry_safe(va__, next__, &(vm_bo)->list.gpuva, gem.entry) + /** * enum drm_gpuva_op_type - GPU VA operation type * @@ -643,8 +787,7 @@ drm_gpuvm_prefetch_ops_create(struct drm_gpuvm *gpuvm, u64 addr, u64 range); 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); void drm_gpuva_ops_free(struct drm_gpuvm *gpuvm, struct drm_gpuva_ops *ops); @@ -688,6 +831,30 @@ struct drm_gpuvm_ops { */ void (*op_free)(struct drm_gpuva_op *op); + /** + * @vm_bo_alloc: called when the &drm_gpuvm allocates + * a struct drm_gpuvm_bo + * + * Some drivers may want to embed struct drm_gpuvm_bo into driver + * specific structures. By implementing this callback drivers can + * allocate memory accordingly. + * + * This callback is optional. + */ + struct drm_gpuvm_bo *(*vm_bo_alloc)(void); + + /** + * @vm_bo_free: called when the &drm_gpuvm frees a + * struct drm_gpuvm_bo + * + * Some drivers may want to embed struct drm_gpuvm_bo into driver + * specific structures. By implementing this callback drivers can + * free the previously allocated memory accordingly. + * + * This callback is optional. + */ + void (*vm_bo_free)(struct drm_gpuvm_bo *vm_bo); + /** * @sm_step_map: called from &drm_gpuvm_sm_map to finally insert the * mapping once all previous steps were completed
This patch adds an abstraction layer between the drm_gpuva mappings of a particular drm_gem_object and this GEM object itself. The abstraction represents a combination of a drm_gem_object and drm_gpuvm. The drm_gem_object holds a list of drm_gpuvm_bo structures (the structure representing this abstraction), while each drm_gpuvm_bo contains list of mappings of this GEM object. This has multiple advantages: 1) We can use the drm_gpuvm_bo structure to attach it to various lists of the drm_gpuvm. This is useful for tracking external and evicted objects per VM, which is introduced in subsequent patches. 2) Finding mappings of a certain drm_gem_object mapped in a certain drm_gpuvm becomes much cheaper. 3) Drivers can derive and extend the structure to easily represent driver specific states of a BO for a certain GPUVM. The idea of this abstraction was taken from amdgpu, hence the credit for this idea goes to the developers of amdgpu. Cc: Christian König <christian.koenig@amd.com> 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(-)