On Thu, Oct 01, 2026 at 05:06:30PM -0700, Matthew Brost wrote: > On Thu, Oct 01, 2026 at 03:06:31PM -0700, Matthew Brost wrote: > > nouveau creates its drm_gpuvm without DRM_GPUVM_RESV_PROTECTED, so the > > extobj and evicted lists are protected by internal spinlocks. Every > > nouveau path but three already touches them with the VM's resv held: > > drm_gpuvm_exec_lock() and drm_gpuvm_validate() on exec, and TTM moves of > > private BOs, which share the VM's resv. The remaining three are: > > > > - nouveau_uvmm_bind_job_submit() adds the vm_bo of a MAP op to the > > extobj list holding no resv at all. Move that into > > bind_lock_validate(), which now locks the VM's resv as well. > > > > - bind_link_gpuvas() unlinks the GPUVAs of UNMAP and REMAP ops, which > > can drop the last reference of their vm_bo. It runs within the bind > > job's drm_exec transaction, which the above makes hold the VM's resv. > > > > - nouveau_uvmm_bind_job_cleanup() and nouveau_uvmm_fini() can drop the > > last reference of a vm_bo holding only the object's resv. Lock the > > VM's resv along with it, through a new > > nouveau_uvmm_lock_vm_and_obj(). > > > > The bind job's fence now always lands in the VM's resv, as BOOKKEEP. > > This already happened whenever a bind job mapped a private BO. > > > > With that the internal spinlocks buy nothing, so set > > DRM_GPUVM_RESV_PROTECTED. This is also what two-pass locking in > > drm_gpuvm requires, which a following patch makes use of. > > > > Cc: Abhinav Kumar <[email protected]> > > Cc: Alice Ryhl <[email protected]> > > Cc: Anna Maniscalco <[email protected]> > > Cc: Antonino Maniscalco <[email protected]> > > Cc: Boris Brezillon <[email protected]> > > Cc: Danilo Krummrich <[email protected]> > > Cc: David Airlie <[email protected]> > > Cc: Dmitry Baryshkov <[email protected]> > > Cc: Jessica Zhang <[email protected]> > > Cc: Jonathan Corbet <[email protected]> > > Cc: Liviu Dudau <[email protected]> > > Cc: Lyude Paul <[email protected]> > > Cc: Maarten Lankhorst <[email protected]> > > Cc: Marijn Suijten <[email protected]> > > Cc: Maxime Ripard <[email protected]> > > Cc: Randy Dunlap <[email protected]> > > Cc: Rob Clark <[email protected]> > > Cc: Rodrigo Vivi <[email protected]> > > Cc: Sean Paul <[email protected]> > > Cc: Shuah Khan <[email protected]> > > Cc: Simona Vetter <[email protected]> > > Cc: Steven Price <[email protected]> > > Cc: Thomas Hellström <[email protected]> > > Cc: Thomas Zimmermann <[email protected]> > > Signed-off-by: Matthew Brost <[email protected]> > > Assisted-by: LLM > > --- > > v3: > > - New patch (Danilo) > > --- > > drivers/gpu/drm/nouveau/nouveau_uvmm.c | 49 ++++++++++++++++++++++---- > > 1 file changed, 42 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c > > b/drivers/gpu/drm/nouveau/nouveau_uvmm.c > > index 2026fe6b48c6..dae612e56d91 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c > > @@ -1187,6 +1187,27 @@ bind_validate_region(struct nouveau_job *job) > > return 0; > > } > > > > +/* > > + * Lock the VM's common dma-resv together with the one of @obj, as needed > > to > > + * drop what may be the last reference of a &drm_gpuvm_bo. > > + */ > > +static void > > +nouveau_uvmm_lock_vm_and_obj(struct nouveau_uvmm *uvmm, struct drm_exec > > *exec, > > + struct drm_gem_object *obj) > > +{ > > + int ret; > > + > > + drm_exec_init(exec, DRM_EXEC_IGNORE_DUPLICATES, 2); > > + drm_exec_until_all_locked(exec) { > > + ret = drm_exec_lock_obj(exec, drm_gpuvm_resv_obj(&uvmm->base)); > > + if (!ret) > > + ret = drm_exec_lock_obj(exec, obj); > > + drm_exec_retry_on_contention(exec); > > + if (drm_WARN_ON(uvmm->base.drm, ret)) > > Sashiko flagged that -ENOMEM is a possible return from drm_exec_lock_obj > if a kmalloc fails and that is correct. > > My plan is to change this and open coded WW transaction which can't > fail. > > Not going to post now as I don't want to spam the list. >
Or this patch merges - then is a moot point: https://patchwork.freedesktop.org/series/175390/ Matt > Matt > > > + break; > > + } > > +} > > + > > static void > > bind_link_gpuvas(struct bind_job_op *bop) > > { > > @@ -1224,12 +1245,24 @@ bind_lock_validate(struct nouveau_job *job, struct > > drm_exec *exec, > > unsigned int num_fences) > > { > > struct nouveau_uvmm_bind_job *bind_job = to_uvmm_bind_job(job); > > + struct nouveau_uvmm *uvmm = nouveau_cli_uvmm(job->cli); > > struct bind_job_op *op; > > int ret; > > > > + /* The VM's dma-resv protects its extobj and evicted lists, and must be > > + * held by bind_link_gpuvas() in case it drops the last reference of a > > + * &drm_gpuvm_bo. > > + */ > > + ret = drm_gpuvm_prepare_vm(&uvmm->base, exec, num_fences); > > + if (ret) > > + return ret; > > + > > list_for_each_op(op, &bind_job->ops) { > > struct drm_gpuva_op *va_op; > > > > + if (op->op == OP_MAP) > > + drm_gpuvm_bo_extobj_add(op->vm_bo); > > + > > if (!op->ops) > > continue; > > > > @@ -1288,8 +1321,6 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job, > > dma_resv_unlock(obj->resv); > > if (IS_ERR(op->vm_bo)) > > return PTR_ERR(op->vm_bo); > > - > > - drm_gpuvm_bo_extobj_add(op->vm_bo); > > } > > > > ret = bind_validate_op(job, op); > > @@ -1603,9 +1634,11 @@ nouveau_uvmm_bind_job_cleanup(struct nouveau_job > > *job) > > drm_gpuva_ops_free(&uvmm->base, op->ops); > > > > if (!IS_ERR_OR_NULL(op->vm_bo)) { > > - dma_resv_lock(obj->resv, NULL); > > + struct drm_exec exec; > > + > > + nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj); > > drm_gpuvm_bo_put(op->vm_bo); > > - dma_resv_unlock(obj->resv); > > + drm_exec_fini(&exec); > > } > > > > if (obj) > > @@ -1941,7 +1974,8 @@ nouveau_uvmm_ioctl_vm_init(struct drm_device *dev, > > mt_init_flags(&uvmm->region_mt, MT_FLAGS_LOCK_EXTERN); > > mt_set_external_lock(&uvmm->region_mt, &uvmm->mutex); > > > > - drm_gpuvm_init(&uvmm->base, cli->name, 0, drm, r_obj, > > + drm_gpuvm_init(&uvmm->base, cli->name, DRM_GPUVM_RESV_PROTECTED, > > + drm, r_obj, > > NOUVEAU_VA_SPACE_START, > > NOUVEAU_VA_SPACE_END, > > init->kernel_managed_addr, > > @@ -1978,6 +2012,7 @@ nouveau_uvmm_fini(struct nouveau_uvmm *uvmm) > > struct nouveau_uvma_region *reg; > > struct nouveau_cli *cli = uvmm->vmm.cli; > > struct drm_gpuva *va, *next; > > + struct drm_exec exec; > > > > nouveau_uvmm_lock(uvmm); > > drm_gpuvm_for_each_va_safe(va, next, &uvmm->base) { > > @@ -1989,9 +2024,9 @@ nouveau_uvmm_fini(struct nouveau_uvmm *uvmm) > > > > drm_gpuva_remove(va); > > > > - dma_resv_lock(obj->resv, NULL); > > + nouveau_uvmm_lock_vm_and_obj(uvmm, &exec, obj); > > drm_gpuva_unlink(va); > > - dma_resv_unlock(obj->resv); > > + drm_exec_fini(&exec); > > > > nouveau_uvma_unmap(uvma); > > nouveau_uvma_vmm_put(uvma); > > -- > > 2.34.1 > >
