Re: [PATCH 1/2] drm/panthor: Extend VM locked region for remap case to be a superset

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

On Tue, 7 Apr 2026 11:24:52 +0100
Liviu Dudau <liviu.dudau@arm.com> wrote:

> On Fri, Apr 03, 2026 at 06:21:11PM +0100, Adrián Larumbe wrote:
> > In the event of an sm_step_remap() that leads to a partial unmap of a
> > transparent huge page, the new locked region required by an extended unmap
> > might not be a superset of the original one. Then, if it leaves a portion
> > of the initially requested one out, the ensuing map will trigger a warning.
> > 
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > Fixes: 8e7460eac786 ("drm/panthor: Support partial unmaps of huge pages")
> > ---
> >  drivers/gpu/drm/panthor/panthor_mmu.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > index fa8b31df85c9..2b96359d3b94 100644
> > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > @@ -1709,6 +1709,19 @@ static int panthor_vm_lock_region(struct panthor_vm *vm, u64 start, u64 size)
> >  	    start + size <= vm->locked_region.start + vm->locked_region.size)
> >  		return 0;
> >  
> > +	/* sm_step_remap() may need a locked region that isn't a strict superset
> > +	 * of the original one because of having to extend unmap boundaries beyond
> > +	 * it to deal with partial unmaps of transparent huge pages. What we want
> > +	 * in those cases is to lock the union of both regions.
> > +	 */
> > +	if (vm->locked_region.size) {  
> 
> Why is this check needed? We're updating the vm->locked_region.size later anyway, and I think
> we can cope with a locked region being of zero size when we are called, unless we consider that
> to be a bug and we should check earlier for a zero value.

It's here to detect if this is the initial lock (==0), or the one
that's done in sm_step_remap() (!=0). If we drop this conditional, the
adjusted start will always be zero on the initial lock, because both
vm->locked_region.start and vm->locked_region.size are zero in that
case (see panthor_vm_unlock_region()).

> 
> > +		u64 end = start + size;  
> 
> Like Boris pointed out, the calculations can be optimized so that we don't need this line.
> 
> > +
> > +		start = min(start, vm->locked_region.start);
> > +		size = max(vm->locked_region.start +
> > +			   vm->locked_region.size, end) - start;  
> 
> If we have something like:
> 
>  ..... [start .. start+size] ...... [vm->locked_region.start .. vm->locked_region.start + vm->locked_region.size] ....

First off, that's not supposed to happen. The 3 cases that exist now are:

[start .. start+size]
		[vm->locked_region.start .. vm->locked_region.start + vm->locked_region.size]

or

									[start .. start+size]
[vm->locked_region.start .. vm->locked_region.start + vm->locked_region.size]

or

[start					..					 start+size]
	[vm->locked_region.start .. vm->locked_region.start + vm->locked_region.size]


> 
> we end up locking
> 
>  ..... [start ................................................. vm->locked_region.start + vm->locked_region.size] ....
> 
> is that intended?

We could add a WARN_ON() is there's no overlap between
the previously locked region and the new one, but I'm
not convinced this is something for panthor_vm_unlock_region() to
enforce. Looks more like something the caller should check.