[PATCH v1 0/4] drm/panthor: Misc fixes

[PATCH v1 0/4] drm/panthor: Misc fixes

[Boris Brezillon]

Hello,

This is a set of fixes for bugs I ran into while looking at [1].
Hopefully that's enough to recover from AS_ACTIVE bit stuck
situations, but it'd be good to understand why the MMU block is
completely blocked in some cases and try to come up with better
mitigations than a full GPU reset.

Regards,

Boris

[1]https://gitlab.freedesktop.org/panfrost/linux/-/issues/57

Boris Brezillon (4):
  drm/panthor: Fix UAF on kernel BO VA nodes
  drm/panthor: Add support for atomic page table updates
  drm/panthor: Make panthor_vm_[un]map_pages() more robust
  drm/panthor: Relax check in panthor_sched_pre_reset()

 drivers/gpu/drm/panthor/panthor_gem.c   |  14 +-
 drivers/gpu/drm/panthor/panthor_mmu.c   | 250 +++++++++++++-----------
 drivers/gpu/drm/panthor/panthor_sched.c |   2 -
 3 files changed, 141 insertions(+), 125 deletions(-)

-- 
2.51.0

[PATCH v1 1/4] drm/panthor: Fix UAF on kernel BO VA nodes

[Boris Brezillon]

If the MMU is down, panthor_vm_unmap_range() might return an error.
We expect the page table to be updated still, and if the MMU is blocked,
the rest of the GPU should be blocked too, so no risk of accessing
physical memory returned to the system (which the current code doesn't
cover for anyway).

Proceed with the rest of the cleanup instead of bailing out and leaving
the va_node inserted in the drm_mm, which leads to UAF when other
adjacent nodes are removed from the drm_mm tree.

Reported-by: Lars-Ivar Hesselberg Simonsen <lars-ivar.simonsen@arm.com>
Closes: https://gitlab.freedesktop.org/panfrost/linux/-/issues/57
Fixes: 8a1cc07578bf ("drm/panthor: Add GEM logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_gem.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
index 7e7d2f223cfa..f369cc3e2a5f 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.c
+++ b/drivers/gpu/drm/panthor/panthor_gem.c
@@ -87,7 +87,6 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
 void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
 {
 	struct panthor_vm *vm;
-	int ret;
 
 	if (IS_ERR_OR_NULL(bo))
 		return;
@@ -95,18 +94,11 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
 	vm = bo->vm;
 	panthor_kernel_bo_vunmap(bo);
 
-	if (drm_WARN_ON(bo->obj->dev,
-			to_panthor_bo(bo->obj)->exclusive_vm_root_gem != panthor_vm_root_gem(vm)))
-		goto out_free_bo;
-
-	ret = panthor_vm_unmap_range(vm, bo->va_node.start, bo->va_node.size);
-	if (ret)
-		goto out_free_bo;
-
+	drm_WARN_ON(bo->obj->dev,
+		    to_panthor_bo(bo->obj)->exclusive_vm_root_gem != panthor_vm_root_gem(vm));
+	panthor_vm_unmap_range(vm, bo->va_node.start, bo->va_node.size);
 	panthor_vm_free_va(vm, &bo->va_node);
 	drm_gem_object_put(bo->obj);
-
-out_free_bo:
 	panthor_vm_put(vm);
 	kfree(bo);
 }
-- 
2.51.0

Re: [PATCH v1 1/4] drm/panthor: Fix UAF on kernel BO VA nodes

[Liviu Dudau]

On Fri, Oct 31, 2025 at 04:48:15PM +0100, Boris Brezillon wrote:
> If the MMU is down, panthor_vm_unmap_range() might return an error.
> We expect the page table to be updated still, and if the MMU is blocked,
> the rest of the GPU should be blocked too, so no risk of accessing
> physical memory returned to the system (which the current code doesn't
> cover for anyway).
> 
> Proceed with the rest of the cleanup instead of bailing out and leaving
> the va_node inserted in the drm_mm, which leads to UAF when other
> adjacent nodes are removed from the drm_mm tree.
> 
> Reported-by: Lars-Ivar Hesselberg Simonsen <lars-ivar.simonsen@arm.com>
> Closes: https://gitlab.freedesktop.org/panfrost/linux/-/issues/57
> Fixes: 8a1cc07578bf ("drm/panthor: Add GEM logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_gem.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index 7e7d2f223cfa..f369cc3e2a5f 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -87,7 +87,6 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
>  void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
>  {
>  	struct panthor_vm *vm;
> -	int ret;
>  
>  	if (IS_ERR_OR_NULL(bo))
>  		return;
> @@ -95,18 +94,11 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
>  	vm = bo->vm;
>  	panthor_kernel_bo_vunmap(bo);
>  
> -	if (drm_WARN_ON(bo->obj->dev,
> -			to_panthor_bo(bo->obj)->exclusive_vm_root_gem != panthor_vm_root_gem(vm)))
> -		goto out_free_bo;
> -
> -	ret = panthor_vm_unmap_range(vm, bo->va_node.start, bo->va_node.size);
> -	if (ret)
> -		goto out_free_bo;
> -
> +	drm_WARN_ON(bo->obj->dev,
> +		    to_panthor_bo(bo->obj)->exclusive_vm_root_gem != panthor_vm_root_gem(vm));
> +	panthor_vm_unmap_range(vm, bo->va_node.start, bo->va_node.size);
>  	panthor_vm_free_va(vm, &bo->va_node);
>  	drm_gem_object_put(bo->obj);
> -
> -out_free_bo:
>  	panthor_vm_put(vm);
>  	kfree(bo);
>  }
> -- 
> 2.51.0
> 

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

Re: [PATCH v1 1/4] drm/panthor: Fix UAF on kernel BO VA nodes

[Liviu Dudau]

On Fri, Oct 31, 2025 at 04:48:15PM +0100, Boris Brezillon wrote:
> If the MMU is down, panthor_vm_unmap_range() might return an error.
> We expect the page table to be updated still, and if the MMU is blocked,
> the rest of the GPU should be blocked too, so no risk of accessing
> physical memory returned to the system (which the current code doesn't
> cover for anyway).
> 
> Proceed with the rest of the cleanup instead of bailing out and leaving
> the va_node inserted in the drm_mm, which leads to UAF when other
> adjacent nodes are removed from the drm_mm tree.
> 
> Reported-by: Lars-Ivar Hesselberg Simonsen <lars-ivar.simonsen@arm.com>
> Closes: https://gitlab.freedesktop.org/panfrost/linux/-/issues/57
> Fixes: 8a1cc07578bf ("drm/panthor: Add GEM logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

I've pushed this patch into drm-misc-next, I'll leave the rest of the series
for review.

Best regards,
Liviu

> ---
>  drivers/gpu/drm/panthor/panthor_gem.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index 7e7d2f223cfa..f369cc3e2a5f 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -87,7 +87,6 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
>  void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
>  {
>  	struct panthor_vm *vm;
> -	int ret;
>  
>  	if (IS_ERR_OR_NULL(bo))
>  		return;
> @@ -95,18 +94,11 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
>  	vm = bo->vm;
>  	panthor_kernel_bo_vunmap(bo);
>  
> -	if (drm_WARN_ON(bo->obj->dev,
> -			to_panthor_bo(bo->obj)->exclusive_vm_root_gem != panthor_vm_root_gem(vm)))
> -		goto out_free_bo;
> -
> -	ret = panthor_vm_unmap_range(vm, bo->va_node.start, bo->va_node.size);
> -	if (ret)
> -		goto out_free_bo;
> -
> +	drm_WARN_ON(bo->obj->dev,
> +		    to_panthor_bo(bo->obj)->exclusive_vm_root_gem != panthor_vm_root_gem(vm));
> +	panthor_vm_unmap_range(vm, bo->va_node.start, bo->va_node.size);
>  	panthor_vm_free_va(vm, &bo->va_node);
>  	drm_gem_object_put(bo->obj);
> -
> -out_free_bo:
>  	panthor_vm_put(vm);
>  	kfree(bo);
>  }
> -- 
> 2.51.0
> 

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

[PATCH v1 2/4] drm/panthor: Add support for atomic page table updates

[Boris Brezillon]

Move the lock/flush_mem operations around the gpuvm_sm_[un]map() calls
so we can implement true atomic page updates, where any access in the
locked range done by the GPU has to wait for the page table updates
to land before proceeding.

This is needed for vkQueueBindSparse(), so we can replace the dummy
page mapped over the entire object by actual BO backed pages in an atomic
way. But it's also useful to avoid "AS_ACTIVE bit stuck" failures in
the sm_[un]map() path, leading to gpuvm state inconsistencies.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 182 +++++++++++++-------------
 1 file changed, 90 insertions(+), 92 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 58fead90533a..ea886c8ac97f 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -389,6 +389,15 @@ struct panthor_vm {
 	 * flagged as faulty as a result.
 	 */
 	bool unhandled_fault;
+
+	/** @locked_region: Information about the currently locked region currently. */
+	struct {
+		/** @locked_region.start: Start of the locked region. */
+		u64 start;
+
+		/** @locked_region.size: Size of the locked region. */
+		u64 size;
+	} locked_region;
 };
 
 /**
@@ -570,79 +579,9 @@ static void lock_region(struct panthor_device *ptdev, u32 as_nr,
 	write_cmd(ptdev, as_nr, AS_COMMAND_LOCK);
 }
 
-static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
-				      u64 iova, u64 size, u32 op)
-{
-	const u32 l2_flush_op = CACHE_CLEAN | CACHE_INV;
-	u32 lsc_flush_op;
-	int ret;
-
-	lockdep_assert_held(&ptdev->mmu->as.slots_lock);
-
-	switch (op) {
-	case AS_COMMAND_FLUSH_MEM:
-		lsc_flush_op = CACHE_CLEAN | CACHE_INV;
-		break;
-	case AS_COMMAND_FLUSH_PT:
-		lsc_flush_op = 0;
-		break;
-	default:
-		drm_WARN(&ptdev->base, 1, "Unexpected AS_COMMAND: %d", op);
-		return -EINVAL;
-	}
-
-	if (as_nr < 0)
-		return 0;
-
-	/*
-	 * If the AS number is greater than zero, then we can be sure
-	 * the device is up and running, so we don't need to explicitly
-	 * power it up
-	 */
-
-	lock_region(ptdev, as_nr, iova, size);
-
-	ret = wait_ready(ptdev, as_nr);
-	if (ret)
-		return ret;
-
-	ret = panthor_gpu_flush_caches(ptdev, l2_flush_op, lsc_flush_op, 0);
-	if (ret)
-		return ret;
-
-	/*
-	 * Explicitly unlock the region as the AS is not unlocked automatically
-	 * at the end of the GPU_CONTROL cache flush command, unlike
-	 * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
-	 */
-	write_cmd(ptdev, as_nr, AS_COMMAND_UNLOCK);
-
-	/* Wait for the unlock command to complete */
-	return wait_ready(ptdev, as_nr);
-}
-
-static int mmu_hw_do_operation(struct panthor_vm *vm,
-			       u64 iova, u64 size, u32 op)
-{
-	struct panthor_device *ptdev = vm->ptdev;
-	int ret;
-
-	mutex_lock(&ptdev->mmu->as.slots_lock);
-	ret = mmu_hw_do_operation_locked(ptdev, vm->as.id, iova, size, op);
-	mutex_unlock(&ptdev->mmu->as.slots_lock);
-
-	return ret;
-}
-
 static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
 				 u64 transtab, u64 transcfg, u64 memattr)
 {
-	int ret;
-
-	ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
-	if (ret)
-		return ret;
-
 	gpu_write64(ptdev, AS_TRANSTAB(as_nr), transtab);
 	gpu_write64(ptdev, AS_MEMATTR(as_nr), memattr);
 	gpu_write64(ptdev, AS_TRANSCFG(as_nr), transcfg);
@@ -654,7 +593,9 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
 {
 	int ret;
 
-	ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
+	/* Flush and invalidate all caches. */
+	write_cmd(ptdev, as_nr, AS_COMMAND_FLUSH_MEM);
+	ret = wait_ready(ptdev, as_nr);
 	if (ret)
 		return ret;
 
@@ -736,6 +677,10 @@ int panthor_vm_active(struct panthor_vm *vm)
 	if (refcount_inc_not_zero(&vm->as.active_cnt))
 		goto out_dev_exit;
 
+	/* Make sure we don't race with lock/unlock_region() calls
+	 * happening around VM bind operations.
+	 */
+	mutex_lock(&vm->op_lock);
 	mutex_lock(&ptdev->mmu->as.slots_lock);
 
 	if (refcount_inc_not_zero(&vm->as.active_cnt))
@@ -803,6 +748,10 @@ int panthor_vm_active(struct panthor_vm *vm)
 		gpu_write(ptdev, MMU_INT_MASK, ~ptdev->mmu->as.faulty_mask);
 	}
 
+	/* The VM update is guarded by ::op_lock, which we take at the beginning
+	 * of this function, so we don't expect any locked region here.
+	 */
+	drm_WARN_ON(&vm->ptdev->base, vm->locked_region.size > 0);
 	ret = panthor_mmu_as_enable(vm->ptdev, vm->as.id, transtab, transcfg, vm->memattr);
 
 out_make_active:
@@ -813,6 +762,7 @@ int panthor_vm_active(struct panthor_vm *vm)
 
 out_unlock:
 	mutex_unlock(&ptdev->mmu->as.slots_lock);
+	mutex_unlock(&vm->op_lock);
 
 out_dev_exit:
 	drm_dev_exit(cookie);
@@ -896,30 +846,15 @@ static size_t get_pgsize(u64 addr, size_t size, size_t *count)
 	return SZ_2M;
 }
 
-static int panthor_vm_flush_range(struct panthor_vm *vm, u64 iova, u64 size)
-{
-	struct panthor_device *ptdev = vm->ptdev;
-	int ret = 0, cookie;
-
-	if (vm->as.id < 0)
-		return 0;
-
-	/* If the device is unplugged, we just silently skip the flush. */
-	if (!drm_dev_enter(&ptdev->base, &cookie))
-		return 0;
-
-	ret = mmu_hw_do_operation(vm, iova, size, AS_COMMAND_FLUSH_PT);
-
-	drm_dev_exit(cookie);
-	return ret;
-}
-
 static int panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)
 {
 	struct panthor_device *ptdev = vm->ptdev;
 	struct io_pgtable_ops *ops = vm->pgtbl_ops;
 	u64 offset = 0;
 
+	drm_WARN_ON(&ptdev->base,
+		    (iova < vm->locked_region.start) ||
+		    (iova + size > vm->locked_region.start + vm->locked_region.size));
 	drm_dbg(&ptdev->base, "unmap: as=%d, iova=%llx, len=%llx", vm->as.id, iova, size);
 
 	while (offset < size) {
@@ -933,13 +868,12 @@ static int panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)
 				iova + offset + unmapped_sz,
 				iova + offset + pgsize * pgcount,
 				iova, iova + size);
-			panthor_vm_flush_range(vm, iova, offset + unmapped_sz);
 			return  -EINVAL;
 		}
 		offset += unmapped_sz;
 	}
 
-	return panthor_vm_flush_range(vm, iova, size);
+	return 0;
 }
 
 static int
@@ -956,6 +890,10 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot,
 	if (!size)
 		return 0;
 
+	drm_WARN_ON(&ptdev->base,
+		    (iova < vm->locked_region.start) ||
+		    (iova + size > vm->locked_region.start + vm->locked_region.size));
+
 	for_each_sgtable_dma_sg(sgt, sgl, count) {
 		dma_addr_t paddr = sg_dma_address(sgl);
 		size_t len = sg_dma_len(sgl);
@@ -1003,7 +941,7 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot,
 		offset = 0;
 	}
 
-	return panthor_vm_flush_range(vm, start_iova, iova - start_iova);
+	return 0;
 }
 
 static int flags_to_prot(u32 flags)
@@ -1672,6 +1610,56 @@ static const char *access_type_name(struct panthor_device *ptdev,
 	}
 }
 
+static int panthor_vm_lock_region(struct panthor_vm *vm, u64 start, u64 size)
+{
+	struct panthor_device *ptdev = vm->ptdev;
+	int ret = 0;
+
+	mutex_lock(&ptdev->mmu->as.slots_lock);
+	drm_WARN_ON(&ptdev->base, vm->locked_region.start || vm->locked_region.size);
+	vm->locked_region.start = start;
+	vm->locked_region.size = size;
+	if (vm->as.id >= 0) {
+		lock_region(ptdev, vm->as.id, start, size);
+
+		/* If the lock failed, reset the locked region. */
+		ret = wait_ready(ptdev, vm->as.id);
+		if (ret) {
+			vm->locked_region.start = 0;
+			vm->locked_region.size = 0;
+		}
+	}
+	mutex_unlock(&ptdev->mmu->as.slots_lock);
+
+	return ret;
+}
+
+static void panthor_vm_unlock_region(struct panthor_vm *vm)
+{
+	struct panthor_device *ptdev = vm->ptdev;
+
+	mutex_lock(&ptdev->mmu->as.slots_lock);
+	if (vm->as.id >= 0) {
+		int ret;
+
+		/* FLUSH_MEM will flush all cache entries covering
+		 * the locked region and unlock it.
+		 */
+		ret = write_cmd(ptdev, vm->as.id, AS_COMMAND_FLUSH_MEM);
+		if (!ret)
+			ret = wait_ready(ptdev, vm->as.id);
+
+		/* If we fail to unlock the region, schedule a GPU reset
+		 * to unblock the situation.
+		 */
+		if (drm_WARN_ON(&ptdev->base, ret))
+			panthor_device_schedule_reset(ptdev);
+	}
+	vm->locked_region.start = 0;
+	vm->locked_region.size = 0;
+	mutex_unlock(&ptdev->mmu->as.slots_lock);
+}
+
 static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
 {
 	bool has_unhandled_faults = false;
@@ -1876,6 +1864,7 @@ static void panthor_vm_free(struct drm_gpuvm *gpuvm)
 	drm_sched_entity_destroy(&vm->entity);
 	drm_sched_fini(&vm->sched);
 
+	mutex_lock(&vm->op_lock);
 	mutex_lock(&ptdev->mmu->as.slots_lock);
 	if (vm->as.id >= 0) {
 		int cookie;
@@ -1890,6 +1879,7 @@ static void panthor_vm_free(struct drm_gpuvm *gpuvm)
 		list_del(&vm->as.lru_node);
 	}
 	mutex_unlock(&ptdev->mmu->as.slots_lock);
+	mutex_unlock(&vm->op_lock);
 
 	free_io_pgtable_ops(vm->pgtbl_ops);
 
@@ -2197,6 +2187,11 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
 
 	mutex_lock(&vm->op_lock);
 	vm->op_ctx = op;
+
+	ret = panthor_vm_lock_region(vm, op->va.addr, op->va.range);
+	if (ret)
+		goto out;
+
 	switch (op_type) {
 	case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: {
 		const struct drm_gpuvm_map_req map_req = {
@@ -2224,6 +2219,9 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
 		break;
 	}
 
+	panthor_vm_unlock_region(vm);
+
+out:
 	if (ret && flag_vm_unusable_on_failure)
 		vm->unusable = true;
 
-- 
2.51.0

Re: [PATCH v1 2/4] drm/panthor: Add support for atomic page table updates

[Steven Price]

On 31/10/2025 15:48, Boris Brezillon wrote:
> Move the lock/flush_mem operations around the gpuvm_sm_[un]map() calls
> so we can implement true atomic page updates, where any access in the
> locked range done by the GPU has to wait for the page table updates
> to land before proceeding.
> 
> This is needed for vkQueueBindSparse(), so we can replace the dummy
> page mapped over the entire object by actual BO backed pages in an atomic
> way. But it's also useful to avoid "AS_ACTIVE bit stuck" failures in
> the sm_[un]map() path, leading to gpuvm state inconsistencies.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

+CC: Karunika

The concept here looks good, but...

This is effectively undoing the changes that Karunika did in commit
dd11c7dec74b ("drm/panthor: Make MMU cache maintenance use FLUSH_CACHES
command"). The FLUSH_MEM/FLUSH_PT commands have been deprecated in
Mali-Gx20, so either this needs refactoring to use the newer
FLUSH_CACHES GPU_COMMAND.

I believe the expected ideal approach on later GPUs is:

1. LOCK
2. Make page table updates
3. FLUSH_RANGE to GPU_COMMAND flushing the *physical* address ranges
4. UNLOCK

Note that on earlier GPUs we don't have FLUSH_RANGE so FLUSH_CACHES
replaces step 3 (and is what we have upstream). But I think this is the
reason why FLUSH_MEM/FLUSH_PT are deprecated because those work on a
virtual address range.

Thanks,
Steve

> ---
>  drivers/gpu/drm/panthor/panthor_mmu.c | 182 +++++++++++++-------------
>  1 file changed, 90 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 58fead90533a..ea886c8ac97f 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -389,6 +389,15 @@ struct panthor_vm {
>  	 * flagged as faulty as a result.
>  	 */
>  	bool unhandled_fault;
> +
> +	/** @locked_region: Information about the currently locked region currently. */
> +	struct {
> +		/** @locked_region.start: Start of the locked region. */
> +		u64 start;
> +
> +		/** @locked_region.size: Size of the locked region. */
> +		u64 size;
> +	} locked_region;
>  };
>  
>  /**
> @@ -570,79 +579,9 @@ static void lock_region(struct panthor_device *ptdev, u32 as_nr,
>  	write_cmd(ptdev, as_nr, AS_COMMAND_LOCK);
>  }
>  
> -static int mmu_hw_do_operation_locked(struct panthor_device *ptdev, int as_nr,
> -				      u64 iova, u64 size, u32 op)
> -{
> -	const u32 l2_flush_op = CACHE_CLEAN | CACHE_INV;
> -	u32 lsc_flush_op;
> -	int ret;
> -
> -	lockdep_assert_held(&ptdev->mmu->as.slots_lock);
> -
> -	switch (op) {
> -	case AS_COMMAND_FLUSH_MEM:
> -		lsc_flush_op = CACHE_CLEAN | CACHE_INV;
> -		break;
> -	case AS_COMMAND_FLUSH_PT:
> -		lsc_flush_op = 0;
> -		break;
> -	default:
> -		drm_WARN(&ptdev->base, 1, "Unexpected AS_COMMAND: %d", op);
> -		return -EINVAL;
> -	}
> -
> -	if (as_nr < 0)
> -		return 0;
> -
> -	/*
> -	 * If the AS number is greater than zero, then we can be sure
> -	 * the device is up and running, so we don't need to explicitly
> -	 * power it up
> -	 */
> -
> -	lock_region(ptdev, as_nr, iova, size);
> -
> -	ret = wait_ready(ptdev, as_nr);
> -	if (ret)
> -		return ret;
> -
> -	ret = panthor_gpu_flush_caches(ptdev, l2_flush_op, lsc_flush_op, 0);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * Explicitly unlock the region as the AS is not unlocked automatically
> -	 * at the end of the GPU_CONTROL cache flush command, unlike
> -	 * AS_COMMAND_FLUSH_MEM or AS_COMMAND_FLUSH_PT.
> -	 */
> -	write_cmd(ptdev, as_nr, AS_COMMAND_UNLOCK);
> -
> -	/* Wait for the unlock command to complete */
> -	return wait_ready(ptdev, as_nr);
> -}
> -
> -static int mmu_hw_do_operation(struct panthor_vm *vm,
> -			       u64 iova, u64 size, u32 op)
> -{
> -	struct panthor_device *ptdev = vm->ptdev;
> -	int ret;
> -
> -	mutex_lock(&ptdev->mmu->as.slots_lock);
> -	ret = mmu_hw_do_operation_locked(ptdev, vm->as.id, iova, size, op);
> -	mutex_unlock(&ptdev->mmu->as.slots_lock);
> -
> -	return ret;
> -}
> -
>  static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
>  				 u64 transtab, u64 transcfg, u64 memattr)
>  {
> -	int ret;
> -
> -	ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
> -	if (ret)
> -		return ret;
> -
>  	gpu_write64(ptdev, AS_TRANSTAB(as_nr), transtab);
>  	gpu_write64(ptdev, AS_MEMATTR(as_nr), memattr);
>  	gpu_write64(ptdev, AS_TRANSCFG(as_nr), transcfg);
> @@ -654,7 +593,9 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
>  {
>  	int ret;
>  
> -	ret = mmu_hw_do_operation_locked(ptdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
> +	/* Flush and invalidate all caches. */
> +	write_cmd(ptdev, as_nr, AS_COMMAND_FLUSH_MEM);
> +	ret = wait_ready(ptdev, as_nr);
>  	if (ret)
>  		return ret;
>  
> @@ -736,6 +677,10 @@ int panthor_vm_active(struct panthor_vm *vm)
>  	if (refcount_inc_not_zero(&vm->as.active_cnt))
>  		goto out_dev_exit;
>  
> +	/* Make sure we don't race with lock/unlock_region() calls
> +	 * happening around VM bind operations.
> +	 */
> +	mutex_lock(&vm->op_lock);
>  	mutex_lock(&ptdev->mmu->as.slots_lock);
>  
>  	if (refcount_inc_not_zero(&vm->as.active_cnt))
> @@ -803,6 +748,10 @@ int panthor_vm_active(struct panthor_vm *vm)
>  		gpu_write(ptdev, MMU_INT_MASK, ~ptdev->mmu->as.faulty_mask);
>  	}
>  
> +	/* The VM update is guarded by ::op_lock, which we take at the beginning
> +	 * of this function, so we don't expect any locked region here.
> +	 */
> +	drm_WARN_ON(&vm->ptdev->base, vm->locked_region.size > 0);
>  	ret = panthor_mmu_as_enable(vm->ptdev, vm->as.id, transtab, transcfg, vm->memattr);
>  
>  out_make_active:
> @@ -813,6 +762,7 @@ int panthor_vm_active(struct panthor_vm *vm)
>  
>  out_unlock:
>  	mutex_unlock(&ptdev->mmu->as.slots_lock);
> +	mutex_unlock(&vm->op_lock);
>  
>  out_dev_exit:
>  	drm_dev_exit(cookie);
> @@ -896,30 +846,15 @@ static size_t get_pgsize(u64 addr, size_t size, size_t *count)
>  	return SZ_2M;
>  }
>  
> -static int panthor_vm_flush_range(struct panthor_vm *vm, u64 iova, u64 size)
> -{
> -	struct panthor_device *ptdev = vm->ptdev;
> -	int ret = 0, cookie;
> -
> -	if (vm->as.id < 0)
> -		return 0;
> -
> -	/* If the device is unplugged, we just silently skip the flush. */
> -	if (!drm_dev_enter(&ptdev->base, &cookie))
> -		return 0;
> -
> -	ret = mmu_hw_do_operation(vm, iova, size, AS_COMMAND_FLUSH_PT);
> -
> -	drm_dev_exit(cookie);
> -	return ret;
> -}
> -
>  static int panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)
>  {
>  	struct panthor_device *ptdev = vm->ptdev;
>  	struct io_pgtable_ops *ops = vm->pgtbl_ops;
>  	u64 offset = 0;
>  
> +	drm_WARN_ON(&ptdev->base,
> +		    (iova < vm->locked_region.start) ||
> +		    (iova + size > vm->locked_region.start + vm->locked_region.size));
>  	drm_dbg(&ptdev->base, "unmap: as=%d, iova=%llx, len=%llx", vm->as.id, iova, size);
>  
>  	while (offset < size) {
> @@ -933,13 +868,12 @@ static int panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)
>  				iova + offset + unmapped_sz,
>  				iova + offset + pgsize * pgcount,
>  				iova, iova + size);
> -			panthor_vm_flush_range(vm, iova, offset + unmapped_sz);
>  			return  -EINVAL;
>  		}
>  		offset += unmapped_sz;
>  	}
>  
> -	return panthor_vm_flush_range(vm, iova, size);
> +	return 0;
>  }
>  
>  static int
> @@ -956,6 +890,10 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot,
>  	if (!size)
>  		return 0;
>  
> +	drm_WARN_ON(&ptdev->base,
> +		    (iova < vm->locked_region.start) ||
> +		    (iova + size > vm->locked_region.start + vm->locked_region.size));
> +
>  	for_each_sgtable_dma_sg(sgt, sgl, count) {
>  		dma_addr_t paddr = sg_dma_address(sgl);
>  		size_t len = sg_dma_len(sgl);
> @@ -1003,7 +941,7 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot,
>  		offset = 0;
>  	}
>  
> -	return panthor_vm_flush_range(vm, start_iova, iova - start_iova);
> +	return 0;
>  }
>  
>  static int flags_to_prot(u32 flags)
> @@ -1672,6 +1610,56 @@ static const char *access_type_name(struct panthor_device *ptdev,
>  	}
>  }
>  
> +static int panthor_vm_lock_region(struct panthor_vm *vm, u64 start, u64 size)
> +{
> +	struct panthor_device *ptdev = vm->ptdev;
> +	int ret = 0;
> +
> +	mutex_lock(&ptdev->mmu->as.slots_lock);
> +	drm_WARN_ON(&ptdev->base, vm->locked_region.start || vm->locked_region.size);
> +	vm->locked_region.start = start;
> +	vm->locked_region.size = size;
> +	if (vm->as.id >= 0) {
> +		lock_region(ptdev, vm->as.id, start, size);
> +
> +		/* If the lock failed, reset the locked region. */
> +		ret = wait_ready(ptdev, vm->as.id);
> +		if (ret) {
> +			vm->locked_region.start = 0;
> +			vm->locked_region.size = 0;
> +		}
> +	}
> +	mutex_unlock(&ptdev->mmu->as.slots_lock);
> +
> +	return ret;
> +}
> +
> +static void panthor_vm_unlock_region(struct panthor_vm *vm)
> +{
> +	struct panthor_device *ptdev = vm->ptdev;
> +
> +	mutex_lock(&ptdev->mmu->as.slots_lock);
> +	if (vm->as.id >= 0) {
> +		int ret;
> +
> +		/* FLUSH_MEM will flush all cache entries covering
> +		 * the locked region and unlock it.
> +		 */
> +		ret = write_cmd(ptdev, vm->as.id, AS_COMMAND_FLUSH_MEM);
> +		if (!ret)
> +			ret = wait_ready(ptdev, vm->as.id);
> +
> +		/* If we fail to unlock the region, schedule a GPU reset
> +		 * to unblock the situation.
> +		 */
> +		if (drm_WARN_ON(&ptdev->base, ret))
> +			panthor_device_schedule_reset(ptdev);
> +	}
> +	vm->locked_region.start = 0;
> +	vm->locked_region.size = 0;
> +	mutex_unlock(&ptdev->mmu->as.slots_lock);
> +}
> +
>  static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
>  {
>  	bool has_unhandled_faults = false;
> @@ -1876,6 +1864,7 @@ static void panthor_vm_free(struct drm_gpuvm *gpuvm)
>  	drm_sched_entity_destroy(&vm->entity);
>  	drm_sched_fini(&vm->sched);
>  
> +	mutex_lock(&vm->op_lock);
>  	mutex_lock(&ptdev->mmu->as.slots_lock);
>  	if (vm->as.id >= 0) {
>  		int cookie;
> @@ -1890,6 +1879,7 @@ static void panthor_vm_free(struct drm_gpuvm *gpuvm)
>  		list_del(&vm->as.lru_node);
>  	}
>  	mutex_unlock(&ptdev->mmu->as.slots_lock);
> +	mutex_unlock(&vm->op_lock);
>  
>  	free_io_pgtable_ops(vm->pgtbl_ops);
>  
> @@ -2197,6 +2187,11 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
>  
>  	mutex_lock(&vm->op_lock);
>  	vm->op_ctx = op;
> +
> +	ret = panthor_vm_lock_region(vm, op->va.addr, op->va.range);
> +	if (ret)
> +		goto out;
> +
>  	switch (op_type) {
>  	case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: {
>  		const struct drm_gpuvm_map_req map_req = {
> @@ -2224,6 +2219,9 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
>  		break;
>  	}
>  
> +	panthor_vm_unlock_region(vm);
> +
> +out:
>  	if (ret && flag_vm_unusable_on_failure)
>  		vm->unusable = true;
>  

Re: [PATCH v1 2/4] drm/panthor: Add support for atomic page table updates

[Boris Brezillon]

On Fri, 7 Nov 2025 16:26:54 +0000
Steven Price <steven.price@arm.com> wrote:

> On 31/10/2025 15:48, Boris Brezillon wrote:
> > Move the lock/flush_mem operations around the gpuvm_sm_[un]map() calls
> > so we can implement true atomic page updates, where any access in the
> > locked range done by the GPU has to wait for the page table updates
> > to land before proceeding.
> > 
> > This is needed for vkQueueBindSparse(), so we can replace the dummy
> > page mapped over the entire object by actual BO backed pages in an atomic
> > way. But it's also useful to avoid "AS_ACTIVE bit stuck" failures in
> > the sm_[un]map() path, leading to gpuvm state inconsistencies.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> +CC: Karunika
> 
> The concept here looks good, but...
> 
> This is effectively undoing the changes that Karunika did in commit
> dd11c7dec74b ("drm/panthor: Make MMU cache maintenance use FLUSH_CACHES
> command"). The FLUSH_MEM/FLUSH_PT commands have been deprecated in
> Mali-Gx20, so either this needs refactoring to use the newer
> FLUSH_CACHES GPU_COMMAND.
> 
> I believe the expected ideal approach on later GPUs is:
> 
> 1. LOCK
> 2. Make page table updates
> 3. FLUSH_RANGE to GPU_COMMAND flushing the *physical* address ranges
> 4. UNLOCK
> 
> Note that on earlier GPUs we don't have FLUSH_RANGE so FLUSH_CACHES
> replaces step 3 (and is what we have upstream). But I think this is the
> reason why FLUSH_MEM/FLUSH_PT are deprecated because those work on a
> virtual address range.

Ah, right. I'll revisit the logic to have it working on newer gens.

[PATCH v1 3/4] drm/panthor: Make panthor_vm_[un]map_pages() more robust

[Boris Brezillon]

There's no reason for panthor_vm_[un]map_pages() to fail unless the
drm_gpuvm state and the page table are out of sync, so let's reflect that
by making panthor_vm_unmap_pages() a void function and adding
WARN_ON()s in various places. We also try to recover from those
unexpected mismatch by checking for already unmapped ranges and skipping
them. But there's only so much we can do to try and cope with such
SW bugs, so when we see a mismatch, we flag the VM unusable and disable
the AS to avoid further GPU accesses to the memory.

It could be that the as_disable() call fails because the MMU unit is
stuck, in which case the whole GPU is frozen, and only a GPU reset can
unblock things. Ater the reset, the VM will be seen as unusable and
any attempt to re-use it will fail, so we should be covered for any
use-after-unmap issues.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 82 ++++++++++++++++++---------
 1 file changed, 55 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index ea886c8ac97f..a4f3ed04b5cc 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -846,12 +846,32 @@ static size_t get_pgsize(u64 addr, size_t size, size_t *count)
 	return SZ_2M;
 }
 
-static int panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)
+static void panthor_vm_declare_unusable(struct panthor_vm *vm)
+{
+	struct panthor_device *ptdev = vm->ptdev;
+	int cookie;
+
+	if (vm->unusable)
+		return;
+
+	vm->unusable = true;
+	mutex_unlock(&ptdev->mmu->as.slots_lock);
+	if (vm->as.id >= 0 && drm_dev_enter(&ptdev->base, &cookie)) {
+		panthor_mmu_as_disable(ptdev, vm->as.id);
+		drm_dev_exit(cookie);
+	}
+	mutex_unlock(&ptdev->mmu->as.slots_lock);
+}
+
+static void panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)
 {
 	struct panthor_device *ptdev = vm->ptdev;
 	struct io_pgtable_ops *ops = vm->pgtbl_ops;
 	u64 offset = 0;
 
+	if (!size)
+		return;
+
 	drm_WARN_ON(&ptdev->base,
 		    (iova < vm->locked_region.start) ||
 		    (iova + size > vm->locked_region.start + vm->locked_region.size));
@@ -862,18 +882,32 @@ static int panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)
 		size_t pgsize = get_pgsize(iova + offset, size - offset, &pgcount);
 
 		unmapped_sz = ops->unmap_pages(ops, iova + offset, pgsize, pgcount, NULL);
+		if (drm_WARN_ON_ONCE(&ptdev->base, unmapped_sz != pgsize * pgcount)) {
+			/* Gracefully handle sparsely unmapped regions to avoid leaving
+			 * page table pages behind when the drm_gpuvm and VM page table
+			 * are out-of-sync. This is not supposed to happen, hence the
+			 * above WARN_ON().
+			 */
+			while (!ops->iova_to_phys(ops, iova + unmapped_sz) &&
+			       unmapped_sz < pgsize * pgcount)
+				unmapped_sz += SZ_4K;
 
-		if (drm_WARN_ON(&ptdev->base, unmapped_sz != pgsize * pgcount)) {
-			drm_err(&ptdev->base, "failed to unmap range %llx-%llx (requested range %llx-%llx)\n",
-				iova + offset + unmapped_sz,
-				iova + offset + pgsize * pgcount,
-				iova, iova + size);
-			return  -EINVAL;
+			/* We're passed the point where we can try to fix things,
+			 * so flag the VM unusable to make sure it's not going
+			 * to be used anymore.
+			 */
+			panthor_vm_declare_unusable(vm);
+
+			/* If we don't make progress, we're screwed. That also means
+			 * something else prevents us from unmapping the region, but
+			 * there's not much we can do here: time for debugging.
+			 */
+			if (drm_WARN_ON_ONCE(&ptdev->base, !unmapped_sz))
+				return;
 		}
+
 		offset += unmapped_sz;
 	}
-
-	return 0;
 }
 
 static int
@@ -921,16 +955,17 @@ panthor_vm_map_pages(struct panthor_vm *vm, u64 iova, int prot,
 			paddr += mapped;
 			len -= mapped;
 
-			if (drm_WARN_ON(&ptdev->base, !ret && !mapped))
+			/* If nothing was mapped, consider it an ENOMEM. */
+			if (!ret && !mapped)
 				ret = -ENOMEM;
 
-			if (ret) {
-				/* If something failed, unmap what we've already mapped before
-				 * returning. The unmap call is not supposed to fail.
+			/* If something fails, we stop there, and flag the VM unusable. */
+			if (drm_WARN_ON_ONCE(&ptdev->base, ret)) {
+				/* Unmap what we've already mapped to avoid leaving page
+				 * table pages behind.
 				 */
-				drm_WARN_ON(&ptdev->base,
-					    panthor_vm_unmap_pages(vm, start_iova,
-								   iova - start_iova));
+				panthor_vm_unmap_pages(vm, start_iova, iova - start_iova);
+				panthor_vm_declare_unusable(vm);
 				return ret;
 			}
 		}
@@ -2092,12 +2127,9 @@ static int panthor_gpuva_sm_step_remap(struct drm_gpuva_op *op,
 	struct panthor_vm_op_ctx *op_ctx = vm->op_ctx;
 	struct panthor_vma *prev_vma = NULL, *next_vma = NULL;
 	u64 unmap_start, unmap_range;
-	int ret;
 
 	drm_gpuva_op_remap_to_unmap_range(&op->remap, &unmap_start, &unmap_range);
-	ret = panthor_vm_unmap_pages(vm, unmap_start, unmap_range);
-	if (ret)
-		return ret;
+	panthor_vm_unmap_pages(vm, unmap_start, unmap_range);
 
 	if (op->remap.prev) {
 		prev_vma = panthor_vm_op_ctx_get_vma(op_ctx);
@@ -2137,13 +2169,9 @@ static int panthor_gpuva_sm_step_unmap(struct drm_gpuva_op *op,
 {
 	struct panthor_vma *unmap_vma = container_of(op->unmap.va, struct panthor_vma, base);
 	struct panthor_vm *vm = priv;
-	int ret;
-
-	ret = panthor_vm_unmap_pages(vm, unmap_vma->base.va.addr,
-				     unmap_vma->base.va.range);
-	if (drm_WARN_ON(&vm->ptdev->base, ret))
-		return ret;
 
+	panthor_vm_unmap_pages(vm, unmap_vma->base.va.addr,
+			       unmap_vma->base.va.range);
 	drm_gpuva_unmap(&op->unmap);
 	panthor_vma_unlink(vm, unmap_vma);
 	return 0;
@@ -2223,7 +2251,7 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
 
 out:
 	if (ret && flag_vm_unusable_on_failure)
-		vm->unusable = true;
+		panthor_vm_declare_unusable(vm);
 
 	vm->op_ctx = NULL;
 	mutex_unlock(&vm->op_lock);
-- 
2.51.0

Re: [PATCH v1 3/4] drm/panthor: Make panthor_vm_[un]map_pages() more robust

[Akash Goel]

On 10/31/25 15:48, Boris Brezillon wrote:
> There's no reason for panthor_vm_[un]map_pages() to fail unless the
> drm_gpuvm state and the page table are out of sync, so let's reflect that
> by making panthor_vm_unmap_pages() a void function and adding
> WARN_ON()s in various places. We also try to recover from those
> unexpected mismatch by checking for already unmapped ranges and skipping
> them. But there's only so much we can do to try and cope with such
> SW bugs, so when we see a mismatch, we flag the VM unusable and disable
> the AS to avoid further GPU accesses to the memory.
>
> It could be that the as_disable() call fails because the MMU unit is
> stuck, in which case the whole GPU is frozen, and only a GPU reset can
> unblock things. Ater the reset, the VM will be seen as unusable and
> any attempt to re-use it will fail, so we should be covered for any
> use-after-unmap issues.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>   drivers/gpu/drm/panthor/panthor_mmu.c | 82 ++++++++++++++++++---------
>   1 file changed, 55 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index ea886c8ac97f..a4f3ed04b5cc 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -846,12 +846,32 @@ static size_t get_pgsize(u64 addr, size_t size, size_t *count)
>       return SZ_2M;
>   }
>
> -static int panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)
> +static void panthor_vm_declare_unusable(struct panthor_vm *vm)
> +{
> +     struct panthor_device *ptdev = vm->ptdev;
> +     int cookie;
> +
> +     if (vm->unusable)
> +             return;
> +
> +     vm->unusable = true;
> +     mutex_unlock(&ptdev->mmu->as.slots_lock);

Please fix this. Need to call mutex_lock.


> +     if (vm->as.id >= 0 && drm_dev_enter(&ptdev->base, &cookie)) {
> +             panthor_mmu_as_disable(ptdev, vm->as.id);
> +             drm_dev_exit(cookie);
> +     }
> +     mutex_unlock(&ptdev->mmu->as.slots_lock);
> +}
> +
> +static void panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v1 3/4] drm/panthor: Make panthor_vm_[un]map_pages() more robust

[Boris Brezillon]

On Mon, 3 Nov 2025 21:00:58 +0000
Akash Goel <akash.goel@arm.com> wrote:

> On 10/31/25 15:48, Boris Brezillon wrote:
> > There's no reason for panthor_vm_[un]map_pages() to fail unless the
> > drm_gpuvm state and the page table are out of sync, so let's reflect that
> > by making panthor_vm_unmap_pages() a void function and adding
> > WARN_ON()s in various places. We also try to recover from those
> > unexpected mismatch by checking for already unmapped ranges and skipping
> > them. But there's only so much we can do to try and cope with such
> > SW bugs, so when we see a mismatch, we flag the VM unusable and disable
> > the AS to avoid further GPU accesses to the memory.
> >
> > It could be that the as_disable() call fails because the MMU unit is
> > stuck, in which case the whole GPU is frozen, and only a GPU reset can
> > unblock things. Ater the reset, the VM will be seen as unusable and
> > any attempt to re-use it will fail, so we should be covered for any
> > use-after-unmap issues.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >   drivers/gpu/drm/panthor/panthor_mmu.c | 82 ++++++++++++++++++---------
> >   1 file changed, 55 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > index ea886c8ac97f..a4f3ed04b5cc 100644
> > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > @@ -846,12 +846,32 @@ static size_t get_pgsize(u64 addr, size_t size, size_t *count)
> >       return SZ_2M;
> >   }
> >
> > -static int panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)
> > +static void panthor_vm_declare_unusable(struct panthor_vm *vm)
> > +{
> > +     struct panthor_device *ptdev = vm->ptdev;
> > +     int cookie;
> > +
> > +     if (vm->unusable)
> > +             return;
> > +
> > +     vm->unusable = true;
> > +     mutex_unlock(&ptdev->mmu->as.slots_lock);  
> 
> Please fix this. Need to call mutex_lock.

Oops. Will fix.

> 
> 
> > +     if (vm->as.id >= 0 && drm_dev_enter(&ptdev->base, &cookie)) {
> > +             panthor_mmu_as_disable(ptdev, vm->as.id);
> > +             drm_dev_exit(cookie);
> > +     }
> > +     mutex_unlock(&ptdev->mmu->as.slots_lock);
> > +}
> > +
> > +static void panthor_vm_unmap_pages(struct panthor_vm *vm, u64 iova, u64 size)  
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH v1 4/4] drm/panthor: Relax check in panthor_sched_pre_reset()

[Boris Brezillon]

A group can become runnable even after reset.in_progress has
been set to true and panthor_sched_suspend() has been called,
because the drm_sched queues are still running at that point,
and ::run_job() might call group_schedule_locked() which moves
the group to the runnable list. And that's fine, because we're
moving those groups to the stopped list anyway when we call
panthor_group_stop(), so just drop the misleading WARN_ON().

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index fc0826db8f48..51a8d842a7a3 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2835,8 +2835,6 @@ void panthor_sched_pre_reset(struct panthor_device *ptdev)
 	 * new jobs while we're resetting.
 	 */
 	for (i = 0; i < ARRAY_SIZE(sched->groups.runnable); i++) {
-		/* All groups should be in the idle lists. */
-		drm_WARN_ON(&ptdev->base, !list_empty(&sched->groups.runnable[i]));
 		list_for_each_entry_safe(group, group_tmp, &sched->groups.runnable[i], run_node)
 			panthor_group_stop(group);
 	}
-- 
2.51.0

Re: [PATCH v1 4/4] drm/panthor: Relax check in panthor_sched_pre_reset()

[Steven Price]

On 31/10/2025 15:48, Boris Brezillon wrote:
> A group can become runnable even after reset.in_progress has
> been set to true and panthor_sched_suspend() has been called,
> because the drm_sched queues are still running at that point,
> and ::run_job() might call group_schedule_locked() which moves
> the group to the runnable list. And that's fine, because we're
> moving those groups to the stopped list anyway when we call
> panthor_group_stop(), so just drop the misleading WARN_ON().

If we've got another thread mutating the runnable list between
panthor_sched_suspend() and list_for_each_entry_safe(), doesn't that
make the list iterator unsafe? (_safe only protects against deleting the
current item, not against concurrent access).

It feels to me like we should be holding the sched mutex - at least
while iterating. I agree the WARN_ON is unnecessary, and will need
removing if we simply guard the iteration - the alternative is to
recolour panthor_sched_suspend() to assume the lock is held (and take
the lock in panthor_sched_pre_reset), but I suspect that's a more ugly
change.

Thanks,
Steve

> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index fc0826db8f48..51a8d842a7a3 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2835,8 +2835,6 @@ void panthor_sched_pre_reset(struct panthor_device *ptdev)
>  	 * new jobs while we're resetting.
>  	 */
>  	for (i = 0; i < ARRAY_SIZE(sched->groups.runnable); i++) {
> -		/* All groups should be in the idle lists. */
> -		drm_WARN_ON(&ptdev->base, !list_empty(&sched->groups.runnable[i]));
>  		list_for_each_entry_safe(group, group_tmp, &sched->groups.runnable[i], run_node)
>  			panthor_group_stop(group);
>  	}

Re: [PATCH v1 4/4] drm/panthor: Relax check in panthor_sched_pre_reset()

[Boris Brezillon]

On Fri, 7 Nov 2025 16:40:53 +0000
Steven Price <steven.price@arm.com> wrote:

> On 31/10/2025 15:48, Boris Brezillon wrote:
> > A group can become runnable even after reset.in_progress has
> > been set to true and panthor_sched_suspend() has been called,
> > because the drm_sched queues are still running at that point,
> > and ::run_job() might call group_schedule_locked() which moves
> > the group to the runnable list. And that's fine, because we're
> > moving those groups to the stopped list anyway when we call
> > panthor_group_stop(), so just drop the misleading WARN_ON().  
> 
> If we've got another thread mutating the runnable list between
> panthor_sched_suspend() and list_for_each_entry_safe(), doesn't that
> make the list iterator unsafe? (_safe only protects against deleting the
> current item, not against concurrent access).

I'm not too sure actually. There's an
atomic_read(&sched->reset.in_progress) to check if we're about to reset
in group_schedule_locked() and cancel the insertion into the runnable
list in that case, meaning we're sure nothing new will be inserted after
we've set the in_progress=true in panthor_sched_pre_reset().

> 
> It feels to me like we should be holding the sched mutex - at least
> while iterating. I agree the WARN_ON is unnecessary, and will need
> removing if we simply guard the iteration - the alternative is to
> recolour panthor_sched_suspend() to assume the lock is held (and take
> the lock in panthor_sched_pre_reset), but I suspect that's a more ugly
> change.

I'd rather ensure that nothing new is inserted in the runnable/idle
lists after sched->reset.in_progress is set to true. Note that
sched->reset.in_progress is set to true with the sched lock held,
meaning any path modifying the sched lists (must be done with the sched
lock held) should complete before we set this to true. As long as those
paths also skip the list insertion, or, for things happening in a work
context (thinking of the tick work here), as long as the work is not
rescheduled until we get a chance to disable this work, we should be
good, no?

Re: [PATCH v1 4/4] drm/panthor: Relax check in panthor_sched_pre_reset()

[Steven Price]

On 12/11/2025 12:47, Boris Brezillon wrote:
> On Fri, 7 Nov 2025 16:40:53 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 31/10/2025 15:48, Boris Brezillon wrote:
>>> A group can become runnable even after reset.in_progress has
>>> been set to true and panthor_sched_suspend() has been called,
>>> because the drm_sched queues are still running at that point,
>>> and ::run_job() might call group_schedule_locked() which moves
>>> the group to the runnable list. And that's fine, because we're
>>> moving those groups to the stopped list anyway when we call
>>> panthor_group_stop(), so just drop the misleading WARN_ON().  
>>
>> If we've got another thread mutating the runnable list between
>> panthor_sched_suspend() and list_for_each_entry_safe(), doesn't that
>> make the list iterator unsafe? (_safe only protects against deleting the
>> current item, not against concurrent access).
> 
> I'm not too sure actually. There's an
> atomic_read(&sched->reset.in_progress) to check if we're about to reset
> in group_schedule_locked() and cancel the insertion into the runnable
> list in that case, meaning we're sure nothing new will be inserted after
> we've set the in_progress=true in panthor_sched_pre_reset().

I was mostly going on your commit message:

> A group can become runnable even after reset.in_progress has
> been set to true and panthor_sched_suspend() has been called

if that is indeed happening then we have a problem (and removing the
WARN_ON is just papering over it). I haven't actually followed through
the logic.

>>
>> It feels to me like we should be holding the sched mutex - at least
>> while iterating. I agree the WARN_ON is unnecessary, and will need
>> removing if we simply guard the iteration - the alternative is to
>> recolour panthor_sched_suspend() to assume the lock is held (and take
>> the lock in panthor_sched_pre_reset), but I suspect that's a more ugly
>> change.
> 
> I'd rather ensure that nothing new is inserted in the runnable/idle
> lists after sched->reset.in_progress is set to true. Note that
> sched->reset.in_progress is set to true with the sched lock held,
> meaning any path modifying the sched lists (must be done with the sched
> lock held) should complete before we set this to true. As long as those
> paths also skip the list insertion, or, for things happening in a work
> context (thinking of the tick work here), as long as the work is not
> rescheduled until we get a chance to disable this work, we should be
> good, no?

Yes that design can work. But atomics can be difficult to reason about,
so unless there's a good reason I think we'd generally be better
sticking with (simple) locks on the slow paths, then we get the benefits
of lockdep etc checking we haven't messed up.

Thanks,
Steve

Re: [PATCH v1 4/4] drm/panthor: Relax check in panthor_sched_pre_reset()

[Boris Brezillon]

On Mon, 17 Nov 2025 09:49:27 +0000
Steven Price <steven.price@arm.com> wrote:

> On 12/11/2025 12:47, Boris Brezillon wrote:
> > On Fri, 7 Nov 2025 16:40:53 +0000
> > Steven Price <steven.price@arm.com> wrote:
> >   
> >> On 31/10/2025 15:48, Boris Brezillon wrote:  
> >>> A group can become runnable even after reset.in_progress has
> >>> been set to true and panthor_sched_suspend() has been called,
> >>> because the drm_sched queues are still running at that point,
> >>> and ::run_job() might call group_schedule_locked() which moves
> >>> the group to the runnable list. And that's fine, because we're
> >>> moving those groups to the stopped list anyway when we call
> >>> panthor_group_stop(), so just drop the misleading WARN_ON().    
> >>
> >> If we've got another thread mutating the runnable list between
> >> panthor_sched_suspend() and list_for_each_entry_safe(), doesn't that
> >> make the list iterator unsafe? (_safe only protects against deleting the
> >> current item, not against concurrent access).  
> > 
> > I'm not too sure actually. There's an
> > atomic_read(&sched->reset.in_progress) to check if we're about to reset
> > in group_schedule_locked() and cancel the insertion into the runnable
> > list in that case, meaning we're sure nothing new will be inserted after
> > we've set the in_progress=true in panthor_sched_pre_reset().  
> 
> I was mostly going on your commit message:
> 
> > A group can become runnable even after reset.in_progress has
> > been set to true and panthor_sched_suspend() has been called  
> 
> if that is indeed happening then we have a problem (and removing the
> WARN_ON is just papering over it). I haven't actually followed through
> the logic.

Sorry, it's not exactly that. The problem is that a group might be
inserted in the runnable list before we've had a chance to set
reset.in_progress=true (earlier in this function), and there's nothing
removing those groups from the runnable list between this assignment and
the loop stopping the groups.

> 
> >>
> >> It feels to me like we should be holding the sched mutex - at least
> >> while iterating. I agree the WARN_ON is unnecessary, and will need
> >> removing if we simply guard the iteration - the alternative is to
> >> recolour panthor_sched_suspend() to assume the lock is held (and take
> >> the lock in panthor_sched_pre_reset), but I suspect that's a more ugly
> >> change.  
> > 
> > I'd rather ensure that nothing new is inserted in the runnable/idle
> > lists after sched->reset.in_progress is set to true. Note that
> > sched->reset.in_progress is set to true with the sched lock held,
> > meaning any path modifying the sched lists (must be done with the sched
> > lock held) should complete before we set this to true. As long as those
> > paths also skip the list insertion, or, for things happening in a work
> > context (thinking of the tick work here), as long as the work is not
> > rescheduled until we get a chance to disable this work, we should be
> > good, no?  
> 
> Yes that design can work. But atomics can be difficult to reason about,
> so unless there's a good reason I think we'd generally be better
> sticking with (simple) locks

Locks alone won't prevent groups from being moved around after the
stop_all_groups loop though. It's the lock plus the fact groups can't
have their state changed while a reset is in progress that gives this
guarantee, at which point I guess checking reset.in_progress with or
without the lock held is the same, no? We do change the
reset.in_progress state with the sched.reset.lock held though, to make
sure any path that could move the group to a different list is out of
the way when we exit the locked section. That means that new threads
entering such paths (path changing the group state) will see the new
value and bail out.

> on the slow paths, then we get the benefits
> of lockdep etc checking we haven't messed up.

I don't mind taking the sched lock in this slow path, but I don't think
taking/releasing it inside panthor_sched_pre_reset() is giving any more
safeness, because what we want is a guarantee that groups won't be
moved around between panthor_sched_pre_reset() and
panthor_sched_post_reset(). If that's really a change we want to push
(reworking the locking in the reset sequence), I'd rather do that in
its own patchset, if you don't mind.

Re: [PATCH v1 4/4] drm/panthor: Relax check in panthor_sched_pre_reset()

[Steven Price]

On 17/11/2025 10:26, Boris Brezillon wrote:
> On Mon, 17 Nov 2025 09:49:27 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 12/11/2025 12:47, Boris Brezillon wrote:
>>> On Fri, 7 Nov 2025 16:40:53 +0000
>>> Steven Price <steven.price@arm.com> wrote:
>>>   
>>>> On 31/10/2025 15:48, Boris Brezillon wrote:  
>>>>> A group can become runnable even after reset.in_progress has
>>>>> been set to true and panthor_sched_suspend() has been called,
>>>>> because the drm_sched queues are still running at that point,
>>>>> and ::run_job() might call group_schedule_locked() which moves
>>>>> the group to the runnable list. And that's fine, because we're
>>>>> moving those groups to the stopped list anyway when we call
>>>>> panthor_group_stop(), so just drop the misleading WARN_ON().    
>>>>
>>>> If we've got another thread mutating the runnable list between
>>>> panthor_sched_suspend() and list_for_each_entry_safe(), doesn't that
>>>> make the list iterator unsafe? (_safe only protects against deleting the
>>>> current item, not against concurrent access).  
>>>
>>> I'm not too sure actually. There's an
>>> atomic_read(&sched->reset.in_progress) to check if we're about to reset
>>> in group_schedule_locked() and cancel the insertion into the runnable
>>> list in that case, meaning we're sure nothing new will be inserted after
>>> we've set the in_progress=true in panthor_sched_pre_reset().  
>>
>> I was mostly going on your commit message:
>>
>>> A group can become runnable even after reset.in_progress has
>>> been set to true and panthor_sched_suspend() has been called  
>>
>> if that is indeed happening then we have a problem (and removing the
>> WARN_ON is just papering over it). I haven't actually followed through
>> the logic.
> 
> Sorry, it's not exactly that. The problem is that a group might be
> inserted in the runnable list before we've had a chance to set
> reset.in_progress=true (earlier in this function), and there's nothing
> removing those groups from the runnable list between this assignment and
> the loop stopping the groups.

Ok, so the commit message is misleading...

My understanding is that the tick and sync_upd is what moves things onto
the runnable list. So we set the "reset.in_progress" flag and cancel
(and sync) with the two work queues which could move things onto runnable.

panthor_sched_suspend() then deals with anything actually running, but
it does appear we're missing anything to deal with the runnable list,
hence the WARN_ON needs dropping.

>>
>>>>
>>>> It feels to me like we should be holding the sched mutex - at least
>>>> while iterating. I agree the WARN_ON is unnecessary, and will need
>>>> removing if we simply guard the iteration - the alternative is to
>>>> recolour panthor_sched_suspend() to assume the lock is held (and take
>>>> the lock in panthor_sched_pre_reset), but I suspect that's a more ugly
>>>> change.  
>>>
>>> I'd rather ensure that nothing new is inserted in the runnable/idle
>>> lists after sched->reset.in_progress is set to true. Note that
>>> sched->reset.in_progress is set to true with the sched lock held,
>>> meaning any path modifying the sched lists (must be done with the sched
>>> lock held) should complete before we set this to true. As long as those
>>> paths also skip the list insertion, or, for things happening in a work
>>> context (thinking of the tick work here), as long as the work is not
>>> rescheduled until we get a chance to disable this work, we should be
>>> good, no?  
>>
>> Yes that design can work. But atomics can be difficult to reason about,
>> so unless there's a good reason I think we'd generally be better
>> sticking with (simple) locks
> 
> Locks alone won't prevent groups from being moved around after the
> stop_all_groups loop though. It's the lock plus the fact groups can't
> have their state changed while a reset is in progress that gives this
> guarantee, at which point I guess checking reset.in_progress with or
> without the lock held is the same, no? We do change the
> reset.in_progress state with the sched.reset.lock held though, to make
> sure any path that could move the group to a different list is out of
> the way when we exit the locked section. That means that new threads
> entering such paths (path changing the group state) will see the new
> value and bail out.

No you're right - the lock by itself won't work because we need to drop
all locks before returning from pre_reset() and we need to keep that
state until post_reset().

>> on the slow paths, then we get the benefits
>> of lockdep etc checking we haven't messed up.
> 
> I don't mind taking the sched lock in this slow path, but I don't think
> taking/releasing it inside panthor_sched_pre_reset() is giving any more
> safeness, because what we want is a guarantee that groups won't be
> moved around between panthor_sched_pre_reset() and
> panthor_sched_post_reset(). If that's really a change we want to push
> (reworking the locking in the reset sequence), I'd rather do that in
> its own patchset, if you don't mind.

I think the real issue was the commit message misleading me. I'd assumed
from the commit message that you'd observed the situation of a group
being put on the runnable list between the call to
panthor_sched_suspend() and the drm_WARN_ON check. If that really did
happen then there's a much deeper bug somewhere that needs fixing.

If I understand correctly what you've actually observed is a group being
put on the runnable list *before* panthor_sched_suspend() and we're not
currently handling that[1]. I agree that simply dropping the
drm_WARN_ON() would be the logic fix for that.

TLDR; The commit message needs some work ;)

Thanks,
Steve

[1] Well we are, but with a big WARN_ON splat ;)