[PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost

[PATCH v5 00/11] RPM, perfcnt 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.

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 well-known 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>
---
Changes in v5:
- Addressed more issues uncovered by Sashiko.
- Discarded two patches to avoid dealing with too many pre-existing issues at once.
- Biggest change is the re-arrangement of the start sequence, made to look like Panthor.
- Link to v4: https://patch.msgid.link/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com

Changes in v4:
- Fixed errors and issues uncovered by Sashiko in the previous revision.
- Changed the way RPM checks whether the device is initialised when resuming.
- Simplified perfcnt treatment of potential resets.
- Link to v3: https://patch.msgid.link/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com

Changes in v3:
- Applied some minor suggestions for the first couple of patches in the series.
- Moved shrinker initialisation into gem initialisation.
- Fixed RPM bugs by moving all clock and power initialisation into RPM resume.
- Added patch for reset sequence fixes and also a userspace knob to trigger it.
- Reworked perfcnt fix by having the reset sequence restore its initial state.
- Link to v2: https://patch.msgid.link/20260604-claude-fixes-v2-0-57c6bd4c1655@collabora.com

Changes in v2:
- Fixed race conditions introduced by the previous revision, in the RPM and
HWPerf commits specifically. These can be consulted at [1]
- Don't attempt to suspend the device unconditionally at the end of
probe(), and do it through autosuspend instead.
- Broke the RPM patch into one that fixes PM refcnt proper and another one
for MMU enablement at device init time.
- Moved perfcnt GPU disable helper into its own commmit. Same for
transplating shrinker initialisation and unplug.
- Added 'Fixes' and 'Reported-by' tags to all the relevant commits
- [1] https://sashiko.dev/#/patchset/20260526-claude-fixes-v1-0-16e92eaa4949%40collabora.com
- Link to v1: https://patch.msgid.link/20260526-claude-fixes-v1-0-16e92eaa4949@collabora.com

To: Boris Brezillon <boris.brezillon@collabora.com>
To: Rob Herring <robh@kernel.org>
To: Steven Price <steven.price@arm.com>
To: Adrián Larumbe <adrian.larumbe@collabora.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Faith Ekstrand <faith.ekstrand@collabora.com>
To: "Marty E. Plummer" <hanetzer@startmail.com>
To: Tomeu Vizoso <tomeu@tomeuvizoso.net>
To: Eric Anholt <eric@anholt.net>
To: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
To: Robin Murphy <robin.murphy@arm.com>
To: Philipp Zabel <p.zabel@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: Neil Armstrong <neil.armstrong@linaro.org>

---
Adrián Larumbe (11):
      drm/panfrost: Check another bo field for cache option query
      drm/panfrost: Prevent division by 0
      drm/panfrost: Remove unnecessary header file include
      drm/panfrost: Move shrinker initialization and unplug one level down
      drm/panfrost: Move all device power up and down into RPM callbacks
      drm/panfrost: Explicitly enable MMU interrupts at device init
      drm/panfrost: Add debugfs knob for manually triggering a GPU reset
      drm/panfrost: Move perfcnt GPU disable sequence into a helper
      drm/panfrost: Introduce a reset lock
      drm/panfrost: Fix races between perfcnt and reset sequence
      drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field

 drivers/gpu/drm/panfrost/panfrost_devfreq.c      |   2 +-
 drivers/gpu/drm/panfrost/panfrost_device.c       | 412 ++++++++++++++---------
 drivers/gpu/drm/panfrost/panfrost_device.h       |  12 +
 drivers/gpu/drm/panfrost/panfrost_drv.c          |  32 +-
 drivers/gpu/drm/panfrost/panfrost_gem.c          |  25 +-
 drivers/gpu/drm/panfrost/panfrost_gem.h          |   7 +-
 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c |   8 +-
 drivers/gpu/drm/panfrost/panfrost_gpu.c          |   9 +-
 drivers/gpu/drm/panfrost/panfrost_gpu.h          |   1 -
 drivers/gpu/drm/panfrost/panfrost_job.c          |   8 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.c          |  20 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.h          |   1 -
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c      | 249 +++++++++-----
 drivers/gpu/drm/panfrost/panfrost_perfcnt.h      |   2 +
 include/uapi/drm/panfrost_drm.h                  |   3 +-
 15 files changed, 495 insertions(+), 296 deletions(-)
---
base-commit: 671b7825dbfe9ea6e3ad3001003aeee0df48d1b5
change-id: 20260523-claude-fixes-82b03a134a8c

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

[PATCH v5 01/11] 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.

Reported-by: Claude <noreply@anthropic.com>
Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/88
Fixes: 62eedf1ccba5 ("drm/panfrost: Add flag to map GEM object Write-Back Cacheable")
Reviewed-by: Steven Price <steven.price@arm.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
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 784e36d72c2b..2e931fa90da0 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -661,7 +661,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.55.0

[PATCH v5 02/11] drm/panfrost: Prevent division by 0

[Adrián Larumbe]

When updating and debug-printing devfreq stats, in the very unlikely
off-chance that total device time is less than 100 ns, division by 0
would happen. Fix the divisor when relevant and rearrange operands.

Reported-by: Claude <noreply@anthropic.com>
Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/88
Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index b51c30778811..0fe81d259274 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -78,7 +78,7 @@ 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->busy_time * 100 / MAX(status->total_time, 1),
 		status->current_frequency / 1000 / 1000);
 
 	return 0;

-- 
2.55.0

[PATCH v5 03/11] drm/panfrost: Remove unnecessary header file include

[Adrián Larumbe]

None of panfrost_features.h definitions are used in panfrost_device.c

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 7fed22d555a5..d8421fd6a662 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -11,7 +11,6 @@
 
 #include "panfrost_device.h"
 #include "panfrost_devfreq.h"
-#include "panfrost_features.h"
 #include "panfrost_gem.h"
 #include "panfrost_issues.h"
 #include "panfrost_gpu.h"

-- 
2.55.0

[PATCH v5 04/11] drm/panfrost: Move shrinker initialization and unplug one level down

[Adrián Larumbe]

Since the moment we call drm_dev_register() the device should be in a
position to accept jobs, so it's best if the shrinker is already
initialized by then.

On top of that, make shrinker functions take an panfrost_device pointer
like other functions in the same sequence and rename them accordingly.

Essentially mimic the init/fini behaviour in Panthor.

On top of that, remove the config feature check, because it's unnecessary.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c       |  8 +++++++-
 drivers/gpu/drm/panfrost/panfrost_drv.c          |  6 ------
 drivers/gpu/drm/panfrost/panfrost_gem.c          | 25 ++++++++++++++----------
 drivers/gpu/drm/panfrost/panfrost_gem.h          |  7 ++++---
 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c |  8 ++------
 5 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index d8421fd6a662..74992deb0b3a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -281,9 +281,14 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 	if (err)
 		goto out_job;
 
-	panfrost_gem_init(pfdev);
+	err = panfrost_gem_init(pfdev);
+	if (err)
+		goto out_perfcnt;
 
 	return 0;
+
+out_perfcnt:
+	panfrost_perfcnt_fini(pfdev);
 out_job:
 	panfrost_jm_fini(pfdev);
 out_mmu:
@@ -305,6 +310,7 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 
 void panfrost_device_fini(struct panfrost_device *pfdev)
 {
+	panfrost_gem_fini(pfdev);
 	panfrost_perfcnt_fini(pfdev);
 	panfrost_jm_fini(pfdev);
 	panfrost_mmu_fini(pfdev);
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 2e931fa90da0..331a3bd5b98c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -1002,14 +1002,9 @@ static int panfrost_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto err_out1;
 
-	err = panfrost_gem_shrinker_init(&pfdev->base);
-	if (err)
-		goto err_out2;
 
 	return 0;
 
-err_out2:
-	drm_dev_unregister(&pfdev->base);
 err_out1:
 	pm_runtime_disable(pfdev->base.dev);
 	panfrost_device_fini(pfdev);
@@ -1023,7 +1018,6 @@ static void panfrost_remove(struct platform_device *pdev)
 	struct panfrost_device *pfdev = platform_get_drvdata(pdev);
 
 	drm_dev_unregister(&pfdev->base);
-	panfrost_gem_shrinker_cleanup(&pfdev->base);
 
 	pm_runtime_get_sync(pfdev->base.dev);
 	pm_runtime_disable(pfdev->base.dev);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 3a7fce428898..7b993a089af2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -15,20 +15,25 @@
 #include "panfrost_gem.h"
 #include "panfrost_mmu.h"
 
-void panfrost_gem_init(struct panfrost_device *pfdev)
+int panfrost_gem_init(struct panfrost_device *pfdev)
 {
 	int err;
 
-	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
-	    !panfrost_transparent_hugepage)
-		return;
+	if (panfrost_transparent_hugepage) {
+		err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
+		if (drm_gem_get_huge_mnt(&pfdev->base))
+			drm_info(&pfdev->base, "Using Transparent Hugepage\n");
+		else if (err)
+			drm_warn(&pfdev->base,
+				 "Can't use Transparent Hugepage (%d)\n", err);
+	}
 
-	err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
-	if (drm_gem_get_huge_mnt(&pfdev->base))
-		drm_info(&pfdev->base, "Using Transparent Hugepage\n");
-	else if (err)
-		drm_warn(&pfdev->base, "Can't use Transparent Hugepage (%d)\n",
-			 err);
+	return panfrost_gem_shrinker_init(pfdev);
+}
+
+void panfrost_gem_fini(struct panfrost_device *pfdev)
+{
+	panfrost_gem_shrinker_fini(pfdev);
 }
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index 79d4377019e9..5c823cdbd980 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -129,7 +129,8 @@ drm_mm_node_to_panfrost_mapping(struct drm_mm_node *node)
 	return container_of(node, struct panfrost_gem_mapping, mmnode);
 }
 
-void panfrost_gem_init(struct panfrost_device *pfdev);
+int panfrost_gem_init(struct panfrost_device *pfdev);
+void panfrost_gem_fini(struct panfrost_device *pfdev);
 
 struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
 
@@ -154,8 +155,8 @@ panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
 void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
 void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo);
 
-int panfrost_gem_shrinker_init(struct drm_device *dev);
-void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
+int panfrost_gem_shrinker_init(struct panfrost_device *pfdev);
+void panfrost_gem_shrinker_fini(struct panfrost_device *pfdev);
 
 void panfrost_gem_set_label(struct drm_gem_object *obj, const char *label);
 int panfrost_gem_sync(struct drm_gem_object *obj, u32 type,
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
index 2fe967a90bcb..fefae87535d6 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
@@ -95,10 +95,8 @@ panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
  *
  * This function registers and sets up the panfrost shrinker.
  */
-int panfrost_gem_shrinker_init(struct drm_device *dev)
+int panfrost_gem_shrinker_init(struct panfrost_device *pfdev)
 {
-	struct panfrost_device *pfdev = to_panfrost_device(dev);
-
 	pfdev->shrinker = shrinker_alloc(0, "drm-panfrost");
 	if (!pfdev->shrinker)
 		return -ENOMEM;
@@ -118,10 +116,8 @@ int panfrost_gem_shrinker_init(struct drm_device *dev)
  *
  * This function unregisters the panfrost shrinker.
  */
-void panfrost_gem_shrinker_cleanup(struct drm_device *dev)
+void panfrost_gem_shrinker_fini(struct panfrost_device *pfdev)
 {
-	struct panfrost_device *pfdev = to_panfrost_device(dev);
-
 	if (pfdev->shrinker)
 		shrinker_free(pfdev->shrinker);
 }

-- 
2.55.0

[PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks

[Adrián Larumbe]

During device probe(), failure to do a PM get() will leave the usage_count
set to 0, which is the value assigned at device creation time. That means
when the autosuspend delay expires, runtime suspend callback won't be
invoked, so the device will remain powered on forever.

On top of that, failure to call PM put() during device unplug means
Panfrost device's PM usage_count increases monotonically for every new
module reload.

The combined outcome of both of the above was that devfreq OPP transition
notifications would be printed all the time, even when no jobs are being
submitted. This quickly fills the kernel ring buffer with junk.

Even direr than that was the fact MMU interrupts are only enabled when
the device is reset, so after device probe() the very first job targeting
the tiler heap BO would always time out, because the driver's PM runtime
resume callback would not be invoked.

Fix all that by moving all GPU enabling and disabling into RPM resume and
suspend callbacks, and making sure we resume the device right before
touching any HW registers. This is done in imitation of the Panthor model.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Fixes: 635430797d3f ("drm/panfrost: Rework runtime PM initialization")
Fixes: 876b15d2c88d ("drm/panfrost: Fix module unload")
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 389 ++++++++++++++++-------------
 drivers/gpu/drm/panfrost/panfrost_device.h |   8 +
 drivers/gpu/drm/panfrost/panfrost_drv.c    |  11 +-
 drivers/gpu/drm/panfrost/panfrost_gpu.c    |   9 +-
 drivers/gpu/drm/panfrost/panfrost_gpu.h    |   1 -
 drivers/gpu/drm/panfrost/panfrost_job.c    |   7 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.c    |   9 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.h    |   1 -
 8 files changed, 236 insertions(+), 199 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 74992deb0b3a..52f4b8c6a05f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -26,11 +26,46 @@ static int panfrost_reset_init(struct panfrost_device *pfdev)
 		return PTR_ERR(pfdev->rstc);
 	}
 
-	return reset_control_deassert(pfdev->rstc);
+	return 0;
 }
 
-static void panfrost_reset_fini(struct panfrost_device *pfdev)
+static int panfrost_clk_enable_deassert_reset(struct panfrost_device *pfdev)
 {
+	int err;
+
+	err = reset_control_deassert(pfdev->rstc);
+	if (err)
+		return err;
+
+	err = clk_enable(pfdev->clock);
+	if (err)
+		goto assert_reset;
+
+	err = clk_enable(pfdev->bus_clock);
+	if (err)
+		goto disable_clock;
+
+	err = clk_enable(pfdev->bus_ace_clock);
+	if (err)
+		goto disable_bus_clock;
+
+	return 0;
+
+disable_bus_clock:
+	clk_disable(pfdev->bus_clock);
+disable_clock:
+	clk_disable(pfdev->clock);
+assert_reset:
+	reset_control_assert(pfdev->rstc);
+
+	return err;
+}
+
+static void panfrost_clk_disable_assert_reset(struct panfrost_device *pfdev)
+{
+	clk_disable(pfdev->bus_ace_clock);
+	clk_disable(pfdev->bus_clock);
+	clk_disable(pfdev->clock);
 	reset_control_assert(pfdev->rstc);
 }
 
@@ -48,7 +83,7 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
 	rate = clk_get_rate(pfdev->clock);
 	dev_info(pfdev->base.dev, "clock rate = %lu\n", rate);
 
-	err = clk_prepare_enable(pfdev->clock);
+	err = clk_prepare(pfdev->clock);
 	if (err)
 		return err;
 
@@ -57,44 +92,55 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
 		dev_err(pfdev->base.dev, "get bus_clock failed %ld\n",
 			PTR_ERR(pfdev->bus_clock));
 		err = PTR_ERR(pfdev->bus_clock);
-		goto disable_clock;
+		goto unprepare_clock;
 	}
 
 	if (pfdev->bus_clock) {
 		rate = clk_get_rate(pfdev->bus_clock);
 		dev_info(pfdev->base.dev, "bus_clock rate = %lu\n", rate);
 
-		err = clk_prepare_enable(pfdev->bus_clock);
+		err = clk_prepare(pfdev->bus_clock);
 		if (err)
-			goto disable_clock;
+			goto unprepare_clock;
 	}
 
 	pfdev->bus_ace_clock = devm_clk_get_optional(pfdev->base.dev, "bus_ace");
 	if (IS_ERR(pfdev->bus_ace_clock)) {
 		err = PTR_ERR(pfdev->bus_ace_clock);
 		dev_err(pfdev->base.dev, "get bus_ace_clock failed %d\n", err);
-		goto disable_bus_clock;
+		goto unprepare_bus_clock;
 	}
 
-	err = clk_prepare_enable(pfdev->bus_ace_clock);
+	err = clk_prepare(pfdev->bus_ace_clock);
 	if (err)
-		goto disable_bus_clock;
+		goto unprepare_bus_clock;
+
+	if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT))) {
+		err = panfrost_clk_enable_deassert_reset(pfdev);
+		if (err)
+			goto unprepare_bus_ace_clock;
+	}
 
 	return 0;
 
-disable_bus_clock:
-	clk_disable_unprepare(pfdev->bus_clock);
-disable_clock:
-	clk_disable_unprepare(pfdev->clock);
+unprepare_bus_ace_clock:
+	clk_unprepare(pfdev->bus_ace_clock);
+unprepare_bus_clock:
+	clk_unprepare(pfdev->bus_clock);
+unprepare_clock:
+	clk_unprepare(pfdev->clock);
 
 	return err;
 }
 
 static void panfrost_clk_fini(struct panfrost_device *pfdev)
 {
-	clk_disable_unprepare(pfdev->bus_ace_clock);
-	clk_disable_unprepare(pfdev->bus_clock);
-	clk_disable_unprepare(pfdev->clock);
+	if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT)))
+		panfrost_clk_disable_assert_reset(pfdev);
+
+	clk_unprepare(pfdev->bus_ace_clock);
+	clk_unprepare(pfdev->bus_clock);
+	clk_unprepare(pfdev->clock);
 }
 
 static int panfrost_regulator_init(struct panfrost_device *pfdev)
@@ -212,6 +258,127 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
 	return err;
 }
 
+static int panfrost_device_runtime_resume(struct device *dev)
+{
+	struct panfrost_device *pfdev = dev_get_drvdata(dev);
+	int ret;
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
+		ret = panfrost_clk_enable_deassert_reset(pfdev);
+		if (ret)
+			return ret;
+	}
+
+	panfrost_devfreq_resume(pfdev);
+
+	if (panfrost_device_started(pfdev))
+		panfrost_device_reset(pfdev, true);
+
+	return 0;
+}
+
+static int panfrost_device_runtime_suspend(struct device *dev)
+{
+	struct panfrost_device *pfdev = dev_get_drvdata(dev);
+
+	if (panfrost_device_started(pfdev) &&
+	    !panfrost_jm_is_idle(pfdev))
+		return -EBUSY;
+
+	panfrost_jm_suspend_irq(pfdev);
+	panfrost_mmu_suspend_irq(pfdev);
+	panfrost_gpu_suspend_irq(pfdev);
+	panfrost_gpu_power_off(pfdev);
+	panfrost_devfreq_suspend(pfdev);
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
+		panfrost_clk_disable_assert_reset(pfdev);
+
+	return 0;
+}
+
+static int panfrost_device_resume(struct device *dev)
+{
+	struct panfrost_device *pfdev = dev_get_drvdata(dev);
+	int ret;
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF)) {
+		unsigned long freq = pfdev->pfdevfreq.fast_rate;
+		struct dev_pm_opp *opp;
+
+		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
+		if (IS_ERR(opp))
+			return PTR_ERR(opp);
+		dev_pm_opp_set_opp(dev, opp);
+		dev_pm_opp_put(opp);
+	}
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
+		ret = clk_enable(pfdev->clock);
+		if (ret)
+			goto err_clk;
+
+		if (pfdev->bus_clock) {
+			ret = clk_enable(pfdev->bus_clock);
+			if (ret)
+				goto err_bus_clk;
+		}
+	}
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		goto err_resume;
+
+	return 0;
+
+err_resume:
+	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS) && pfdev->bus_clock)
+		clk_disable(pfdev->bus_clock);
+err_bus_clk:
+	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS))
+		clk_disable(pfdev->clock);
+err_clk:
+	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
+		dev_pm_opp_set_opp(dev, NULL);
+	return ret;
+}
+
+static int panfrost_device_suspend(struct device *dev)
+{
+	struct panfrost_device *pfdev = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
+		if (pfdev->bus_clock)
+			clk_disable(pfdev->bus_clock);
+
+		clk_disable(pfdev->clock);
+	}
+
+	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
+		dev_pm_opp_set_opp(dev, NULL);
+
+	return 0;
+}
+
+EXPORT_GPL_DEV_PM_OPS(panfrost_pm_ops) = {
+	RUNTIME_PM_OPS(panfrost_device_runtime_suspend, panfrost_device_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(panfrost_device_suspend, panfrost_device_resume)
+};
+
+void panfrost_try_suspend_device(struct panfrost_device *pfdev)
+{
+	pm_runtime_put_sync_suspend(pfdev->base.dev);
+
+	/* If PM is disabled, we need to call the suspend handler manually. */
+	if (!IS_ENABLED(CONFIG_PM))
+		panfrost_device_runtime_suspend(pfdev->base.dev);
+}
+
 int panfrost_device_init(struct panfrost_device *pfdev)
 {
 	int err;
@@ -242,7 +409,7 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 	err = panfrost_clk_init(pfdev);
 	if (err) {
 		dev_err(pfdev->base.dev, "clk init failed %d\n", err);
-		goto out_reset;
+		goto out_pm_domain;
 	}
 
 	err = panfrost_devfreq_init(pfdev);
@@ -265,60 +432,70 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 		goto out_regulator;
 	}
 
-	err = panfrost_gpu_init(pfdev);
+	err = devm_pm_runtime_enable(pfdev->base.dev);
 	if (err)
 		goto out_regulator;
 
-	err = panfrost_mmu_init(pfdev);
+	err = pm_runtime_resume_and_get(pfdev->base.dev);
 	if (err)
-		goto out_gpu;
+		goto out_regulator;
 
-	err = panfrost_jm_init(pfdev);
+	/* If PM is disabled, we need to call panfrost_device_runtime_resume() manually. */
+	if (!IS_ENABLED(CONFIG_PM)) {
+		err = panfrost_device_runtime_resume(pfdev->base.dev);
+		if (err)
+			goto out_regulator;
+	}
+
+	err = panfrost_gpu_init(pfdev);
 	if (err)
-		goto out_mmu;
+		goto out_rpm_put;
+
+	err = panfrost_mmu_init(pfdev);
+	if (err)
+		goto out_rpm_put;
 
 	err = panfrost_perfcnt_init(pfdev);
 	if (err)
-		goto out_job;
+		goto out_rpm_put;
 
 	err = panfrost_gem_init(pfdev);
 	if (err)
 		goto out_perfcnt;
 
+	err = panfrost_jm_init(pfdev);
+	if (err)
+		goto out_gem;
+
 	return 0;
 
+out_gem:
+	panfrost_gem_fini(pfdev);
 out_perfcnt:
 	panfrost_perfcnt_fini(pfdev);
-out_job:
-	panfrost_jm_fini(pfdev);
-out_mmu:
-	panfrost_mmu_fini(pfdev);
-out_gpu:
-	panfrost_gpu_fini(pfdev);
+out_rpm_put:
+	panfrost_try_suspend_device(pfdev);
 out_regulator:
 	panfrost_regulator_fini(pfdev);
 out_devfreq:
 	panfrost_devfreq_fini(pfdev);
 out_clk:
 	panfrost_clk_fini(pfdev);
-out_reset:
-	panfrost_reset_fini(pfdev);
 out_pm_domain:
 	panfrost_pm_domain_fini(pfdev);
 	return err;
 }
 
-void panfrost_device_fini(struct panfrost_device *pfdev)
+void
+panfrost_device_fini(struct panfrost_device *pfdev)
 {
+	panfrost_jm_fini(pfdev);
 	panfrost_gem_fini(pfdev);
 	panfrost_perfcnt_fini(pfdev);
-	panfrost_jm_fini(pfdev);
-	panfrost_mmu_fini(pfdev);
-	panfrost_gpu_fini(pfdev);
-	panfrost_devfreq_fini(pfdev);
+	panfrost_try_suspend_device(pfdev);
 	panfrost_regulator_fini(pfdev);
+	panfrost_devfreq_fini(pfdev);
 	panfrost_clk_fini(pfdev);
-	panfrost_reset_fini(pfdev);
 	panfrost_pm_domain_fini(pfdev);
 }
 
@@ -425,147 +602,9 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
 void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 {
 	panfrost_gpu_soft_reset(pfdev);
-
 	panfrost_gpu_power_on(pfdev);
 	panfrost_mmu_reset(pfdev);
-
 	panfrost_jm_reset_interrupts(pfdev);
 	if (enable_job_int)
 		panfrost_jm_enable_interrupts(pfdev);
 }
-
-static int panfrost_device_runtime_resume(struct device *dev)
-{
-	struct panfrost_device *pfdev = dev_get_drvdata(dev);
-	int ret;
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
-		ret = reset_control_deassert(pfdev->rstc);
-		if (ret)
-			return ret;
-
-		ret = clk_enable(pfdev->clock);
-		if (ret)
-			goto err_clk;
-
-		ret = clk_enable(pfdev->bus_clock);
-		if (ret)
-			goto err_bus_clk;
-
-		ret = clk_enable(pfdev->bus_ace_clock);
-		if (ret)
-			goto err_bus_ace_clk;
-	}
-
-	panfrost_device_reset(pfdev, true);
-	panfrost_devfreq_resume(pfdev);
-
-	return 0;
-
-err_bus_ace_clk:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
-		clk_disable(pfdev->bus_clock);
-err_bus_clk:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
-		clk_disable(pfdev->clock);
-err_clk:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
-		reset_control_assert(pfdev->rstc);
-	return ret;
-}
-
-static int panfrost_device_runtime_suspend(struct device *dev)
-{
-	struct panfrost_device *pfdev = dev_get_drvdata(dev);
-
-	if (!panfrost_jm_is_idle(pfdev))
-		return -EBUSY;
-
-	panfrost_devfreq_suspend(pfdev);
-	panfrost_jm_suspend_irq(pfdev);
-	panfrost_mmu_suspend_irq(pfdev);
-	panfrost_gpu_suspend_irq(pfdev);
-	panfrost_gpu_power_off(pfdev);
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
-		clk_disable(pfdev->bus_ace_clock);
-		clk_disable(pfdev->bus_clock);
-		clk_disable(pfdev->clock);
-		reset_control_assert(pfdev->rstc);
-	}
-
-	return 0;
-}
-
-static int panfrost_device_resume(struct device *dev)
-{
-	struct panfrost_device *pfdev = dev_get_drvdata(dev);
-	int ret;
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF)) {
-		unsigned long freq = pfdev->pfdevfreq.fast_rate;
-		struct dev_pm_opp *opp;
-
-		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
-		if (IS_ERR(opp))
-			return PTR_ERR(opp);
-		dev_pm_opp_set_opp(dev, opp);
-		dev_pm_opp_put(opp);
-	}
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
-		ret = clk_enable(pfdev->clock);
-		if (ret)
-			goto err_clk;
-
-		if (pfdev->bus_clock) {
-			ret = clk_enable(pfdev->bus_clock);
-			if (ret)
-				goto err_bus_clk;
-		}
-	}
-
-	ret = pm_runtime_force_resume(dev);
-	if (ret)
-		goto err_resume;
-
-	return 0;
-
-err_resume:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS) && pfdev->bus_clock)
-		clk_disable(pfdev->bus_clock);
-err_bus_clk:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS))
-		clk_disable(pfdev->clock);
-err_clk:
-	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
-		dev_pm_opp_set_opp(dev, NULL);
-	return ret;
-}
-
-static int panfrost_device_suspend(struct device *dev)
-{
-	struct panfrost_device *pfdev = dev_get_drvdata(dev);
-	int ret;
-
-	ret = pm_runtime_force_suspend(dev);
-	if (ret)
-		return ret;
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
-		if (pfdev->bus_clock)
-			clk_disable(pfdev->bus_clock);
-
-		clk_disable(pfdev->clock);
-	}
-
-	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
-		dev_pm_opp_set_opp(dev, NULL);
-
-	return 0;
-}
-
-EXPORT_GPL_DEV_PM_OPS(panfrost_pm_ops) = {
-	RUNTIME_PM_OPS(panfrost_device_runtime_suspend, panfrost_device_runtime_resume, NULL)
-	SYSTEM_SLEEP_PM_OPS(panfrost_device_suspend, panfrost_device_resume)
-};
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index ec55c136b1b6..0fd33bc5b86f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -253,6 +253,8 @@ int panfrost_device_init(struct panfrost_device *pfdev);
 void panfrost_device_fini(struct panfrost_device *pfdev);
 void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
 
+void panfrost_try_suspend_device(struct panfrost_device *pfdev);
+
 extern const struct dev_pm_ops panfrost_pm_ops;
 
 enum drm_panfrost_exception_type {
@@ -342,4 +344,10 @@ panfrost_device_schedule_reset(struct panfrost_device *pfdev)
 	queue_work(pfdev->reset.wq, &pfdev->reset.work);
 }
 
+static inline bool
+panfrost_device_started(struct panfrost_device *pfdev)
+{
+	return pfdev->js;
+}
+
 #endif
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 331a3bd5b98c..8410de95e364 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -988,9 +988,6 @@ 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_set_autosuspend_delay(pfdev->base.dev, 50); /* ~3 frames */
 	pm_runtime_use_autosuspend(pfdev->base.dev);
 
@@ -1002,13 +999,12 @@ static int panfrost_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto err_out1;
 
+	pm_runtime_put_autosuspend(pfdev->base.dev);
 
 	return 0;
 
 err_out1:
-	pm_runtime_disable(pfdev->base.dev);
 	panfrost_device_fini(pfdev);
-	pm_runtime_set_suspended(pfdev->base.dev);
 err_out0:
 	return err;
 }
@@ -1019,10 +1015,9 @@ static void panfrost_remove(struct platform_device *pdev)
 
 	drm_dev_unregister(&pfdev->base);
 
-	pm_runtime_get_sync(pfdev->base.dev);
-	pm_runtime_disable(pfdev->base.dev);
+	drm_WARN_ON(&pfdev->base, pm_runtime_get_sync(pfdev->base.dev) < 0);
+
 	panfrost_device_fini(pfdev);
-	pm_runtime_set_suspended(pfdev->base.dev);
 }
 
 static ssize_t profiling_show(struct device *dev,
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index 7d555e63e21a..0a3a68b561c9 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -509,7 +509,9 @@ void panfrost_gpu_suspend_irq(struct panfrost_device *pfdev)
 	set_bit(PANFROST_COMP_BIT_GPU, pfdev->is_suspended);
 
 	gpu_write(pfdev, GPU_INT_MASK, 0);
-	synchronize_irq(pfdev->gpu_irq);
+
+	if (pfdev->gpu_irq > 0)
+		synchronize_irq(pfdev->gpu_irq);
 }
 
 int panfrost_gpu_init(struct panfrost_device *pfdev)
@@ -548,11 +550,6 @@ int panfrost_gpu_init(struct panfrost_device *pfdev)
 	return 0;
 }
 
-void panfrost_gpu_fini(struct panfrost_device *pfdev)
-{
-	panfrost_gpu_power_off(pfdev);
-}
-
 u32 panfrost_gpu_get_latest_flush_id(struct panfrost_device *pfdev)
 {
 	u32 flush_id;
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.h b/drivers/gpu/drm/panfrost/panfrost_gpu.h
index b4fef11211d5..3b39eaaa4d8f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.h
@@ -8,7 +8,6 @@
 struct panfrost_device;
 
 int panfrost_gpu_init(struct panfrost_device *pfdev);
-void panfrost_gpu_fini(struct panfrost_device *pfdev);
 
 u32 panfrost_gpu_get_latest_flush_id(struct panfrost_device *pfdev);
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 2d12b83e900a..c761379851da 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -446,7 +446,9 @@ void panfrost_jm_suspend_irq(struct panfrost_device *pfdev)
 	set_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended);
 
 	job_write(pfdev, JOB_INT_MASK, 0);
-	synchronize_irq(pfdev->js->irq);
+
+	if (pfdev->js)
+		synchronize_irq(pfdev->js->irq);
 }
 
 static void panfrost_job_handle_err(struct panfrost_device *pfdev,
@@ -869,7 +871,6 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
 	js = devm_kzalloc(pfdev->base.dev, sizeof(*js), GFP_KERNEL);
 	if (!js)
 		return -ENOMEM;
-	pfdev->js = js;
 
 	INIT_WORK(&pfdev->reset.work, panfrost_reset_work);
 	spin_lock_init(&js->job_lock);
@@ -904,6 +905,8 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
 		}
 	}
 
+	pfdev->js = js;
+
 	panfrost_jm_reset_interrupts(pfdev);
 	panfrost_jm_enable_interrupts(pfdev);
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 4a3162c3b659..3fe37dd12360 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -973,15 +973,12 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
 	return 0;
 }
 
-void panfrost_mmu_fini(struct panfrost_device *pfdev)
-{
-	mmu_write(pfdev, MMU_INT_MASK, 0);
-}
-
 void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev)
 {
 	set_bit(PANFROST_COMP_BIT_MMU, pfdev->is_suspended);
 
 	mmu_write(pfdev, MMU_INT_MASK, 0);
-	synchronize_irq(pfdev->mmu_irq);
+
+	if (pfdev->mmu_irq > 0)
+		synchronize_irq(pfdev->mmu_irq);
 }
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h
index 27c3c65ed074..f69ac4a880b3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.h
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h
@@ -13,7 +13,6 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping);
 void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping);
 
 int panfrost_mmu_init(struct panfrost_device *pfdev);
-void panfrost_mmu_fini(struct panfrost_device *pfdev);
 void panfrost_mmu_reset(struct panfrost_device *pfdev);
 void panfrost_mmu_suspend_irq(struct panfrost_device *pfdev);
 

-- 
2.55.0

[PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init

[Adrián Larumbe]

Because the device must be in a position to accept jobs between the time
drm_dev_register() is called and autosuspend first kicks in, there's a very
narrow window inbetween during which jobs targeting the tiler buffer
object would time out, since the device's PM status is 'Active', but no MMU
interrupts were enabled at device initialisation time.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Fixes: 73e467f60acd ("drm/panfrost: Consolidate reset handling")
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 3fe37dd12360..a8ce5d65825c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -336,6 +336,12 @@ void panfrost_mmu_as_put(struct panfrost_device *pfdev, struct panfrost_mmu *mmu
 	WARN_ON(atomic_read(&mmu->as_count) < 0);
 }
 
+static void panfrost_mmu_enable_interrupts(struct panfrost_device *pfdev)
+{
+	mmu_write(pfdev, MMU_INT_CLEAR, ~0);
+	mmu_write(pfdev, MMU_INT_MASK, ~0);
+}
+
 void panfrost_mmu_reset(struct panfrost_device *pfdev)
 {
 	struct panfrost_mmu *mmu, *mmu_tmp;
@@ -355,8 +361,7 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev)
 
 	spin_unlock(&pfdev->as_lock);
 
-	mmu_write(pfdev, MMU_INT_CLEAR, ~0);
-	mmu_write(pfdev, MMU_INT_MASK, ~0);
+	panfrost_mmu_enable_interrupts(pfdev);
 }
 
 static size_t get_pgsize(u64 addr, size_t size, size_t *count)
@@ -970,6 +975,8 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
 		return err;
 	}
 
+	panfrost_mmu_enable_interrupts(pfdev);
+
 	return 0;
 }
 

-- 
2.55.0

[PATCH v5 07/11] drm/panfrost: Add debugfs knob for manually triggering a GPU reset

[Adrián Larumbe]

This will be of great help when testing potential races between the GPU
reset sequence and other parts of the code accessing HW registers.

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

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 52f4b8c6a05f..5b66173c75b9 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -2,6 +2,7 @@
 /* Copyright 2018 Marty E. Plummer <hanetzer@startmail.com> */
 /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
 
+#include <linux/debugfs.h>
 #include <linux/clk.h>
 #include <linux/reset.h>
 #include <linux/platform_device.h>
@@ -608,3 +609,41 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 	if (enable_job_int)
 		panfrost_jm_enable_interrupts(pfdev);
 }
+
+#ifdef CONFIG_DEBUG_FS
+static int reset_get(void *data, u64 *val)
+{
+	struct panfrost_device *pfdev =
+		container_of(data, struct panfrost_device, base);
+
+	*val = atomic_read(&pfdev->reset.pending);
+	return 0;
+}
+
+static int reset_set(void *data, u64 val)
+{
+	struct panfrost_device *pfdev =
+		container_of(data, struct panfrost_device, base);
+	int ret;
+
+	ret = pm_runtime_get_if_in_use(pfdev->base.dev);
+
+	if (ret > 0) {
+		panfrost_device_schedule_reset(pfdev);
+		flush_work(&pfdev->reset.work);
+		pm_runtime_put(pfdev->base.dev);
+	}
+
+	return 0;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(panfrost_reset_debugfs_fops,
+			 reset_get, reset_set,
+			 "0x%08llx\n");
+
+void panfrost_reset_debugfs_init(struct drm_minor *minor)
+{
+	debugfs_create_file("reset", 0600, minor->debugfs_root,
+			    minor->dev, &panfrost_reset_debugfs_fops);
+}
+#endif // CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 0fd33bc5b86f..4bbaaaf827a5 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -350,4 +350,7 @@ panfrost_device_started(struct panfrost_device *pfdev)
 	return pfdev->js;
 }
 
+#ifdef CONFIG_DEBUG_FS
+void panfrost_reset_debugfs_init(struct drm_minor *minor);
+#endif // CONFIG_DEBUG_FS
 #endif
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 8410de95e364..958f1d36ab10 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -916,6 +916,7 @@ static void panfrost_debugfs_init(struct drm_minor *minor)
 {
 	panthor_gems_debugfs_init(minor);
 	panfrost_sched_debugfs_init(minor);
+	panfrost_reset_debugfs_init(minor);
 }
 #endif
 

-- 
2.55.0

[PATCH v5 08/11] drm/panfrost: Move perfcnt GPU disable sequence into a helper

[Adrián Larumbe]

Just for the sake of avoiding repetition.

Reviewed-by: Steven Price <steven.price@arm.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 31 ++++++++++++-----------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
index 7020c0192e18..ad1156678e91 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
@@ -35,6 +35,16 @@ struct panfrost_perfcnt {
 	struct completion dump_comp;
 };
 
+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);
+}
+
 void panfrost_perfcnt_clean_cache_done(struct panfrost_device *pfdev)
 {
 	complete(&pfdev->perfcnt->dump_comp);
@@ -193,12 +203,7 @@ 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);
@@ -327,12 +332,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 +344,5 @@ 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);
 }

-- 
2.55.0

[PATCH v5 09/11] drm/panfrost: Introduce a reset lock

[Adrián Larumbe]

So as to avoid accessing HW register while a reset is ongoing, a read/write
semaphore that envelopes the reset sequence will help driver entry points
avoid racing with it. For now, the only such racy entry point is the ioctl
that returns the current GPU timestmap.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 2 ++
 drivers/gpu/drm/panfrost/panfrost_device.h | 1 +
 drivers/gpu/drm/panfrost/panfrost_drv.c    | 9 ++++++---
 drivers/gpu/drm/panfrost/panfrost_job.c    | 1 +
 4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 5b66173c75b9..e0390b6c0d22 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -602,6 +602,8 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
 
 void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 {
+	guard(rwsem_read)(&pfdev->reset.lock);
+
 	panfrost_gpu_soft_reset(pfdev);
 	panfrost_gpu_power_on(pfdev);
 	panfrost_mmu_reset(pfdev);
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 4bbaaaf827a5..a2a68e042225 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -166,6 +166,7 @@ struct panfrost_device {
 	struct {
 		struct workqueue_struct *wq;
 		struct work_struct work;
+		struct rw_semaphore lock;
 		atomic_t pending;
 	} reset;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 958f1d36ab10..ff23b1a979bb 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -41,9 +41,12 @@ static int panfrost_ioctl_query_timestamp(struct panfrost_device *pfdev,
 	if (ret)
 		return ret;
 
-	panfrost_cycle_counter_get(pfdev);
-	*arg = panfrost_timestamp_read(pfdev);
-	panfrost_cycle_counter_put(pfdev);
+	/* We should not read timestamp register while the GPU  is being reset */
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		panfrost_cycle_counter_get(pfdev);
+		*arg = panfrost_timestamp_read(pfdev);
+		panfrost_cycle_counter_put(pfdev);
+	}
 
 	pm_runtime_put(pfdev->base.dev);
 	return 0;
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index c761379851da..9d7dafa29f19 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -874,6 +874,7 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
 
 	INIT_WORK(&pfdev->reset.work, panfrost_reset_work);
 	spin_lock_init(&js->job_lock);
+	init_rwsem(&pfdev->reset.lock);
 
 	js->irq = platform_get_irq_byname(to_platform_device(pfdev->base.dev), "job");
 	if (js->irq < 0)

-- 
2.55.0

[PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence

[Adrián Larumbe]

Formerly, the reset sequence would race with panfrost_mmu_as_put()
when tearing down a perfcnt session. On top of that, poking GPU
registers to program a perfcnt session or obtaining a dump might lead to
undefined behaviour when done at the same time a reset was ongoing.

Use the reset r/w semaphore to govern access to the hardware at reset
time. On top of that, expand the DRM uAPI for the perfcnt DUMP operation
so that userspace can be made aware of a reset having happened, because
that means counters will go back to 0 and can no longer be accumulated
to values previously kept in user space.

The new perfcnt-aware reset sequence also takes care to reestablish
perfcnt to its original configuration if there was an enabled session.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c  |   9 +-
 drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 220 ++++++++++++++++++++--------
 drivers/gpu/drm/panfrost/panfrost_perfcnt.h |   2 +
 include/uapi/drm/panfrost_drm.h             |   3 +-
 4 files changed, 171 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index e0390b6c0d22..c81d8ca67ae4 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -602,14 +602,21 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
 
 void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
 {
-	guard(rwsem_read)(&pfdev->reset.lock);
+	guard(rwsem_write)(&pfdev->reset.lock);
 
+	/* Pre-reset */
+	panfrost_perfcnt_reset(pfdev);
+
+	/* Do the actual device reset */
 	panfrost_gpu_soft_reset(pfdev);
 	panfrost_gpu_power_on(pfdev);
+
+	/* Post-reset */
 	panfrost_mmu_reset(pfdev);
 	panfrost_jm_reset_interrupts(pfdev);
 	if (enable_job_int)
 		panfrost_jm_enable_interrupts(pfdev);
+	panfrost_perfcnt_postreset(pfdev);
 }
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
index ad1156678e91..01d477f7fce0 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
@@ -11,6 +11,7 @@
 #include <drm/drm_file.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/panfrost_drm.h>
+#include <drm/drm_print.h>
 
 #include "panfrost_device.h"
 #include "panfrost_features.h"
@@ -25,14 +26,18 @@
 #define BYTES_PER_COUNTER		4
 #define BLOCKS_PER_COREGROUP		8
 #define V4_SHADERS_PER_COREGROUP	4
+#define PERFCNT_DUMP_MAX_RETRIES	5
 
 struct panfrost_perfcnt {
 	struct panfrost_gem_mapping *mapping;
+	unsigned int counterset;
 	size_t bosize;
 	void *buf;
 	struct panfrost_file_priv *user;
 	struct mutex lock;
 	struct completion dump_comp;
+	bool reset_happened;
+	bool reset_failed;
 };
 
 static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfdev)
@@ -55,25 +60,93 @@ void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev)
 	gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_CACHES);
 }
 
-static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
+static int panfrost_perfcnt_hw_enable(struct panfrost_device *pfdev)
 {
-	u64 gpuva;
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+	u32 cfg, as;
+	int ret;
+
+	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
+	if (ret < 0)
+		return ret;
+
+	as = ret;
+	cfg = GPU_PERFCNT_CFG_AS(as) |
+	      GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL);
+
+	/*
+	 * Bifrost GPUs have 2 set of counters, but we're only interested by
+	 * the first one for now.
+	 */
+	if (panfrost_model_is_bifrost(pfdev))
+		cfg |= GPU_PERFCNT_CFG_SETSEL(perfcnt->counterset);
+
+	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0xffffffff);
+	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0xffffffff);
+	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0xffffffff);
+
+	/*
+	 * Due to PRLAM-8186 we need to disable the Tiler before we enable HW
+	 * counters.
+	 */
+	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
+		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
+	else
+		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
+
+	gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
+
+	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
+		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
+
+	return 0;
+}
+
+static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev,
+					u64 *reset_happened)
+{
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+	u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT;
+	s64 retries = PERFCNT_DUMP_MAX_RETRIES;
 	int ret;
 
-	reinit_completion(&pfdev->perfcnt->dump_comp);
-	gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT;
-	gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva));
-	gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva));
-	gpu_write(pfdev, GPU_INT_CLEAR,
-		  GPU_IRQ_CLEAN_CACHES_COMPLETED |
-		  GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
-	gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE);
+dump_retry:
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		*reset_happened = perfcnt->reset_happened;
+		perfcnt->reset_happened = false;
+		if (perfcnt->reset_failed) {
+			ret = panfrost_perfcnt_hw_enable(pfdev);
+			if (ret)
+				return ret;
+			perfcnt->reset_failed = false;
+		}
+
+		reinit_completion(&pfdev->perfcnt->dump_comp);
+
+		gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva));
+		gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva));
+		gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_CLEAN_CACHES_COMPLETED |
+						GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
+		gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE);
+	}
+
 	ret = wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp,
 							msecs_to_jiffies(1000));
-	if (!ret)
-		ret = -ETIMEDOUT;
-	else if (ret > 0)
-		ret = 0;
+
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		if (ret > 0) {
+			if (perfcnt->reset_happened) {
+				if (--retries >= 0)
+					goto dump_retry;
+				else
+					ret = -EBUSY;
+			} else {
+				ret = 0;
+			}
+		} else if (!ret) {
+			ret = -ETIMEDOUT;
+		}
+	}
 
 	return ret;
 }
@@ -84,9 +157,8 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 {
 	struct panfrost_file_priv *user = file_priv->driver_priv;
 	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
-	struct iosys_map map;
 	struct drm_gem_shmem_object *bo;
-	u32 cfg, as;
+	struct iosys_map map;
 	int ret;
 
 	if (user == perfcnt->user)
@@ -119,7 +191,9 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 	ret = drm_gem_vmap(&bo->base, &map);
 	if (ret)
 		goto err_put_mapping;
+
 	perfcnt->buf = map.vaddr;
+	perfcnt->counterset = counterset;
 
 	panfrost_gem_internal_set_label(&bo->base, "Perfcnt sample buffer");
 
@@ -127,60 +201,47 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
 	 * Invalidate the cache and clear the counters to start from a fresh
 	 * state.
 	 */
-	reinit_completion(&pfdev->perfcnt->dump_comp);
-	gpu_write(pfdev, GPU_INT_CLEAR,
-		  GPU_IRQ_CLEAN_CACHES_COMPLETED |
-		  GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
-	gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
-	gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_INV_CACHES);
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		reinit_completion(&pfdev->perfcnt->dump_comp);
+		gpu_write(pfdev, GPU_INT_CLEAR,
+			  GPU_IRQ_CLEAN_CACHES_COMPLETED |
+			  GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
+		gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
+		gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_INV_CACHES);
+		perfcnt->reset_happened = false;
+		perfcnt->user = user;
+	}
+
+	/*
+	 * If a reset happens during the wait for the IRQ notification that caches
+	 * are clean and invalidated, then we know the reset sequence did the job
+	 * for us, even if it takes long enough for the completion to time out.
+	 */
 	ret = wait_for_completion_timeout(&pfdev->perfcnt->dump_comp,
 					  msecs_to_jiffies(1000));
-	if (!ret) {
+	if (!ret && !perfcnt->reset_happened) {
 		ret = -ETIMEDOUT;
 		goto err_vunmap;
 	}
 
-	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
-	if (ret < 0)
-		goto err_vunmap;
-
-	as = ret;
-	cfg = GPU_PERFCNT_CFG_AS(as) |
-	      GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL);
-
-	/*
-	 * Bifrost GPUs have 2 set of counters, but we're only interested by
-	 * the first one for now.
-	 */
-	if (panfrost_model_is_bifrost(pfdev))
-		cfg |= GPU_PERFCNT_CFG_SETSEL(counterset);
-
-	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0xffffffff);
-	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0xffffffff);
-	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0xffffffff);
-
-	/*
-	 * Due to PRLAM-8186 we need to disable the Tiler before we enable HW
-	 * counters.
-	 */
-	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
-		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
-	else
-		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
-
-	gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
-
-	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
-		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		if (!perfcnt->reset_happened || perfcnt->reset_failed) {
+			ret = panfrost_perfcnt_hw_enable(pfdev);
+			if (ret)
+				goto err_vunmap;
+		}
+		perfcnt->reset_happened = false;
+		perfcnt->reset_failed = false;
+	}
 
 	/* The BO ref is retained by the mapping. */
 	drm_gem_object_put(&bo->base);
 
-	perfcnt->user = user;
-
 	return 0;
 
 err_vunmap:
+	scoped_guard(rwsem_read, &pfdev->reset.lock)
+		perfcnt->user = NULL;
 	drm_gem_vunmap(&bo->base, &map);
 err_put_mapping:
 	panfrost_gem_mapping_put(perfcnt->mapping);
@@ -203,13 +264,15 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
 	if (user != perfcnt->user)
 		return -EINVAL;
 
-	panfrost_perfcnt_gpu_disable(pfdev);
+	scoped_guard(rwsem_read, &pfdev->reset.lock) {
+		panfrost_perfcnt_gpu_disable(pfdev);
+		panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
+		perfcnt->user = NULL;
+	}
 
-	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);
 	panfrost_gem_mapping_put(perfcnt->mapping);
 	perfcnt->mapping = NULL;
 	pm_runtime_put_autosuspend(pfdev->base.dev);
@@ -263,7 +326,7 @@ int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	ret = panfrost_perfcnt_dump_locked(pfdev);
+	ret = panfrost_perfcnt_dump_locked(pfdev, &req->hw_reset);
 	if (ret)
 		goto out;
 
@@ -346,3 +409,38 @@ void panfrost_perfcnt_fini(struct panfrost_device *pfdev)
 	/* Disable everything before leaving. */
 	panfrost_perfcnt_gpu_disable(pfdev);
 }
+
+void panfrost_perfcnt_reset(struct panfrost_device *pfdev)
+{
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+
+	if (drm_WARN_ON(&pfdev->base, !perfcnt))
+		return;
+
+	lockdep_assert_held(&pfdev->reset.lock);
+
+	if (!perfcnt->user)
+		return;
+
+	perfcnt->reset_happened = true;
+	complete(&perfcnt->dump_comp);
+	panfrost_perfcnt_gpu_disable(pfdev);
+}
+
+void panfrost_perfcnt_postreset(struct panfrost_device *pfdev)
+{
+	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
+	int ret;
+
+	if (drm_WARN_ON(&pfdev->base, !perfcnt))
+		return;
+
+	lockdep_assert_held(&pfdev->reset.lock);
+
+	if (!perfcnt->user)
+		return;
+
+	ret = panfrost_perfcnt_hw_enable(pfdev);
+	if (ret)
+		perfcnt->reset_failed = true;
+}
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
index 8bbcf5f5fb33..e14e760641fd 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
@@ -14,5 +14,7 @@ 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);
+void panfrost_perfcnt_postreset(struct panfrost_device *pfdev);
 
 #endif
diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
index 50d5337f35ef..3bbf9220103d 100644
--- a/include/uapi/drm/panfrost_drm.h
+++ b/include/uapi/drm/panfrost_drm.h
@@ -47,7 +47,7 @@ extern "C" {
  * them for anything but debugging purpose.
  */
 #define DRM_IOCTL_PANFROST_PERFCNT_ENABLE	DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_ENABLE, struct drm_panfrost_perfcnt_enable)
-#define DRM_IOCTL_PANFROST_PERFCNT_DUMP		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct drm_panfrost_perfcnt_dump)
+#define DRM_IOCTL_PANFROST_PERFCNT_DUMP		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct drm_panfrost_perfcnt_dump)
 
 #define PANFROST_JD_REQ_FS (1 << 0)
 #define PANFROST_JD_REQ_CYCLE_COUNT (1 << 1)
@@ -272,6 +272,7 @@ struct drm_panfrost_perfcnt_enable {
 
 struct drm_panfrost_perfcnt_dump {
 	__u64 buf_ptr;
+	__u64 hw_reset;
 };
 
 /* madvise provides a way to tell the kernel in case a buffers contents

-- 
2.55.0

[PATCH v5 11/11] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field

[Adrián Larumbe]

Expose expanded PERFCNT_DUMP IOCTL interface to user mode.

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

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index ff23b1a979bb..2949ab3ca055 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -936,6 +936,7 @@ static void panfrost_debugfs_init(struct drm_minor *minor)
  * - 1.6 - adds PANFROST_BO_MAP_WB, PANFROST_IOCTL_SYNC_BO,
  *	   PANFROST_IOCTL_QUERY_BO_INFO and
  *	   DRM_PANFROST_PARAM_SELECTED_COHERENCY
+ * - 1.7 - adds PERFCNT_DUMP req hw_reset field
  */
 static const struct drm_driver panfrost_drm_driver = {
 	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,
@@ -948,7 +949,7 @@ static const struct drm_driver panfrost_drm_driver = {
 	.name			= "panfrost",
 	.desc			= "panfrost DRM",
 	.major			= 1,
-	.minor			= 6,
+	.minor			= 7,
 
 	.gem_create_object	= panfrost_gem_create_object,
 	.gem_prime_import	= panfrost_gem_prime_import,

-- 
2.55.0

Re: [PATCH v5 07/11] drm/panfrost: Add debugfs knob for manually triggering a GPU reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The debugfs reset knob silently ignores manual reset requests if the GPU is idle or if the kernel is built without CONFIG_PM, returning success without performing the requested reset.
--

--- Patch [7]: [PATCH v5 07/11] drm/panfrost: Add debugfs knob for manually triggering a GPU reset ---
commit b4df81709774bfc91adda2d5d8e8a0109269e143
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Add debugfs knob for manually triggering a GPU reset

This commit adds a debugfs knob to manually trigger a GPU reset. This is
intended to help test potential races between the GPU reset sequence and
other parts of the code accessing HW registers.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 52f4b8c6a05fe..5b66173c75b9b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c

[ ... ]

> +static int reset_set(void *data, u64 val)
> +{
> +	struct panfrost_device *pfdev =
> +		container_of(data, struct panfrost_device, base);
> +	int ret;
> +
> +	ret = pm_runtime_get_if_in_use(pfdev->base.dev);
> +
> +	if (ret > 0) {
> +		panfrost_device_schedule_reset(pfdev);
> +		flush_work(&pfdev->reset.work);
> +		pm_runtime_put(pfdev->base.dev);
> +	}
> +
> +	return 0;

[Severity: Medium]
Will this silently ignore the manual reset request when the GPU is idle or if
CONFIG_PM is disabled?

If pm_runtime_get_if_in_use() returns <= 0, such as 0 when the device is idle
or -EINVAL when the kernel is built without CONFIG_PM, the reset is bypassed.
However, the function still returns 0 (success).

This could cause test scripts writing to this debugfs file to become flaky or
unreliable, as they will falsely assume the GPU was reset. Would it be better
to return an error code when the reset is skipped?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-claude-fixes-v5-0-3d692c9e98c2@collabora.com?part=7

Re: [PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Removing the explicit `pm_runtime_disable()` call in `panfrost_remove()` leaves PM runtime active while the driver tears down software structures and hardware abstractions. Because `devm_pm_runtime_enable()` postpones disabling until after `remove()` returns, a concurrent sysfs write (e.g., `echo on > power/control`) can trigger `panfrost_device_runtime_resume()` on a partially destroyed device, causing use-after-free or null pointer dereferences.
- [High] The patch moves hardware teardown into `panfrost_device_runtime_suspend()` and relies on `pm_runtime_put_sync_suspend()` in `panfrost_try_suspend_device()` to execute it during device removal. However, if the PM usage count is > 0 (e.g., userspace held a sysfs power reference via `power/control = on`), the device will not actually suspend. The driver then proceeds to unconditionally unprepare clocks (`clk_unprepare()`) and regulators in `panfrost_device_fini()`, even though they are still enabled and powering active hardware.

Pre-existing issues:
- [High] In `panfrost_remove()`, the driver calls `pm_runtime_get_sync()` to wake the hardware for teardown. If this fails, the hardware remains suspended (clocks and power domains off). The driver warns but unconditionally continues to `panfrost_device_fini()`, which invokes `panfrost_jm_fini()` and performs an MMIO write (`job_write(pfdev, JOB_INT_MASK, 0)`). Writing to unpowered/unclocked MMIO registers triggers a synchronous external abort on ARM.
--

--- Patch [5]: [PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks ---
commit 01a89b3f3fd9da2c5ff706aa75efcef9f9333b91
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Move all device power up and down into RPM callbacks

This commit moves all GPU enabling and disabling into runtime PM resume and
suspend callbacks to ensure the device is resumed before touching registers.
It also replaces explicit pm_runtime_disable() calls during module unload
with devres management.

> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -212,6 +258,127 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
[ ... ]
> +void panfrost_try_suspend_device(struct panfrost_device *pfdev)
> +{
> +	pm_runtime_put_sync_suspend(pfdev->base.dev);

[Severity: High]
If userspace holds a power reference (for example, by setting power/control
to "on" via sysfs), the PM usage count will be greater than zero and the
device will not actually suspend here.

If the suspend silently aborts, the driver will proceed to
panfrost_device_fini() where panfrost_clk_fini() unconditionally calls
clk_unprepare():

panfrost_device_fini() ->
  panfrost_clk_fini()
    clk_unprepare(pfdev->bus_ace_clock);
    clk_unprepare(pfdev->bus_clock);
    clk_unprepare(pfdev->clock);

Does this leave the hardware abruptly deprived of prepared clocks and
regulators while it is still enabled?

> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -1015,10 +1015,9 @@ static void panfrost_remove(struct platform_device *pdev)
>  
>  	drm_dev_unregister(&pfdev->base);
>  
> -	pm_runtime_get_sync(pfdev->base.dev);
> -	pm_runtime_disable(pfdev->base.dev);
> +	drm_WARN_ON(&pfdev->base, pm_runtime_get_sync(pfdev->base.dev) < 0);
> +

[Severity: High]
This is a pre-existing issue, but if pm_runtime_get_sync() fails, the
hardware will remain suspended with its clocks and power domains off.

Because execution unconditionally continues to panfrost_device_fini(),
panfrost_jm_fini() will perform an MMIO write to JOB_INT_MASK:

panfrost_remove() ->
  panfrost_device_fini() ->
    panfrost_jm_fini()
       job_write(pfdev, JOB_INT_MASK, 0);

Writing to unpowered or unclocked MMIO registers usually triggers a
synchronous external abort on ARM. Should the teardown path be skipped or
handled differently if the resume fails?

>  	panfrost_device_fini(pfdev);
> -	pm_runtime_set_suspended(pfdev->base.dev);
>  }

[Severity: High]
Can removing the explicit pm_runtime_disable() call here cause a
use-after-free during teardown?

Since devm_pm_runtime_enable() postpones disabling PM runtime until after
the remove() function returns, PM runtime remains active while
panfrost_device_fini() destroys software structures and hardware abstractions.

Could a concurrent sysfs write (like echoing "on" to power/control) trigger
panfrost_device_runtime_resume() on a partially destroyed device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-claude-fixes-v5-0-3d692c9e98c2@collabora.com?part=5

Re: [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `synchronize_irq()` in `panfrost_mmu_fini()` allows the MMU IRQ handler thread to execute concurrently with downstream teardown steps, potentially causing a kernel panic.
- [High] Writing to `MMU_INT_MASK` before clearing `as_faulty_mask` leaves the Address Space (AS) interrupt masked.
--

--- Patch [6]: [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init ---
commit d0741154918a6f24b94515670ed05d32bd8b695a
Author: Adrián Larumbe <adrian.larumbe@collabora.com>
Subject: drm/panfrost: Explicitly enable MMU interrupts at device init

This commit explicitly enables MMU interrupts at device initialization.
Because the device must be in a position to accept jobs between the time
drm_dev_register() is called and autosuspend first kicks in, there's a very
narrow window where jobs could time out since the device's PM status is
active but no MMU interrupts were enabled.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 3fe37dd123609..a8ce5d65825c9 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c

[ ... ]

> @@ -336,6 +336,12 @@ void panfrost_mmu_as_put(struct panfrost_device *pfdev, struct panfrost_mmu *mmu
>  	WARN_ON(atomic_read(&mmu->as_count) < 0);
>  }
>  
> +static void panfrost_mmu_enable_interrupts(struct panfrost_device *pfdev)
> +{
> +	mmu_write(pfdev, MMU_INT_CLEAR, ~0);
> +	mmu_write(pfdev, MMU_INT_MASK, ~0);

[Severity: High]
This isn't a bug introduced by this patch, but is there an issue with how
MMU_INT_MASK is programmed when re-acquiring a faulty address space?

Looking at panfrost_mmu_as_get() in the same file:

panfrost_mmu_as_get() {
	...
	mmu_write(pfdev, MMU_INT_MASK, ~pfdev->as_faulty_mask);
	pfdev->as_faulty_mask &= ~mask;
	...
}

Because the inverted faulty mask is written to hardware before the bit is
cleared from pfdev->as_faulty_mask in software, does this program the
hardware with a 0 (masked) for that address space?

If a workload causes a page fault (making the address space faulty) and then
attempts to reuse it, will the required interrupt remain masked, causing jobs
to time out and hang indefinitely?

> +}
> +

[ ... ]

> @@ -970,6 +975,8 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>  		return err;
>  	}
>  
> +	panfrost_mmu_enable_interrupts(pfdev);

[Severity: High]
This is a pre-existing issue, but does enabling the interrupts earlier in
probe expand a race condition during teardown?

If panfrost_device_init() fails after panfrost_mmu_init() (or during
driver unbind), panfrost_mmu_fini() is called:

panfrost_mmu_fini() {
	mmu_write(pfdev, MMU_INT_MASK, 0);
}

This masks the interrupt but doesn't call synchronize_irq().

Subsequently, the downstream teardown disables the clocks:

panfrost_device_fini() {
	panfrost_mmu_fini(pfdev);
	panfrost_gpu_fini(pfdev);
	panfrost_devfreq_fini(pfdev);
	panfrost_regulator_fini(pfdev);
	panfrost_clk_fini(pfdev);
}

If panfrost_mmu_irq_handler_thread() is still running concurrently after
the clocks are disabled, could this lead to a synchronous external abort
or kernel panic when accessing memory-mapped GPU registers?

> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-claude-fixes-v5-0-3d692c9e98c2@collabora.com?part=6

Re: [PATCH v5 09/11] drm/panfrost: Introduce a reset lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The GPU reset path acquires the read/write semaphore in shared (read) mode instead of exclusive (write) mode, failing to provide mutual exclusion against driver entry points.
--

--- Patch [9]: [PATCH v5 09/11] drm/panfrost: Introduce a reset lock ---
commit 4a244ba91270b92c6bb1ef0025f7f540dbdc5eff
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: envelope GPU reset sequence with rw_semaphore

This commit introduces a read/write semaphore to prevent driver entry
points from accessing hardware registers while a GPU reset is ongoing.
The query timestamp ioctl is updated to hold this semaphore during
execution.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 5b66173c75b9..e0390b6c0d22 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -602,6 +602,8 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>  
>  void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  {
> +	guard(rwsem_read)(&pfdev->reset.lock);

[Severity: High]
Does acquiring this lock as a shared reader (rwsem_read) instead of an
exclusive writer (rwsem_write) fail to provide mutual exclusion?

Since the protected ioctl path in panfrost_ioctl_query_timestamp() also
acquires the lock in read mode, won't both paths execute concurrently?

This could allow an unprivileged userspace program calling the timestamp
ioctl to access hardware registers while the GPU reset is ongoing, which
defeats the intended mutual exclusion.

> +
>  	panfrost_gpu_soft_reset(pfdev);
>  	panfrost_gpu_power_on(pfdev);
>  	panfrost_mmu_reset(pfdev);

[ ... ]

> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 958f1d36ab10..ff23b1a979bb 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -41,9 +41,12 @@ static int panfrost_ioctl_query_timestamp(struct panfrost_device *pfdev,
>  	if (ret)
>  		return ret;
>  
> -	panfrost_cycle_counter_get(pfdev);
> -	*arg = panfrost_timestamp_read(pfdev);
> -	panfrost_cycle_counter_put(pfdev);
> +	/* We should not read timestamp register while the GPU  is being reset */
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {

[Severity: High]
As noted above, since both the reset path and this ioctl path acquire the
lock in read mode, will this scoped_guard fail to wait for an ongoing reset
to complete?

> +		panfrost_cycle_counter_get(pfdev);
> +		*arg = panfrost_timestamp_read(pfdev);
> +		panfrost_cycle_counter_put(pfdev);
> +	}
>  
>  	pm_runtime_put(pfdev->base.dev);
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-claude-fixes-v5-0-3d692c9e98c2@collabora.com?part=9

Re: [PATCH v5 04/11] drm/panfrost: Move shrinker initialization and unplug one level down

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Removing the `IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)` check causes a build failure (undefined reference to `panfrost_transparent_hugepage`) when `CONFIG_TRANSPARENT_HUGEPAGE` is disabled.
--

--- Patch [4]: [PATCH v5 04/11] drm/panfrost: Move shrinker initialization and unplug one level down ---
commit a1e5fed49d6d030d377938e3c3e5a1b2314d6290
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Move shrinker initialization and unplug one level down

Since the moment we call drm_dev_register() the device should be in a
position to accept jobs, so it's best if the shrinker is already
initialized by then.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 3a7fce4288987..7b993a089af2d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -15,20 +15,25 @@
>  #include "panfrost_gem.h"
>  #include "panfrost_mmu.h"
>  
> -void panfrost_gem_init(struct panfrost_device *pfdev)
> +int panfrost_gem_init(struct panfrost_device *pfdev)
>  {
>  	int err;
>  
> -	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
> -	    !panfrost_transparent_hugepage)
> -		return;
> +	if (panfrost_transparent_hugepage) {
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
Will this cause a build failure when CONFIG_TRANSPARENT_HUGEPAGE is disabled?

Looking at drivers/gpu/drm/panfrost/panfrost_drv.c,
panfrost_transparent_hugepage is defined under an #ifdef:

#ifdef CONFIG_TRANSPARENT_HUGEPAGE
bool panfrost_transparent_hugepage = true;
module_param_named(transparent_hugepage, panfrost_transparent_hugepage, bool, 0400);
MODULE_PARM_DESC(transparent_hugepage, "Use a dedicated tmpfs mount point with Transparent Hugepage enabled (true = default)");
#endif

Without the IS_ENABLED() check, evaluating panfrost_transparent_hugepage here
could result in an undefined reference error during linking if the config is
disabled.

> +		err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
> +		if (drm_gem_get_huge_mnt(&pfdev->base))
> +			drm_info(&pfdev->base, "Using Transparent Hugepage\n");
> +		else if (err)
> +			drm_warn(&pfdev->base,
> +				 "Can't use Transparent Hugepage (%d)\n", err);
> +	}
>  
> -	err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
> -	if (drm_gem_get_huge_mnt(&pfdev->base))
> -		drm_info(&pfdev->base, "Using Transparent Hugepage\n");
> -	else if (err)
> -		drm_warn(&pfdev->base, "Can't use Transparent Hugepage (%d)\n",
> -			 err);
> +	return panfrost_gem_shrinker_init(pfdev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-claude-fixes-v5-0-3d692c9e98c2@collabora.com?part=4

Re: [PATCH v5 11/11] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete patch falsely advertises new UAPI feature (hw_reset) by bumping the driver minor version, but omits the actual UAPI and driver implementations.
--

--- Patch [11]: [PATCH v5 11/11] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field ---
commit 05a98b7146163b4b69a5fa2ced180f71b22a96f4
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

Expose expanded PERFCNT_DUMP IOCTL interface to user mode.

This commit bumps the panfrost driver minor version to 7 and adds a comment
indicating the addition of a hw_reset field to the PERFCNT_DUMP ioctl.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index ff23b1a979bb..2949ab3ca055 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -936,6 +936,7 @@ static void panfrost_debugfs_init(struct drm_minor *minor)
>   * - 1.6 - adds PANFROST_BO_MAP_WB, PANFROST_IOCTL_SYNC_BO,
>   *	   PANFROST_IOCTL_QUERY_BO_INFO and
>   *	   DRM_PANFROST_PARAM_SELECTED_COHERENCY
> + * - 1.7 - adds PERFCNT_DUMP req hw_reset field
>   */
>  static const struct drm_driver panfrost_drm_driver = {
>  	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,
> @@ -948,7 +949,7 @@ static const struct drm_driver panfrost_drm_driver = {
>  	.name			= "panfrost",
>  	.desc			= "panfrost DRM",
>  	.major			= 1,
> -	.minor			= 6,
> +	.minor			= 7,

[Severity: Medium]
Is this patch missing the corresponding UAPI and implementation changes?

The comment and version bump explicitly advertise the new hw_reset field to
userspace, but the modifications to struct drm_panfrost_perfcnt_dump in the
UAPI header and the actual driver logic in panfrost_ioctl_perfcnt_dump() 
appear to be omitted.

Userspace checking the minor version might falsely believe the feature is
supported, leading to IOCTL failures or ignored fields. Should the version
bump occur together with the feature implementation in the same patch to
avoid intermediate breakage?

>  
>  	.gem_create_object	= panfrost_gem_create_object,
>  	.gem_prime_import	= panfrost_gem_prime_import,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-claude-fixes-v5-0-3d692c9e98c2@collabora.com?part=11

Re: [PATCH v5 02/11] drm/panfrost: Prevent division by 0

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Multiplying `status->busy_time` by 100 introduces a 32-bit integer overflow on 32-bit architectures, resulting in wildly incorrect GPU load percentages in the debug log.
- [Low] The use of the type-unsafe `MAX()` macro violates kernel coding style guidelines.
--

--- Patch [2]: [PATCH v5 02/11] drm/panfrost: Prevent division by 0 ---
commit 40a64804964495fc5a5f268bdf1f61ec7215f8a4
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Prevent division by 0

This commit attempts to fix a potential division by zero when calculating the
GPU load percentage for debug logging by rearranging the operands. It changes
the calculation to multiply busy_time by 100 before dividing, and uses MAX()
to ensure the divisor is at least 1.

Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/88

> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -78,7 +78,7 @@ 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->busy_time * 100 / MAX(status->total_time, 1),

[Severity: Medium]
Does this calculation introduce a 32-bit integer overflow on 32-bit platforms
like ARMv7?

The fields busy_time and total_time in struct devfreq_dev_status are of type
unsigned long. On 32-bit architectures, unsigned long is 32 bits, with a
maximum value of 4,294,967,295.

Since time is measured in nanoseconds and the devfreq polling interval is
typically 50ms, a fully loaded GPU will have a busy_time close to 50,000,000 ns.

Multiplying this by 100 yields 5,000,000,000, which overflows the 32-bit
unsigned integer and wraps around to ~705 million. This results in the debug
log incorrectly reporting a ~14% load instead of 100% when the GPU is fully
utilized.

[Severity: Low]
This isn't a bug introduced by this patch, but does the use of MAX() here
bypass strict kernel type-checking mechanisms?

The MAX() macro defined in linux/minmax.h is explicitly documented with a
warning to use it carefully for obvious constants only, as it performs no type
checking and uses the arguments multiple times.

While this specific instance does not cause a runtime bug, using the type-safe
max(status->total_time, 1UL) or max_t(unsigned long, status->total_time, 1)
might be a better fit for standard kernel coding style guidelines.

>  		status->current_frequency / 1000 / 1000);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-claude-fixes-v5-0-3d692c9e98c2@collabora.com?part=2

Re: [PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition on timeout in panfrost_perfcnt_enable_locked() causes MMU Address Space leak and use-after-free risk.
- [High] Incorrect handling of perfcnt->reset_failed causes MMU AS reference underflows and leaks.
- [Medium] State loss of reset notification flag in panfrost_perfcnt_dump_locked() due to premature clearing.
--

--- Patch [10]: [PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence ---
Note: The format of this review has been altered to use a free-form summary
rather than inline diff quoting due to recitation restrictions.

commit 3ad1a2b0fa637dba7c60b8e41dbe3e3239112852
Author: Adrián Larumbe <adrian.larumbe@collabora.com>
Subject: drm/panfrost: Fix perfcnt reset sequence and expand dump uAPI

This commit modifies the Panfrost driver to use a reset read/write semaphore
to prevent race conditions during performance counter teardowns. It also
expands the DRM uAPI for the perfcnt dump operation to notify userspace if
a reset occurred, allowing it to handle counter resets properly.

[Severity: High]
Can this sequence cause an MMU Address Space leak and a use-after-free risk?

In panfrost_perfcnt_enable_locked() in
drivers/gpu/drm/panfrost/panfrost_perfcnt.c, the code locklessly checks
!perfcnt->reset_happened after the wait for completion times out. 

If a GPU reset occurs between this lockless check and the acquisition of
pfdev->reset.lock at the err_vunmap label, the reset worker will see an active
session because perfcnt->user is not yet NULL. The reset worker would then
call panfrost_perfcnt_postreset(), allocating an AS reference and programming
the GPU. 

When the original thread subsequently acquires the lock, it clears
perfcnt->user and frees the buffer without calling panfrost_mmu_as_put().
Would this leave the GPU hardware active and writing to an unmapped buffer?

[Severity: Medium]
Does clearing the reset flag before the wait completes successfully cause
userspace to lose the reset notification?

In panfrost_perfcnt_dump_locked() in
drivers/gpu/drm/panfrost/panfrost_perfcnt.c, perfcnt->reset_happened is
destructively cleared to false before calling
wait_for_completion_interruptible_timeout(). 

If the wait is interrupted by a signal, it returns -ERESTARTSYS. The syscall
would be restarted, but since the reset_happened flag was already cleared,
the second attempt will falsely report to userspace that no reset occurred,
leading to incorrect accumulation of performance counters.

[Severity: High]
Could the current handling of perfcnt->reset_failed cause MMU AS reference
underflows or leaks?

In panfrost_perfcnt_postreset() in
drivers/gpu/drm/panfrost/panfrost_perfcnt.c, perfcnt->reset_failed is set to
true on failure, but it does not appear to be cleared on success. 

If panfrost_mmu_reset() resets all AS counts to 0 and postreset fails,
panfrost_perfcnt_disable_locked() still unconditionally calls
panfrost_mmu_as_put() without checking if reset_failed is true. This drops
the count to -1 and triggers a WARN_ON.

Alternatively, if a subsequent reset occurs and postreset succeeds, the
failure to clear reset_failed means the next call to
panfrost_perfcnt_dump_locked() will see reset_failed as true and call
panfrost_perfcnt_hw_enable() again. This would double-acquire the AS, which
is only put once when disabled, leading to a permanent leak.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-claude-fixes-v5-0-3d692c9e98c2@collabora.com?part=10

Re: [PATCH v5 02/11] drm/panfrost: Prevent division by 0

[Boris Brezillon]

On Tue, 11 Aug 2026 22:42:11 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> When updating and debug-printing devfreq stats, in the very unlikely
> off-chance that total device time is less than 100 ns, division by 0
> would happen. Fix the divisor when relevant and rearrange operands.
> 
> Reported-by: Claude <noreply@anthropic.com>
> Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/88
> Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>

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

> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index b51c30778811..0fe81d259274 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -78,7 +78,7 @@ 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->busy_time * 100 / MAX(status->total_time, 1),
>  		status->current_frequency / 1000 / 1000);
>  
>  	return 0;
> 

Re: [PATCH v5 03/11] drm/panfrost: Remove unnecessary header file include

[Boris Brezillon]

On Tue, 11 Aug 2026 22:42:12 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> None of panfrost_features.h definitions are used in panfrost_device.c
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>

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

> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 7fed22d555a5..d8421fd6a662 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -11,7 +11,6 @@
>  
>  #include "panfrost_device.h"
>  #include "panfrost_devfreq.h"
> -#include "panfrost_features.h"
>  #include "panfrost_gem.h"
>  #include "panfrost_issues.h"
>  #include "panfrost_gpu.h"
> 

Re: [PATCH v5 04/11] drm/panfrost: Move shrinker initialization and unplug one level down

[Boris Brezillon]

On Tue, 11 Aug 2026 22:42:13 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Since the moment we call drm_dev_register() the device should be in a
> position to accept jobs, so it's best if the shrinker is already
> initialized by then.
> 
> On top of that, make shrinker functions take an panfrost_device pointer
> like other functions in the same sequence and rename them accordingly.
> 
> Essentially mimic the init/fini behaviour in Panthor.
> 
> On top of that, remove the config feature check, because it's unnecessary.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c       |  8 +++++++-
>  drivers/gpu/drm/panfrost/panfrost_drv.c          |  6 ------
>  drivers/gpu/drm/panfrost/panfrost_gem.c          | 25 ++++++++++++++----------
>  drivers/gpu/drm/panfrost/panfrost_gem.h          |  7 ++++---
>  drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c |  8 ++------
>  5 files changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index d8421fd6a662..74992deb0b3a 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -281,9 +281,14 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  	if (err)
>  		goto out_job;
>  
> -	panfrost_gem_init(pfdev);
> +	err = panfrost_gem_init(pfdev);
> +	if (err)
> +		goto out_perfcnt;
>  
>  	return 0;
> +
> +out_perfcnt:
> +	panfrost_perfcnt_fini(pfdev);
>  out_job:
>  	panfrost_jm_fini(pfdev);
>  out_mmu:
> @@ -305,6 +310,7 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  
>  void panfrost_device_fini(struct panfrost_device *pfdev)
>  {
> +	panfrost_gem_fini(pfdev);
>  	panfrost_perfcnt_fini(pfdev);
>  	panfrost_jm_fini(pfdev);
>  	panfrost_mmu_fini(pfdev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 2e931fa90da0..331a3bd5b98c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -1002,14 +1002,9 @@ static int panfrost_probe(struct platform_device *pdev)
>  	if (err < 0)
>  		goto err_out1;
>  
> -	err = panfrost_gem_shrinker_init(&pfdev->base);
> -	if (err)
> -		goto err_out2;
>  
>  	return 0;
>  
> -err_out2:
> -	drm_dev_unregister(&pfdev->base);
>  err_out1:
>  	pm_runtime_disable(pfdev->base.dev);
>  	panfrost_device_fini(pfdev);
> @@ -1023,7 +1018,6 @@ static void panfrost_remove(struct platform_device *pdev)
>  	struct panfrost_device *pfdev = platform_get_drvdata(pdev);
>  
>  	drm_dev_unregister(&pfdev->base);
> -	panfrost_gem_shrinker_cleanup(&pfdev->base);
>  
>  	pm_runtime_get_sync(pfdev->base.dev);
>  	pm_runtime_disable(pfdev->base.dev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 3a7fce428898..7b993a089af2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -15,20 +15,25 @@
>  #include "panfrost_gem.h"
>  #include "panfrost_mmu.h"
>  
> -void panfrost_gem_init(struct panfrost_device *pfdev)
> +int panfrost_gem_init(struct panfrost_device *pfdev)
>  {
>  	int err;
>  
> -	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
> -	    !panfrost_transparent_hugepage)
> -		return;
> +	if (panfrost_transparent_hugepage) {

Hm, are you sure it compiles fine when CONFIG_TRANSPARENT_HUGEPAGE=n?
Feels like the linker would complain about a missing symbol in that
case.

If you really want to simplify it this way, you probably need

#if IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)
extern bool panfrost_transparent_hugepage;
#else
#define panfrost_transparent_hugepage false
#endif

in panfrost_drv.h.

> +		err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
> +		if (drm_gem_get_huge_mnt(&pfdev->base))
> +			drm_info(&pfdev->base, "Using Transparent Hugepage\n");
> +		else if (err)
> +			drm_warn(&pfdev->base,
> +				 "Can't use Transparent Hugepage (%d)\n", err);
> +	}
>  
> -	err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
> -	if (drm_gem_get_huge_mnt(&pfdev->base))
> -		drm_info(&pfdev->base, "Using Transparent Hugepage\n");
> -	else if (err)
> -		drm_warn(&pfdev->base, "Can't use Transparent Hugepage (%d)\n",
> -			 err);
> +	return panfrost_gem_shrinker_init(pfdev);
> +}
> +
> +void panfrost_gem_fini(struct panfrost_device *pfdev)
> +{
> +	panfrost_gem_shrinker_fini(pfdev);
>  }
>  
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index 79d4377019e9..5c823cdbd980 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -129,7 +129,8 @@ drm_mm_node_to_panfrost_mapping(struct drm_mm_node *node)
>  	return container_of(node, struct panfrost_gem_mapping, mmnode);
>  }
>  
> -void panfrost_gem_init(struct panfrost_device *pfdev);
> +int panfrost_gem_init(struct panfrost_device *pfdev);
> +void panfrost_gem_fini(struct panfrost_device *pfdev);
>  
>  struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
>  
> @@ -154,8 +155,8 @@ panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
>  void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
>  void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo);
>  
> -int panfrost_gem_shrinker_init(struct drm_device *dev);
> -void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
> +int panfrost_gem_shrinker_init(struct panfrost_device *pfdev);
> +void panfrost_gem_shrinker_fini(struct panfrost_device *pfdev);
>  
>  void panfrost_gem_set_label(struct drm_gem_object *obj, const char *label);
>  int panfrost_gem_sync(struct drm_gem_object *obj, u32 type,
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> index 2fe967a90bcb..fefae87535d6 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> @@ -95,10 +95,8 @@ panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
>   *
>   * This function registers and sets up the panfrost shrinker.
>   */
> -int panfrost_gem_shrinker_init(struct drm_device *dev)
> +int panfrost_gem_shrinker_init(struct panfrost_device *pfdev)
>  {
> -	struct panfrost_device *pfdev = to_panfrost_device(dev);
> -
>  	pfdev->shrinker = shrinker_alloc(0, "drm-panfrost");
>  	if (!pfdev->shrinker)
>  		return -ENOMEM;
> @@ -118,10 +116,8 @@ int panfrost_gem_shrinker_init(struct drm_device *dev)
>   *
>   * This function unregisters the panfrost shrinker.
>   */
> -void panfrost_gem_shrinker_cleanup(struct drm_device *dev)
> +void panfrost_gem_shrinker_fini(struct panfrost_device *pfdev)
>  {
> -	struct panfrost_device *pfdev = to_panfrost_device(dev);
> -
>  	if (pfdev->shrinker)
>  		shrinker_free(pfdev->shrinker);
>  }
> 

Re: [PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks

[Boris Brezillon]

On Tue, 11 Aug 2026 22:42:14 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> During device probe(), failure to do a PM get() will leave the usage_count
> set to 0, which is the value assigned at device creation time. That means
> when the autosuspend delay expires, runtime suspend callback won't be
> invoked, so the device will remain powered on forever.
> 
> On top of that, failure to call PM put() during device unplug means
> Panfrost device's PM usage_count increases monotonically for every new
> module reload.
> 
> The combined outcome of both of the above was that devfreq OPP transition
> notifications would be printed all the time, even when no jobs are being
> submitted. This quickly fills the kernel ring buffer with junk.
> 
> Even direr than that was the fact MMU interrupts are only enabled when
> the device is reset, so after device probe() the very first job targeting
> the tiler heap BO would always time out, because the driver's PM runtime
> resume callback would not be invoked.
> 
> Fix all that by moving all GPU enabling and disabling into RPM resume and
> suspend callbacks, and making sure we resume the device right before
> touching any HW registers. This is done in imitation of the Panthor model.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: 635430797d3f ("drm/panfrost: Rework runtime PM initialization")
> Fixes: 876b15d2c88d ("drm/panfrost: Fix module unload")
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 389 ++++++++++++++++-------------
>  drivers/gpu/drm/panfrost/panfrost_device.h |   8 +
>  drivers/gpu/drm/panfrost/panfrost_drv.c    |  11 +-
>  drivers/gpu/drm/panfrost/panfrost_gpu.c    |   9 +-
>  drivers/gpu/drm/panfrost/panfrost_gpu.h    |   1 -
>  drivers/gpu/drm/panfrost/panfrost_job.c    |   7 +-
>  drivers/gpu/drm/panfrost/panfrost_mmu.c    |   9 +-
>  drivers/gpu/drm/panfrost/panfrost_mmu.h    |   1 -
>  8 files changed, 236 insertions(+), 199 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 74992deb0b3a..52f4b8c6a05f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -26,11 +26,46 @@ static int panfrost_reset_init(struct panfrost_device *pfdev)
>  		return PTR_ERR(pfdev->rstc);
>  	}
>  
> -	return reset_control_deassert(pfdev->rstc);
> +	return 0;
>  }
>  
> -static void panfrost_reset_fini(struct panfrost_device *pfdev)
> +static int panfrost_clk_enable_deassert_reset(struct panfrost_device *pfdev)
>  {
> +	int err;
> +
> +	err = reset_control_deassert(pfdev->rstc);
> +	if (err)
> +		return err;
> +
> +	err = clk_enable(pfdev->clock);

I'd go for clk_prepare_enable() (and disable_unprepare() in
clk_disable_assert_reset()), just so you don't end up with a
prepare count lower than your enable count if the rpm get/put
section is covering the clk_fini() one. Note that the extra
prepare is cheap (just a refcnt increment since the clk has
been prepared already in clk_init()).

> +	if (err)
> +		goto assert_reset;
> +
> +	err = clk_enable(pfdev->bus_clock);
> +	if (err)
> +		goto disable_clock;
> +
> +	err = clk_enable(pfdev->bus_ace_clock);
> +	if (err)
> +		goto disable_bus_clock;
> +
> +	return 0;
> +
> +disable_bus_clock:
> +	clk_disable(pfdev->bus_clock);
> +disable_clock:
> +	clk_disable(pfdev->clock);
> +assert_reset:
> +	reset_control_assert(pfdev->rstc);
> +
> +	return err;
> +}
> +
> +static void panfrost_clk_disable_assert_reset(struct panfrost_device *pfdev)
> +{
> +	clk_disable(pfdev->bus_ace_clock);
> +	clk_disable(pfdev->bus_clock);
> +	clk_disable(pfdev->clock);
>  	reset_control_assert(pfdev->rstc);
>  }
>  
> @@ -48,7 +83,7 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
>  	rate = clk_get_rate(pfdev->clock);
>  	dev_info(pfdev->base.dev, "clock rate = %lu\n", rate);
>  
> -	err = clk_prepare_enable(pfdev->clock);
> +	err = clk_prepare(pfdev->clock);
>  	if (err)
>  		return err;
>  
> @@ -57,44 +92,55 @@ static int panfrost_clk_init(struct panfrost_device *pfdev)
>  		dev_err(pfdev->base.dev, "get bus_clock failed %ld\n",
>  			PTR_ERR(pfdev->bus_clock));
>  		err = PTR_ERR(pfdev->bus_clock);
> -		goto disable_clock;
> +		goto unprepare_clock;
>  	}
>  
>  	if (pfdev->bus_clock) {
>  		rate = clk_get_rate(pfdev->bus_clock);
>  		dev_info(pfdev->base.dev, "bus_clock rate = %lu\n", rate);
>  
> -		err = clk_prepare_enable(pfdev->bus_clock);
> +		err = clk_prepare(pfdev->bus_clock);
>  		if (err)
> -			goto disable_clock;
> +			goto unprepare_clock;
>  	}
>  
>  	pfdev->bus_ace_clock = devm_clk_get_optional(pfdev->base.dev, "bus_ace");
>  	if (IS_ERR(pfdev->bus_ace_clock)) {
>  		err = PTR_ERR(pfdev->bus_ace_clock);
>  		dev_err(pfdev->base.dev, "get bus_ace_clock failed %d\n", err);
> -		goto disable_bus_clock;
> +		goto unprepare_bus_clock;
>  	}
>  
> -	err = clk_prepare_enable(pfdev->bus_ace_clock);
> +	err = clk_prepare(pfdev->bus_ace_clock);
>  	if (err)
> -		goto disable_bus_clock;
> +		goto unprepare_bus_clock;
> +
> +	if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT))) {
> +		err = panfrost_clk_enable_deassert_reset(pfdev);
> +		if (err)
> +			goto unprepare_bus_ace_clock;
> +	}
>  
>  	return 0;
>  
> -disable_bus_clock:
> -	clk_disable_unprepare(pfdev->bus_clock);
> -disable_clock:
> -	clk_disable_unprepare(pfdev->clock);
> +unprepare_bus_ace_clock:
> +	clk_unprepare(pfdev->bus_ace_clock);
> +unprepare_bus_clock:
> +	clk_unprepare(pfdev->bus_clock);
> +unprepare_clock:
> +	clk_unprepare(pfdev->clock);
>  
>  	return err;
>  }
>  
>  static void panfrost_clk_fini(struct panfrost_device *pfdev)
>  {
> -	clk_disable_unprepare(pfdev->bus_ace_clock);
> -	clk_disable_unprepare(pfdev->bus_clock);
> -	clk_disable_unprepare(pfdev->clock);
> +	if (!(pfdev->comp->pm_features & BIT(GPU_PM_RT)))
> +		panfrost_clk_disable_assert_reset(pfdev);
> +
> +	clk_unprepare(pfdev->bus_ace_clock);
> +	clk_unprepare(pfdev->bus_clock);
> +	clk_unprepare(pfdev->clock);
>  }
>  
>  static int panfrost_regulator_init(struct panfrost_device *pfdev)
> @@ -212,6 +258,127 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
>  	return err;
>  }
>  
> +static int panfrost_device_runtime_resume(struct device *dev)
> +{
> +	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> +		ret = panfrost_clk_enable_deassert_reset(pfdev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	panfrost_devfreq_resume(pfdev);
> +
> +	if (panfrost_device_started(pfdev))
> +		panfrost_device_reset(pfdev, true);
> +
> +	return 0;
> +}
> +
> +static int panfrost_device_runtime_suspend(struct device *dev)
> +{
> +	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> +
> +	if (panfrost_device_started(pfdev) &&
> +	    !panfrost_jm_is_idle(pfdev))

Uh, if this is being called and JM is not idle, there's a serious issue
that needs fixing (JM should hold a PM ref when it's active). It probably
deserves a WARN_ON()

> +		return -EBUSY;
> +
> +	panfrost_jm_suspend_irq(pfdev);
> +	panfrost_mmu_suspend_irq(pfdev);
> +	panfrost_gpu_suspend_irq(pfdev);
> +	panfrost_gpu_power_off(pfdev);
> +	panfrost_devfreq_suspend(pfdev);

I've seen extra checks added to panfrost_gpu_suspend_irq() to cover
for some early suspend call. What I think we should do instead is
skip those sub-component calls if the device is not fully initialized
(panfrost_device_started() == true). And then, in the _fini() helpers,
you make sure to suspend/disable stuff, so that, if they're called
in from the device_init() error path, things are undone as the should
without relying on the runtime PM stuff for that.

> +
> +	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
> +		panfrost_clk_disable_assert_reset(pfdev);
> +
> +	return 0;
> +}
> +
> +static int panfrost_device_resume(struct device *dev)
> +{
> +	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF)) {
> +		unsigned long freq = pfdev->pfdevfreq.fast_rate;
> +		struct dev_pm_opp *opp;
> +
> +		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> +		if (IS_ERR(opp))
> +			return PTR_ERR(opp);
> +		dev_pm_opp_set_opp(dev, opp);
> +		dev_pm_opp_put(opp);
> +	}
> +
> +	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
> +		ret = clk_enable(pfdev->clock);
> +		if (ret)
> +			goto err_clk;
> +
> +		if (pfdev->bus_clock) {
> +			ret = clk_enable(pfdev->bus_clock);
> +			if (ret)
> +				goto err_bus_clk;
> +		}
> +	}
> +
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret)
> +		goto err_resume;
> +
> +	return 0;
> +
> +err_resume:
> +	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS) && pfdev->bus_clock)
> +		clk_disable(pfdev->bus_clock);
> +err_bus_clk:
> +	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS))
> +		clk_disable(pfdev->clock);
> +err_clk:
> +	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
> +		dev_pm_opp_set_opp(dev, NULL);
> +	return ret;
> +}
> +
> +static int panfrost_device_suspend(struct device *dev)
> +{
> +	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret)
> +		return ret;
> +
> +	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
> +		if (pfdev->bus_clock)
> +			clk_disable(pfdev->bus_clock);
> +
> +		clk_disable(pfdev->clock);
> +	}
> +
> +	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
> +		dev_pm_opp_set_opp(dev, NULL);
> +
> +	return 0;
> +}
> +
> +EXPORT_GPL_DEV_PM_OPS(panfrost_pm_ops) = {
> +	RUNTIME_PM_OPS(panfrost_device_runtime_suspend, panfrost_device_runtime_resume, NULL)
> +	SYSTEM_SLEEP_PM_OPS(panfrost_device_suspend, panfrost_device_resume)
> +};
> +
> +void panfrost_try_suspend_device(struct panfrost_device *pfdev)
> +{
> +	pm_runtime_put_sync_suspend(pfdev->base.dev);
> +
> +	/* If PM is disabled, we need to call the suspend handler manually. */
> +	if (!IS_ENABLED(CONFIG_PM))
> +		panfrost_device_runtime_suspend(pfdev->base.dev);

I'm not too sure it's wise to combine the two things in the same
helper. pm_runtime_put_sync_suspend() is the helper you call when
you're done interacting with the HW in some code section and want it to
enter suspend if there's no other users left.

The conditional !PM panfrost_device_runtime_suspend() call is supposed
to be in the device_remove() path in pair with the conditional
panfrost_device_runtime_resume() that exists in the probe() path.

> +}
> +
>  int panfrost_device_init(struct panfrost_device *pfdev)
>  {
>  	int err;
> @@ -242,7 +409,7 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  	err = panfrost_clk_init(pfdev);
>  	if (err) {
>  		dev_err(pfdev->base.dev, "clk init failed %d\n", err);
> -		goto out_reset;
> +		goto out_pm_domain;
>  	}
>  
>  	err = panfrost_devfreq_init(pfdev);
> @@ -265,60 +432,70 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  		goto out_regulator;
>  	}
>  
> -	err = panfrost_gpu_init(pfdev);
> +	err = devm_pm_runtime_enable(pfdev->base.dev);
>  	if (err)
>  		goto out_regulator;
>  
> -	err = panfrost_mmu_init(pfdev);
> +	err = pm_runtime_resume_and_get(pfdev->base.dev);

I don't see a pm_runtime_put to go with that runtime_get in the
device_init() function. To me, it looks like this keeps the device
active until the device_fini() function is called, which is not what we
want.

>  	if (err)
> -		goto out_gpu;
> +		goto out_regulator;
>  
> -	err = panfrost_jm_init(pfdev);
> +	/* If PM is disabled, we need to call panfrost_device_runtime_resume() manually. */
> +	if (!IS_ENABLED(CONFIG_PM)) {
> +		err = panfrost_device_runtime_resume(pfdev->base.dev);
> +		if (err)
> +			goto out_regulator;
> +	}
> +
> +	err = panfrost_gpu_init(pfdev);
>  	if (err)
> -		goto out_mmu;
> +		goto out_rpm_put;
> +
> +	err = panfrost_mmu_init(pfdev);
> +	if (err)
> +		goto out_rpm_put;
>  
>  	err = panfrost_perfcnt_init(pfdev);
>  	if (err)
> -		goto out_job;
> +		goto out_rpm_put;
>  
>  	err = panfrost_gem_init(pfdev);
>  	if (err)
>  		goto out_perfcnt;
>  
> +	err = panfrost_jm_init(pfdev);
> +	if (err)
> +		goto out_gem;
> +
>  	return 0;
>  
> +out_gem:
> +	panfrost_gem_fini(pfdev);
>  out_perfcnt:
>  	panfrost_perfcnt_fini(pfdev);
> -out_job:
> -	panfrost_jm_fini(pfdev);
> -out_mmu:
> -	panfrost_mmu_fini(pfdev);
> -out_gpu:
> -	panfrost_gpu_fini(pfdev);
> +out_rpm_put:
> +	panfrost_try_suspend_device(pfdev);
>  out_regulator:
>  	panfrost_regulator_fini(pfdev);
>  out_devfreq:
>  	panfrost_devfreq_fini(pfdev);
>  out_clk:
>  	panfrost_clk_fini(pfdev);
> -out_reset:
> -	panfrost_reset_fini(pfdev);
>  out_pm_domain:
>  	panfrost_pm_domain_fini(pfdev);
>  	return err;
>  }
>  
> -void panfrost_device_fini(struct panfrost_device *pfdev)
> +void
> +panfrost_device_fini(struct panfrost_device *pfdev)
>  {

There should be pm_runtime_resume_and_get() here...

> +	panfrost_jm_fini(pfdev);
>  	panfrost_gem_fini(pfdev);
>  	panfrost_perfcnt_fini(pfdev);
> -	panfrost_jm_fini(pfdev);
> -	panfrost_mmu_fini(pfdev);
> -	panfrost_gpu_fini(pfdev);
> -	panfrost_devfreq_fini(pfdev);

... and a pm_runtime_put_sync() here. We can probably even used a

	scoped_cond_guard(pm_runtime_active_try_enabled,
			  /* FIXME: can't resume fallback */,
			  pfdev->base.dev) {
	}

	/* If PM is disabled, we need to call the suspend handler manually. */
	if (!IS_ENABLED(CONFIG_PM))
		panfrost_device_runtime_suspend(pfdev->base.dev);

> +	panfrost_try_suspend_device(pfdev);
>  	panfrost_regulator_fini(pfdev);
> +	panfrost_devfreq_fini(pfdev);
>  	panfrost_clk_fini(pfdev);
> -	panfrost_reset_fini(pfdev);
>  	panfrost_pm_domain_fini(pfdev);
>  }
>  
> @@ -425,147 +602,9 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>  void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  {
>  	panfrost_gpu_soft_reset(pfdev);
> -
>  	panfrost_gpu_power_on(pfdev);
>  	panfrost_mmu_reset(pfdev);
> -

Looks like unrelated cosmetic changes.

>  	panfrost_jm_reset_interrupts(pfdev);
>  	if (enable_job_int)
>  		panfrost_jm_enable_interrupts(pfdev);
>  }
> -
> -static int panfrost_device_runtime_resume(struct device *dev)
> -{
> -	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> -	int ret;
> -
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> -		ret = reset_control_deassert(pfdev->rstc);
> -		if (ret)
> -			return ret;
> -
> -		ret = clk_enable(pfdev->clock);
> -		if (ret)
> -			goto err_clk;
> -
> -		ret = clk_enable(pfdev->bus_clock);
> -		if (ret)
> -			goto err_bus_clk;
> -
> -		ret = clk_enable(pfdev->bus_ace_clock);
> -		if (ret)
> -			goto err_bus_ace_clk;
> -	}
> -
> -	panfrost_device_reset(pfdev, true);
> -	panfrost_devfreq_resume(pfdev);
> -
> -	return 0;
> -
> -err_bus_ace_clk:
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
> -		clk_disable(pfdev->bus_clock);
> -err_bus_clk:
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
> -		clk_disable(pfdev->clock);
> -err_clk:
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_RT))
> -		reset_control_assert(pfdev->rstc);
> -	return ret;
> -}
> -
> -static int panfrost_device_runtime_suspend(struct device *dev)
> -{
> -	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> -
> -	if (!panfrost_jm_is_idle(pfdev))
> -		return -EBUSY;
> -
> -	panfrost_devfreq_suspend(pfdev);
> -	panfrost_jm_suspend_irq(pfdev);
> -	panfrost_mmu_suspend_irq(pfdev);
> -	panfrost_gpu_suspend_irq(pfdev);
> -	panfrost_gpu_power_off(pfdev);
> -
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> -		clk_disable(pfdev->bus_ace_clock);
> -		clk_disable(pfdev->bus_clock);
> -		clk_disable(pfdev->clock);
> -		reset_control_assert(pfdev->rstc);
> -	}
> -
> -	return 0;
> -}
> -
> -static int panfrost_device_resume(struct device *dev)
> -{
> -	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> -	int ret;
> -
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF)) {
> -		unsigned long freq = pfdev->pfdevfreq.fast_rate;
> -		struct dev_pm_opp *opp;
> -
> -		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> -		if (IS_ERR(opp))
> -			return PTR_ERR(opp);
> -		dev_pm_opp_set_opp(dev, opp);
> -		dev_pm_opp_put(opp);
> -	}
> -
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
> -		ret = clk_enable(pfdev->clock);
> -		if (ret)
> -			goto err_clk;
> -
> -		if (pfdev->bus_clock) {
> -			ret = clk_enable(pfdev->bus_clock);
> -			if (ret)
> -				goto err_bus_clk;
> -		}
> -	}
> -
> -	ret = pm_runtime_force_resume(dev);
> -	if (ret)
> -		goto err_resume;
> -
> -	return 0;
> -
> -err_resume:
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS) && pfdev->bus_clock)
> -		clk_disable(pfdev->bus_clock);
> -err_bus_clk:
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS))
> -		clk_disable(pfdev->clock);
> -err_clk:
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
> -		dev_pm_opp_set_opp(dev, NULL);
> -	return ret;
> -}
> -
> -static int panfrost_device_suspend(struct device *dev)
> -{
> -	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> -	int ret;
> -
> -	ret = pm_runtime_force_suspend(dev);
> -	if (ret)
> -		return ret;
> -
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
> -		if (pfdev->bus_clock)
> -			clk_disable(pfdev->bus_clock);
> -
> -		clk_disable(pfdev->clock);
> -	}
> -
> -	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
> -		dev_pm_opp_set_opp(dev, NULL);
> -
> -	return 0;
> -}
> -
> -EXPORT_GPL_DEV_PM_OPS(panfrost_pm_ops) = {
> -	RUNTIME_PM_OPS(panfrost_device_runtime_suspend, panfrost_device_runtime_resume, NULL)
> -	SYSTEM_SLEEP_PM_OPS(panfrost_device_suspend, panfrost_device_resume)
> -};
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index ec55c136b1b6..0fd33bc5b86f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -253,6 +253,8 @@ int panfrost_device_init(struct panfrost_device *pfdev);
>  void panfrost_device_fini(struct panfrost_device *pfdev);
>  void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int);
>  
> +void panfrost_try_suspend_device(struct panfrost_device *pfdev);
> +
>  extern const struct dev_pm_ops panfrost_pm_ops;
>  
>  enum drm_panfrost_exception_type {
> @@ -342,4 +344,10 @@ panfrost_device_schedule_reset(struct panfrost_device *pfdev)
>  	queue_work(pfdev->reset.wq, &pfdev->reset.work);
>  }
>  
> +static inline bool
> +panfrost_device_started(struct panfrost_device *pfdev)
> +{
> +	return pfdev->js;
> +}
> +
>  #endif
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 331a3bd5b98c..8410de95e364 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -988,9 +988,6 @@ 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_set_autosuspend_delay(pfdev->base.dev, 50); /* ~3 frames */
>  	pm_runtime_use_autosuspend(pfdev->base.dev);

I'd recommend moving those to panfrost_device_init(), since this is were
the rest of the PM related initialization happens.

>  
> @@ -1002,13 +999,12 @@ static int panfrost_probe(struct platform_device *pdev)
>  	if (err < 0)
>  		goto err_out1;
>  
> +	pm_runtime_put_autosuspend(pfdev->base.dev);

Oh, so here is the runtime_put_autosuspend() I was looking for
in panfrost_device_init().

>  
>  	return 0;
>  
>  err_out1:
> -	pm_runtime_disable(pfdev->base.dev);
>  	panfrost_device_fini(pfdev);
> -	pm_runtime_set_suspended(pfdev->base.dev);
>  err_out0:
>  	return err;
>  }
> @@ -1019,10 +1015,9 @@ static void panfrost_remove(struct platform_device *pdev)
>  
>  	drm_dev_unregister(&pfdev->base);
>  
> -	pm_runtime_get_sync(pfdev->base.dev);
> -	pm_runtime_disable(pfdev->base.dev);
> +	drm_WARN_ON(&pfdev->base, pm_runtime_get_sync(pfdev->base.dev) < 0);

And here's the pm_runtime_get() that was missing in panthor_device_fini().
Let's move anything PM related to device_{init,fini}() to clarify things.

Also, we probably want a resume_and_get() instead of get_sync(), and some
fallback in case the resume fails (though I'm not too sure what the fallback
could be).

> +
>  	panfrost_device_fini(pfdev);
> -	pm_runtime_set_suspended(pfdev->base.dev);

I assume this is now handled by the action registered by
devm_pm_runtime_enable().

>  }
>  
>  static ssize_t profiling_show(struct device *dev,
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> index 7d555e63e21a..0a3a68b561c9 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -509,7 +509,9 @@ void panfrost_gpu_suspend_irq(struct panfrost_device *pfdev)
>  	set_bit(PANFROST_COMP_BIT_GPU, pfdev->is_suspended);
>  
>  	gpu_write(pfdev, GPU_INT_MASK, 0);
> -	synchronize_irq(pfdev->gpu_irq);
> +
> +	if (pfdev->gpu_irq > 0)
> +		synchronize_irq(pfdev->gpu_irq);

As mentioned above, I don't think panfrost_gpu_suspend_irq() should be called
before panthor_gpu_init() (applies to all xxx_suspend_irq() helpers actually),
and that's something to address at the RPM implementation level (skip those
when the device is not yet initialized/started).

Re: [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init

[Boris Brezillon]

On Tue, 11 Aug 2026 22:42:15 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Because the device must be in a position to accept jobs between the time
> drm_dev_register() is called and autosuspend first kicks in, there's a very
> narrow window inbetween during which jobs targeting the tiler buffer
> object would time out, since the device's PM status is 'Active', but no MMU
> interrupts were enabled at device initialisation time.

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

It's probably good to have the IRQ regs initialized in the init()
function, though I'd like to be sure this is enough. If we assume
that a reset is what it takes to have a functional device, shouldn't we
do this reset in the device_init() path, before drm_dev_register() is
called?

> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: 73e467f60acd ("drm/panfrost: Consolidate reset handling")
> ---
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 3fe37dd12360..a8ce5d65825c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -336,6 +336,12 @@ void panfrost_mmu_as_put(struct panfrost_device *pfdev, struct panfrost_mmu *mmu
>  	WARN_ON(atomic_read(&mmu->as_count) < 0);
>  }
>  
> +static void panfrost_mmu_enable_interrupts(struct panfrost_device *pfdev)
> +{
> +	mmu_write(pfdev, MMU_INT_CLEAR, ~0);
> +	mmu_write(pfdev, MMU_INT_MASK, ~0);
> +}
> +
>  void panfrost_mmu_reset(struct panfrost_device *pfdev)
>  {
>  	struct panfrost_mmu *mmu, *mmu_tmp;
> @@ -355,8 +361,7 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev)
>  
>  	spin_unlock(&pfdev->as_lock);
>  
> -	mmu_write(pfdev, MMU_INT_CLEAR, ~0);
> -	mmu_write(pfdev, MMU_INT_MASK, ~0);
> +	panfrost_mmu_enable_interrupts(pfdev);
>  }
>  
>  static size_t get_pgsize(u64 addr, size_t size, size_t *count)
> @@ -970,6 +975,8 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>  		return err;
>  	}
>  
> +	panfrost_mmu_enable_interrupts(pfdev);
> +
>  	return 0;
>  }
>  
> 

Re: [PATCH v5 07/11] drm/panfrost: Add debugfs knob for manually triggering a GPU reset

[Boris Brezillon]

On Tue, 11 Aug 2026 22:42:16 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> This will be of great help when testing potential races between the GPU
> reset sequence and other parts of the code accessing HW registers.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 39 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_device.h |  3 +++
>  drivers/gpu/drm/panfrost/panfrost_drv.c    |  1 +
>  3 files changed, 43 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 52f4b8c6a05f..5b66173c75b9 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -2,6 +2,7 @@
>  /* Copyright 2018 Marty E. Plummer <hanetzer@startmail.com> */
>  /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
>  
> +#include <linux/debugfs.h>
>  #include <linux/clk.h>
>  #include <linux/reset.h>
>  #include <linux/platform_device.h>
> @@ -608,3 +609,41 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  	if (enable_job_int)
>  		panfrost_jm_enable_interrupts(pfdev);
>  }
> +
> +#ifdef CONFIG_DEBUG_FS
> +static int reset_get(void *data, u64 *val)
> +{
> +	struct panfrost_device *pfdev =
> +		container_of(data, struct panfrost_device, base);
> +
> +	*val = atomic_read(&pfdev->reset.pending);
> +	return 0;
> +}
> +
> +static int reset_set(void *data, u64 val)
> +{
> +	struct panfrost_device *pfdev =
> +		container_of(data, struct panfrost_device, base);
> +	int ret;
> +
> +	ret = pm_runtime_get_if_in_use(pfdev->base.dev);
> +
> +	if (ret > 0) {
> +		panfrost_device_schedule_reset(pfdev);
> +		flush_work(&pfdev->reset.work);
> +		pm_runtime_put(pfdev->base.dev);
> +	}
> +
> +	return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(panfrost_reset_debugfs_fops,
> +			 reset_get, reset_set,
> +			 "0x%08llx\n");
> +
> +void panfrost_reset_debugfs_init(struct drm_minor *minor)
> +{
> +	debugfs_create_file("reset", 0600, minor->debugfs_root,
> +			    minor->dev, &panfrost_reset_debugfs_fops);
> +}
> +#endif // CONFIG_DEBUG_FS
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 0fd33bc5b86f..4bbaaaf827a5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -350,4 +350,7 @@ panfrost_device_started(struct panfrost_device *pfdev)
>  	return pfdev->js;
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +void panfrost_reset_debugfs_init(struct drm_minor *minor);
> +#endif // CONFIG_DEBUG_FS
>  #endif
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 8410de95e364..958f1d36ab10 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -916,6 +916,7 @@ static void panfrost_debugfs_init(struct drm_minor *minor)
>  {
>  	panthor_gems_debugfs_init(minor);
>  	panfrost_sched_debugfs_init(minor);
> +	panfrost_reset_debugfs_init(minor);

Nit: I'd probably go for panfrost_device_debugfs_init(), and since gems
and sched are sub-components of the device, I'd call the other
panfrost_{gems,sched}_debugfs_init() helpers from
panfrost_device_debugfs_init().

>  }
>  #endif
>  
> 

Re: [PATCH v5 09/11] drm/panfrost: Introduce a reset lock

[Boris Brezillon]

On Tue, 11 Aug 2026 22:42:18 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> So as to avoid accessing HW register while a reset is ongoing, a read/write
> semaphore that envelopes the reset sequence will help driver entry points
> avoid racing with it. For now, the only such racy entry point is the ioctl
> that returns the current GPU timestmap.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>

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

> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 2 ++
>  drivers/gpu/drm/panfrost/panfrost_device.h | 1 +
>  drivers/gpu/drm/panfrost/panfrost_drv.c    | 9 ++++++---
>  drivers/gpu/drm/panfrost/panfrost_job.c    | 1 +
>  4 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 5b66173c75b9..e0390b6c0d22 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -602,6 +602,8 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>  
>  void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  {
> +	guard(rwsem_read)(&pfdev->reset.lock);
> +
>  	panfrost_gpu_soft_reset(pfdev);
>  	panfrost_gpu_power_on(pfdev);
>  	panfrost_mmu_reset(pfdev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 4bbaaaf827a5..a2a68e042225 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -166,6 +166,7 @@ struct panfrost_device {
>  	struct {
>  		struct workqueue_struct *wq;
>  		struct work_struct work;
> +		struct rw_semaphore lock;
>  		atomic_t pending;
>  	} reset;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 958f1d36ab10..ff23b1a979bb 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -41,9 +41,12 @@ static int panfrost_ioctl_query_timestamp(struct panfrost_device *pfdev,
>  	if (ret)
>  		return ret;
>  
> -	panfrost_cycle_counter_get(pfdev);
> -	*arg = panfrost_timestamp_read(pfdev);
> -	panfrost_cycle_counter_put(pfdev);
> +	/* We should not read timestamp register while the GPU  is being reset */
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		panfrost_cycle_counter_get(pfdev);
> +		*arg = panfrost_timestamp_read(pfdev);
> +		panfrost_cycle_counter_put(pfdev);
> +	}
>  
>  	pm_runtime_put(pfdev->base.dev);
>  	return 0;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index c761379851da..9d7dafa29f19 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -874,6 +874,7 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
>  
>  	INIT_WORK(&pfdev->reset.work, panfrost_reset_work);
>  	spin_lock_init(&js->job_lock);
> +	init_rwsem(&pfdev->reset.lock);
>  
>  	js->irq = platform_get_irq_byname(to_platform_device(pfdev->base.dev), "job");
>  	if (js->irq < 0)
> 

Re: [PATCH v5 09/11] drm/panfrost: Introduce a reset lock

[Boris Brezillon]

On Tue, 11 Aug 2026 22:42:18 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> So as to avoid accessing HW register while a reset is ongoing, a read/write
> semaphore that envelopes the reset sequence will help driver entry points
> avoid racing with it. For now, the only such racy entry point is the ioctl
> that returns the current GPU timestmap.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 2 ++
>  drivers/gpu/drm/panfrost/panfrost_device.h | 1 +
>  drivers/gpu/drm/panfrost/panfrost_drv.c    | 9 ++++++---
>  drivers/gpu/drm/panfrost/panfrost_job.c    | 1 +
>  4 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 5b66173c75b9..e0390b6c0d22 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -602,6 +602,8 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>  
>  void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  {
> +	guard(rwsem_read)(&pfdev->reset.lock);

Oops, s/rwsem_read/rwsem_write/. This is actually fixed in the next
patch.

> +
>  	panfrost_gpu_soft_reset(pfdev);
>  	panfrost_gpu_power_on(pfdev);
>  	panfrost_mmu_reset(pfdev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 4bbaaaf827a5..a2a68e042225 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -166,6 +166,7 @@ struct panfrost_device {
>  	struct {
>  		struct workqueue_struct *wq;
>  		struct work_struct work;
> +		struct rw_semaphore lock;
>  		atomic_t pending;
>  	} reset;
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 958f1d36ab10..ff23b1a979bb 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -41,9 +41,12 @@ static int panfrost_ioctl_query_timestamp(struct panfrost_device *pfdev,
>  	if (ret)
>  		return ret;
>  
> -	panfrost_cycle_counter_get(pfdev);
> -	*arg = panfrost_timestamp_read(pfdev);
> -	panfrost_cycle_counter_put(pfdev);
> +	/* We should not read timestamp register while the GPU  is being reset */
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		panfrost_cycle_counter_get(pfdev);
> +		*arg = panfrost_timestamp_read(pfdev);
> +		panfrost_cycle_counter_put(pfdev);
> +	}
>  
>  	pm_runtime_put(pfdev->base.dev);
>  	return 0;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index c761379851da..9d7dafa29f19 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -874,6 +874,7 @@ int panfrost_jm_init(struct panfrost_device *pfdev)
>  
>  	INIT_WORK(&pfdev->reset.work, panfrost_reset_work);
>  	spin_lock_init(&js->job_lock);
> +	init_rwsem(&pfdev->reset.lock);
>  
>  	js->irq = platform_get_irq_byname(to_platform_device(pfdev->base.dev), "job");
>  	if (js->irq < 0)
> 

Re: [PATCH v5 08/11] drm/panfrost: Move perfcnt GPU disable sequence into a helper

[Boris Brezillon]

On Tue, 11 Aug 2026 22:42:17 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Just for the sake of avoiding repetition.
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 31 ++++++++++++-----------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index 7020c0192e18..ad1156678e91 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> @@ -35,6 +35,16 @@ struct panfrost_perfcnt {
>  	struct completion dump_comp;
>  };
>  
> +static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfdev)

I see you introduce panfrost_perfcnt_hw_enable() in patch 10, so maybe
s/panfrost_perfcnt_gpu_disable/panfrost_perfcnt_hw_disable/ to be
consistent.

> +{
> +	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);
> +}
> +
>  void panfrost_perfcnt_clean_cache_done(struct panfrost_device *pfdev)
>  {
>  	complete(&pfdev->perfcnt->dump_comp);
> @@ -193,12 +203,7 @@ 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);
> @@ -327,12 +332,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 +344,5 @@ 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);
>  }
> 

Re: [PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence

[Boris Brezillon]

On Tue, 11 Aug 2026 22:42:19 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Formerly, the reset sequence would race with panfrost_mmu_as_put()
> when tearing down a perfcnt session. On top of that, poking GPU
> registers to program a perfcnt session or obtaining a dump might lead to
> undefined behaviour when done at the same time a reset was ongoing.
> 
> Use the reset r/w semaphore to govern access to the hardware at reset
> time. On top of that, expand the DRM uAPI for the perfcnt DUMP operation
> so that userspace can be made aware of a reset having happened, because
> that means counters will go back to 0 and can no longer be accumulated
> to values previously kept in user space.
> 
> The new perfcnt-aware reset sequence also takes care to reestablish
> perfcnt to its original configuration if there was an enabled session.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c  |   9 +-
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 220 ++++++++++++++++++++--------
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.h |   2 +
>  include/uapi/drm/panfrost_drm.h             |   3 +-
>  4 files changed, 171 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index e0390b6c0d22..c81d8ca67ae4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -602,14 +602,21 @@ bool panfrost_exception_needs_reset(const struct panfrost_device *pfdev,
>  
>  void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  {
> -	guard(rwsem_read)(&pfdev->reset.lock);
> +	guard(rwsem_write)(&pfdev->reset.lock);
>  
> +	/* Pre-reset */
> +	panfrost_perfcnt_reset(pfdev);
> +
> +	/* Do the actual device reset */
>  	panfrost_gpu_soft_reset(pfdev);
>  	panfrost_gpu_power_on(pfdev);
> +
> +	/* Post-reset */
>  	panfrost_mmu_reset(pfdev);
>  	panfrost_jm_reset_interrupts(pfdev);
>  	if (enable_job_int)
>  		panfrost_jm_enable_interrupts(pfdev);
> +	panfrost_perfcnt_postreset(pfdev);
>  }
>  
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index ad1156678e91..01d477f7fce0 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> @@ -11,6 +11,7 @@
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem_shmem_helper.h>
>  #include <drm/panfrost_drm.h>
> +#include <drm/drm_print.h>
>  
>  #include "panfrost_device.h"
>  #include "panfrost_features.h"
> @@ -25,14 +26,18 @@
>  #define BYTES_PER_COUNTER		4
>  #define BLOCKS_PER_COREGROUP		8
>  #define V4_SHADERS_PER_COREGROUP	4
> +#define PERFCNT_DUMP_MAX_RETRIES	5
>  
>  struct panfrost_perfcnt {
>  	struct panfrost_gem_mapping *mapping;
> +	unsigned int counterset;
>  	size_t bosize;
>  	void *buf;
>  	struct panfrost_file_priv *user;
>  	struct mutex lock;
>  	struct completion dump_comp;
> +	bool reset_happened;
> +	bool reset_failed;
>  };
>  
>  static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfdev)
> @@ -55,25 +60,93 @@ void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev)
>  	gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_CACHES);
>  }
>  
> -static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
> +static int panfrost_perfcnt_hw_enable(struct panfrost_device *pfdev)
>  {
> -	u64 gpuva;
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +	u32 cfg, as;
> +	int ret;
> +
> +	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
> +	if (ret < 0)
> +		return ret;
> +
> +	as = ret;
> +	cfg = GPU_PERFCNT_CFG_AS(as) |
> +	      GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL);
> +
> +	/*
> +	 * Bifrost GPUs have 2 set of counters, but we're only interested by
> +	 * the first one for now.
> +	 */
> +	if (panfrost_model_is_bifrost(pfdev))
> +		cfg |= GPU_PERFCNT_CFG_SETSEL(perfcnt->counterset);
> +
> +	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0xffffffff);
> +	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0xffffffff);
> +	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0xffffffff);
> +
> +	/*
> +	 * Due to PRLAM-8186 we need to disable the Tiler before we enable HW
> +	 * counters.
> +	 */
> +	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> +		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
> +	else
> +		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> +
> +	gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
> +
> +	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> +		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> +
> +	return 0;
> +}
> +
> +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev,
> +					u64 *reset_happened)
> +{
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +	u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> +	s64 retries = PERFCNT_DUMP_MAX_RETRIES;
>  	int ret;
>  
> -	reinit_completion(&pfdev->perfcnt->dump_comp);
> -	gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> -	gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva));
> -	gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva));
> -	gpu_write(pfdev, GPU_INT_CLEAR,
> -		  GPU_IRQ_CLEAN_CACHES_COMPLETED |
> -		  GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
> -	gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE);
> +dump_retry:
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		*reset_happened = perfcnt->reset_happened;
> +		perfcnt->reset_happened = false;
> +		if (perfcnt->reset_failed) {
> +			ret = panfrost_perfcnt_hw_enable(pfdev);
> +			if (ret)
> +				return ret;
> +			perfcnt->reset_failed = false;
> +		}
> +
> +		reinit_completion(&pfdev->perfcnt->dump_comp);
> +
> +		gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva));
> +		gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva));
> +		gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_CLEAN_CACHES_COMPLETED |
> +						GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
> +		gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE);
> +	}
> +
>  	ret = wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp,
>  							msecs_to_jiffies(1000));
> -	if (!ret)
> -		ret = -ETIMEDOUT;
> -	else if (ret > 0)
> -		ret = 0;
> +
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		if (ret > 0) {
> +			if (perfcnt->reset_happened) {
> +				if (--retries >= 0)
> +					goto dump_retry;
> +				else
> +					ret = -EBUSY;
> +			} else {
> +				ret = 0;
> +			}
> +		} else if (!ret) {
> +			ret = -ETIMEDOUT;
> +		}
> +	}
>  
>  	return ret;
>  }
> @@ -84,9 +157,8 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  {
>  	struct panfrost_file_priv *user = file_priv->driver_priv;
>  	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> -	struct iosys_map map;
>  	struct drm_gem_shmem_object *bo;
> -	u32 cfg, as;
> +	struct iosys_map map;
>  	int ret;
>  
>  	if (user == perfcnt->user)
> @@ -119,7 +191,9 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  	ret = drm_gem_vmap(&bo->base, &map);
>  	if (ret)
>  		goto err_put_mapping;
> +
>  	perfcnt->buf = map.vaddr;
> +	perfcnt->counterset = counterset;
>  
>  	panfrost_gem_internal_set_label(&bo->base, "Perfcnt sample buffer");
>  
> @@ -127,60 +201,47 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  	 * Invalidate the cache and clear the counters to start from a fresh
>  	 * state.
>  	 */
> -	reinit_completion(&pfdev->perfcnt->dump_comp);
> -	gpu_write(pfdev, GPU_INT_CLEAR,
> -		  GPU_IRQ_CLEAN_CACHES_COMPLETED |
> -		  GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
> -	gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
> -	gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_INV_CACHES);
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		reinit_completion(&pfdev->perfcnt->dump_comp);
> +		gpu_write(pfdev, GPU_INT_CLEAR,
> +			  GPU_IRQ_CLEAN_CACHES_COMPLETED |
> +			  GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
> +		gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
> +		gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_INV_CACHES);

Can you remind me why we need a cache flush in the enable path? Feels
to me that this is something we want after a dump but not when we
enable the perfcnt block, especially since all dumps are currently
manual. Dropping this pre-flush would actually simplify quite a few
things.

> +		perfcnt->reset_happened = false;
> +		perfcnt->user = user;
> +	}
> +
> +	/*
> +	 * If a reset happens during the wait for the IRQ notification that caches
> +	 * are clean and invalidated, then we know the reset sequence did the job
> +	 * for us, even if it takes long enough for the completion to time out.
> +	 */
>  	ret = wait_for_completion_timeout(&pfdev->perfcnt->dump_comp,
>  					  msecs_to_jiffies(1000));
> -	if (!ret) {
> +	if (!ret && !perfcnt->reset_happened) {
>  		ret = -ETIMEDOUT;
>  		goto err_vunmap;
>  	}
>  
> -	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
> -	if (ret < 0)
> -		goto err_vunmap;
> -
> -	as = ret;
> -	cfg = GPU_PERFCNT_CFG_AS(as) |
> -	      GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL);
> -
> -	/*
> -	 * Bifrost GPUs have 2 set of counters, but we're only interested by
> -	 * the first one for now.
> -	 */
> -	if (panfrost_model_is_bifrost(pfdev))
> -		cfg |= GPU_PERFCNT_CFG_SETSEL(counterset);
> -
> -	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0xffffffff);
> -	gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0xffffffff);
> -	gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0xffffffff);
> -
> -	/*
> -	 * Due to PRLAM-8186 we need to disable the Tiler before we enable HW
> -	 * counters.
> -	 */
> -	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> -		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
> -	else
> -		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> -
> -	gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
> -
> -	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> -		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		if (!perfcnt->reset_happened || perfcnt->reset_failed) {
> +			ret = panfrost_perfcnt_hw_enable(pfdev);
> +			if (ret)
> +				goto err_vunmap;
> +		}
> +		perfcnt->reset_happened = false;
> +		perfcnt->reset_failed = false;
> +	}
>  
>  	/* The BO ref is retained by the mapping. */
>  	drm_gem_object_put(&bo->base);
>  
> -	perfcnt->user = user;
> -
>  	return 0;
>  
>  err_vunmap:
> +	scoped_guard(rwsem_read, &pfdev->reset.lock)
> +		perfcnt->user = NULL;
>  	drm_gem_vunmap(&bo->base, &map);
>  err_put_mapping:
>  	panfrost_gem_mapping_put(perfcnt->mapping);
> @@ -203,13 +264,15 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
>  	if (user != perfcnt->user)
>  		return -EINVAL;
>  
> -	panfrost_perfcnt_gpu_disable(pfdev);
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		panfrost_perfcnt_gpu_disable(pfdev);
> +		panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
> +		perfcnt->user = NULL;
> +	}
>  
> -	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);
>  	panfrost_gem_mapping_put(perfcnt->mapping);
>  	perfcnt->mapping = NULL;
>  	pm_runtime_put_autosuspend(pfdev->base.dev);
> @@ -263,7 +326,7 @@ int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> -	ret = panfrost_perfcnt_dump_locked(pfdev);
> +	ret = panfrost_perfcnt_dump_locked(pfdev, &req->hw_reset);
>  	if (ret)
>  		goto out;
>  
> @@ -346,3 +409,38 @@ void panfrost_perfcnt_fini(struct panfrost_device *pfdev)
>  	/* Disable everything before leaving. */
>  	panfrost_perfcnt_gpu_disable(pfdev);
>  }
> +
> +void panfrost_perfcnt_reset(struct panfrost_device *pfdev)
> +{
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +
> +	if (drm_WARN_ON(&pfdev->base, !perfcnt))
> +		return;
> +
> +	lockdep_assert_held(&pfdev->reset.lock);
> +
> +	if (!perfcnt->user)
> +		return;
> +
> +	perfcnt->reset_happened = true;
> +	complete(&perfcnt->dump_comp);
> +	panfrost_perfcnt_gpu_disable(pfdev);

Do we really need both a _reset() and post_reset(). Feels to me that
what we need is a post_reset() that re-enables the counters if they
were enabled, and unblock dump_comp after setting reset_happened=true.
It's then up to the dump logic to retry (maybe a couple times max, to
bail out if things keep failing).

> +}
> +
> +void panfrost_perfcnt_postreset(struct panfrost_device *pfdev)
> +{
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +	int ret;
> +
> +	if (drm_WARN_ON(&pfdev->base, !perfcnt))
> +		return;
> +
> +	lockdep_assert_held(&pfdev->reset.lock);
> +
> +	if (!perfcnt->user)
> +		return;
> +
> +	ret = panfrost_perfcnt_hw_enable(pfdev);
> +	if (ret)
> +		perfcnt->reset_failed = true;
> +}
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
> index 8bbcf5f5fb33..e14e760641fd 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
> @@ -14,5 +14,7 @@ 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);
> +void panfrost_perfcnt_postreset(struct panfrost_device *pfdev);
>  
>  #endif
> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> index 50d5337f35ef..3bbf9220103d 100644
> --- a/include/uapi/drm/panfrost_drm.h
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -47,7 +47,7 @@ extern "C" {
>   * them for anything but debugging purpose.
>   */
>  #define DRM_IOCTL_PANFROST_PERFCNT_ENABLE	DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_ENABLE, struct drm_panfrost_perfcnt_enable)
> -#define DRM_IOCTL_PANFROST_PERFCNT_DUMP		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct drm_panfrost_perfcnt_dump)
> +#define DRM_IOCTL_PANFROST_PERFCNT_DUMP		DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct drm_panfrost_perfcnt_dump)
>  
>  #define PANFROST_JD_REQ_FS (1 << 0)
>  #define PANFROST_JD_REQ_CYCLE_COUNT (1 << 1)
> @@ -272,6 +272,7 @@ struct drm_panfrost_perfcnt_enable {
>  
>  struct drm_panfrost_perfcnt_dump {
>  	__u64 buf_ptr;
> +	__u64 hw_reset;
>  };
>  
>  /* madvise provides a way to tell the kernel in case a buffers contents
>