* [PATCH 1/4] drm/imagination: Fix the vm_bo split mappings are linked to
2026-08-16 19:42 [PATCH 0/4] drm/imagination: Add async VM_BIND with sparse mappings Gyeyoung Baek
@ 2026-08-16 19:42 ` Gyeyoung Baek
2026-08-16 19:59 ` sashiko-bot
2026-08-16 19:42 ` [PATCH 2/4] drm/imagination: Switch the GPUVM to immediate mode Gyeyoung Baek
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Gyeyoung Baek @ 2026-08-16 19:42 UTC (permalink / raw)
To: Alessio Belle, Luigi Santivetti, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Matt Coster,
Donald Robson, Sarah Walker, Sumit Semwal, Christian König
Cc: imagination, dri-devel, linux-kernel, linux-media, linaro-mm-sig,
Gyeyoung Baek
A remap does not map anything new. It splits an existing mapping and
re-creates the parts the request did not cover, and that mapping is handed
to the callback in op->remap.unmap->va, carrying the &drm_gpuvm_bo those
parts belong to.
However, pvr_vm_gpuva_remap() linked the parts to
pvr_vm_bind_op::gpuvm_bo, which belongs to the object the request maps
rather than the object the parts came from.
Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
drivers/gpu/drm/imagination/pvr_vm.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index 396d349fb6c..608e8c0467d 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -418,6 +418,8 @@ 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_gpuva *unmap_va = op->remap.unmap->va;
+ struct drm_gpuvm_bo *vm_bo = unmap_va->vm_bo;
struct pvr_vm_bind_op *ctx = op_ctx;
u64 va_start = 0, va_range = 0;
int err;
@@ -434,18 +436,18 @@ pvr_vm_gpuva_remap(struct drm_gpuva_op *op, void *op_ctx)
if (op->remap.prev) {
pvr_gem_object_get(gem_to_pvr_gem(ctx->prev_va->base.gem.obj));
- drm_gpuva_link(&ctx->prev_va->base, ctx->gpuvm_bo);
+ drm_gpuva_link(&ctx->prev_va->base, vm_bo);
ctx->prev_va = NULL;
}
if (op->remap.next) {
pvr_gem_object_get(gem_to_pvr_gem(ctx->next_va->base.gem.obj));
- drm_gpuva_link(&ctx->next_va->base, ctx->gpuvm_bo);
+ drm_gpuva_link(&ctx->next_va->base, vm_bo);
ctx->next_va = NULL;
}
- drm_gpuva_unlink(op->remap.unmap->va);
- kfree(to_pvr_vm_gpuva(op->remap.unmap->va));
+ drm_gpuva_unlink(unmap_va);
+ kfree(to_pvr_vm_gpuva(unmap_va));
return 0;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/4] drm/imagination: Fix the vm_bo split mappings are linked to
2026-08-16 19:42 ` [PATCH 1/4] drm/imagination: Fix the vm_bo split mappings are linked to Gyeyoung Baek
@ 2026-08-16 19:59 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-16 19:59 UTC (permalink / raw)
To: Gyeyoung Baek; +Cc: dri-devel, media-ci
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Unbalanced `pvr_gem_object_get()` in `pvr_vm_gpuva_remap()` causes a permanent memory leak of GEM objects.
- [High] Missing `dma_resv` locking during `drm_gpuva_link()` and `drm_gpuva_unlink()` causes data races and list corruption.
- [High] Missing `vm_ctx->lock` acquisition in `pvr_vm_map()` causes concurrent execution data races and potential RB tree corruption.
--
--- Patch [1]: [PATCH 1/4] drm/imagination: Fix the vm_bo split mappings are linked to ---
commit c145cb50bdcd3046dfd6222a4b0282d9524af76c
Author: Gyeyoung Baek <gye976@gmail.com>
drm/imagination: Fix the vm_bo split mappings are linked to
A remap does not map anything new. It splits an existing mapping and
re-creates the parts the request did not cover, and that mapping is handed
to the callback in op->remap.unmap->va, carrying the &drm_gpuvm_bo those
parts belong to.
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 396d349fb6ce4..608e8c0467dce 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -434,18 +436,18 @@ pvr_vm_gpuva_remap(struct drm_gpuva_op *op, void *op_ctx)
>
> if (op->remap.prev) {
> pvr_gem_object_get(gem_to_pvr_gem(ctx->prev_va->base.gem.obj));
> - drm_gpuva_link(&ctx->prev_va->base, ctx->gpuvm_bo);
> + drm_gpuva_link(&ctx->prev_va->base, vm_bo);
[Severity: High]
This is a pre-existing issue, but does this code permanently leak GEM objects?
During a remap operation, this explicitly calls pvr_gem_object_get() to take
references on the backing GEM objects for the new split mappings (prev_va and
next_va).
However, the teardown paths like pvr_vm_gpuva_unmap() and the unmap phase
of pvr_vm_gpuva_remap() just call drm_gpuva_unlink() and kfree() without a
corresponding pvr_gem_object_put(). Since the DRM GPUVM framework internally
manages GEM object lifetimes through vm_bo structures, are these manual
references redundant and never released?
[Severity: High]
This is another pre-existing issue, but can this cause data races and list
corruption?
pvr_vm does not use DRM_GPUVM_IMMEDIATE_MODE, so it relies on the GEM object's
dma_resv lock to protect gpuva.list operations. When a new mapping overlaps an
existing mapping belonging to a different GEM object, the core calls
pvr_vm_gpuva_remap() or pvr_vm_gpuva_unmap().
These callbacks then invoke drm_gpuva_link() and drm_gpuva_unlink() on the
mappings without holding their GEM object's dma_resv lock. Since pvr_vm_map()
only locks the new GEM object being mapped (via pvr_vm_lock_extra()) and
doesn't populate the GPUVM's extobj list, does this concurrent lockless
modification of gpuva.list result in list corruption?
> ctx->prev_va = NULL;
> }
>
> if (op->remap.next) {
> pvr_gem_object_get(gem_to_pvr_gem(ctx->next_va->base.gem.obj));
> - drm_gpuva_link(&ctx->next_va->base, ctx->gpuvm_bo);
> + drm_gpuva_link(&ctx->next_va->base, vm_bo);
> ctx->next_va = NULL;
> }
>
> - drm_gpuva_unlink(op->remap.unmap->va);
> - kfree(to_pvr_vm_gpuva(op->remap.unmap->va));
> + drm_gpuva_unlink(unmap_va);
> + kfree(to_pvr_vm_gpuva(unmap_va));
>
> return 0;
> }
[Severity: High]
This is a pre-existing issue in this file outside of the diff, but does
pvr_vm_map() have a data race that could corrupt the GPUVM RB-tree?
In pvr_vm_unmap(), the driver acquires vm_ctx->lock before searching the
GPUVM's RB-tree via drm_gpuva_find(). However, pvr_vm_map() mutates the RB-tree
via pvr_vm_bind_op_exec() invoking drm_gpuvm_sm_map() but doesn't acquire
vm_ctx->lock:
drivers/gpu/drm/imagination/pvr_vm.c:pvr_vm_map() {
...
err = pvr_vm_bind_op_exec(&bind_op);
...
}
While pvr_vm_map() acquires the VM's dma_resv lock, pvr_vm_unmap() does not
hold this lock during its traversal:
drivers/gpu/drm/imagination/pvr_vm.c:pvr_vm_unmap() {
...
va = drm_gpuva_find(&vm_ctx->gpuvm_mgr, device_addr, size);
...
}
Since there is no shared lock between the reader and writer of the RB-tree, can
a map operation modify the tree concurrently with an unmap operation's
traversal?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-pvr-vm-bind-v1-0-0a0f21be7d38@gmail.com?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/4] drm/imagination: Switch the GPUVM to immediate mode
2026-08-16 19:42 [PATCH 0/4] drm/imagination: Add async VM_BIND with sparse mappings Gyeyoung Baek
2026-08-16 19:42 ` [PATCH 1/4] drm/imagination: Fix the vm_bo split mappings are linked to Gyeyoung Baek
@ 2026-08-16 19:42 ` Gyeyoung Baek
2026-08-16 19:55 ` sashiko-bot
2026-08-16 19:42 ` [PATCH 3/4] drm/imagination: Add async VM_BIND ioctl Gyeyoung Baek
2026-08-16 19:42 ` [PATCH 4/4] drm/imagination: Add sparse mappings to VM_BIND Gyeyoung Baek
3 siblings, 1 reply; 9+ messages in thread
From: Gyeyoung Baek @ 2026-08-16 19:42 UTC (permalink / raw)
To: Alessio Belle, Luigi Santivetti, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Matt Coster,
Donald Robson, Sarah Walker, Sumit Semwal, Christian König
Cc: imagination, dri-devel, linux-kernel, linux-media, linaro-mm-sig,
Gyeyoung Baek
In preparation for the asynchronous VM_BIND that follows.
Why DRM_GPUVM_IMMEDIATE_MODE is needed:
Running binds from a async-job makes a deadlock. run_job() takes a BO's
dma_resv to update that BO's gpuva list, and has to signal a fence once
done(=signalling section). Another thread can be holding the same dma_resv
while waiting on that fence, so the two wait on each other.
Switching to DRM_GPUVM_IMMEDIATE_MODE requires two things:
1. Drop drm_gpuvm_exec.
It calls dma_resv_lock() internally, which is the path that deadlocks.
It did also serve as the address space tree lock, so take
pvr_vm_context::lock explicitly instead.
2. Let the GEM's gpuva.lock protect the list.
That lock is only ever held across list surgery, so no fence waiter can
be holding it. Panthor went this way first, for the same reason.
No functional change intended.
Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
drivers/gpu/drm/imagination/pvr_vm.c | 80 +++++++++++++-----------------------
1 file changed, 29 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index 608e8c0467d..45df76e61f7 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;
@@ -419,6 +423,7 @@ static int
pvr_vm_gpuva_remap(struct drm_gpuva_op *op, void *op_ctx)
{
struct drm_gpuva *unmap_va = op->remap.unmap->va;
+ struct drm_gem_object *obj = unmap_va->gem.obj;
struct drm_gpuvm_bo *vm_bo = unmap_va->vm_bo;
struct pvr_vm_bind_op *ctx = op_ctx;
u64 va_start = 0, va_range = 0;
@@ -436,17 +441,21 @@ pvr_vm_gpuva_remap(struct drm_gpuva_op *op, void *op_ctx)
if (op->remap.prev) {
pvr_gem_object_get(gem_to_pvr_gem(ctx->prev_va->base.gem.obj));
+ 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) {
pvr_gem_object_get(gem_to_pvr_gem(ctx->next_va->base.gem.obj));
+ 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(unmap_va);
+ drm_gpuva_unlink_defer(unmap_va);
kfree(to_pvr_vm_gpuva(unmap_va));
return 0;
@@ -590,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);
@@ -624,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);
@@ -693,16 +704,6 @@ void pvr_destroy_vm_contexts_for_file(struct pvr_file *pvr_file)
}
}
-static int
-pvr_vm_lock_extra(struct drm_gpuvm_exec *vm_exec)
-{
- struct pvr_vm_bind_op *bind_op = vm_exec->extra.priv;
- struct pvr_gem_object *pvr_obj = bind_op->pvr_obj;
-
- /* Acquire lock on the GEM object being mapped/unmapped. */
- return drm_exec_lock_obj(&vm_exec->exec, gem_from_pvr_gem(pvr_obj));
-}
-
/**
* pvr_vm_map() - Map a section of physical memory into a section of
* device-virtual memory.
@@ -730,15 +731,6 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
u64 pvr_obj_offset, u64 device_addr, u64 size)
{
struct pvr_vm_bind_op bind_op = {0};
- struct drm_gpuvm_exec vm_exec = {
- .vm = &vm_ctx->gpuvm_mgr,
- .flags = DRM_EXEC_INTERRUPTIBLE_WAIT |
- DRM_EXEC_IGNORE_DUPLICATES,
- .extra = {
- .fn = pvr_vm_lock_extra,
- .priv = &bind_op,
- },
- };
int err = pvr_vm_bind_op_map_init(&bind_op, vm_ctx, pvr_obj,
pvr_obj_offset, device_addr,
@@ -749,16 +741,12 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
pvr_gem_object_get(pvr_obj);
- err = drm_gpuvm_exec_lock(&vm_exec);
- if (err)
- goto err_cleanup;
-
+ mutex_lock(&vm_ctx->lock);
err = pvr_vm_bind_op_exec(&bind_op);
+ mutex_unlock(&vm_ctx->lock);
- drm_gpuvm_exec_unlock(&vm_exec);
-
-err_cleanup:
pvr_vm_bind_op_fini(&bind_op);
+ drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr);
return err;
}
@@ -787,15 +775,6 @@ pvr_vm_unmap_obj_locked(struct pvr_vm_context *vm_ctx,
u64 device_addr, u64 size)
{
struct pvr_vm_bind_op bind_op = {0};
- struct drm_gpuvm_exec vm_exec = {
- .vm = &vm_ctx->gpuvm_mgr,
- .flags = DRM_EXEC_INTERRUPTIBLE_WAIT |
- DRM_EXEC_IGNORE_DUPLICATES,
- .extra = {
- .fn = pvr_vm_lock_extra,
- .priv = &bind_op,
- },
- };
int err = pvr_vm_bind_op_unmap_init(&bind_op, vm_ctx, pvr_obj,
device_addr, size);
@@ -804,15 +783,8 @@ pvr_vm_unmap_obj_locked(struct pvr_vm_context *vm_ctx,
pvr_gem_object_get(pvr_obj);
- err = drm_gpuvm_exec_lock(&vm_exec);
- if (err)
- goto err_cleanup;
-
err = pvr_vm_bind_op_exec(&bind_op);
- drm_gpuvm_exec_unlock(&vm_exec);
-
-err_cleanup:
pvr_vm_bind_op_fini(&bind_op);
return err;
@@ -840,6 +812,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 +848,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 +882,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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/4] drm/imagination: Switch the GPUVM to immediate mode
2026-08-16 19:42 ` [PATCH 2/4] drm/imagination: Switch the GPUVM to immediate mode Gyeyoung Baek
@ 2026-08-16 19:55 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-16 19:55 UTC (permalink / raw)
To: Gyeyoung Baek; +Cc: media-ci, dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] When splitting an existing mapping, `pvr_vm_gpuva_remap` permanently leaks references to the GEM object by explicitly calling `pvr_gem_object_get` for `prev_va` and `next_va` without any corresponding put in the unmap path.
--
--- Patch [2]: [PATCH 2/4] drm/imagination: Switch the GPUVM to immediate mode ---
commit 62abfc02f1a7752ea9adac50d4fa08b736d27170
Author: Gyeyoung Baek <gye976@gmail.com>
drm/imagination: Switch the GPUVM to immediate mode
This commit switches the PowerVR GPUVM implementation to use
DRM_GPUVM_IMMEDIATE_MODE instead of the deferred execution model. This change
aims to avoid deadlocks during asynchronous VM_BIND operations by removing
the use of drm_gpuvm_exec and directly protecting the gpuva list with the GEM
object's gpuva.lock.
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 608e8c0467dce..45df76e61f7cf 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
[ ... ]
> @@ -436,17 +441,21 @@ pvr_vm_gpuva_remap(struct drm_gpuva_op *op, void *op_ctx)
>
> if (op->remap.prev) {
> pvr_gem_object_get(gem_to_pvr_gem(ctx->prev_va->base.gem.obj));
> + 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) {
> pvr_gem_object_get(gem_to_pvr_gem(ctx->next_va->base.gem.obj));
> + mutex_lock(&obj->gpuva.lock);
> drm_gpuva_link(&ctx->next_va->base, vm_bo);
> + mutex_unlock(&obj->gpuva.lock);
> ctx->next_va = NULL;
> }
[Severity: High]
This isn't a bug introduced by this patch, but does pvr_vm_gpuva_remap()
permanently leak references to the GEM object here?
When splitting an existing mapping (e.g., when an unprivileged userspace
client unmaps a subset or hole of an existing mapping), this code explicitly
calls pvr_gem_object_get() for prev_va and next_va to take an additional
reference to the GEM object for the newly split mappings.
Looking at the unmap path in pvr_vm_gpuva_unmap(), it relies purely on the
vm_bo lifecycle and does not appear to drop these per-VA GEM references:
pvr_vm_gpuva_unmap() {
...
drm_gpuva_unmap(&op->unmap);
drm_gpuva_unlink_defer(op->unmap.va);
kfree(to_pvr_vm_gpuva(op->unmap.va));
...
}
Because these extra references taken during the remap operation are never
dropped when the mapping is subsequently unmapped, can this lead to a
permanent memory leak of GEM objects and eventual memory exhaustion if
userspace repeatedly splits mappings?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-pvr-vm-bind-v1-0-0a0f21be7d38@gmail.com?part=2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] drm/imagination: Add async VM_BIND ioctl
2026-08-16 19:42 [PATCH 0/4] drm/imagination: Add async VM_BIND with sparse mappings Gyeyoung Baek
2026-08-16 19:42 ` [PATCH 1/4] drm/imagination: Fix the vm_bo split mappings are linked to Gyeyoung Baek
2026-08-16 19:42 ` [PATCH 2/4] drm/imagination: Switch the GPUVM to immediate mode Gyeyoung Baek
@ 2026-08-16 19:42 ` Gyeyoung Baek
2026-08-16 20:00 ` sashiko-bot
2026-08-16 19:42 ` [PATCH 4/4] drm/imagination: Add sparse mappings to VM_BIND Gyeyoung Baek
3 siblings, 1 reply; 9+ messages in thread
From: Gyeyoung Baek @ 2026-08-16 19:42 UTC (permalink / raw)
To: Alessio Belle, Luigi Santivetti, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Matt Coster,
Donald Robson, Sarah Walker, Sumit Semwal, Christian König
Cc: imagination, dri-devel, linux-kernel, linux-media, linaro-mm-sig,
Gyeyoung Baek
DRM_IOCTL_PVR_VM_MAP and DRM_IOCTL_PVR_VM_UNMAP fall short of what Vulkan
sparse binding needs in two ways:
1. Each carries a single operation, while vkQueueBindSparse hands over an
array, so one request turns into hundreds of ioctls.
2. Neither takes sync operations. A bind can neither wait on a fence nor
signal one, so ordering it against GPU work means blocking the CPU.
Add DRM_IOCTL_PVR_VM_BIND, which extends them with an array of bind
operations and an array of sync operations. With DRM_PVR_VM_BIND_ASYNC
a request runs asynchronously, as a drm_sched job.
The interface follows panthor throughout. Routing VM_MAP and VM_UNMAP
through VM_BIND is left to a follow-up.
Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
drivers/gpu/drm/imagination/pvr_drv.c | 88 ++++++
drivers/gpu/drm/imagination/pvr_drv.h | 4 +-
drivers/gpu/drm/imagination/pvr_job.c | 6 +
drivers/gpu/drm/imagination/pvr_queue.c | 3 +
drivers/gpu/drm/imagination/pvr_vm.c | 504 ++++++++++++++++++++++++++++++++
drivers/gpu/drm/imagination/pvr_vm.h | 27 ++
include/uapi/drm/pvr_drm.h | 115 ++++++++
7 files changed, 746 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index 5c965ef0274..ec6ed610e29 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -1130,6 +1130,93 @@ pvr_ioctl_vm_unmap(struct drm_device *drm_dev, void *raw_args,
return err;
}
+/**
+ * pvr_ioctl_vm_bind() - IOCTL to apply a batch of VM bind operations.
+ * @drm_dev: [IN] DRM device.
+ * @raw_args: [IN] Arguments passed to this IOCTL. This must be of type
+ * &struct drm_pvr_ioctl_vm_bind_args.
+ * @file: [IN] DRM file private data.
+ *
+ * Called from userspace with %DRM_IOCTL_PVR_VM_BIND.
+ *
+ * Return:
+ * * 0 on success,
+ * * -%EINVAL if arguments are invalid, or
+ * * Any error returned by pvr_vm_bind().
+ */
+static int
+pvr_ioctl_vm_bind(struct drm_device *drm_dev, void *raw_args,
+ struct drm_file *file)
+{
+ struct drm_pvr_ioctl_vm_bind_args *args = raw_args;
+ struct pvr_file *pvr_file = to_pvr_file(file);
+ struct drm_pvr_vm_bind_op *uapi_ops = NULL;
+ struct drm_pvr_sync_op *sync_ops = NULL;
+ struct pvr_vm_context *vm_ctx;
+ struct pvr_vm_bind_req req;
+ int idx;
+ int err;
+
+ if (!drm_dev_enter(drm_dev, &idx))
+ return -EIO;
+
+ if (args->flags & ~DRM_PVR_VM_BIND_FLAGS_MASK) {
+ err = -EINVAL;
+ goto err_drm_dev_exit;
+ }
+
+ if (!(args->flags & DRM_PVR_VM_BIND_ASYNC) && args->sync_ops.count) {
+ err = -EINVAL;
+ goto err_drm_dev_exit;
+ }
+
+ if (!args->ops.count && !args->sync_ops.count) {
+ err = 0;
+ goto err_drm_dev_exit;
+ }
+
+ vm_ctx = pvr_vm_context_lookup(pvr_file, args->vm_context_handle);
+ if (!vm_ctx) {
+ err = -EINVAL;
+ goto err_drm_dev_exit;
+ }
+
+ if (args->ops.count) {
+ err = PVR_UOBJ_GET_ARRAY(uapi_ops, &args->ops);
+ if (err)
+ goto err_put_vm_context;
+ }
+
+ if (args->sync_ops.count) {
+ err = PVR_UOBJ_GET_ARRAY(sync_ops, &args->sync_ops);
+ if (err)
+ goto err_free_uapi_ops;
+ }
+
+ req = (struct pvr_vm_bind_req){
+ .ops = uapi_ops,
+ .op_count = args->ops.count,
+ .sync_ops = sync_ops,
+ .sync_op_count = args->sync_ops.count,
+ .async = args->flags & DRM_PVR_VM_BIND_ASYNC,
+ };
+
+ err = pvr_vm_bind(vm_ctx, pvr_file, &req);
+
+ kvfree(sync_ops);
+
+err_free_uapi_ops:
+ kvfree(uapi_ops);
+
+err_put_vm_context:
+ pvr_vm_context_put(vm_ctx);
+
+err_drm_dev_exit:
+ drm_dev_exit(idx);
+
+ return err;
+}
+
/*
* pvr_ioctl_submit_job() - IOCTL to submit a job to the GPU
* @drm_dev: [IN] DRM device.
@@ -1290,6 +1377,7 @@ static const struct drm_ioctl_desc pvr_drm_driver_ioctls[] = {
DRM_PVR_IOCTL(CREATE_HWRT_DATASET, create_hwrt_dataset, DRM_RENDER_ALLOW),
DRM_PVR_IOCTL(DESTROY_HWRT_DATASET, destroy_hwrt_dataset, DRM_RENDER_ALLOW),
DRM_PVR_IOCTL(SUBMIT_JOBS, submit_jobs, DRM_RENDER_ALLOW),
+ DRM_PVR_IOCTL(VM_BIND, vm_bind, DRM_RENDER_ALLOW),
};
/* clang-format on */
diff --git a/drivers/gpu/drm/imagination/pvr_drv.h b/drivers/gpu/drm/imagination/pvr_drv.h
index 7fa147312dd..9ca8f8780a3 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.h
+++ b/drivers/gpu/drm/imagination/pvr_drv.h
@@ -13,9 +13,10 @@
/*
* Driver interface version:
* - 1.0: Initial interface
+ * - 1.1: adds DRM_IOCTL_PVR_VM_BIND
*/
#define PVR_DRIVER_MAJOR 1
-#define PVR_DRIVER_MINOR 0
+#define PVR_DRIVER_MINOR 1
#define PVR_DRIVER_PATCHLEVEL 0
int pvr_get_uobj(u64 usr_ptr, u32 usr_size, u32 min_size, u32 obj_size, void *out);
@@ -60,6 +61,7 @@ int pvr_set_uobj_array(const struct drm_pvr_obj_array *out, u32 min_stride, u32
#define PVR_UOBJ_MIN_SIZE(_obj_name) _Generic(_obj_name \
PVR_UOBJ_DECL(struct drm_pvr_job, hwrt) \
PVR_UOBJ_DECL(struct drm_pvr_sync_op, value) \
+ PVR_UOBJ_DECL(struct drm_pvr_vm_bind_op, size) \
PVR_UOBJ_DECL(struct drm_pvr_dev_query_gpu_info, num_phantoms) \
PVR_UOBJ_DECL(struct drm_pvr_dev_query_runtime_info, cdm_max_local_mem_size_regs) \
PVR_UOBJ_DECL(struct drm_pvr_dev_query_quirks, _padding_c) \
diff --git a/drivers/gpu/drm/imagination/pvr_job.c b/drivers/gpu/drm/imagination/pvr_job.c
index b8a58d81700..04f920aaf12 100644
--- a/drivers/gpu/drm/imagination/pvr_job.c
+++ b/drivers/gpu/drm/imagination/pvr_job.c
@@ -15,6 +15,7 @@
#include "pvr_stream_defs.h"
#include "pvr_sync.h"
#include "pvr_trace.h"
+#include "pvr_vm.h"
#include <drm/drm_exec.h>
#include <drm/drm_gem.h>
@@ -434,6 +435,11 @@ create_job(struct pvr_device *pvr_dev,
goto err_put_job;
}
+ if (pvr_vm_context_is_unusable(job->ctx->vm_ctx)) {
+ err = -ECANCELED;
+ goto err_put_job;
+ }
+
if (args->hwrt.set_handle) {
job->hwrt = pvr_hwrt_data_lookup(pvr_file, args->hwrt.set_handle,
args->hwrt.data_index);
diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c
index 09993e858df..6b83734604d 100644
--- a/drivers/gpu/drm/imagination/pvr_queue.c
+++ b/drivers/gpu/drm/imagination/pvr_queue.c
@@ -761,6 +761,9 @@ static struct dma_fence *pvr_queue_run_job(struct drm_sched_job *sched_job)
return dma_fence_get(job->done_fence);
}
+ if (pvr_vm_context_is_unusable(job->ctx->vm_ctx))
+ return ERR_PTR(-ECANCELED);
+
/* The only kind of jobs that can be paired are geometry and fragment, and
* we bail out early if we see a fragment job that's paired with a geometry job.
* Paired jobs must also target the same context and point to the same HWRT.
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index 45df76e61f7..f92bfeacd7a 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -9,20 +9,27 @@
#include "pvr_mmu.h"
#include "pvr_rogue_fwif.h"
#include "pvr_rogue_heap_config.h"
+#include "pvr_sync.h"
#include <drm/drm_exec.h>
#include <drm/drm_gem.h>
#include <drm/drm_gpuvm.h>
#include <drm/drm_print.h>
+#include <drm/gpu_scheduler.h>
#include <linux/bug.h>
#include <linux/container_of.h>
+#include <linux/dma-fence.h>
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/gfp_types.h>
#include <linux/kref.h>
#include <linux/mutex.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
#include <linux/stddef.h>
+#include <linux/workqueue.h>
+#include <linux/xarray.h>
/**
* DOC: Memory context
@@ -50,6 +57,32 @@ struct pvr_vm_context {
/** @lock: Global lock on this VM. */
struct mutex lock;
+ /**
+ * @sched: Scheduler used to serialise asynchronous VM_BIND requests.
+ *
+ * Only initialised for userspace VM contexts; see @sched_initialised.
+ */
+ struct drm_gpu_scheduler sched;
+
+ /** @entity: Scheduling entity feeding @sched. */
+ struct drm_sched_entity entity;
+
+ /** @sched_initialised: True if @sched and @entity need tearing down. */
+ bool sched_initialised;
+
+ /**
+ * @unusable: An asynchronous bind failed part way through, leaving the
+ * address space in a state nobody can reason about.
+ *
+ * Only the asynchronous path sets this; a synchronous failure reaches
+ * its caller directly, who then owns the recovery. Set once and never
+ * cleared: further operations are rejected with -%ECANCELED and the
+ * context has to be destroyed and recreated.
+ *
+ * Written under @lock, read without it.
+ */
+ bool unusable;
+
/**
* @fw_mem_ctx_obj: Firmware object representing firmware memory
* context.
@@ -72,6 +105,9 @@ struct pvr_vm_context *to_pvr_vm_context(struct drm_gpuvm *gpuvm)
return container_of(gpuvm, struct pvr_vm_context, gpuvm_mgr);
}
+static int pvr_vm_bind_sched_init(struct pvr_vm_context *vm_ctx);
+static void pvr_vm_bind_sched_fini(struct pvr_vm_context *vm_ctx);
+
struct pvr_vm_context *pvr_vm_context_get(struct pvr_vm_context *vm_ctx)
{
if (vm_ctx)
@@ -606,8 +642,26 @@ pvr_vm_create_context(struct pvr_device *pvr_dev, bool is_userspace_context)
mutex_init(&vm_ctx->lock);
kref_init(&vm_ctx->ref_count);
+ if (is_userspace_context) {
+ err = pvr_vm_bind_sched_init(vm_ctx);
+ if (err)
+ goto err_gpuvm_put;
+ }
+
return vm_ctx;
+err_gpuvm_put:
+ if (vm_ctx->fw_mem_ctx_obj)
+ pvr_fw_object_destroy(vm_ctx->fw_mem_ctx_obj);
+
+ pvr_mmu_context_destroy(vm_ctx->mmu_ctx);
+ drm_gem_private_object_fini(&vm_ctx->dummy_gem);
+ mutex_destroy(&vm_ctx->lock);
+
+ drm_gpuvm_put(&vm_ctx->gpuvm_mgr);
+
+ return ERR_PTR(err);
+
err_page_table_destroy:
pvr_mmu_context_destroy(vm_ctx->mmu_ctx);
@@ -630,6 +684,8 @@ pvr_vm_context_release(struct kref *ref_count)
struct pvr_vm_context *vm_ctx =
container_of(ref_count, struct pvr_vm_context, ref_count);
+ pvr_vm_bind_sched_fini(vm_ctx);
+
if (vm_ctx->fw_mem_ctx_obj)
pvr_fw_object_destroy(vm_ctx->fw_mem_ctx_obj);
@@ -853,6 +909,18 @@ pvr_vm_unmap(struct pvr_vm_context *vm_ctx, u64 device_addr, u64 size)
return err;
}
+/**
+ * pvr_vm_context_is_unusable() - Test whether a VM context has been left in an
+ * undefined state by a failed operation.
+ * @vm_ctx: Target VM context.
+ *
+ * Return: %true if the context rejects further operations.
+ */
+bool pvr_vm_context_is_unusable(struct pvr_vm_context *vm_ctx)
+{
+ return READ_ONCE(vm_ctx->unusable);
+}
+
/**
* pvr_vm_unmap_all() - Unmap all mappings associated with a VM context.
* @vm_ctx: Target VM context.
@@ -1174,3 +1242,439 @@ pvr_vm_get_fw_mem_context(struct pvr_vm_context *vm_ctx)
{
return vm_ctx->fw_mem_ctx_obj;
}
+
+/**
+ * DOC: Asynchronous VM_BIND
+ *
+ * %DRM_IOCTL_PVR_VM_BIND can queue a batch of bind operations instead of
+ * applying them inline. Each request becomes a &pvr_vm_bind_job pushed to a
+ * per-VM-context &drm_gpu_scheduler, which guarantees that requests targeting
+ * the same VM context are applied in submission order.
+ *
+ * Everything that can fail or allocate - argument validation, page table
+ * pre-allocation, page pinning - happens while building the job, because
+ * &drm_sched_backend_ops.run_job executes inside the dma-fence signalling
+ * critical path. For the same reason the GPUVM is initialised with
+ * %DRM_GPUVM_IMMEDIATE_MODE, so that mappings are tracked under the GEM's
+ * gpuva.lock rather than its dma_resv.
+ */
+
+/**
+ * struct pvr_vm_bind_job - A queued batch of VM bind operations.
+ */
+struct pvr_vm_bind_job {
+ /** @base: Inherited &drm_sched_job object. */
+ struct drm_sched_job base;
+
+ /** @vm_ctx: VM context targeted by this job. Holds a reference. */
+ struct pvr_vm_context *vm_ctx;
+
+ /** @op_count: Number of entries in @ops. */
+ u32 op_count;
+
+ /** @ops: Prepared bind operations, applied in array order. */
+ struct pvr_vm_bind_op *ops;
+
+ /**
+ * @cleanup_work: Releases @ops and the reference on @vm_ctx.
+ *
+ * free_job() cannot do this itself: dropping what may be the last VM
+ * context reference there would call drm_sched_fini(), which flushes
+ * the very worker free_job() runs on.
+ */
+ struct work_struct cleanup_work;
+};
+
+#define to_pvr_vm_bind_job(sched_job) \
+ container_of((sched_job), struct pvr_vm_bind_job, base)
+
+/**
+ * pvr_vm_bind_ops_free() - Release an array of prepared bind operations.
+ * @ops: Array to release. May be %NULL.
+ * @count: Number of prepared entries in @ops.
+ */
+static void pvr_vm_bind_ops_free(struct pvr_vm_bind_op *ops, u32 count)
+{
+ if (!ops)
+ return;
+
+ for (u32 i = 0; i < count; i++)
+ pvr_vm_bind_op_fini(&ops[i]);
+
+ kvfree(ops);
+}
+
+static void pvr_vm_bind_job_free(struct pvr_vm_bind_job *job)
+{
+ if (!job)
+ return;
+
+ pvr_vm_bind_ops_free(job->ops, job->op_count);
+
+ if (job->vm_ctx) {
+ drm_gpuvm_bo_deferred_cleanup(&job->vm_ctx->gpuvm_mgr);
+ pvr_vm_context_put(job->vm_ctx);
+ }
+
+ kfree(job);
+}
+
+static void pvr_vm_bind_job_cleanup_work(struct work_struct *work)
+{
+ struct pvr_vm_bind_job *job =
+ container_of(work, struct pvr_vm_bind_job, cleanup_work);
+
+ pvr_vm_bind_job_free(job);
+}
+
+static struct dma_fence *
+pvr_vm_bind_run_job(struct drm_sched_job *sched_job)
+{
+ struct pvr_vm_bind_job *job = to_pvr_vm_bind_job(sched_job);
+ struct pvr_vm_context *vm_ctx = job->vm_ctx;
+ int err = 0;
+ bool cookie;
+
+ if (pvr_vm_context_is_unusable(vm_ctx))
+ return ERR_PTR(-ECANCELED);
+
+ cookie = dma_fence_begin_signalling();
+
+ mutex_lock(&vm_ctx->lock);
+
+ for (u32 i = 0; i < job->op_count; i++) {
+ err = pvr_vm_bind_op_exec(&job->ops[i]);
+ if (err)
+ break;
+ }
+
+ if (err)
+ WRITE_ONCE(vm_ctx->unusable, true);
+
+ mutex_unlock(&vm_ctx->lock);
+
+ dma_fence_end_signalling(cookie);
+
+ /* NULL completes the job: the page tables are already updated. */
+ return err ? ERR_PTR(err) : NULL;
+}
+
+static enum drm_gpu_sched_stat
+pvr_vm_bind_timedout_job(struct drm_sched_job *sched_job)
+{
+ WARN(1, "VM bind jobs run on a CPU worker and cannot hang\n");
+
+ return DRM_GPU_SCHED_STAT_RESET;
+}
+
+static void pvr_vm_bind_free_job(struct drm_sched_job *sched_job)
+{
+ struct pvr_vm_bind_job *job = to_pvr_vm_bind_job(sched_job);
+
+ drm_sched_job_cleanup(sched_job);
+
+ /* Flushed before the device goes away, so it cannot outlive it. */
+ queue_work(job->vm_ctx->pvr_dev->sched_wq, &job->cleanup_work);
+}
+
+static const struct drm_sched_backend_ops pvr_vm_bind_sched_ops = {
+ .run_job = pvr_vm_bind_run_job,
+ .timedout_job = pvr_vm_bind_timedout_job,
+ .free_job = pvr_vm_bind_free_job,
+};
+
+/**
+ * pvr_vm_bind_sched_init() - Set up the VM_BIND scheduler of a VM context.
+ * @vm_ctx: Target VM context.
+ *
+ * Return:
+ * * 0 on success, or
+ * * Any error returned by drm_sched_init() or drm_sched_entity_init().
+ */
+static int pvr_vm_bind_sched_init(struct pvr_vm_context *vm_ctx)
+{
+ struct pvr_device *pvr_dev = vm_ctx->pvr_dev;
+ struct drm_gpu_scheduler *sched = &vm_ctx->sched;
+ const struct drm_sched_init_args sched_args = {
+ .ops = &pvr_vm_bind_sched_ops,
+ .submit_wq = pvr_dev->sched_wq,
+ .credit_limit = 1,
+ .hang_limit = 0,
+ /* Bind jobs run on a CPU worker and cannot hang. */
+ .timeout = MAX_SCHEDULE_TIMEOUT,
+ .name = "pvr-vm-bind",
+ .dev = from_pvr_device(pvr_dev)->dev,
+ };
+ int err;
+
+ err = drm_sched_init(sched, &sched_args);
+ if (err)
+ return err;
+
+ err = drm_sched_entity_init(&vm_ctx->entity, DRM_SCHED_PRIORITY_NORMAL,
+ &sched, 1, NULL);
+ if (err)
+ goto err_sched_fini;
+
+ vm_ctx->sched_initialised = true;
+
+ return 0;
+
+err_sched_fini:
+ drm_sched_fini(sched);
+
+ return err;
+}
+
+/**
+ * pvr_vm_bind_sched_fini() - Tear down the VM_BIND scheduler of a VM context.
+ * @vm_ctx: Target VM context.
+ *
+ * Waits for all queued bind jobs to be applied before returning.
+ */
+static void pvr_vm_bind_sched_fini(struct pvr_vm_context *vm_ctx)
+{
+ if (!vm_ctx->sched_initialised)
+ return;
+
+ drm_sched_entity_destroy(&vm_ctx->entity);
+ drm_sched_fini(&vm_ctx->sched);
+ vm_ctx->sched_initialised = false;
+}
+
+/**
+ * pvr_vm_bind_op_init_from_uapi() - Prepare a single bind op from its
+ * userspace description.
+ * @bind_op: Bind op to initialise.
+ * @vm_ctx: Target VM context.
+ * @pvr_file: PowerVR file used to resolve buffer object handles.
+ * @uapi_op: Userspace description of the operation.
+ *
+ * On success @bind_op owns every resource it needs to be executed later,
+ * and must be released with pvr_vm_bind_op_fini().
+ *
+ * Return:
+ * * 0 on success,
+ * * -%EINVAL if @uapi_op is malformed, or
+ * * -%ENOENT if @uapi_op refers to an unknown buffer object.
+ */
+static int
+pvr_vm_bind_op_init_from_uapi(struct pvr_vm_bind_op *bind_op,
+ struct pvr_vm_context *vm_ctx,
+ struct pvr_file *pvr_file,
+ const struct drm_pvr_vm_bind_op *uapi_op)
+{
+ struct pvr_gem_object *pvr_obj;
+ int err;
+
+ if (uapi_op->flags & ~DRM_PVR_VM_BIND_OP_FLAGS_MASK)
+ return -EINVAL;
+
+ if (!uapi_op->size)
+ return -EINVAL;
+
+ switch (uapi_op->flags & DRM_PVR_VM_BIND_OP_TYPE_MASK) {
+ case DRM_PVR_VM_BIND_OP_TYPE_MAP:
+ pvr_obj = pvr_gem_object_from_handle(pvr_file, uapi_op->handle);
+ if (!pvr_obj)
+ return -ENOENT;
+
+ err = pvr_vm_bind_op_map_init(bind_op, vm_ctx, pvr_obj,
+ uapi_op->offset,
+ uapi_op->device_addr,
+ uapi_op->size);
+ if (err) {
+ pvr_gem_object_put(pvr_obj);
+ return err;
+ }
+
+ return 0;
+
+ case DRM_PVR_VM_BIND_OP_TYPE_UNMAP:
+ if (uapi_op->handle || uapi_op->offset)
+ return -EINVAL;
+
+ return pvr_vm_bind_op_unmap_init(bind_op, vm_ctx, NULL,
+ uapi_op->device_addr,
+ uapi_op->size);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+/**
+ * pvr_vm_bind_ops_create_from_uapi() - Prepare bind operations from their
+ * userspace description.
+ * @vm_ctx: Target VM context.
+ * @pvr_file: PowerVR file used to resolve buffer object handles.
+ * @uapi_ops: Array of userspace operation descriptions.
+ * @op_count: Number of entries in @uapi_ops.
+ *
+ * Every allocation needed to apply the operations is performed here, so that
+ * applying them later - possibly from inside the dma-fence signalling critical
+ * path - cannot fail for want of memory.
+ *
+ * Return: The new array on success, or an ERR_PTR on failure.
+ */
+static struct pvr_vm_bind_op *
+pvr_vm_bind_ops_create_from_uapi(struct pvr_vm_context *vm_ctx,
+ struct pvr_file *pvr_file,
+ const struct drm_pvr_vm_bind_op *uapi_ops,
+ u32 op_count)
+{
+ struct pvr_vm_bind_op *ops;
+ int err;
+
+ ops = kvzalloc_objs(*ops, op_count, GFP_KERNEL);
+ if (!ops)
+ return ERR_PTR(-ENOMEM);
+
+ for (u32 prepared = 0; prepared < op_count; prepared++) {
+ err = pvr_vm_bind_op_init_from_uapi(&ops[prepared], vm_ctx,
+ pvr_file,
+ &uapi_ops[prepared]);
+ if (err) {
+ pvr_vm_bind_ops_free(ops, prepared);
+ return ERR_PTR(err);
+ }
+ }
+
+ return ops;
+}
+
+/**
+ * pvr_vm_bind_exec_async() - Queue a batch of bind operations.
+ * @vm_ctx: Target VM context.
+ * @ops: Prepared bind operations. Consumed by this function.
+ * @op_count: Number of entries in @ops.
+ * @pvr_file: PowerVR file the request was issued on.
+ * @sync_ops: Sync operations to apply to the request.
+ * @sync_op_count: Number of entries in @sync_ops.
+ *
+ * Wraps @ops in a &pvr_vm_bind_job and hands it to the VM context scheduler.
+ * The synchronous path needs no job at all; it applies @ops inline.
+ *
+ * Return:
+ * * 0 on success, or
+ * * Any error returned while resolving @sync_ops or arming the job.
+ */
+static int pvr_vm_bind_exec_async(struct pvr_vm_context *vm_ctx,
+ struct pvr_vm_bind_op *ops, u32 op_count,
+ struct pvr_file *pvr_file,
+ const struct drm_pvr_sync_op *sync_ops,
+ u32 sync_op_count)
+{
+ struct dma_fence *finished_fence;
+ struct pvr_vm_bind_job *job;
+ struct xarray signal_array;
+ int err;
+
+ job = kzalloc_obj(*job);
+ if (!job) {
+ pvr_vm_bind_ops_free(ops, op_count);
+ return -ENOMEM;
+ }
+
+ job->vm_ctx = pvr_vm_context_get(vm_ctx);
+ job->ops = ops;
+ job->op_count = op_count;
+ INIT_WORK(&job->cleanup_work, pvr_vm_bind_job_cleanup_work);
+
+ xa_init_flags(&signal_array, XA_FLAGS_ALLOC);
+
+ err = drm_sched_job_init(&job->base, &vm_ctx->entity, 1, pvr_file,
+ from_pvr_file(pvr_file)->client_id);
+ if (err)
+ goto err_cleanup_signal_array;
+
+ err = pvr_sync_signal_array_collect_ops(&signal_array,
+ from_pvr_file(pvr_file),
+ sync_op_count, sync_ops);
+ if (err)
+ goto err_cleanup_job;
+
+ err = pvr_sync_add_deps_to_job(pvr_file, &job->base, sync_op_count,
+ sync_ops, &signal_array);
+ if (err)
+ goto err_cleanup_job;
+
+ drm_sched_job_arm(&job->base);
+ finished_fence = &job->base.s_fence->finished;
+
+ /*
+ * Arming is the point of no return: the job has to be pushed now. The
+ * update below only touches entries the collect above created, so it
+ * cannot fail, and a driver bug that made it fail has already warned.
+ */
+ pvr_sync_signal_array_update_fences(&signal_array, sync_op_count,
+ sync_ops, finished_fence);
+
+ drm_sched_entity_push_job(&job->base);
+ pvr_sync_signal_array_push_fences(&signal_array);
+
+ pvr_sync_signal_array_cleanup(&signal_array);
+
+ return 0;
+
+err_cleanup_job:
+ drm_sched_job_cleanup(&job->base);
+
+err_cleanup_signal_array:
+ pvr_sync_signal_array_cleanup(&signal_array);
+ pvr_vm_bind_job_free(job);
+
+ return err;
+}
+
+/**
+ * pvr_vm_bind() - Apply a batch of bind operations to a VM context.
+ * @vm_ctx: Target VM context.
+ * @pvr_file: PowerVR file the request was issued on.
+ * @req: The request to apply.
+ *
+ * This is the single entry point for every userspace-initiated mapping change:
+ * %DRM_IOCTL_PVR_VM_BIND passes its whole operation array, while the legacy
+ * %DRM_IOCTL_PVR_VM_MAP and %DRM_IOCTL_PVR_VM_UNMAP build a one-element array.
+ *
+ * Return:
+ * * 0 on success, or
+ * * A negative error code on failure.
+ */
+int pvr_vm_bind(struct pvr_vm_context *vm_ctx, struct pvr_file *pvr_file,
+ const struct pvr_vm_bind_req *req)
+{
+ struct pvr_vm_bind_op *ops;
+ int err = 0;
+
+ if (pvr_vm_context_is_unusable(vm_ctx))
+ return -ECANCELED;
+
+ if (req->async && !vm_ctx->sched_initialised)
+ return -EINVAL;
+
+ ops = pvr_vm_bind_ops_create_from_uapi(vm_ctx, pvr_file, req->ops,
+ req->op_count);
+ if (IS_ERR(ops))
+ return PTR_ERR(ops);
+
+ if (req->async)
+ return pvr_vm_bind_exec_async(vm_ctx, ops, req->op_count,
+ pvr_file, req->sync_ops,
+ req->sync_op_count);
+
+ mutex_lock(&vm_ctx->lock);
+
+ if (pvr_vm_context_is_unusable(vm_ctx))
+ err = -ECANCELED;
+
+ for (u32 i = 0; !err && i < req->op_count; i++)
+ err = pvr_vm_bind_op_exec(&ops[i]);
+
+ mutex_unlock(&vm_ctx->lock);
+
+ pvr_vm_bind_ops_free(ops, req->op_count);
+ drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr);
+
+ return err;
+}
diff --git a/drivers/gpu/drm/imagination/pvr_vm.h b/drivers/gpu/drm/imagination/pvr_vm.h
index b0528dffa7f..76762133c64 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.h
+++ b/drivers/gpu/drm/imagination/pvr_vm.h
@@ -22,6 +22,8 @@ struct pvr_vm_context;
/* Forward declaration from <uapi/drm/pvr_drm.h> */
struct drm_pvr_ioctl_get_heap_info_args;
+struct drm_pvr_sync_op;
+struct drm_pvr_vm_bind_op;
/* Forward declaration from <drm/drm_exec.h> */
struct drm_exec;
@@ -44,6 +46,31 @@ int pvr_vm_unmap_obj(struct pvr_vm_context *vm_ctx,
int pvr_vm_unmap(struct pvr_vm_context *vm_ctx, u64 device_addr, u64 size);
void pvr_vm_unmap_all(struct pvr_vm_context *vm_ctx);
+bool pvr_vm_context_is_unusable(struct pvr_vm_context *vm_ctx);
+
+/**
+ * struct pvr_vm_bind_req - A VM bind request, as passed to pvr_vm_bind().
+ */
+struct pvr_vm_bind_req {
+ /** @ops: Array of userspace operation descriptions. */
+ const struct drm_pvr_vm_bind_op *ops;
+
+ /** @op_count: Number of entries in @ops. */
+ u32 op_count;
+
+ /** @sync_ops: Array of sync operations, or %NULL if there are none. */
+ const struct drm_pvr_sync_op *sync_ops;
+
+ /** @sync_op_count: Number of entries in @sync_ops. */
+ u32 sync_op_count;
+
+ /** @async: Queue the request instead of applying it inline. */
+ bool async;
+};
+
+int pvr_vm_bind(struct pvr_vm_context *vm_ctx, struct pvr_file *pvr_file,
+ const struct pvr_vm_bind_req *req);
+
dma_addr_t pvr_vm_get_page_table_root_addr(struct pvr_vm_context *vm_ctx);
struct dma_resv *pvr_vm_get_dma_resv(struct pvr_vm_context *vm_ctx);
diff --git a/include/uapi/drm/pvr_drm.h b/include/uapi/drm/pvr_drm.h
index ccf6c211246..eb1535cd513 100644
--- a/include/uapi/drm/pvr_drm.h
+++ b/include/uapi/drm/pvr_drm.h
@@ -108,6 +108,7 @@ struct drm_pvr_obj_array {
#define DRM_IOCTL_PVR_CREATE_HWRT_DATASET PVR_IOCTL(0x0b, DRM_IOWR, create_hwrt_dataset)
#define DRM_IOCTL_PVR_DESTROY_HWRT_DATASET PVR_IOCTL(0x0c, DRM_IOW, destroy_hwrt_dataset)
#define DRM_IOCTL_PVR_SUBMIT_JOBS PVR_IOCTL(0x0d, DRM_IOW, submit_jobs)
+#define DRM_IOCTL_PVR_VM_BIND PVR_IOCTL(0x0e, DRM_IOW, vm_bind)
/**
* DOC: PowerVR IOCTL DEV_QUERY interface
@@ -1288,6 +1289,120 @@ struct drm_pvr_ioctl_submit_jobs_args {
struct drm_pvr_obj_array jobs;
};
+/**
+ * DOC: PowerVR IOCTL VM_BIND interface
+ *
+ * %DRM_IOCTL_PVR_VM_BIND applies a batch of map and/or unmap operations to a
+ * single VM context, either before the IOCTL returns or, with
+ * %DRM_PVR_VM_BIND_ASYNC, from a queue.
+ *
+ * Operations within a request are applied in array order, and queued requests
+ * targeting one VM context in submission order. A synchronous request does not
+ * wait for the queued ones; a caller mixing the two on one VM context has to
+ * order them itself.
+ *
+ * A request that fails part way through may leave the address space in an
+ * undefined state; how much of it was applied is not reported.
+ */
+
+/**
+ * DOC: Flags for VM_BIND operations.
+ *
+ * The type of a VM bind operation is stored in the top four bits of
+ * &drm_pvr_vm_bind_op.flags.
+ *
+ * .. c:macro:: DRM_PVR_VM_BIND_OP_TYPE_MAP
+ *
+ * Create a new mapping. &drm_pvr_vm_bind_op.handle must be a valid buffer
+ * object handle.
+ *
+ * .. c:macro:: DRM_PVR_VM_BIND_OP_TYPE_UNMAP
+ *
+ * Remove existing mappings. &drm_pvr_vm_bind_op.handle and
+ * &drm_pvr_vm_bind_op.offset must both be zero.
+ *
+ * .. c:macro:: DRM_PVR_VM_BIND_OP_TYPE_MASK
+ *
+ * Mask used to extract the operation type.
+ */
+#define DRM_PVR_VM_BIND_OP_TYPE_MAP (0u << 28)
+#define DRM_PVR_VM_BIND_OP_TYPE_UNMAP (1u << 28)
+#define DRM_PVR_VM_BIND_OP_TYPE_MASK (0xfu << 28)
+
+#define DRM_PVR_VM_BIND_OP_FLAGS_MASK DRM_PVR_VM_BIND_OP_TYPE_MASK
+
+/**
+ * struct drm_pvr_vm_bind_op - A single VM bind operation.
+ */
+struct drm_pvr_vm_bind_op {
+ /** @flags: [IN] Combination of ``DRM_PVR_VM_BIND_OP_`` flags. */
+ __u32 flags;
+
+ /**
+ * @handle: [IN] Handle of the target buffer object.
+ *
+ * Must be a valid handle returned by %DRM_IOCTL_PVR_CREATE_BO for map
+ * operations. MBZ for unmap operations.
+ */
+ __u32 handle;
+
+ /**
+ * @offset: [IN] Offset into the target buffer object from which to
+ * begin the mapping. MBZ for unmap operations.
+ */
+ __u64 offset;
+
+ /**
+ * @device_addr: [IN] Device-virtual address at the start of the target
+ * range. This must be non-zero and must obey the same alignment and
+ * heap containment rules as %DRM_IOCTL_PVR_VM_MAP.
+ */
+ __u64 device_addr;
+
+ /** @size: [IN] Size in bytes of the target range. Must be non-zero. */
+ __u64 size;
+};
+
+/**
+ * DOC: Flags for the VM_BIND ioctl.
+ *
+ * .. c:macro:: DRM_PVR_VM_BIND_ASYNC
+ *
+ * Queue the request instead of applying it synchronously. Completion is
+ * reported through &drm_pvr_ioctl_vm_bind_args.sync_ops.
+ */
+#define DRM_PVR_VM_BIND_ASYNC _BITUL(0)
+
+#define DRM_PVR_VM_BIND_FLAGS_MASK DRM_PVR_VM_BIND_ASYNC
+
+/**
+ * struct drm_pvr_ioctl_vm_bind_args - Arguments for %DRM_IOCTL_PVR_VM_BIND.
+ */
+struct drm_pvr_ioctl_vm_bind_args {
+ /**
+ * @vm_context_handle: [IN] Handle for the VM context these operations
+ * apply to.
+ */
+ __u32 vm_context_handle;
+
+ /** @flags: [IN] Combination of ``DRM_PVR_VM_BIND_`` flags. */
+ __u32 flags;
+
+ /** @ops: [IN] Array of &struct drm_pvr_vm_bind_op to apply. */
+ struct drm_pvr_obj_array ops;
+
+ /**
+ * @sync_ops: [IN] Sync operations applied to the request as a whole.
+ * Waits are honoured before any of @ops is applied, signals fire once
+ * all of them have been. Must be empty unless %DRM_PVR_VM_BIND_ASYNC is
+ * set in @flags.
+ *
+ * A request with no operations but a non-empty @sync_ops is valid, and
+ * places a bare synchronisation point on the VM bind queue.
+ */
+ struct drm_pvr_obj_array sync_ops;
+};
+
#if defined(__cplusplus)
}
#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/4] drm/imagination: Add async VM_BIND ioctl
2026-08-16 19:42 ` [PATCH 3/4] drm/imagination: Add async VM_BIND ioctl Gyeyoung Baek
@ 2026-08-16 20:00 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-16 20:00 UTC (permalink / raw)
To: Gyeyoung Baek; +Cc: media-ci, dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Taking `vm_ctx->lock` inside a dma-fence signalling critical section creates a deadlock risk because legacy ioctl paths hold this lock during `GFP_KERNEL` memory allocations.
- [High] The `DRM_IOCTL_PVR_VM_BIND` ioctl accepts an unbounded number of operations, allowing a local denial-of-service via unbounded kernel memory allocations.
--
--- Patch [3]: [PATCH 3/4] drm/imagination: Add async VM_BIND ioctl ---
commit d69b2e6682fdd9c9fc81f9944e1b72971b7b0c0f
Author: Gyeyoung Baek <gye976@gmail.com>
Subject: drm/imagination: Add async VM_BIND ioctl
This commit adds a DRM_IOCTL_PVR_VM_BIND ioctl to the PowerVR driver to
support Vulkan sparse binding needs. It allows batching bind and sync
operations into a single request. Requests can be run asynchronously
as drm_sched jobs when the DRM_PVR_VM_BIND_ASYNC flag is provided.
> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index 5c965ef0274ff..ec6ed610e29e1 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
[ ... ]
> @@ -1130,6 +1130,93 @@ pvr_ioctl_vm_unmap(struct drm_device *drm_dev, void *raw_args,
[ ... ]
> +static int
> +pvr_ioctl_vm_bind(struct drm_device *drm_dev, void *raw_args,
> + struct drm_file *file)
> +{
> + struct drm_pvr_ioctl_vm_bind_args *args = raw_args;
[ ... ]
> + vm_ctx = pvr_vm_context_lookup(pvr_file, args->vm_context_handle);
> + if (!vm_ctx) {
> + err = -EINVAL;
> + goto err_drm_dev_exit;
> + }
> +
[Severity: High]
Could args->ops.count or args->sync_ops.count be arbitrarily large here?
Without an upper bound, could an unprivileged user exhaust system memory by
passing an extremely large count, as these allocations bypass memory cgroup
limits?
> + if (args->ops.count) {
> + err = PVR_UOBJ_GET_ARRAY(uapi_ops, &args->ops);
> + if (err)
> + goto err_put_vm_context;
> + }
> +
> + if (args->sync_ops.count) {
> + err = PVR_UOBJ_GET_ARRAY(sync_ops, &args->sync_ops);
> + if (err)
> + goto err_free_uapi_ops;
> + }
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 45df76e61f7cf..f92bfeacd7ac4 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
[ ... ]
> @@ -1174,3 +1242,439 @@ pvr_vm_get_fw_mem_context(struct pvr_vm_context *vm_ctx)
[ ... ]
> +static struct dma_fence *
> +pvr_vm_bind_run_job(struct drm_sched_job *sched_job)
> +{
> + struct pvr_vm_bind_job *job = to_pvr_vm_bind_job(sched_job);
> + struct pvr_vm_context *vm_ctx = job->vm_ctx;
> + int err = 0;
> + bool cookie;
> +
> + if (pvr_vm_context_is_unusable(vm_ctx))
> + return ERR_PTR(-ECANCELED);
> +
> + cookie = dma_fence_begin_signalling();
> +
[Severity: High]
Does acquiring vm_ctx->lock inside the dma-fence signalling critical section
create a circular deadlock risk?
The legacy pvr_vm_unmap() path acquires vm_ctx->lock to look up the mapping,
and then calls pvr_vm_unmap_obj_locked() -> pvr_vm_bind_op_unmap_init().
That init function performs GFP_KERNEL allocations under this same lock:
bind_op->prev_va = kzalloc_obj(*bind_op->prev_va);
bind_op->next_va = kzalloc_obj(*bind_op->next_va);
...
bind_op->mmu_op_ctx =
pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0);
Direct memory reclaim during these allocations might wait on dma-fences to be
signalled, which could deadlock if this lock is held.
> + mutex_lock(&vm_ctx->lock);
> +
> + for (u32 i = 0; i < job->op_count; i++) {
> + err = pvr_vm_bind_op_exec(&job->ops[i]);
> + if (err)
> + break;
> + }
[ ... ]
> +static struct pvr_vm_bind_op *
> +pvr_vm_bind_ops_create_from_uapi(struct pvr_vm_context *vm_ctx,
> + struct pvr_file *pvr_file,
> + const struct drm_pvr_vm_bind_op *uapi_ops,
> + u32 op_count)
> +{
> + struct pvr_vm_bind_op *ops;
> + int err;
> +
[Severity: High]
Could op_count be unbounded here? Since this allocates using GFP_KERNEL
without __GFP_ACCOUNT, large allocations won't be charged to a memcg,
potentially leading to system memory exhaustion or soft lockups during the
subsequent loop.
> + ops = kvzalloc_objs(*ops, op_count, GFP_KERNEL);
> + if (!ops)
> + return ERR_PTR(-ENOMEM);
> +
> + for (u32 prepared = 0; prepared < op_count; prepared++) {
> + err = pvr_vm_bind_op_init_from_uapi(&ops[prepared], vm_ctx,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-pvr-vm-bind-v1-0-0a0f21be7d38@gmail.com?part=3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 4/4] drm/imagination: Add sparse mappings to VM_BIND
2026-08-16 19:42 [PATCH 0/4] drm/imagination: Add async VM_BIND with sparse mappings Gyeyoung Baek
` (2 preceding siblings ...)
2026-08-16 19:42 ` [PATCH 3/4] drm/imagination: Add async VM_BIND ioctl Gyeyoung Baek
@ 2026-08-16 19:42 ` Gyeyoung Baek
2026-08-16 20:03 ` sashiko-bot
3 siblings, 1 reply; 9+ messages in thread
From: Gyeyoung Baek @ 2026-08-16 19:42 UTC (permalink / raw)
To: Alessio Belle, Luigi Santivetti, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Matt Coster,
Donald Robson, Sarah Walker, Sumit Semwal, Christian König
Cc: imagination, dri-devel, linux-kernel, linux-media, linaro-mm-sig,
Gyeyoung Baek
Vulkan sparse residency requires that an access to an unbound part of a
sparse resource does not fault, so a range has to be mappable without
naming any buffer object.
Add DRM_PVR_VM_BIND_OP_MAP_SPARSE:
every page table entry of the range points at the same dummy page,
so the mapping costs one physical page however wide it is.
The dummy is created per file rather than per device - writes to a sparse
range stay in this page, and one shared device-wide would let clients see
each other's writes.
Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
drivers/gpu/drm/imagination/pvr_device.h | 9 ++++
drivers/gpu/drm/imagination/pvr_drv.c | 14 ++++++-
drivers/gpu/drm/imagination/pvr_drv.h | 1 +
drivers/gpu/drm/imagination/pvr_mmu.c | 71 ++++++++++++++++++++++++++++++++
drivers/gpu/drm/imagination/pvr_mmu.h | 2 +
drivers/gpu/drm/imagination/pvr_vm.c | 52 ++++++++++++++++++-----
include/uapi/drm/pvr_drm.h | 32 +++++++++++---
7 files changed, 164 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
index d51c57cf933..133b5c789bf 100644
--- a/drivers/gpu/drm/imagination/pvr_device.h
+++ b/drivers/gpu/drm/imagination/pvr_device.h
@@ -380,6 +380,15 @@ struct pvr_file {
*/
struct xarray hwrt_handles;
+ /**
+ * @sparse_dummy_bo: Single page every sparse mapping made through this
+ * file points at.
+ *
+ * Writes to a sparse range land here, so it is kept per file rather
+ * than per device to keep them out of other clients' view.
+ */
+ struct pvr_gem_object *sparse_dummy_bo;
+
/**
* @vm_ctx_handles: Array of VM contexts belonging to this file. Array
* members are of type "struct pvr_vm_context *".
diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index ec6ed610e29..ae11fbf7528 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -1402,11 +1402,21 @@ static int
pvr_drm_driver_open(struct drm_device *drm_dev, struct drm_file *file)
{
struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
+ struct pvr_gem_object *dummy_bo;
struct pvr_file *pvr_file;
+ dummy_bo = pvr_gem_object_create(pvr_dev, PVR_DEVICE_PAGE_SIZE,
+ DRM_PVR_BO_BYPASS_DEVICE_CACHE);
+ if (IS_ERR(dummy_bo))
+ return PTR_ERR(dummy_bo);
+
pvr_file = kzalloc_obj(*pvr_file);
- if (!pvr_file)
+ if (!pvr_file) {
+ pvr_gem_object_put(dummy_bo);
return -ENOMEM;
+ }
+
+ pvr_file->sparse_dummy_bo = dummy_bo;
/*
* Store reference to base DRM file private data for use by
@@ -1460,6 +1470,8 @@ pvr_drm_driver_postclose(__always_unused struct drm_device *drm_dev,
pvr_destroy_hwrt_datasets_for_file(pvr_file);
pvr_destroy_vm_contexts_for_file(pvr_file);
+ pvr_gem_object_put(pvr_file->sparse_dummy_bo);
+
kfree(pvr_file);
file->driver_priv = NULL;
}
diff --git a/drivers/gpu/drm/imagination/pvr_drv.h b/drivers/gpu/drm/imagination/pvr_drv.h
index 9ca8f8780a3..109b99ea062 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.h
+++ b/drivers/gpu/drm/imagination/pvr_drv.h
@@ -14,6 +14,7 @@
* Driver interface version:
* - 1.0: Initial interface
* - 1.1: adds DRM_IOCTL_PVR_VM_BIND
+ * adds DRM_PVR_VM_BIND_OP_MAP_SPARSE flag
*/
#define PVR_DRIVER_MAJOR 1
#define PVR_DRIVER_MINOR 1
diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
index 3cac482e103..fc1347d6f23 100644
--- a/drivers/gpu/drm/imagination/pvr_mmu.c
+++ b/drivers/gpu/drm/imagination/pvr_mmu.c
@@ -2558,6 +2558,77 @@ pvr_mmu_map_sgl(struct pvr_mmu_op_context *op_ctx, struct scatterlist *sgl,
return err;
}
+/**
+ * pvr_mmu_map_dummy() - Point a range of device-virtual memory at a single
+ * repeated physical page.
+ * @op_ctx: Target MMU op context.
+ * @size: Size of memory to be mapped in bytes. Must be a non-zero multiple
+ * of the device page size.
+ * @flags: Flags from pvr_gem_object associated with the mapping.
+ * @device_addr: Virtual device address to map to. Must be device page-aligned.
+ *
+ * Every entry of the range is pointed at the first page of
+ * &pvr_mmu_op_context.map.sgt, so the mapping costs one page whatever @size is.
+ *
+ * Return:
+ * * 0 on success,
+ * * -%EINVAL if @size or @device_addr is misaligned, or
+ * * Any error encountered while creating a page with pvr_page_create(), or
+ * * Any error encountered while advancing @op_ctx.curr_page.
+ */
+int pvr_mmu_map_dummy(struct pvr_mmu_op_context *op_ctx, u64 size, u64 flags,
+ u64 device_addr)
+{
+ const u64 pages = size >> PVR_DEVICE_PAGE_SHIFT;
+ struct pvr_page_table_ptr ptr_copy;
+ struct pvr_page_flags_raw flags_raw;
+ dma_addr_t dma_addr;
+ u64 page;
+ int err;
+
+ if (!size)
+ return 0;
+
+ if (size & ~PVR_DEVICE_PAGE_MASK)
+ return -EINVAL;
+
+ dma_addr = sg_dma_address(op_ctx->map.sgt->sgl);
+
+ err = pvr_mmu_op_context_set_curr_page(op_ctx, device_addr, true);
+ if (err)
+ return -EINVAL;
+
+ memcpy(&ptr_copy, &op_ctx->curr_page, sizeof(ptr_copy));
+
+ flags_raw = pvr_page_flags_raw_create(false, false,
+ flags & DRM_PVR_BO_BYPASS_DEVICE_CACHE,
+ flags & DRM_PVR_BO_PM_FW_PROTECT);
+
+ err = pvr_page_create(op_ctx, dma_addr, flags_raw);
+ if (err)
+ return err;
+
+ for (page = 1; page < pages; ++page) {
+ err = pvr_mmu_op_context_next_page(op_ctx, true);
+ if (err)
+ goto err_destroy_pages;
+
+ err = pvr_page_create(op_ctx, dma_addr, flags_raw);
+ if (err)
+ goto err_destroy_pages;
+ }
+
+ pvr_mmu_op_context_require_sync(op_ctx, PVR_MMU_SYNC_LEVEL_0);
+
+ return 0;
+
+err_destroy_pages:
+ memcpy(&op_ctx->curr_page, &ptr_copy, sizeof(op_ctx->curr_page));
+ pvr_mmu_op_context_unmap_curr_page(op_ctx, page);
+
+ return err;
+}
+
/**
* pvr_mmu_map() - Map an object's virtual memory to physical memory.
* @op_ctx: Target MMU op context.
diff --git a/drivers/gpu/drm/imagination/pvr_mmu.h b/drivers/gpu/drm/imagination/pvr_mmu.h
index a8ecd460168..b014ea8efb0 100644
--- a/drivers/gpu/drm/imagination/pvr_mmu.h
+++ b/drivers/gpu/drm/imagination/pvr_mmu.h
@@ -101,6 +101,8 @@ struct pvr_mmu_op_context *
pvr_mmu_op_context_create(struct pvr_mmu_context *ctx,
struct sg_table *sgt, u64 sgt_offset, u64 size);
+int pvr_mmu_map_dummy(struct pvr_mmu_op_context *op_ctx, u64 size, u64 flags,
+ u64 device_addr);
int pvr_mmu_map(struct pvr_mmu_op_context *op_ctx, u64 size, u64 flags,
u64 device_addr);
int pvr_mmu_unmap(struct pvr_mmu_op_context *op_ctx, u64 device_addr, u64 size);
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index f92bfeacd7a..d11e192c772 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -200,6 +200,12 @@ struct pvr_vm_bind_op {
*/
struct pvr_vm_gpuva *next_va;
+ /**
+ * @sparse: The mapping repeats the file's dummy page rather than
+ * covering real pages of @pvr_obj.
+ */
+ bool sparse;
+
/** @offset: Offset into @pvr_obj to begin mapping from. */
u64 offset;
@@ -267,7 +273,7 @@ static int
pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
struct pvr_vm_context *vm_ctx,
struct pvr_gem_object *pvr_obj, u64 offset,
- u64 device_addr, u64 size)
+ u64 device_addr, u64 size, bool sparse)
{
struct drm_gem_object *obj = gem_from_pvr_gem(pvr_obj);
const bool is_user = vm_ctx != vm_ctx->pvr_dev->kernel_vm_ctx;
@@ -285,11 +291,19 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
}
if (!pvr_device_addr_and_size_are_valid(vm_ctx, device_addr, size) ||
- offset & ~PAGE_MASK || size & ~PAGE_MASK ||
- offset >= pvr_obj_size || offset_plus_size > pvr_obj_size)
+ offset & ~PAGE_MASK || size & ~PAGE_MASK)
+ return -EINVAL;
+
+ /*
+ * A sparse mapping repeats one page over a deliberately wider range,
+ * so the containment check only applies to ordinary mappings.
+ */
+ if (!sparse &&
+ (offset >= pvr_obj_size || offset_plus_size > pvr_obj_size))
return -EINVAL;
bind_op->type = PVR_VM_BIND_TYPE_MAP;
+ bind_op->sparse = sparse;
bind_op->gpuvm_bo = drm_gpuvm_bo_create(&vm_ctx->gpuvm_mgr, obj);
if (!bind_op->gpuvm_bo)
@@ -396,8 +410,12 @@ pvr_vm_gpuva_map(struct drm_gpuva_op *op, void *op_ctx)
if ((op->map.gem.offset | op->map.va.range) & ~PVR_DEVICE_PAGE_MASK)
return -EINVAL;
- err = pvr_mmu_map(ctx->mmu_op_ctx, op->map.va.range, pvr_gem->flags,
- op->map.va.addr);
+ if (ctx->sparse)
+ err = pvr_mmu_map_dummy(ctx->mmu_op_ctx, op->map.va.range,
+ pvr_gem->flags, op->map.va.addr);
+ else
+ err = pvr_mmu_map(ctx->mmu_op_ctx, op->map.va.range,
+ pvr_gem->flags, op->map.va.addr);
if (err)
return err;
@@ -790,7 +808,7 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
int err = pvr_vm_bind_op_map_init(&bind_op, vm_ctx, pvr_obj,
pvr_obj_offset, device_addr,
- size);
+ size, false);
if (err)
return err;
@@ -1475,14 +1493,25 @@ pvr_vm_bind_op_init_from_uapi(struct pvr_vm_bind_op *bind_op,
switch (uapi_op->flags & DRM_PVR_VM_BIND_OP_TYPE_MASK) {
case DRM_PVR_VM_BIND_OP_TYPE_MAP:
- pvr_obj = pvr_gem_object_from_handle(pvr_file, uapi_op->handle);
- if (!pvr_obj)
- return -ENOENT;
+ if (uapi_op->flags & DRM_PVR_VM_BIND_OP_MAP_SPARSE) {
+ if (uapi_op->handle || uapi_op->offset)
+ return -EINVAL;
+
+ pvr_obj = pvr_file->sparse_dummy_bo;
+ pvr_gem_object_get(pvr_obj);
+ } else {
+ pvr_obj = pvr_gem_object_from_handle(pvr_file,
+ uapi_op->handle);
+ if (!pvr_obj)
+ return -ENOENT;
+ }
err = pvr_vm_bind_op_map_init(bind_op, vm_ctx, pvr_obj,
uapi_op->offset,
uapi_op->device_addr,
- uapi_op->size);
+ uapi_op->size,
+ uapi_op->flags &
+ DRM_PVR_VM_BIND_OP_MAP_SPARSE);
if (err) {
pvr_gem_object_put(pvr_obj);
return err;
@@ -1494,6 +1523,9 @@ pvr_vm_bind_op_init_from_uapi(struct pvr_vm_bind_op *bind_op,
if (uapi_op->handle || uapi_op->offset)
return -EINVAL;
+ if (uapi_op->flags & DRM_PVR_VM_BIND_OP_MAP_SPARSE)
+ return -EINVAL;
+
return pvr_vm_bind_op_unmap_init(bind_op, vm_ctx, NULL,
uapi_op->device_addr,
uapi_op->size);
diff --git a/include/uapi/drm/pvr_drm.h b/include/uapi/drm/pvr_drm.h
index eb1535cd513..5cb63eb8182 100644
--- a/include/uapi/drm/pvr_drm.h
+++ b/include/uapi/drm/pvr_drm.h
@@ -1309,27 +1309,45 @@ struct drm_pvr_ioctl_submit_jobs_args {
* DOC: Flags for VM_BIND operations.
*
* The type of a VM bind operation is stored in the top four bits of
- * &drm_pvr_vm_bind_op.flags.
+ * &drm_pvr_vm_bind_op.flags. The remaining bits carry modifiers, each of which
+ * is only valid with one type.
+ *
+ * .. c:macro:: DRM_PVR_VM_BIND_OP_MAP_SPARSE
+ *
+ * Sparsely map a range of virtual addresses, without naming a buffer
+ * object. Only valid with %DRM_PVR_VM_BIND_OP_TYPE_MAP;
+ * &drm_pvr_vm_bind_op.handle and &drm_pvr_vm_bind_op.offset must both be
+ * zero.
+ *
+ * GPU accesses to the range do not fault. Reads return undefined values,
+ * and writes may become visible through other sparse mappings. Nothing
+ * more is guaranteed; in particular
+ * %VkPhysicalDeviceSparseProperties.residencyNonResidentStrict is not
+ * provided.
*
* .. c:macro:: DRM_PVR_VM_BIND_OP_TYPE_MAP
*
* Create a new mapping. &drm_pvr_vm_bind_op.handle must be a valid buffer
- * object handle.
+ * object handle, unless %DRM_PVR_VM_BIND_OP_MAP_SPARSE is also set.
*
* .. c:macro:: DRM_PVR_VM_BIND_OP_TYPE_UNMAP
*
* Remove existing mappings. &drm_pvr_vm_bind_op.handle and
- * &drm_pvr_vm_bind_op.offset must both be zero.
+ * &drm_pvr_vm_bind_op.offset must both be zero, as must
+ * %DRM_PVR_VM_BIND_OP_MAP_SPARSE.
*
* .. c:macro:: DRM_PVR_VM_BIND_OP_TYPE_MASK
*
* Mask used to extract the operation type.
*/
+#define DRM_PVR_VM_BIND_OP_MAP_SPARSE _BITUL(0)
+
#define DRM_PVR_VM_BIND_OP_TYPE_MAP (0u << 28)
#define DRM_PVR_VM_BIND_OP_TYPE_UNMAP (1u << 28)
#define DRM_PVR_VM_BIND_OP_TYPE_MASK (0xfu << 28)
-#define DRM_PVR_VM_BIND_OP_FLAGS_MASK DRM_PVR_VM_BIND_OP_TYPE_MASK
+#define DRM_PVR_VM_BIND_OP_FLAGS_MASK \
+ (DRM_PVR_VM_BIND_OP_MAP_SPARSE | DRM_PVR_VM_BIND_OP_TYPE_MASK)
/**
* struct drm_pvr_vm_bind_op - A single VM bind operation.
@@ -1342,13 +1360,15 @@ struct drm_pvr_vm_bind_op {
* @handle: [IN] Handle of the target buffer object.
*
* Must be a valid handle returned by %DRM_IOCTL_PVR_CREATE_BO for map
- * operations. MBZ for unmap operations.
+ * operations. MBZ for unmap operations, and for sparse map operations,
+ * which have no buffer object to name.
*/
__u32 handle;
/**
* @offset: [IN] Offset into the target buffer object from which to
- * begin the mapping. MBZ for unmap operations.
+ * begin the mapping. MBZ for unmap operations and for sparse map
+ * operations.
*/
__u64 offset;
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 4/4] drm/imagination: Add sparse mappings to VM_BIND
2026-08-16 19:42 ` [PATCH 4/4] drm/imagination: Add sparse mappings to VM_BIND Gyeyoung Baek
@ 2026-08-16 20:03 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-16 20:03 UTC (permalink / raw)
To: Gyeyoung Baek; +Cc: dri-devel, media-ci
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Integer underflow in page table preallocation size calculation allows unprivileged users to cause OOM panic.
- [High] Page table preallocation calculates requirements using object offset instead of device address, leading to -ENOMEM during valid mappings.
- [High] Mapping errors are masked during scatterlist processing, leaving holes in hardware page tables.
--
--- Patch [4]: [PATCH 4/4] drm/imagination: Add sparse mappings to VM_BIND ---
commit 8758f312b45a84c218f5a0a8c5687d9c21928110
Author: Gyeyoung Baek <gye976@gmail.com>
Subject: drm/imagination: Add sparse mappings to VM_BIND
This commit adds support for Vulkan sparse residency by allowing ranges
to be mapped without naming a buffer object. It introduces
DRM_PVR_VM_BIND_OP_MAP_SPARSE, where every page table entry in the range
points to the same file-specific dummy page.
> diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
> index 3cac482e10347..fc1347d6f233b 100644
> --- a/drivers/gpu/drm/imagination/pvr_mmu.c
> +++ b/drivers/gpu/drm/imagination/pvr_mmu.c
> @@ -2558,6 +2558,77 @@ pvr_mmu_map_sgl(struct pvr_mmu_op_context *op_ctx, struct scatterlist *sgl,
> return err;
> }
[Severity: High]
This is a pre-existing issue, but does pvr_mmu_map_sgl() mask the original
error code on failure?
If an error occurs during page table entry creation in pvr_mmu_map_sgl(), the
code jumps to the err_destroy_pages label:
drivers/gpu/drm/imagination/pvr_mmu.c:pvr_mmu_map_sgl() {
...
err_destroy_pages:
memcpy(&op_ctx->curr_page, &ptr_copy, sizeof(op_ctx->curr_page));
err = pvr_mmu_op_context_unmap_curr_page(op_ctx, page);
return err;
}
Since pvr_mmu_op_context_unmap_curr_page() typically succeeds and returns 0,
the original error code is overwritten with 0. The function then incorrectly
signals success to the caller, leaving holes in the hardware page tables.
This could cause unpredictable hardware page faults when accessed by the GPU.
[ ... ]
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index f92bfeacd7ac4..d11e192c7726d 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -267,7 +273,7 @@ static int
> pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
> struct pvr_vm_context *vm_ctx,
> struct pvr_gem_object *pvr_obj, u64 offset,
> - u64 device_addr, u64 size)
> + u64 device_addr, u64 size, bool sparse)
> {
> struct drm_gem_object *obj = gem_from_pvr_gem(pvr_obj);
> const bool is_user = vm_ctx != vm_ctx->pvr_dev->kernel_vm_ctx;
> @@ -285,11 +291,19 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
> }
>
> if (!pvr_device_addr_and_size_are_valid(vm_ctx, device_addr, size) ||
> - offset & ~PAGE_MASK || size & ~PAGE_MASK ||
> - offset >= pvr_obj_size || offset_plus_size > pvr_obj_size)
> + offset & ~PAGE_MASK || size & ~PAGE_MASK)
> + return -EINVAL;
[Severity: High]
This isn't a bug introduced by this patch, but does the page table preallocation
calculate requirements using the object offset instead of the device address?
When a VM_BIND request is made, pvr_vm_bind_op_map_init() calls
pvr_mmu_op_context_create(), passing offset as the sgt_offset:
drivers/gpu/drm/imagination/pvr_mmu.c:pvr_mmu_op_context_create() {
...
const u32 l0_start_idx = pvr_page_table_l1_idx(sgt_offset);
const u32 l0_end_idx = pvr_page_table_l1_idx(sgt_offset + size);
const u32 l0_count = l0_end_idx - l0_start_idx + 1;
...
}
If the device_addr alignment causes the mapping to span more 2MB or 1GB
boundaries than sgt_offset does, this will under-allocate page tables.
For instance, mapping an 8KB chunk with offset=0 to a device_addr that crosses
a 2MB boundary requires 2 L0 page tables, but the logic using sgt_offset
calculates only 1.
When the driver attempts to insert the second page, the preallocated pool is
empty and returns -ENOMEM. This would cause valid mappings to fail, and for
the newly introduced sparse mappings where offset is always 0, it guarantees
this failure whenever a sparse mapping crosses a 2MB boundary.
[Severity: Critical]
This is a pre-existing issue related to the above calculation, but can this
integer underflow allow unprivileged users to cause an OOM panic?
In the same calculation:
drivers/gpu/drm/imagination/pvr_mmu.c:pvr_mmu_op_context_create() {
...
const u32 l0_start_idx = pvr_page_table_l1_idx(sgt_offset);
const u32 l0_end_idx = pvr_page_table_l1_idx(sgt_offset + size);
const u32 l0_count = l0_end_idx - l0_start_idx + 1;
...
}
The function pvr_page_table_l1_idx() masks out higher bits to return a bounded
9-bit local index:
drivers/gpu/drm/imagination/pvr_mmu.c:pvr_page_table_l1_idx() {
return (device_addr & ~ROGUE_MMUCTRL_VADDR_PD_INDEX_CLRMSK) >>
ROGUE_MMUCTRL_VADDR_PD_INDEX_SHIFT;
}
If an unprivileged user calls the VM_BIND ioctl with an offset and size such
that offset + size crosses a 1GB page directory boundary, l0_end_idx will be
smaller than l0_start_idx (e.g., 0 < 511).
This causes l0_count to underflow to roughly 4.29 billion, leading to an
immediate kernel out-of-memory (OOM) panic as the driver attempts to allocate
billions of page table structures in a tight loop.
> + /*
> + * A sparse mapping repeats one page over a deliberately wider range,
> + * so the containment check only applies to ordinary mappings.
> + */
> + if (!sparse &&
> + (offset >= pvr_obj_size || offset_plus_size > pvr_obj_size))
> return -EINVAL;
>
> bind_op->type = PVR_VM_BIND_TYPE_MAP;
> + bind_op->sparse = sparse;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-pvr-vm-bind-v1-0-0a0f21be7d38@gmail.com?part=4
^ permalink raw reply [flat|nested] 9+ messages in thread