[PATCH v2 0/2] drm/imagination: Fix pre-existing bugs flagged by Sashiko's review of a VM_BIND series

[PATCH v2 0/2] drm/imagination: Fix pre-existing bugs flagged by Sashiko's review of a VM_BIND series

[Gyeyoung Baek]

These two patches fix pre-existing bugs in this driver that a VM_BIND
series depends on. They address what Sashiko flagged while reviewing
that series (see Link below).

This series is based on drm-misc-fixes, which already includes the
"Fixes for map() path" series (see Link below). Patch 2 is built on top
of 0a8224058a58 from that series.

tests/imagination/pvr_vm_map.c, sent separately to igt-dev (see Link
below) and not yet merged, tests these paths and passes with this
series applied.

Link: https://lore.kernel.org/all/20260817-pvr-vm-bind-v1-0-0a0f21be7d38@gmail.com/ # VM_BIND series Sashiko reviewed
Link: https://lore.kernel.org/all/20260926152341.783867-1-gye976@gmail.com/ # IGT reproducer (not yet merged)
Link: https://lore.kernel.org/all/20260922-mmu_fix-v4-0-12f1a871456a@imgtec.com/ # "Fixes for map() path" v4
Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
Changes in v2:
- Drop v1 patch 2, already fixed by 7b824c293a6b ("drm/imagination:
  Propagate map failures correctly from pvr_mmu_map_sgl()").
- Patch 2: add 0a8224058a58 to Fixes, as requested by Brajesh.
- Link to v1: https://patch.msgid.link/20260927-pvr-fixes-a-v1-0-7f5b18ab989a@gmail.com

---
Gyeyoung Baek (2):
      drm/imagination: Fix reference and vm_bo handling in remap()
      drm/imagination: Size page table preallocation by device address

 drivers/gpu/drm/imagination/pvr_mmu.c | 25 ++++++++++++-------------
 drivers/gpu/drm/imagination/pvr_vm.c  |  8 ++++----
 2 files changed, 16 insertions(+), 17 deletions(-)
---
base-commit: 4d0e2704118511e37790641d591454354e7c0d6c
change-id: 20260927-pvr-fixes-a-40876d8bbb25

Best regards,
--  
Gyeyoung Baek <gye976@gmail.com>

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

[Gyeyoung Baek]

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@gmail.com>
Reviewed-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
 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 55cc999f370..cbdd15ed74f 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;
 	}
 

-- 
2.43.0

[PATCH v2 2/2] drm/imagination: Size page table preallocation by device address

[Gyeyoung Baek]

Commit 0a8224058a58 ("drm/imagination: Fix page count for page table for
map() interface") passed the device address to
pvr_mmu_op_context_create(), but the preallocation is still sized from
device_addr + sgt_offset with an exclusive end. The offset into the
object's pages has no place in a device-virtual range, and the exclusive
end allocates one table too many when the range ends on a table
boundary.

Count the tables from device_addr and the inclusive end of the range.

Fixes: 0a8224058a58 ("drm/imagination: Fix page count for page table for map() interface")
Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 drivers/gpu/drm/imagination/pvr_mmu.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
index 62eae7fcd5a..67e73d7d5d0 100644
--- a/drivers/gpu/drm/imagination/pvr_mmu.c
+++ b/drivers/gpu/drm/imagination/pvr_mmu.c
@@ -2337,7 +2337,7 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
  * @ctx: MMU context associated with owning VM context.
  * @sgt: Scatter gather table containing pages pinned for use by this context.
  * @device_addr: Virtual device address at the start of the requested mapping.
- * @sgt_offset: Start offset of the requested device-virtual memory mapping.
+ * @sgt_offset: Offset into @sgt of the start of the requested mapping.
  * @size: Size in bytes of the requested device-virtual memory mapping. For an
  * unmapping, this should be zero so that no page tables are allocated.
  *
@@ -2350,7 +2350,6 @@ struct pvr_mmu_op_context *
 pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
 			  u64 device_addr, u64 sgt_offset, u64 size)
 {
-	u64 start_addr = device_addr + sgt_offset;
 	int err;
 
 	struct pvr_mmu_op_context *op_ctx = kzalloc_obj(*op_ctx);
@@ -2365,18 +2364,18 @@ pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
 
 	if (size) {
 		/*
-		 * The number of page table objects we need to prealloc is
-		 * indicated by the mapping size, start address and the sizes
-		 * of the areas mapped per PT or PD. The range calculation is
-		 * identical to that for the index into a table for a device
-		 * address, so we reuse those functions here.
+		 * The page tables needed are set by the device-virtual range
+		 * being mapped: one level 1 table per 1GiB region and one
+		 * level 0 table per 2MiB region the range touches. Tables that
+		 * already exist are not consumed, so this is an upper bound.
 		 */
-		const u32 l1_start_idx = pvr_page_table_l2_idx(start_addr);
-		const u32 l1_end_idx = pvr_page_table_l2_idx(start_addr + size);
-		const u32 l1_count = l1_end_idx - l1_start_idx + 1;
-		const u32 l0_start_idx = pvr_page_table_l1_idx(start_addr);
-		const u32 l0_end_idx = pvr_page_table_l1_idx(start_addr + size);
-		const u32 l0_count = l0_end_idx - l0_start_idx + 1;
+		const u64 last_addr = device_addr + size - 1;
+		const u64 l1_count =
+			(last_addr >> ROGUE_MMUCTRL_VADDR_PC_INDEX_SHIFT) -
+			(device_addr >> ROGUE_MMUCTRL_VADDR_PC_INDEX_SHIFT) + 1;
+		const u64 l0_count =
+			(last_addr >> ROGUE_MMUCTRL_VADDR_PD_INDEX_SHIFT) -
+			(device_addr >> ROGUE_MMUCTRL_VADDR_PD_INDEX_SHIFT) + 1;
 
 		/*
 		 * Alloc and push page table entries until we have enough of

-- 
2.43.0

Re: [PATCH v2 2/2] drm/imagination: Size page table preallocation by device address

[Brajesh Gupta]

On Thu, 2026-10-01 at 22:39 +0900, Gyeyoung Baek wrote:
Hi Gyeyoung,
> Commit 0a8224058a58 ("drm/imagination: Fix page count for page table for
> map() interface") passed the device address to
> pvr_mmu_op_context_create(), but the preallocation is still sized from
> device_addr + sgt_offset with an exclusive end. The offset into the
> object's pages has no place in a device-virtual range, and the exclusive
> end allocates one table too many when the range ends on a table
> boundary.
> 
> Count the tables from device_addr and the inclusive end of the range.
> 
> Fixes: 0a8224058a58 ("drm/imagination: Fix page count for page table for map() interface")
> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---
>  drivers/gpu/drm/imagination/pvr_mmu.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
> index 62eae7fcd5a..67e73d7d5d0 100644
> --- a/drivers/gpu/drm/imagination/pvr_mmu.c
> +++ b/drivers/gpu/drm/imagination/pvr_mmu.c
> @@ -2337,7 +2337,7 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
>   * @ctx: MMU context associated with owning VM context.
>   * @sgt: Scatter gather table containing pages pinned for use by this context.
>   * @device_addr: Virtual device address at the start of the requested mapping.
> - * @sgt_offset: Start offset of the requested device-virtual memory mapping.
> + * @sgt_offset: Offset into @sgt of the start of the requested mapping.
>   * @size: Size in bytes of the requested device-virtual memory mapping. For an
>   * unmapping, this should be zero so that no page tables are allocated.
>   *
> @@ -2350,7 +2350,6 @@ struct pvr_mmu_op_context *
>  pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
>  			  u64 device_addr, u64 sgt_offset, u64 size)
>  {
> -	u64 start_addr = device_addr + sgt_offset;
>  	int err;
>  
>  	struct pvr_mmu_op_context *op_ctx = kzalloc_obj(*op_ctx);
> @@ -2365,18 +2364,18 @@ pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
>  
>  	if (size) {
>  		/*
> -		 * The number of page table objects we need to prealloc is
> -		 * indicated by the mapping size, start address and the sizes
> -		 * of the areas mapped per PT or PD. The range calculation is
> -		 * identical to that for the index into a table for a device
> -		 * address, so we reuse those functions here.
> +		 * The page tables needed are set by the device-virtual range
> +		 * being mapped: one level 1 table per 1GiB region and one
> +		 * level 0 table per 2MiB region the range touches. Tables that
> +		 * already exist are not consumed, so this is an upper bound.
>  		 */
> -		const u32 l1_start_idx = pvr_page_table_l2_idx(start_addr);
> -		const u32 l1_end_idx = pvr_page_table_l2_idx(start_addr + size);
> -		const u32 l1_count = l1_end_idx - l1_start_idx + 1;
> -		const u32 l0_start_idx = pvr_page_table_l1_idx(start_addr);
> -		const u32 l0_end_idx = pvr_page_table_l1_idx(start_addr + size);
> -		const u32 l0_count = l0_end_idx - l0_start_idx + 1;
> +		const u64 last_addr = device_addr + size - 1;
> +		const u64 l1_count =
> +			(last_addr >> ROGUE_MMUCTRL_VADDR_PC_INDEX_SHIFT) -
> +			(device_addr >> ROGUE_MMUCTRL_VADDR_PC_INDEX_SHIFT) + 1;
> +		const u64 l0_count =
> +			(last_addr >> ROGUE_MMUCTRL_VADDR_PD_INDEX_SHIFT) -
> +			(device_addr >> ROGUE_MMUCTRL_VADDR_PD_INDEX_SHIFT) + 1;
Shouldn't we use 'device_addr + sgt_offset' instead of just 'device_addr' as
start address for l0/l1 count calculation? Only account for requested mapping
instead of whole memory.

Thanks,
Brajesh
>  
>  		/*
>  		 * Alloc and push page table entries until we have enough of
> 

Re: [PATCH v2 2/2] drm/imagination: Size page table preallocation by device address

[Gyeyoung Baek]

Hi Brajesh,

> > -             const u32 l1_start_idx = pvr_page_table_l2_idx(start_addr);
> > -             const u32 l1_end_idx = pvr_page_table_l2_idx(start_addr + size);
> > -             const u32 l1_count = l1_end_idx - l1_start_idx + 1;
> > -             const u32 l0_start_idx = pvr_page_table_l1_idx(start_addr);
> > -             const u32 l0_end_idx = pvr_page_table_l1_idx(start_addr + size);
> > -             const u32 l0_count = l0_end_idx - l0_start_idx + 1;
> > +             const u64 last_addr = device_addr + size - 1;
> > +             const u64 l1_count =
> > +                     (last_addr >> ROGUE_MMUCTRL_VADDR_PC_INDEX_SHIFT) -
> > +                     (device_addr >> ROGUE_MMUCTRL_VADDR_PC_INDEX_SHIFT) + 1;
> > +             const u64 l0_count =
> > +                     (last_addr >> ROGUE_MMUCTRL_VADDR_PD_INDEX_SHIFT) -
> > +                     (device_addr >> ROGUE_MMUCTRL_VADDR_PD_INDEX_SHIFT) + 1;
> Shouldn't we use 'device_addr + sgt_offset' instead of just 'device_addr' as
> start address for l0/l1 count calculation? Only account for requested mapping
> instead of whole memory.
>

This already counts only the requested mapping. `device_addr` is not
the start of the BO but the GPU address where this mapping starts, and
[device_addr, device_addr + size) is exactly the range pvr_mmu_map()
fills.

Adding sgt_offset does not narrow the range; it shifts it by
sgt_offset, which is an offset into the BO, not a GPU address. This
can leave too few preallocated tables, and VM_MAP then fails with
-ENOMEM.

-- 
Thanks,
Gyeyoung