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

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

[Adrián Larumbe]

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) {
+		u64 end = start + size;
+
+		start = min(start, vm->locked_region.start);
+		size = max(vm->locked_region.start +
+			   vm->locked_region.size, end) - start;
+	}
+
 	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

[PATCH 2/2] drm/panthor: Fix outdated function documentation

[Adrián Larumbe]

'vm' is no longer allowed to be NULL.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
index 69cef05b6ef7..13295d7a593d 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.c
+++ b/drivers/gpu/drm/panthor/panthor_gem.c
@@ -1264,7 +1264,7 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
 /**
  * panthor_kernel_bo_create() - Create and map a GEM object to a VM
  * @ptdev: Device.
- * @vm: VM to map the GEM to. If NULL, the kernel object is not GPU mapped.
+ * @vm: VM to map the GEM to.
  * @size: Size of the buffer object.
  * @bo_flags: Combination of drm_panthor_bo_flags flags.
  * @vm_map_flags: Combination of drm_panthor_vm_bind_op_flags (only those
-- 
2.53.0

Re: [PATCH 2/2] drm/panthor: Fix outdated function documentation

[Boris Brezillon]

On Fri,  3 Apr 2026 18:21:12 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> 'vm' is no longer allowed to be NULL.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panthor/panthor_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index 69cef05b6ef7..13295d7a593d 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -1264,7 +1264,7 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
>  /**
>   * panthor_kernel_bo_create() - Create and map a GEM object to a VM
>   * @ptdev: Device.
> - * @vm: VM to map the GEM to. If NULL, the kernel object is not GPU mapped.
> + * @vm: VM to map the GEM to.
>   * @size: Size of the buffer object.
>   * @bo_flags: Combination of drm_panthor_bo_flags flags.
>   * @vm_map_flags: Combination of drm_panthor_vm_bind_op_flags (only those

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

[Boris Brezillon]

On Fri,  3 Apr 2026 18:21:11 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> 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")

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

One suggestion below.

> ---
>  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) {
> +		u64 end = start + size;
> +
> +		start = min(start, vm->locked_region.start);
> +		size = max(vm->locked_region.start +
> +			   vm->locked_region.size, end) - start;

I'd probably go:

		u64 end = max(vm->locked_region.start + vm->locked_region.size,
			      start + size);

		start = min(start, vm->locked_region.start);
		size = end - start;

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

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

[Liviu Dudau]

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!  /
  ---------------
    ¯\_(ツ)_/¯

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

[Boris Brezillon]

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.

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

[Liviu Dudau]

On Tue, Apr 07, 2026 at 12:43:53PM +0200, Boris Brezillon wrote:
> 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()).

It makes sense to test the vm->locked_region.start being zero, not the vm->locked_region.size.

In your suggested update of the math, I would go:

	if (vm->locked_region.start)
		start = min(start, vm->locked_region.start);

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

Yeah, I was thinking from a defensive coding perspective where this function gets attacked.

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

The only caller that might be exposed is panthor_vm_evict_bo_mappings_locked() and it doesn't look like
it could benefit from having the range check. I get it that it is not an expected scenario, just wanted
to double check.

Best regards,
Liviu

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

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

[Boris Brezillon]

On Tue, 7 Apr 2026 12:07:27 +0100
Liviu Dudau <liviu.dudau@arm.com> wrote:

> On Tue, Apr 07, 2026 at 12:43:53PM +0200, Boris Brezillon wrote:
> > 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()).  
> 
> It makes sense to test the vm->locked_region.start being zero, not the vm->locked_region.size.
> 
> In your suggested update of the math, I would go:
> 
> 	if (vm->locked_region.start)
> 		start = min(start, vm->locked_region.start);

Well, you'd still need the vm->locked_region.size > 0 check for the
size update, because vm->locked_region.size > 0 and
vm->locked_region.start == 0 is allowed. In practice it won't
happen because we reserve the first 32M of the VA space in
mesa(panvk,gallium), but that's not enforced by the kernel, so I still
believe the check should be vm->locked_region.size > 0 rather than
vm->locked_region.start > 0.

> 
> >   
> > >   
> > > > +		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.  
> 
> Yeah, I was thinking from a defensive coding perspective where this function gets attacked.

Fair enough. Let's add a WARN_ON_ONCE() and a comment explaining why the
overlap between old and new locked region is expected.