[PATCH 0/3] drm/panthor: Misc MMU fixes/robustness improvements

[PATCH 0/3] drm/panthor: Misc MMU fixes/robustness improvements

[Boris Brezillon]

The first commit is fixing a bug we've seen on v13 HW. Even though
we don't quite understand what happens (race in the flush-elimination
logic when flush requests are sent concurrently from the CPU and the
MCU), it seems that the downstream driver has always been doing a
flush+invalidate of RW caches from the start, and that we were doing
so up until the atomic page table update changes, so let's go back to
that state and leave the RO L1 caches untouched.

The second patch a fix for a race that could very well happen if we
ever end up with a failure between the as_disable() and as_enable()
calls. We've not experienced this so far, but it seems worth plugging
the hole regardless. The last patch is a much more theoretical bug,
which would involve a buggy FW telling us that a CSG is suspended,
when it's actually. I've deliberately not added a Fixes tag on the
last one for this very reason, but I think it's worth staying on
the safe side by addressing this theoretical issue, still.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Boris Brezillon (3):
      drm/panthor: Don't invalidate OTHER caches
      drm/panthor: Fully disable the AS even if it's going to be re-assigned
      drm/panthor: Move cache-flush after UPDATE(UNMAPPED)

 drivers/gpu/drm/panthor/panthor_mmu.c | 43 ++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 23 deletions(-)
---
base-commit: 45585c3aa285854face65293acc95eff73063d6d
change-id: 20260924-panthor-mmu-fixes-a1832dc60908

Best regards,
--  
Boris Brezillon <boris.brezillon@collabora.com>

[PATCH 1/3] drm/panthor: Don't invalidate OTHER caches

[Boris Brezillon]

Don't invalidate the OTHER caches (AKA L1 read-only caches) since
this seems to trip out the flush-elimination logic on v13, and it's not
needed in practice (can't leak other context data because of TLB
invalidation or corrupt physical pages returned to the system because
it's a RO cache).

It's also worth noting that the downstream driver has been avoid
OTHER caches invalidation from the start, and that we were doing
the same until atomic page table update was introduced.

Fixes: 6e2d3b3e8589 ("drm/panthor: Add support for atomic page table updates")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 9f63a048df61..7e98084b9a1e 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -634,7 +634,7 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr,
 
 	/* Flush+invalidate RW caches, invalidate RO ones. */
 	ret = panthor_gpu_flush_caches(ptdev, CACHE_CLEAN | CACHE_INV,
-				       CACHE_CLEAN | CACHE_INV, CACHE_INV);
+				       CACHE_CLEAN | CACHE_INV, 0);
 	if (ret)
 		return ret;
 
@@ -1841,8 +1841,7 @@ static void panthor_vm_unlock_region(struct panthor_vm *vm)
 		 * range is narrow enough and the HW supports it.
 		 */
 		ret = panthor_gpu_flush_caches(ptdev, CACHE_CLEAN | CACHE_INV,
-					       CACHE_CLEAN | CACHE_INV,
-					       CACHE_INV);
+					       CACHE_CLEAN | CACHE_INV, 0);
 
 		/* Unlock the region if the flush is effective. */
 		if (!ret)

-- 
2.55.0

Re: [PATCH 1/3] drm/panthor: Don't invalidate OTHER caches

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] doc: comment contradicts code on RO cache invalidation
--

commit 97f084533582df463bbfd6d17bb8670c85068160
Author: Boris Brezillon <boris.brezillon@collabora.com>

drm/panthor: Don't invalidate OTHER caches

This commit stops invalidating OTHER (L1 read-only) caches during MMU
operations to prevent issues with flush-elimination logic on v13 hardware.
The invalidation is deemed unnecessary as TLB invalidation prevents context
data leaks and the cache is read-only.

> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 9f63a048df61c..7e98084b9a1ed 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c

[ ... ]

> @@ -1841,8 +1841,7 @@ static void panthor_vm_unlock_region(struct panthor_vm *vm)
>  		 * range is narrow enough and the HW supports it.
>  		 */
>  		ret = panthor_gpu_flush_caches(ptdev, CACHE_CLEAN | CACHE_INV,
> -					       CACHE_CLEAN | CACHE_INV,
> -					       CACHE_INV);
> +					       CACHE_CLEAN | CACHE_INV, 0);

[Severity: Low]
This isn't a bug, but should the comment just above this call in
panthor_vm_unlock_region() be updated?

The comment still explicitly says "and invalidate RO ones", which now
contradicts the implementation since 0 is being passed for the OTHER (RO)
caches.

>  
>  		/* Unlock the region if the flush is effective. */
>  		if (!ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-panthor-mmu-fixes-v1-0-e25f9b106466@collabora.com?part=1

[PATCH 2/3] drm/panthor: Fully disable the AS even if it's going to be re-assigned

[Boris Brezillon]

We're trying to be smart by skipping the UPDATE(UNMAPPED) step, but
it remains to be proven it adds any noticeable overhead. Moreover, it's
breaking the assumption that, once the VM is unbound, no access can
happen on it.

For instance, say the panthor_mmu_as_enable() call in panthor_vm_active()
fails after we've called panthor_vm_release_as_locked(), we're now in a
state where the HW still points to the old page table, but SW slot points
to the new VM, which is not truly HW-bound.

If, after the as_slots lock is released, the evicted VM itself is
released, the HW might have access to memory that has been returned
to the system until the reset we scheduled (because of the faulty
AS_COMMAND) is effective.

Let's make this bullet-proof by doing a full bound -> unbound -> bound
cycle on AS slot recycling.

Fixes: 32e593d74c39 ("drm/panthor: Make sure caches are flushed/invalidated when an AS is recycled")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 7e98084b9a1e..038a092fd08c 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -620,8 +620,7 @@ static int panthor_mmu_as_enable(struct panthor_device *ptdev, u32 as_nr,
 	return as_send_cmd_and_wait(ptdev, as_nr, AS_COMMAND_UPDATE);
 }
 
-static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr,
-				  bool recycle_slot)
+static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
 {
 	struct panthor_mmu *mmu = ptdev->mmu;
 	struct panthor_vm *vm = ptdev->mmu->as.slots[as_nr].vm;
@@ -645,12 +644,6 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr,
 			return ret;
 	}
 
-	/* If the slot is going to be used immediately, don't bother changing
-	 * the config.
-	 */
-	if (recycle_slot)
-		return 0;
-
 	gpu_write64(mmu->iomem, AS_TRANSTAB(as_nr), 0);
 	gpu_write64(mmu->iomem, AS_MEMATTR(as_nr), 0);
 	gpu_write64(mmu->iomem, AS_TRANSCFG(as_nr), AS_TRANSCFG_ADRMODE_UNMAPPED);
@@ -777,7 +770,7 @@ int panthor_vm_active(struct panthor_vm *vm)
 		drm_WARN_ON(&ptdev->base, refcount_read(&lru_vm->as.active_cnt));
 		as = lru_vm->as.id;
 
-		ret = panthor_mmu_as_disable(ptdev, as, true);
+		ret = panthor_mmu_as_disable(ptdev, as);
 		if (ret)
 			goto out_unlock;
 
@@ -919,7 +912,7 @@ static void panthor_vm_declare_unusable(struct panthor_vm *vm)
 	vm->unusable = true;
 	mutex_lock(&ptdev->mmu->as.slots_lock);
 	if (vm->as.id >= 0 && drm_dev_enter(&ptdev->base, &cookie)) {
-		panthor_mmu_as_disable(ptdev, vm->as.id, false);
+		panthor_mmu_as_disable(ptdev, vm->as.id);
 		drm_dev_exit(cookie);
 	}
 	mutex_unlock(&ptdev->mmu->as.slots_lock);
@@ -1911,7 +1904,7 @@ static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status)
 			ptdev->mmu->as.slots[as].vm->unhandled_fault = true;
 
 		/* Disable the MMU to kill jobs on this AS. */
-		panthor_mmu_as_disable(ptdev, as, false);
+		panthor_mmu_as_disable(ptdev, as);
 		mutex_unlock(&ptdev->mmu->as.slots_lock);
 
 		status &= ~mask;
@@ -1940,7 +1933,7 @@ void panthor_mmu_suspend(struct panthor_device *ptdev)
 
 		if (vm) {
 			drm_WARN_ON(&ptdev->base,
-				    panthor_mmu_as_disable(ptdev, i, false));
+				    panthor_mmu_as_disable(ptdev, i));
 			panthor_vm_release_as_locked(vm);
 		}
 	}
@@ -2065,7 +2058,7 @@ static void panthor_vm_free(struct drm_gpuvm *gpuvm)
 		int cookie;
 
 		if (drm_dev_enter(&ptdev->base, &cookie)) {
-			panthor_mmu_as_disable(ptdev, vm->as.id, false);
+			panthor_mmu_as_disable(ptdev, vm->as.id);
 			drm_dev_exit(cookie);
 		}
 
@@ -3360,7 +3353,7 @@ void panthor_mmu_unplug(struct panthor_device *ptdev)
 
 		if (vm) {
 			drm_WARN_ON(&ptdev->base,
-				    panthor_mmu_as_disable(ptdev, i, false));
+				    panthor_mmu_as_disable(ptdev, i));
 			panthor_vm_release_as_locked(vm);
 		}
 	}

-- 
2.55.0

[PATCH 3/3] drm/panthor: Move cache-flush after UPDATE(UNMAPPED)

[Boris Brezillon]

Right now, there's a theoretical window during which the GPU can populate
the cache with data from a VM that's about to be evicted through the
UPDATE(UNMAPPED) command.
In practice this won't happen (hence the absence of Fixes tag) because
when panthor_mmu_as_disable() is called, the VM is guaranteed to be idle
(no active CSG pointing to this VM), but as we say, better safe than
sorry.

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

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 038a092fd08c..bb71274aa36b 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -631,12 +631,6 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
 	panthor_mmu_irq_disable_events(&ptdev->mmu->irq,
 				       panthor_mmu_as_fault_mask(ptdev, as_nr));
 
-	/* Flush+invalidate RW caches, invalidate RO ones. */
-	ret = panthor_gpu_flush_caches(ptdev, CACHE_CLEAN | CACHE_INV,
-				       CACHE_CLEAN | CACHE_INV, 0);
-	if (ret)
-		return ret;
-
 	if (vm && vm->locked_region.size) {
 		/* Unlock the region if there's a lock pending. */
 		ret = as_send_cmd_and_wait(ptdev, vm->as.id, AS_COMMAND_UNLOCK);
@@ -648,7 +642,18 @@ static int panthor_mmu_as_disable(struct panthor_device *ptdev, u32 as_nr)
 	gpu_write64(mmu->iomem, AS_MEMATTR(as_nr), 0);
 	gpu_write64(mmu->iomem, AS_TRANSCFG(as_nr), AS_TRANSCFG_ADRMODE_UNMAPPED);
 
-	return as_send_cmd_and_wait(ptdev, as_nr, AS_COMMAND_UPDATE);
+	ret = as_send_cmd_and_wait(ptdev, as_nr, AS_COMMAND_UPDATE);
+	if (ret)
+		return ret;
+
+	/* Flush+invalidate RW caches after we've unmapped, to make sure any
+	 * cacheline eviction is effective before we potentially return
+	 * memory pointed by this VM to the system. This needs to be done
+	 * after the UPDATE(UNMAPPED) operation to guarantee that not further
+	 * PT-walk can pull VM data into the cache.
+	 */
+	return panthor_gpu_flush_caches(ptdev, CACHE_CLEAN | CACHE_INV,
+					CACHE_CLEAN | CACHE_INV, 0);
 }
 
 static u32 panthor_mmu_fault_mask(struct panthor_device *ptdev, u32 value)

-- 
2.55.0