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 | 37 +++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c
b/drivers/gpu/drm/imagination/pvr_vm.c
index cbdd15ed74f..3921b765688 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);
@@ -366,7 +366,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 +400,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 +422,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 +440,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 +597,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 +632,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 +769,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 +853,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 +889,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 +923,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
