[PATCH 0/4] RPM, percnt and other minor fixes for Panfrost

[PATCH 0/4] RPM, percnt and other minor fixes for Panfrost

[Adrián Larumbe]

This patch series comes with some minor fixes for Panfrost. Some of the
issues addressed were uncovered by LLM tools, while the one about RPM was
traditionally debugged.

I haven't yet added any Fixes or Reported-By tags, because the purpose of
the first revision is probing appetite for these changes and eliciting
early discussion.

The only proper bug fixes are the ones for perfcnt and RPM, and the others
are more about ensuring robustness. perfcnt bug was never observed in the
open, because no one had faced a HW reset when a perfcnt session was
ongoing. RPM fix addresses a long-standing issue I had been observing for
quite some time, but that I had completely misunderstood.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
Adrián Larumbe (4):
      drm/panfrost: Check another bo field for cache option query
      drm/panfrost: Make reset sequence deal with an active HWPerf session
      drm/panfrost: Prevent division by 0
      drm/panfrost: Fix RPM so device suspends when no jobs are in flight

 drivers/gpu/drm/panfrost/panfrost_devfreq.c |  3 +-
 drivers/gpu/drm/panfrost/panfrost_device.c  |  1 +
 drivers/gpu/drm/panfrost/panfrost_drv.c     | 10 +++--
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 58 +++++++++++++++++++----------
 drivers/gpu/drm/panfrost/panfrost_perfcnt.h |  1 +
 5 files changed, 49 insertions(+), 24 deletions(-)
---
base-commit: c1079aebb4de218caa86c44f9a53700d1a582683
change-id: 20260523-claude-fixes-82b03a134a8c

Best regards,
--  
Adrián Larumbe <adrian.larumbe@collabora.com>

[PATCH 1/4] drm/panfrost: Check another bo field for cache option query

[Adrián Larumbe]

When querying the properties of a BO, whether it was created in
Writeback caching mode for userspace mappings, is recorded in a Panfrost
BO's wb_mmap field.

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

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 3d0bdba2a474..cb8e5015847f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -659,7 +659,7 @@ static int panfrost_ioctl_query_bo_info(struct drm_device *dev, void *data,
 		if (bo->is_heap)
 			args->create_flags |= PANFROST_BO_HEAP;
 
-		if (!bo->base.map_wc)
+		if (bo->wb_mmap)
 			args->create_flags |= PANFROST_BO_WB_MMAP;
 	}
 

-- 
2.53.0

Claude review: drm/panfrost: Check another bo field for cache option query

[Claude Code Review Bot]

Patch Review

**Status: Looks correct, good fix.**

The change in `panfrost_drv.c`:
```c
-		if (!bo->base.map_wc)
+		if (bo->wb_mmap)
```

The old code infers WB_MMAP by checking `!map_wc`. But `map_wc` is derived from *two* inputs, as seen in `panfrost_gem.c:should_map_wc()`:
```c
if (pfdev->coherent)
    return false;
if (bo->wb_mmap)
    return false;
return true;
```

On a coherent device, `map_wc` is always `false` regardless of `wb_mmap`. So `!bo->base.map_wc` would be `true` for *every* BO on a coherent device, incorrectly reporting `PANFROST_BO_WB_MMAP` for BOs that were never created with that flag. The fix to check `bo->wb_mmap` directly is the right thing to do -- it queries the user's original intent rather than a derived value.

**Minor**: The commit message could be clearer about *why* `map_wc` is wrong -- specifically mentioning the coherent-device case would help reviewers understand the concrete bug.

---

---
Generated by Claude Code Patch Reviewer

[PATCH 2/4] drm/panfrost: Make reset sequence deal with an active HWPerf session

[Adrián Larumbe]

Right now, if there's a HW reset and an HWPerf session is active,
panfrost_mmu_reset() will reset mmu structs for all all open files to
have zero users and their HW address space assignment. Then, when
disabling hwperf, panfrost_mmu_as_put() will WARN that mmu->as_count is
less than zero.

Fix this by introducing a perfcnt HW reset path.

The choice was made to render perfcnt unusable after reset, so that a
user might have to reprogram it with a full disable/enable sequence
before requesting more perfcnt dumps.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c  |  1 +
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 58 +++++++++++++++++++----------
 drivers/gpu/drm/panfrost/panfrost_perfcnt.h |  1 +
 3 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 7fed22d555a5..2a4417bc4065 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -421,6 +421,7 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 {
 	panfrost_gpu_soft_reset(pfdev);
 
+	panfrost_perfcnt_reset(pfdev);
 	panfrost_gpu_power_on(pfdev);
 	panfrost_mmu_reset(pfdev);
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
index 7020c0192e18..d6a6fbe35578 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
@@ -33,6 +33,7 @@ struct panfrost_perfcnt {
 	struct panfrost_file_priv *user;
 	struct mutex lock;
 	struct completion dump_comp;
+	bool hw_reset_happened;
 };
 
 void panfrost_perfcnt_clean_cache_done(struct panfrost_device *pfdev)
@@ -166,6 +167,7 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 	/* The BO ref is retained by the mapping. */
 	drm_gem_object_put(&bo->base);
 
+	perfcnt->hw_reset_happened = false;
 	perfcnt->user = user;
 
 	return 0;
@@ -183,6 +185,16 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 	return ret;
 }
 
+static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfdev)
+{
+	gpu_write(pfdev, GPU_PERFCNT_CFG,
+		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
+	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0);
+	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0x0);
+	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0x0);
+	gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
+}
+
 static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
 					   struct drm_file *file_priv)
 {
@@ -193,18 +205,14 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
 	if (user != perfcnt->user)
 		return -EINVAL;
 
-	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0);
-	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0x0);
-	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0x0);
-	gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
-	gpu_write(pfdev, GPU_PERFCNT_CFG,
-		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
+	panfrost_perfcnt_gpu_disable(pfdev);
 
 	perfcnt->user = NULL;
 	drm_gem_vunmap(&perfcnt->mapping->obj->base.base, &map);
 	perfcnt->buf = NULL;
 	panfrost_gem_close(&perfcnt->mapping->obj->base.base, file_priv);
-	panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
+	if (!perfcnt->hw_reset_happened)
+		panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
 	panfrost_gem_mapping_put(perfcnt->mapping);
 	perfcnt->mapping = NULL;
 	pm_runtime_put_autosuspend(pfdev->base.dev);
@@ -258,6 +266,15 @@ int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
 		goto out;
 	}
 
+	/*
+	 * A HW reset when HWPerf was active mean user should go through
+	 * a disable/enable sequence before requesting more frame dumps.
+	 */
+	if (perfcnt->hw_reset_happened) {
+		ret = -EAGAIN;
+		goto out;
+	}
+
 	ret = panfrost_perfcnt_dump_locked(pfdev);
 	if (ret)
 		goto out;
@@ -327,12 +344,7 @@ int panfrost_perfcnt_init(struct panfrost_device *pfdev)
 	perfcnt->bosize = size;
 
 	/* Start with everything disabled. */
-	gpu_write(pfdev, GPU_PERFCNT_CFG,
-		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
-	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
+	panfrost_perfcnt_gpu_disable(pfdev);
 
 	init_completion(&perfcnt->dump_comp);
 	mutex_init(&perfcnt->lock);
@@ -344,10 +356,18 @@ int panfrost_perfcnt_init(struct panfrost_device *pfdev)
 void panfrost_perfcnt_fini(struct panfrost_device *pfdev)
 {
 	/* Disable everything before leaving. */
-	gpu_write(pfdev, GPU_PERFCNT_CFG,
-		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
-	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0);
-	gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
+	panfrost_perfcnt_gpu_disable(pfdev);
+}
+
+void panfrost_perfcnt_reset(struct panfrost_device *pfdev)
+{
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+
+	mutex_lock(&perfcnt->lock);
+	if (perfcnt->user) {
+		panfrost_perfcnt_gpu_disable(pfdev);
+		panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
+		perfcnt->hw_reset_happened = true;
+	}
+	mutex_unlock(&perfcnt->lock);
 }
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
index 8bbcf5f5fb33..8b9bc704b634 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
@@ -14,5 +14,6 @@ int panfrost_ioctl_perfcnt_enable(struct drm_device *dev, void *data,
 				  struct drm_file *file_priv);
 int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
 				struct drm_file *file_priv);
+void panfrost_perfcnt_reset(struct panfrost_device *pfdev);
 
 #endif

-- 
2.53.0

Claude review: drm/panfrost: Make reset sequence deal with an active HWPerf session

[Claude Code Review Bot]

Patch Review

**Status: Reasonable approach with one ordering concern.**

The design is sound: during a GPU reset, if perfcnt is active, do the `panfrost_mmu_as_put()` before `panfrost_mmu_reset()` zeroes the refcounts, and set a flag so the later disable path skips the (now-redundant) put. This prevents the WARN at `panfrost_mmu.c:336`:
```c
atomic_dec(&mmu->as_count);
WARN_ON(atomic_read(&mmu->as_count) < 0);
```

**Refactoring of GPU register writes** into `panfrost_perfcnt_gpu_disable()` is clean and eliminates duplication across init, fini, disable, and the new reset path. One note: the original `panfrost_perfcnt_disable_locked` wrote the enable registers first and the CFG mode-off last, while `panfrost_perfcnt_gpu_disable` writes CFG mode-off first:
```c
+static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfdev)
+{
+	gpu_write(pfdev, GPU_PERFCNT_CFG,
+		  GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
+	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0);
```
This order reversal is fine for disable (clearing config then enables is no worse than the reverse), but it's worth noting intentionally.

**Ordering concern**: `panfrost_perfcnt_reset()` is called after `panfrost_gpu_soft_reset()` but *before* `panfrost_gpu_power_on()`:
```c
panfrost_gpu_soft_reset(pfdev);
panfrost_perfcnt_reset(pfdev);   // <-- here
panfrost_gpu_power_on(pfdev);
```
Inside `panfrost_perfcnt_reset()`, `panfrost_perfcnt_gpu_disable()` writes to GPU registers and `panfrost_mmu_as_put()` may write to MMU registers -- all on a soft-reset GPU that hasn't been powered back on. The register writes are likely harmless (they'll be overwritten by the subsequent `panfrost_mmu_reset`), but it would be cleaner to skip the `panfrost_perfcnt_gpu_disable()` call here since the GPU was just soft-reset and the registers are already in their default state. Only the `panfrost_mmu_as_put()` and the flag-set are needed in the reset path.

**EAGAIN semantics**: Returning `-EAGAIN` from `panfrost_ioctl_perfcnt_dump` is a reasonable choice, signaling the user to retry after a disable/enable cycle. The comment explains this well.

---

---
Generated by Claude Code Patch Reviewer

[PATCH 3/4] drm/panfrost: Prevent division by 0

[Adrián Larumbe]

When updating and debug-printing devfreq stats, in the very unlikely
offchance that total device time is less than 100 ns, clamp it to 0 to
avoid division by 0.

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

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index b51c30778811..e34de47bf43b 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -78,7 +78,8 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
 
 	dev_dbg(pfdev->base.dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
 		status->busy_time, status->total_time,
-		status->busy_time / (status->total_time / 100),
+		status->total_time >= 100 ?
+			status->busy_time / (status->total_time / 100) : 0,
 		status->current_frequency / 1000 / 1000);
 
 	return 0;

-- 
2.53.0

Claude review: drm/panfrost: Prevent division by 0

[Claude Code Review Bot]

Patch Review

**Status: Correct but very minor scope.**

The fix:
```c
-		status->busy_time / (status->total_time / 100),
+		status->total_time >= 100 ?
+			status->busy_time / (status->total_time / 100) : 0,
```

This is in a `dev_dbg()` call, so it only fires with dynamic debug enabled. The division by zero can occur when `total_time` is an `unsigned long` and `total_time / 100` truncates to 0 for values < 100. The check is correct.

**Nit**: The commit message says "clamp it to 0" when it's really "clamp the percentage display to 0" -- the total_time itself isn't clamped. Also, the 100 ns threshold mentioned in the commit message may confuse readers since `total_time` units aren't necessarily nanoseconds (they depend on ktime conversion).

---

---
Generated by Claude Code Patch Reviewer

[PATCH 4/4] drm/panfrost: Fix RPM so device suspends when no jobs are in flight

[Adrián Larumbe]

Right now, because of an apparent bug in the management of runtime PM,
the device remains powered on at all times. This was causing three
different issues:

- Because MMU interrupts are only enabled when the device resets, the very
first job targeting the tiler heap would always time out, as a device reset
is only performed during a runtime resume operation.
- Devfreq OPP transition notifications would be printed all the time, even
when the device is meant to be idle, which polute the kernel ring buffer.
- PM refcnt would eventually underflow.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index cb8e5015847f..68b831add0bb 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -986,9 +986,8 @@ static int panfrost_probe(struct platform_device *pdev)
 		goto err_out0;
 	}
 
-	pm_runtime_set_active(pfdev->base.dev);
-	pm_runtime_mark_last_busy(pfdev->base.dev);
 	pm_runtime_enable(pfdev->base.dev);
+	pm_runtime_resume_and_get(pfdev->base.dev);
 	pm_runtime_set_autosuspend_delay(pfdev->base.dev, 50); /* ~3 frames */
 	pm_runtime_use_autosuspend(pfdev->base.dev);
 
@@ -1004,6 +1003,8 @@ static int panfrost_probe(struct platform_device *pdev)
 	if (err)
 		goto err_out2;
 
+	pm_runtime_put_sync_suspend(pfdev->base.dev);
+
 	return 0;
 
 err_out2:
@@ -1024,8 +1025,9 @@ static void panfrost_remove(struct platform_device *pdev)
 	panfrost_gem_shrinker_cleanup(&pfdev->base);
 
 	pm_runtime_get_sync(pfdev->base.dev);
-	pm_runtime_disable(pfdev->base.dev);
 	panfrost_device_fini(pfdev);
+	pm_runtime_put_sync(pfdev->base.dev);
+	pm_runtime_disable(pfdev->base.dev);
 	pm_runtime_set_suspended(pfdev->base.dev);
 }
 

-- 
2.53.0

Claude review: drm/panfrost: Fix RPM so device suspends when no jobs are in flight

[Claude Code Review Bot]

Patch Review

**Status: Right direction, but has implementation issues that should be fixed.**

The core problem is well-identified: the old code uses `pm_runtime_set_active()` which tells the PM core the device is already active without actually going through the runtime-resume path. Since `panfrost_device_runtime_resume()` calls `panfrost_device_reset()` which calls `panfrost_mmu_reset()` (which enables MMU interrupts via `mmu_write(pfdev, MMU_INT_MASK, ~0)`), skipping the resume path means MMU interrupts are never enabled on first boot.

**Issue 1 -- Missing error check**: `pm_runtime_resume_and_get()` can fail and returns an int. The return value is not checked:
```c
pm_runtime_enable(pfdev->base.dev);
pm_runtime_resume_and_get(pfdev->base.dev);  // return value ignored
```
This should be:
```c
err = pm_runtime_resume_and_get(pfdev->base.dev);
if (err)
    goto err_out_pm;
```

**Issue 2 -- Double initialization**: `panfrost_device_init()` already calls `panfrost_gpu_init()`, `panfrost_mmu_init()`, `panfrost_jm_init()`, etc. Then `pm_runtime_resume_and_get()` triggers `panfrost_device_runtime_resume()` which calls `panfrost_device_reset()`, re-initializing the GPU, MMU, and job manager. The device gets set up twice. While this works, the real fix might be to ensure `panfrost_device_init()` enables MMU interrupts (matching what `panfrost_mmu_reset()` does), rather than relying on the side-effect of going through the runtime-resume path. The current approach is fragile because it depends on an ordering invariant that isn't documented.

**Issue 3 -- remove path `pm_runtime_put_sync`**: In the remove function:
```c
pm_runtime_get_sync(pfdev->base.dev);
panfrost_device_fini(pfdev);          // tears down GPU, MMU, perfcnt, etc.
pm_runtime_put_sync(pfdev->base.dev); // may trigger runtime_suspend callback!
pm_runtime_disable(pfdev->base.dev);
```
After `panfrost_device_fini()`, the hardware resources are torn down. `pm_runtime_put_sync()` (as opposed to `pm_runtime_put_noidle()`) will attempt to trigger the `panfrost_device_runtime_suspend` callback, which accesses `pfdev->comp->pm_features`, calls `panfrost_jm_is_idle()`, `panfrost_gpu_power_off()`, etc. -- all on finalized hardware. This should use `pm_runtime_put_noidle()` instead, or `pm_runtime_disable()` should be called before the put.

**Issue 4 -- Error path**: The error paths in probe (labels `err_out1`, `err_out2`) don't call `pm_runtime_put` to balance the `pm_runtime_resume_and_get`, so error exits would leak a PM reference:
```c
err_out2:
	drm_dev_unregister(&pfdev->base);
err_out1:
	pm_runtime_disable(pfdev->base.dev);    // no put to balance resume_and_get
	panfrost_device_fini(pfdev);
```

**Typo in commit message**: "polute" should be "pollute".

---
Generated by Claude Code Patch Reviewer

Claude review: RPM, percnt and other minor fixes for Panfrost

[Claude Code Review Bot]

Overall Series Review

Subject: RPM, percnt and other minor fixes for Panfrost
Author: =?utf-8?q?Adri=C3=A1n_Larumbe?= <adrian.larumbe@collabora.com>
Patches: 5
Reviewed: 2026-05-27T15:22:49.422144

---

This is a 4-patch series for the Panfrost DRM driver addressing four distinct issues: a BO cache-flag query bug, perfcnt behavior across GPU resets, a division-by-zero in devfreq debug logging, and a runtime PM lifecycle fix. The series ranges from straightforward correctness fixes (patches 1 and 3) to more substantial changes in reset handling and PM management (patches 2 and 4).

Patches 1, 2, and 3 look reasonable. Patch 4 has the right intent but has issues in its implementation that need addressing before merge: a missing error-check on `pm_runtime_resume_and_get()`, and `pm_runtime_put_sync()` in the remove path potentially triggering the runtime-suspend callback on already-finalized hardware.

The cover letter notes the absence of `Fixes:` and `Reported-by:` tags -- those should be added for patches 1, 2, and 4 before the final version, as they are genuine bug fixes.

---

---
Generated by Claude Code Patch Reviewer