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

[Liviu Dudau <liviu.dudau@arm.com>]

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.

> +		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] ....

we end up locking

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

is that intended?

Best regards,
Liviu


> +	}
> +
>  	mutex_lock(&ptdev->mmu->as.slots_lock);
>  	if (vm->as.id >= 0 && size) {
>  		/* Lock the region that needs to be updated */
> 
> base-commit: fb42964e2a76f68a5fb581d382b5d2be2d5bda9b
> -- 
> 2.53.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯