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@xxxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > >  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