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

[Brajesh Gupta <Brajesh.Gupta@imgtec.com>]

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