The infinite loop issue in pvr_vm_unmap_all() reported by Sashiko is not
introduced by this patch; it has existed since commit a5c4c3ba95a5
("drm/imagination: Hold drm_gem_gpuva lock for unmap").Fixed in a separate patch: https://lore.kernel.org/r/[email protected]/ On Sun, Oct 11, 2026 at 6:45 PM Gyeyoung Baek <[email protected]> wrote: > > Use DRM_GPUVM_IMMEDIATE_MODE so that each GEM's GPUVA list is protected > by its gpuva.lock instead of its dma_resv. This prepares for > asynchronous VM_BIND, whose run_job() will update the GPUVA lists > before signalling its fence. That section imposes three rules: > > 1. No dma_resv: another thread may hold it while waiting for the job's > fence, directly or through a dependency. > -> Protect the lists with gpuva.lock, and take it around > drm_gpuva_link(). > > 2. No allocation under a lock run_job() takes: reclaim may wait for a > fence that depends on the job. > -> Allocate the vm_bo before taking gpuva.lock: > drm_gpuvm_bo_obtain_locked() becomes drm_gpuvm_bo_create() + > drm_gpuvm_bo_obtain_prealloc(). > > 3. No freeing of GEM objects: that takes their dma_resv, and under > gpuva.lock would free the held lock too. > -> Defer it: drm_gpuva_unlink() and drm_gpuvm_bo_put() become > drm_gpuva_unlink_defer() and drm_gpuvm_bo_put_deferred(), and > drm_gpuvm_bo_deferred_cleanup() frees once the locks are dropped. > > This also fixes an unlocked GPUVA list update: when a map fails, > pvr_vm_bind_op_fini() drops the last vm_bo reference without holding > the object's dma_resv. > > Failing VM_MAP allocations with fail-nth shows > it on a lockdep kernel (real allocation failures under memory pressure > take the same path): > > WARNING: drivers/gpu/drm/drm_gpuvm.c:1661 at drm_gpuvm_bo_put+0x2ec/0x660 > [drm_gpuvm] > Call trace: > drm_gpuvm_bo_put > pvr_vm_bind_op_fini > pvr_vm_map > pvr_ioctl_vm_map > > Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code") > Signed-off-by: Gyeyoung Baek <[email protected]> > --- > drivers/gpu/drm/imagination/pvr_vm.c | 39 > +++++++++++++++++++++++++++--------- > 1 file changed, 30 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c > b/drivers/gpu/drm/imagination/pvr_vm.c > index cbdd15ed74f..807a01aab1d 100644 > --- a/drivers/gpu/drm/imagination/pvr_vm.c > +++ b/drivers/gpu/drm/imagination/pvr_vm.c > @@ -214,7 +214,7 @@ static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op > *bind_op) > > static void pvr_vm_bind_op_fini(struct pvr_vm_bind_op *bind_op) > { > - drm_gpuvm_bo_put(bind_op->gpuvm_bo); > + drm_gpuvm_bo_put_deferred(bind_op->gpuvm_bo); > > kfree(bind_op->new_va); > kfree(bind_op->prev_va); > @@ -255,11 +255,11 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op, > > bind_op->type = PVR_VM_BIND_TYPE_MAP; > > - dma_resv_lock(obj->resv, NULL); > - bind_op->gpuvm_bo = drm_gpuvm_bo_obtain_locked(&vm_ctx->gpuvm_mgr, > obj); > - dma_resv_unlock(obj->resv); > - if (IS_ERR(bind_op->gpuvm_bo)) > - return PTR_ERR(bind_op->gpuvm_bo); > + bind_op->gpuvm_bo = drm_gpuvm_bo_create(&vm_ctx->gpuvm_mgr, obj); > + if (!bind_op->gpuvm_bo) > + return -ENOMEM; > + > + bind_op->gpuvm_bo = drm_gpuvm_bo_obtain_prealloc(bind_op->gpuvm_bo); > > bind_op->new_va = kzalloc_obj(*bind_op->new_va); > bind_op->prev_va = kzalloc_obj(*bind_op->prev_va); > @@ -293,6 +293,8 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op, > > err_bind_op_fini: > pvr_vm_bind_op_fini(bind_op); > + /* Drain what fini() deferred. */ > + drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr); > > return err; > } > @@ -366,7 +368,11 @@ pvr_vm_gpuva_map(struct drm_gpuva_op *op, void *op_ctx) > return err; > > drm_gpuva_map(&ctx->vm_ctx->gpuvm_mgr, &ctx->new_va->base, &op->map); > + > + mutex_lock(&op->map.gem.obj->gpuva.lock); > drm_gpuva_link(&ctx->new_va->base, ctx->gpuvm_bo); > + mutex_unlock(&op->map.gem.obj->gpuva.lock); > + > ctx->new_va = NULL; > > return 0; > @@ -396,7 +402,7 @@ pvr_vm_gpuva_unmap(struct drm_gpuva_op *op, void *op_ctx) > return err; > > drm_gpuva_unmap(&op->unmap); > - drm_gpuva_unlink(op->unmap.va); > + drm_gpuva_unlink_defer(op->unmap.va); > kfree(to_pvr_vm_gpuva(op->unmap.va)); > > return 0; > @@ -418,6 +424,7 @@ pvr_vm_gpuva_unmap(struct drm_gpuva_op *op, void *op_ctx) > static int > pvr_vm_gpuva_remap(struct drm_gpuva_op *op, void *op_ctx) > { > + struct drm_gem_object *obj = op->remap.unmap->va->gem.obj; > /* The split parts belong to the object of the mapping being split. */ > struct drm_gpuvm_bo *vm_bo = op->remap.unmap->va->vm_bo; > struct pvr_vm_bind_op *ctx = op_ctx; > @@ -435,16 +442,20 @@ pvr_vm_gpuva_remap(struct drm_gpuva_op *op, void > *op_ctx) > drm_gpuva_remap(&ctx->prev_va->base, &ctx->next_va->base, &op->remap); > > if (op->remap.prev) { > + mutex_lock(&obj->gpuva.lock); > drm_gpuva_link(&ctx->prev_va->base, vm_bo); > + mutex_unlock(&obj->gpuva.lock); > ctx->prev_va = NULL; > } > > if (op->remap.next) { > + mutex_lock(&obj->gpuva.lock); > drm_gpuva_link(&ctx->next_va->base, vm_bo); > + mutex_unlock(&obj->gpuva.lock); > ctx->next_va = NULL; > } > > - drm_gpuva_unlink(op->remap.unmap->va); > + drm_gpuva_unlink_defer(op->remap.unmap->va); > kfree(to_pvr_vm_gpuva(op->remap.unmap->va)); > > return 0; > @@ -588,7 +599,8 @@ pvr_vm_create_context(struct pvr_device *pvr_dev, bool > is_userspace_context) > drm_gem_private_object_init(&pvr_dev->base, &vm_ctx->dummy_gem, 0); > drm_gpuvm_init(&vm_ctx->gpuvm_mgr, > is_userspace_context ? "PowerVR-user-VM" : > "PowerVR-FW-VM", > - 0, &pvr_dev->base, &vm_ctx->dummy_gem, > + DRM_GPUVM_IMMEDIATE_MODE, &pvr_dev->base, > + &vm_ctx->dummy_gem, > 0, 1ULL << device_addr_bits, 0, 0, &pvr_vm_gpuva_ops); > > mutex_init(&vm_ctx->lock); > @@ -622,6 +634,7 @@ pvr_vm_context_release(struct kref *ref_count) > pvr_fw_object_destroy(vm_ctx->fw_mem_ctx_obj); > > pvr_vm_unmap_all(vm_ctx); > + drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr); > > pvr_mmu_context_destroy(vm_ctx->mmu_ctx); > drm_gem_private_object_fini(&vm_ctx->dummy_gem); > @@ -758,7 +771,9 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct > pvr_gem_object *pvr_obj, > > err_cleanup: > mutex_unlock(&vm_ctx->lock); > + > pvr_vm_bind_op_fini(&bind_op); > + drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr); > > return err; > } > @@ -840,6 +855,8 @@ pvr_vm_unmap_obj(struct pvr_vm_context *vm_ctx, struct > pvr_gem_object *pvr_obj, > err = pvr_vm_unmap_obj_locked(vm_ctx, pvr_obj, device_addr, size); > mutex_unlock(&vm_ctx->lock); > > + drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr); > + > return err; > } > > @@ -874,6 +891,8 @@ pvr_vm_unmap(struct pvr_vm_context *vm_ctx, u64 > device_addr, u64 size) > > mutex_unlock(&vm_ctx->lock); > > + drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr); > + > return err; > } > > @@ -906,6 +925,8 @@ pvr_vm_unmap_all(struct pvr_vm_context *vm_ctx) > } > > mutex_unlock(&vm_ctx->lock); > + > + drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr); > } > > /* Static data areas are determined by firmware. */ > > -- > 2.43.0 > -- Thanks, Gyeyoung
