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

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

[Gyeyoung Baek]

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

tests/imagination/pvr_vm_map.c, sent separately to igt-dev (see Link
below) and not yet merged, reproduces all three bugs and shows they are
fixed by these patches.

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)
Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
Gyeyoung Baek (3):
      drm/imagination: Fix reference and vm_bo handling in remap()
      drm/imagination: Fix pvr_mmu_map_sgl() overwriting its own error
      drm/imagination: Size page table preallocation by device address

 drivers/gpu/drm/imagination/pvr_mmu.c | 29 +++++++++++++++--------------
 drivers/gpu/drm/imagination/pvr_mmu.h |  3 ++-
 drivers/gpu/drm/imagination/pvr_vm.c  | 13 +++++++------
 3 files changed, 24 insertions(+), 21 deletions(-)
---
base-commit: 98c7fc4219b93f953ee85c07fb6c8256046aba73
change-id: 20260927-pvr-fixes-a-40876d8bbb25

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

[PATCH 1/3] 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>
---
 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;
 	}
 

-- 
2.43.0

[PATCH 2/3] drm/imagination: Fix pvr_mmu_map_sgl() overwriting its own error

[Gyeyoung Baek]

pvr_mmu_op_context_unmap_curr_page() is called here to clean up after an
error. Its own success overwrites that error, so success is returned.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
index 3cac482e103..3e494bfa936 100644
--- a/drivers/gpu/drm/imagination/pvr_mmu.c
+++ b/drivers/gpu/drm/imagination/pvr_mmu.c
@@ -2553,7 +2553,7 @@ pvr_mmu_map_sgl(struct pvr_mmu_op_context *op_ctx, struct scatterlist *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);
+	pvr_mmu_op_context_unmap_curr_page(op_ctx, page);
 
 	return err;
 }

-- 
2.43.0

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

[Gyeyoung Baek]

pvr_mmu_op_context_create() preallocates the page tables a mapping
needs. How many is set by the device-virtual address being mapped, not
by the offset into the object's pages, which it used instead.

Size the preallocation from the device address instead.

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 | 27 ++++++++++++++-------------
 drivers/gpu/drm/imagination/pvr_mmu.h |  3 ++-
 drivers/gpu/drm/imagination/pvr_vm.c  |  5 +++--
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
index 3e494bfa936..dd5f6d32864 100644
--- a/drivers/gpu/drm/imagination/pvr_mmu.c
+++ b/drivers/gpu/drm/imagination/pvr_mmu.c
@@ -2335,7 +2335,8 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
  * pvr_mmu_op_context_create() - Create an MMU op context.
  * @ctx: MMU context associated with owning VM context.
  * @sgt: Scatter gather table containing pages pinned for use by this context.
- * @sgt_offset: Start offset of the requested device-virtual memory mapping.
+ * @sgt_offset: Offset into @sgt of the start of the requested mapping.
+ * @device_addr: Device-virtual address at 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.
  *
@@ -2346,7 +2347,7 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
  */
 struct pvr_mmu_op_context *
 pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
-			  u64 sgt_offset, u64 size)
+			  u64 sgt_offset, u64 device_addr, u64 size)
 {
 	int err;
 
@@ -2362,18 +2363,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 offset 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(sgt_offset);
-		const u32 l1_end_idx = pvr_page_table_l2_idx(sgt_offset + size);
-		const u32 l1_count = l1_end_idx - l1_start_idx + 1;
-		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;
+		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
diff --git a/drivers/gpu/drm/imagination/pvr_mmu.h b/drivers/gpu/drm/imagination/pvr_mmu.h
index a8ecd460168..7088f80805e 100644
--- a/drivers/gpu/drm/imagination/pvr_mmu.h
+++ b/drivers/gpu/drm/imagination/pvr_mmu.h
@@ -99,7 +99,8 @@ dma_addr_t pvr_mmu_get_root_table_dma_addr(struct pvr_mmu_context *ctx);
 void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx);
 struct pvr_mmu_op_context *
 pvr_mmu_op_context_create(struct pvr_mmu_context *ctx,
-			  struct sg_table *sgt, u64 sgt_offset, u64 size);
+			  struct sg_table *sgt, u64 sgt_offset,
+			  u64 device_addr, u64 size);
 
 int pvr_mmu_map(struct pvr_mmu_op_context *op_ctx, u64 size, u64 flags,
 		u64 device_addr);
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index c5ae0b79fe6..7bb58ca14e4 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -276,7 +276,8 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
 		goto err_bind_op_fini;
 
 	bind_op->mmu_op_ctx =
-		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, sgt, offset, size);
+		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, sgt, offset,
+					  device_addr, size);
 	err = PTR_ERR_OR_ZERO(bind_op->mmu_op_ctx);
 	if (err) {
 		bind_op->mmu_op_ctx = NULL;
@@ -318,7 +319,7 @@ pvr_vm_bind_op_unmap_init(struct pvr_vm_bind_op *bind_op,
 	}
 
 	bind_op->mmu_op_ctx =
-		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0);
+		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0, 0);
 	err = PTR_ERR_OR_ZERO(bind_op->mmu_op_ctx);
 	if (err) {
 		bind_op->mmu_op_ctx = NULL;

-- 
2.43.0

Re: [PATCH 2/3] drm/imagination: Fix pvr_mmu_map_sgl() overwriting its own error

[Brajesh Gupta]

On Sun, 2026-09-27 at 17:25 +0900, Gyeyoung Baek wrote:
Hi Gyeyoung,
> pvr_mmu_op_context_unmap_curr_page() is called here to clean up after an
> error. Its own success overwrites that error, so success is returned.
> 
This issue is recently fixed under
https://patchwork.freedesktop.org/patch/755273/. Please have a look.

Thanks,
Brajesh

> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
> index 3cac482e103..3e494bfa936 100644
> --- a/drivers/gpu/drm/imagination/pvr_mmu.c
> +++ b/drivers/gpu/drm/imagination/pvr_mmu.c
> @@ -2553,7 +2553,7 @@ pvr_mmu_map_sgl(struct pvr_mmu_op_context *op_ctx, struct scatterlist *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);
> +	pvr_mmu_op_context_unmap_curr_page(op_ctx, page);
>  
>  	return err;
>  }
> 

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

[Brajesh Gupta]

On Sun, 2026-09-27 at 17:25 +0900, Gyeyoung Baek wrote:
Hi Gyeyoung,
> pvr_mmu_op_context_create() preallocates the page tables a mapping
> needs. How many is set by the device-virtual address being mapped, not
> by the offset into the object's pages, which it used instead.
> 
> Size the preallocation from the device address instead.
> 
This issue is recently fixed under
https://patchwork.freedesktop.org/patch/755274/. Please have a look.

Thanks,
Brajesh
> 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 | 27 ++++++++++++++-------------
>  drivers/gpu/drm/imagination/pvr_mmu.h |  3 ++-
>  drivers/gpu/drm/imagination/pvr_vm.c  |  5 +++--
>  3 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
> index 3e494bfa936..dd5f6d32864 100644
> --- a/drivers/gpu/drm/imagination/pvr_mmu.c
> +++ b/drivers/gpu/drm/imagination/pvr_mmu.c
> @@ -2335,7 +2335,8 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
>   * pvr_mmu_op_context_create() - Create an MMU op context.
>   * @ctx: MMU context associated with owning VM context.
>   * @sgt: Scatter gather table containing pages pinned for use by this context.
> - * @sgt_offset: Start offset of the requested device-virtual memory mapping.
> + * @sgt_offset: Offset into @sgt of the start of the requested mapping.
> + * @device_addr: Device-virtual address at 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.
>   *
> @@ -2346,7 +2347,7 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
>   */
>  struct pvr_mmu_op_context *
>  pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
> -			  u64 sgt_offset, u64 size)
> +			  u64 sgt_offset, u64 device_addr, u64 size)
>  {
>  	int err;
>  
> @@ -2362,18 +2363,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 offset 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(sgt_offset);
> -		const u32 l1_end_idx = pvr_page_table_l2_idx(sgt_offset + size);
> -		const u32 l1_count = l1_end_idx - l1_start_idx + 1;
> -		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;
> +		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
> diff --git a/drivers/gpu/drm/imagination/pvr_mmu.h b/drivers/gpu/drm/imagination/pvr_mmu.h
> index a8ecd460168..7088f80805e 100644
> --- a/drivers/gpu/drm/imagination/pvr_mmu.h
> +++ b/drivers/gpu/drm/imagination/pvr_mmu.h
> @@ -99,7 +99,8 @@ dma_addr_t pvr_mmu_get_root_table_dma_addr(struct pvr_mmu_context *ctx);
>  void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx);
>  struct pvr_mmu_op_context *
>  pvr_mmu_op_context_create(struct pvr_mmu_context *ctx,
> -			  struct sg_table *sgt, u64 sgt_offset, u64 size);
> +			  struct sg_table *sgt, u64 sgt_offset,
> +			  u64 device_addr, u64 size);
>  
>  int pvr_mmu_map(struct pvr_mmu_op_context *op_ctx, u64 size, u64 flags,
>  		u64 device_addr);
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index c5ae0b79fe6..7bb58ca14e4 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -276,7 +276,8 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
>  		goto err_bind_op_fini;
>  
>  	bind_op->mmu_op_ctx =
> -		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, sgt, offset, size);
> +		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, sgt, offset,
> +					  device_addr, size);
>  	err = PTR_ERR_OR_ZERO(bind_op->mmu_op_ctx);
>  	if (err) {
>  		bind_op->mmu_op_ctx = NULL;
> @@ -318,7 +319,7 @@ pvr_vm_bind_op_unmap_init(struct pvr_vm_bind_op *bind_op,
>  	}
>  
>  	bind_op->mmu_op_ctx =
> -		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0);
> +		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0, 0);
>  	err = PTR_ERR_OR_ZERO(bind_op->mmu_op_ctx);
>  	if (err) {
>  		bind_op->mmu_op_ctx = NULL;
> 

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@gmail.com>

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

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;
>  	}
>  
> 

Re: [PATCH 2/3] drm/imagination: Fix pvr_mmu_map_sgl() overwriting its own error

[Gyeyoung Baek]

Hi, Thanks for pointing that out, I should have rechecked the tree before
sending v2.
Sorry for the noise!


> This issue is recently fixed under
> https://patchwork.freedesktop.org/patch/755273/. Please have a look.
>

--
Thanks,
Gyeyoung

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

[Brajesh Gupta]

On Mon, 2026-09-28 at 09:03 +0000, Brajesh Gupta wrote:
> On Sun, 2026-09-27 at 17:25 +0900, Gyeyoung Baek wrote:
Hi Gyeyoung,
> Hi Gyeyoung,
> > pvr_mmu_op_context_create() preallocates the page tables a mapping
> > needs. How many is set by the device-virtual address being mapped, not
> > by the offset into the object's pages, which it used instead.
> > 
> > Size the preallocation from the device address instead.
> > 
> This issue is recently fixed under
> https://patchwork.freedesktop.org/patch/755274/. Please have a look.
> 
> Thanks,
> Brajesh
> > 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 | 27 ++++++++++++++-------------
> >  drivers/gpu/drm/imagination/pvr_mmu.h |  3 ++-
> >  drivers/gpu/drm/imagination/pvr_vm.c  |  5 +++--
> >  3 files changed, 19 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
> > index 3e494bfa936..dd5f6d32864 100644
> > --- a/drivers/gpu/drm/imagination/pvr_mmu.c
> > +++ b/drivers/gpu/drm/imagination/pvr_mmu.c
> > @@ -2335,7 +2335,8 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
> >   * pvr_mmu_op_context_create() - Create an MMU op context.
> >   * @ctx: MMU context associated with owning VM context.
> >   * @sgt: Scatter gather table containing pages pinned for use by this context.
> > - * @sgt_offset: Start offset of the requested device-virtual memory mapping.
> > + * @sgt_offset: Offset into @sgt of the start of the requested mapping.
> > + * @device_addr: Device-virtual address at 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.
> >   *
> > @@ -2346,7 +2347,7 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
> >   */
> >  struct pvr_mmu_op_context *
> >  pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
> > -			  u64 sgt_offset, u64 size)
> > +			  u64 sgt_offset, u64 device_addr, u64 size)
> >  {
> >  	int err;
> >  
> > @@ -2362,18 +2363,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 offset 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(sgt_offset);
> > -		const u32 l1_end_idx = pvr_page_table_l2_idx(sgt_offset + size);
> > -		const u32 l1_count = l1_end_idx - l1_start_idx + 1;
> > -		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;
> > +		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;
> >  
We identified over allocation of page tables due to incorrect calculation of end
offset. And your change is correctly takes that into account. 
Can you please update your patch on top
https://patchwork.freedesktop.org/patch/755274?

Also update fixes tag to include https://patchwork.freedesktop.org/patch/755274?

Thanks,
Brajesh

> >  		/*
> >  		 * Alloc and push page table entries until we have enough of
> > diff --git a/drivers/gpu/drm/imagination/pvr_mmu.h b/drivers/gpu/drm/imagination/pvr_mmu.h
> > index a8ecd460168..7088f80805e 100644
> > --- a/drivers/gpu/drm/imagination/pvr_mmu.h
> > +++ b/drivers/gpu/drm/imagination/pvr_mmu.h
> > @@ -99,7 +99,8 @@ dma_addr_t pvr_mmu_get_root_table_dma_addr(struct pvr_mmu_context *ctx);
> >  void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx);
> >  struct pvr_mmu_op_context *
> >  pvr_mmu_op_context_create(struct pvr_mmu_context *ctx,
> > -			  struct sg_table *sgt, u64 sgt_offset, u64 size);
> > +			  struct sg_table *sgt, u64 sgt_offset,
> > +			  u64 device_addr, u64 size);
> >  
> >  int pvr_mmu_map(struct pvr_mmu_op_context *op_ctx, u64 size, u64 flags,
> >  		u64 device_addr);
> > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> > index c5ae0b79fe6..7bb58ca14e4 100644
> > --- a/drivers/gpu/drm/imagination/pvr_vm.c
> > +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> > @@ -276,7 +276,8 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
> >  		goto err_bind_op_fini;
> >  
> >  	bind_op->mmu_op_ctx =
> > -		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, sgt, offset, size);
> > +		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, sgt, offset,
> > +					  device_addr, size);
> >  	err = PTR_ERR_OR_ZERO(bind_op->mmu_op_ctx);
> >  	if (err) {
> >  		bind_op->mmu_op_ctx = NULL;
> > @@ -318,7 +319,7 @@ pvr_vm_bind_op_unmap_init(struct pvr_vm_bind_op *bind_op,
> >  	}
> >  
> >  	bind_op->mmu_op_ctx =
> > -		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0);
> > +		pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0, 0);
> >  	err = PTR_ERR_OR_ZERO(bind_op->mmu_op_ctx);
> >  	if (err) {
> >  		bind_op->mmu_op_ctx = NULL;
> > 
> 

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

[Gyeyoung Baek]

Sure!

On Thu, Oct 1, 2026 at 1:47 PM Brajesh Gupta <Brajesh.Gupta@imgtec.com> wrote:
>
> On Mon, 2026-09-28 at 09:03 +0000, Brajesh Gupta wrote:
> > On Sun, 2026-09-27 at 17:25 +0900, Gyeyoung Baek wrote:
> Hi Gyeyoung,
> > Hi Gyeyoung,
> > > pvr_mmu_op_context_create() preallocates the page tables a mapping
> > > needs. How many is set by the device-virtual address being mapped, not
> > > by the offset into the object's pages, which it used instead.
> > >
> > > Size the preallocation from the device address instead.
> > >
> > This issue is recently fixed under
> > https://patchwork.freedesktop.org/patch/755274/. Please have a look.
> >
> > Thanks,
> > Brajesh
> > > 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 | 27 ++++++++++++++-------------
> > >  drivers/gpu/drm/imagination/pvr_mmu.h |  3 ++-
> > >  drivers/gpu/drm/imagination/pvr_vm.c  |  5 +++--
> > >  3 files changed, 19 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c b/drivers/gpu/drm/imagination/pvr_mmu.c
> > > index 3e494bfa936..dd5f6d32864 100644
> > > --- a/drivers/gpu/drm/imagination/pvr_mmu.c
> > > +++ b/drivers/gpu/drm/imagination/pvr_mmu.c
> > > @@ -2335,7 +2335,8 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
> > >   * pvr_mmu_op_context_create() - Create an MMU op context.
> > >   * @ctx: MMU context associated with owning VM context.
> > >   * @sgt: Scatter gather table containing pages pinned for use by this context.
> > > - * @sgt_offset: Start offset of the requested device-virtual memory mapping.
> > > + * @sgt_offset: Offset into @sgt of the start of the requested mapping.
> > > + * @device_addr: Device-virtual address at 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.
> > >   *
> > > @@ -2346,7 +2347,7 @@ void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx)
> > >   */
> > >  struct pvr_mmu_op_context *
> > >  pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt,
> > > -                     u64 sgt_offset, u64 size)
> > > +                     u64 sgt_offset, u64 device_addr, u64 size)
> > >  {
> > >     int err;
> > >
> > > @@ -2362,18 +2363,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 offset 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(sgt_offset);
> > > -           const u32 l1_end_idx = pvr_page_table_l2_idx(sgt_offset + size);
> > > -           const u32 l1_count = l1_end_idx - l1_start_idx + 1;
> > > -           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;
> > > +           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;
> > >
> We identified over allocation of page tables due to incorrect calculation of end
> offset. And your change is correctly takes that into account.
> Can you please update your patch on top
> https://patchwork.freedesktop.org/patch/755274?
>
> Also update fixes tag to include https://patchwork.freedesktop.org/patch/755274?
>
> Thanks,
> Brajesh
>
> > >             /*
> > >              * Alloc and push page table entries until we have enough of
> > > diff --git a/drivers/gpu/drm/imagination/pvr_mmu.h b/drivers/gpu/drm/imagination/pvr_mmu.h
> > > index a8ecd460168..7088f80805e 100644
> > > --- a/drivers/gpu/drm/imagination/pvr_mmu.h
> > > +++ b/drivers/gpu/drm/imagination/pvr_mmu.h
> > > @@ -99,7 +99,8 @@ dma_addr_t pvr_mmu_get_root_table_dma_addr(struct pvr_mmu_context *ctx);
> > >  void pvr_mmu_op_context_destroy(struct pvr_mmu_op_context *op_ctx);
> > >  struct pvr_mmu_op_context *
> > >  pvr_mmu_op_context_create(struct pvr_mmu_context *ctx,
> > > -                     struct sg_table *sgt, u64 sgt_offset, u64 size);
> > > +                     struct sg_table *sgt, u64 sgt_offset,
> > > +                     u64 device_addr, u64 size);
> > >
> > >  int pvr_mmu_map(struct pvr_mmu_op_context *op_ctx, u64 size, u64 flags,
> > >             u64 device_addr);
> > > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> > > index c5ae0b79fe6..7bb58ca14e4 100644
> > > --- a/drivers/gpu/drm/imagination/pvr_vm.c
> > > +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> > > @@ -276,7 +276,8 @@ pvr_vm_bind_op_map_init(struct pvr_vm_bind_op *bind_op,
> > >             goto err_bind_op_fini;
> > >
> > >     bind_op->mmu_op_ctx =
> > > -           pvr_mmu_op_context_create(vm_ctx->mmu_ctx, sgt, offset, size);
> > > +           pvr_mmu_op_context_create(vm_ctx->mmu_ctx, sgt, offset,
> > > +                                     device_addr, size);
> > >     err = PTR_ERR_OR_ZERO(bind_op->mmu_op_ctx);
> > >     if (err) {
> > >             bind_op->mmu_op_ctx = NULL;
> > > @@ -318,7 +319,7 @@ pvr_vm_bind_op_unmap_init(struct pvr_vm_bind_op *bind_op,
> > >     }
> > >
> > >     bind_op->mmu_op_ctx =
> > > -           pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0);
> > > +           pvr_mmu_op_context_create(vm_ctx->mmu_ctx, NULL, 0, 0, 0);
> > >     err = PTR_ERR_OR_ZERO(bind_op->mmu_op_ctx);
> > >     if (err) {
> > >             bind_op->mmu_op_ctx = NULL;
> > >
> >
>


-- 
Thanks,
Gyeyoung