Re: [PATCH 1/3] drm/imagination: Fix reference and vm_bo handling in remap()

[Brajesh Gupta]

On Sun, 2026-09-27 at 17:25 +0900, Gyeyoung Baek wrote:
Hi Gyeyoung,
> When a map overlaps part of an existing mapping, pvr_vm_gpuva_remap()
> splits the mapping into prev/next parts covering what the request did
> not take, instead of creating something new. It gets two things wrong.
> 
> - A GEM reference is taken for each part but it's never needed and never
>   dropped, so it leaks one reference per split (remap-next-in-2m in
>   tests/imagination/pvr_vm_map.c):
> 
>     CRITICAL: 33558528 bytes of shmem still held after close
> 
> - The parts still belong to the original object being split, but
>   pvr_vm_gpuva_remap() links them to ctx->gpuvm_bo, the new object's
>   vm_bo:
> 
>     prev_va --obj--> BO_A          BO_B <--obj-- vm_bo (ctx->gpuvm_bo)
>             \___________link___________/  (mismatch: BO_A != BO_B)
> 
>   drm_gpuva_link() catches the mismatch:
> 
>     WARNING: drivers/gpu/drm/drm_gpuvm.c:2108 at drm_gpuva_link+0x2ec/0x310
>       drm_WARN_ON(obj != vm_bo->obj)
>     Call trace:
>      drm_gpuva_link
>      pvr_vm_gpuva_remap
>      __drm_gpuvm_sm_map
>      pvr_vm_map
>      pvr_ioctl_vm_map
> 
>   Link them to op->remap.unmap->va->vm_bo instead.
> 
> The locking of the GPUVA lists of the other objects touched by a split is
> not addressed here; it is handled by switching the GPUVM to immediate
> mode.
> 
> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
> Signed-off-by: Gyeyoung Baek <gye976@xxxxxxxxx>

Reviewed-by: Brajesh Gupta <brajesh.gupta@xxxxxxxxxx>

Thanks,
Brajesh
> ---
>  drivers/gpu/drm/imagination/pvr_vm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index ceb78694cd9..c5ae0b79fe6 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)
>  {
> +	/* 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;
>  	u64 va_start = 0, va_range = 0;
>  	int err;
> @@ -433,14 +435,12 @@ 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) {
> -		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;
>  	}
>  
>