Re: [PATCH v2 2/6] drm/panthor: Kill lock_region()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 17 Nov 2025 12:44:58 +0000
Steven Price <steven.price@arm.com> wrote:

> On 13/11/2025 10:39, Boris Brezillon wrote:
> > The meat in lock_region() is about packing a region range into a
> > single u64. The rest is just a regular reg write plus a
> > as_send_cmd_and_wait() call that can easily be inlined in
> > mmu_hw_do_operation_locked().
> > 
> > v2:
> > - New patch
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_mmu.c | 14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > index 186048fc2c25..f109c1588186 100644
> > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > @@ -538,11 +538,9 @@ static int as_send_cmd_and_wait(struct panthor_device *ptdev, u32 as_nr, u32 cmd
> >  	return status;
> >  }
> >  
> > -static int lock_region(struct panthor_device *ptdev, u32 as_nr,
> > -		       u64 region_start, u64 size)
> > +static u64 pack_region_range(u64 region_start, u64 size)
> >  {
> >  	u8 region_width;
> > -	u64 region;
> >  	u64 region_end = region_start + size;
> >  
> >  	if (!size)  
> Extra context:
> >		return 0;  
> Rather than skipping the lock for size==0 you are now performing a lock
> with LOCKADDR=0. The best documentation I can find for that says (for
> LOCKADDR_SIZE in the Midgard architecture): "Values in the range 0 to 10
> are undefined and should not be used".
> 
> I think later versions upped the minimum to 14 (log2(1<<15)-1) for
> reasons due to supporting larger page sizes.
> 
> While I suspect this might work (since we don't actually need a lock
> region when size==0) I don't think we should be relying on undefined
> behaviour.
> 
> I think the simple solution is to move the size==0 check up into the caller.

Right. I addressed that in patch 4 (which basically removes
mmu_hw_do_operation_locked()), but I can do it here too.

> 
> Thanks,
> Steve
> 
> > @@ -565,11 +563,7 @@ static int lock_region(struct panthor_device *ptdev, u32 as_nr,
> >  	 */
> >  	region_start &= GENMASK_ULL(63, region_width);
> >  
> > -	region = region_width | region_start;
> > -
> > -	/* Lock the region that needs to be updated */
> > -	gpu_write64(ptdev, AS_LOCKADDR(as_nr), region);
> > -	return as_send_cmd_and_wait(ptdev, as_nr, AS_COMMAND_LOCK);
> > +	return region_width | region_start;
> >  }
> >  
> >  static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
> > @@ -602,7 +596,9 @@ static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
> >  	 * power it up
> >  	 */
> >  
> > -	ret = lock_region(ptdev, as_nr, iova, size);
> > +	/* Lock the region that needs to be updated */
> > +	gpu_write64(ptdev, AS_LOCKADDR(as_nr), pack_region_range(iova, size));
> > +	ret = as_send_cmd_and_wait(ptdev, as_nr, AS_COMMAND_LOCK);
> >  	if (ret)
> >  		return ret;
> >    
>