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.
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
>
