* [PATCH v5 01/11] drm/panfrost: Check another bo field for cache option query
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
2026-08-11 21:42 ` [PATCH v5 02/11] drm/panfrost: Prevent division by 0 Adrián Larumbe
` (9 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong, Claude
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH v5 02/11] drm/panfrost: Prevent division by 0
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
2026-08-11 21:42 ` [PATCH v5 01/11] drm/panfrost: Check another bo field for cache option query Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
2026-08-12 7:59 ` Boris Brezillon
2026-08-11 21:42 ` [PATCH v5 03/11] drm/panfrost: Remove unnecessary header file include Adrián Larumbe
` (8 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong, Claude
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 02/11] drm/panfrost: Prevent division by 0
2026-08-11 21:42 ` [PATCH v5 02/11] drm/panfrost: Prevent division by 0 Adrián Larumbe
@ 2026-08-12 7:59 ` Boris Brezillon
0 siblings, 0 replies; 29+ messages in thread
From: Boris Brezillon @ 2026-08-12 7:59 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong, Claude
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;
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 03/11] drm/panfrost: Remove unnecessary header file include
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
2026-08-11 21:42 ` [PATCH v5 01/11] drm/panfrost: Check another bo field for cache option query Adrián Larumbe
2026-08-11 21:42 ` [PATCH v5 02/11] drm/panfrost: Prevent division by 0 Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
2026-08-12 8:00 ` Boris Brezillon
2026-08-11 21:42 ` [PATCH v5 04/11] drm/panfrost: Move shrinker initialization and unplug one level down Adrián Larumbe
` (7 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 03/11] drm/panfrost: Remove unnecessary header file include
2026-08-11 21:42 ` [PATCH v5 03/11] drm/panfrost: Remove unnecessary header file include Adrián Larumbe
@ 2026-08-12 8:00 ` Boris Brezillon
0 siblings, 0 replies; 29+ messages in thread
From: Boris Brezillon @ 2026-08-12 8:00 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
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"
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 04/11] drm/panfrost: Move shrinker initialization and unplug one level down
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
` (2 preceding siblings ...)
2026-08-11 21:42 ` [PATCH v5 03/11] drm/panfrost: Remove unnecessary header file include Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
2026-08-12 8:07 ` Boris Brezillon
2026-08-11 21:42 ` [PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks Adrián Larumbe
` (6 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 04/11] drm/panfrost: Move shrinker initialization and unplug one level down
2026-08-11 21:42 ` [PATCH v5 04/11] drm/panfrost: Move shrinker initialization and unplug one level down Adrián Larumbe
@ 2026-08-12 8:07 ` Boris Brezillon
2026-08-13 12:13 ` Adrián Larumbe
0 siblings, 1 reply; 29+ messages in thread
From: Boris Brezillon @ 2026-08-12 8:07 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
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);
> }
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 04/11] drm/panfrost: Move shrinker initialization and unplug one level down
2026-08-12 8:07 ` Boris Brezillon
@ 2026-08-13 12:13 ` Adrián Larumbe
0 siblings, 0 replies; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-13 12:13 UTC (permalink / raw)
To: Boris Brezillon
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
On 12.08.2026 10:07, Boris Brezillon wrote:
> 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.
Yep, I don't have it enabled in the build config, and yet it compiles troublefree.
Steven had suggested getting rid of the feature check altogether, and I thought it
might trigger a compiler error, but it seems the linker might be resolving undefined
external variable symbols to 0.
> 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
This makes it more clear so I'll go for it in the next iteration.
> 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);
> > }
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
` (3 preceding siblings ...)
2026-08-11 21:42 ` [PATCH v5 04/11] drm/panfrost: Move shrinker initialization and unplug one level down Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
2026-08-12 9:07 ` Boris Brezillon
2026-08-11 21:42 ` [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init Adrián Larumbe
` (5 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks
2026-08-11 21:42 ` [PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks Adrián Larumbe
@ 2026-08-12 9:07 ` Boris Brezillon
2026-08-13 13:22 ` Adrián Larumbe
0 siblings, 1 reply; 29+ messages in thread
From: Boris Brezillon @ 2026-08-12 9:07 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
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).
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks
2026-08-12 9:07 ` Boris Brezillon
@ 2026-08-13 13:22 ` Adrián Larumbe
0 siblings, 0 replies; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-13 13:22 UTC (permalink / raw)
To: Boris Brezillon
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
On 12.08.2026 11:07, Boris Brezillon wrote:
> 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()).
Should I then move all the prepares and unprepares into clk_disable_assert_reset()
and its counterpart, and leave panfrost_clk_init() only to do clk_get's and clk_get_rate's ?
> > + 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()
Acked.
> > + 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.
I think I removed some fini calls in the driver probe init error path because
I thought this was already being done inside the RPM suspend callback.
I'll restore them and then like you said, make sure in RPM suspend they aren't
called it the device wasn't initialised.
> > +
> > + 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.
I had assumed by the time drive_removed() is called, there are no remaining open
contexts and no inflight jobs, but seems that I was wrong about it.
> 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.
I threw them both into the same helper because I thought panfrost_device_runtime_suspend
should remain statice inside panfrost_device.c. Are you fine with me making it public?
> > +}
> > +
> > 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.
I reordered the call sequence to be the inverse of that in the panfrost device init function.
> > 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.
Acked.
> >
> > @@ -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).
Acked.
> > +
> > 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().
I thought pm_runtime_set_suspended() would be unnecessary because after getting rid of
pm_runtime_disable(), we can trust pm_runtime_put_sync_suspend() would set the device
to 'suspended' when the usage count falls to 0 and suspends the device synchronously.
> > }
> >
> > 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).
Acked.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
` (4 preceding siblings ...)
2026-08-11 21:42 ` [PATCH v5 05/11] drm/panfrost: Move all device power up and down into RPM callbacks Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
2026-08-12 9:14 ` Boris Brezillon
2026-08-11 21:42 ` [PATCH v5 07/11] drm/panfrost: Add debugfs knob for manually triggering a GPU reset Adrián Larumbe
` (4 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init
2026-08-11 21:42 ` [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init Adrián Larumbe
@ 2026-08-12 9:14 ` Boris Brezillon
2026-08-13 12:41 ` Adrián Larumbe
0 siblings, 1 reply; 29+ messages in thread
From: Boris Brezillon @ 2026-08-12 9:14 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
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;
> }
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init
2026-08-12 9:14 ` Boris Brezillon
@ 2026-08-13 12:41 ` Adrián Larumbe
0 siblings, 0 replies; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-13 12:41 UTC (permalink / raw)
To: Boris Brezillon
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
On 12.08.2026 11:14, Boris Brezillon wrote:
> 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?
Do you mean doing it right before returning from panfrost_device_init()?
I thought about this, that maybe all device interrupts (gpu, mmu, jm) should be
enabled only when all the other subsystems are initialised, perhaps in a sort of
panfrost_device_init_initerrupts() function at the very end of panfrost_device_init().
Some of the pre-existing issues detected by Sashiko were about potential nasty interactions
of the IRQ handler when triggered by a spurious interrupt at the time some of the driver
structures aren't fully initialised, so this might be good. What do you think?
> >
> > 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;
> > }
> >
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 07/11] drm/panfrost: Add debugfs knob for manually triggering a GPU reset
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
` (5 preceding siblings ...)
2026-08-11 21:42 ` [PATCH v5 06/11] drm/panfrost: Explicitly enable MMU interrupts at device init Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
2026-08-12 9:17 ` Boris Brezillon
2026-08-11 21:42 ` [PATCH v5 08/11] drm/panfrost: Move perfcnt GPU disable sequence into a helper Adrián Larumbe
` (3 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 07/11] drm/panfrost: Add debugfs knob for manually triggering a GPU reset
2026-08-11 21:42 ` [PATCH v5 07/11] drm/panfrost: Add debugfs knob for manually triggering a GPU reset Adrián Larumbe
@ 2026-08-12 9:17 ` Boris Brezillon
2026-08-13 12:20 ` Adrián Larumbe
0 siblings, 1 reply; 29+ messages in thread
From: Boris Brezillon @ 2026-08-12 9:17 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
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
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 07/11] drm/panfrost: Add debugfs knob for manually triggering a GPU reset
2026-08-12 9:17 ` Boris Brezillon
@ 2026-08-13 12:20 ` Adrián Larumbe
0 siblings, 0 replies; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-13 12:20 UTC (permalink / raw)
To: Boris Brezillon
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
On 12.08.2026 11:17, Boris Brezillon wrote:
> 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().
Acked, will do for the next iteration. Also, I noticed there's quite a bit of debugfs stuff
in panfrost_drv.c. I should probably move it all under the relevant subsystem file and make
the debugfs init functions available to panfrost_drv.c
> > }
> > #endif
> >
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 08/11] drm/panfrost: Move perfcnt GPU disable sequence into a helper
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
` (6 preceding siblings ...)
2026-08-11 21:42 ` [PATCH v5 07/11] drm/panfrost: Add debugfs knob for manually triggering a GPU reset Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
2026-08-12 9:48 ` Boris Brezillon
2026-08-11 21:42 ` [PATCH v5 09/11] drm/panfrost: Introduce a reset lock Adrián Larumbe
` (2 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 08/11] drm/panfrost: Move perfcnt GPU disable sequence into a helper
2026-08-11 21:42 ` [PATCH v5 08/11] drm/panfrost: Move perfcnt GPU disable sequence into a helper Adrián Larumbe
@ 2026-08-12 9:48 ` Boris Brezillon
2026-08-13 12:28 ` Adrián Larumbe
0 siblings, 1 reply; 29+ messages in thread
From: Boris Brezillon @ 2026-08-12 9:48 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
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);
> }
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 08/11] drm/panfrost: Move perfcnt GPU disable sequence into a helper
2026-08-12 9:48 ` Boris Brezillon
@ 2026-08-13 12:28 ` Adrián Larumbe
0 siblings, 0 replies; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-13 12:28 UTC (permalink / raw)
To: Boris Brezillon
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
On 12.08.2026 11:48, Boris Brezillon wrote:
> 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.
Acked.
> > +{
> > + 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);
> > }
> >
Adrian Larumbe
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 09/11] drm/panfrost: Introduce a reset lock
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
` (7 preceding siblings ...)
2026-08-11 21:42 ` [PATCH v5 08/11] drm/panfrost: Move perfcnt GPU disable sequence into a helper Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
2026-08-12 9:44 ` Boris Brezillon
2026-08-12 9:45 ` Boris Brezillon
2026-08-11 21:42 ` [PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence Adrián Larumbe
2026-08-11 21:42 ` [PATCH v5 11/11] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field Adrián Larumbe
10 siblings, 2 replies; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 09/11] drm/panfrost: Introduce a reset lock
2026-08-11 21:42 ` [PATCH v5 09/11] drm/panfrost: Introduce a reset lock Adrián Larumbe
@ 2026-08-12 9:44 ` Boris Brezillon
2026-08-12 9:45 ` Boris Brezillon
1 sibling, 0 replies; 29+ messages in thread
From: Boris Brezillon @ 2026-08-12 9:44 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
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)
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 09/11] drm/panfrost: Introduce a reset lock
2026-08-11 21:42 ` [PATCH v5 09/11] drm/panfrost: Introduce a reset lock Adrián Larumbe
2026-08-12 9:44 ` Boris Brezillon
@ 2026-08-12 9:45 ` Boris Brezillon
2026-08-13 12:50 ` Adrián Larumbe
1 sibling, 1 reply; 29+ messages in thread
From: Boris Brezillon @ 2026-08-12 9:45 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
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)
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 09/11] drm/panfrost: Introduce a reset lock
2026-08-12 9:45 ` Boris Brezillon
@ 2026-08-13 12:50 ` Adrián Larumbe
0 siblings, 0 replies; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-13 12:50 UTC (permalink / raw)
To: Boris Brezillon
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
On 12.08.2026 11:45, Boris Brezillon wrote:
> 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.
Sorry about this, I seemed to have botched the final interactive rebase.
> > +
> > 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)
> >
Adrian Larumbe
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
` (8 preceding siblings ...)
2026-08-11 21:42 ` [PATCH v5 09/11] drm/panfrost: Introduce a reset lock Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
2026-08-12 10:06 ` Boris Brezillon
2026-08-11 21:42 ` [PATCH v5 11/11] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field Adrián Larumbe
10 siblings, 1 reply; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong
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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence
2026-08-11 21:42 ` [PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence Adrián Larumbe
@ 2026-08-12 10:06 ` Boris Brezillon
2026-08-13 12:50 ` Adrián Larumbe
0 siblings, 1 reply; 29+ messages in thread
From: Boris Brezillon @ 2026-08-12 10:06 UTC (permalink / raw)
To: Adrián Larumbe
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
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
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence
2026-08-12 10:06 ` Boris Brezillon
@ 2026-08-13 12:50 ` Adrián Larumbe
0 siblings, 0 replies; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-13 12:50 UTC (permalink / raw)
To: Boris Brezillon
Cc: Rob Herring, Steven Price, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Faith Ekstrand,
Marty E. Plummer, Tomeu Vizoso, Eric Anholt, Alyssa Rosenzweig,
Robin Murphy, Philipp Zabel, dri-devel, linux-kernel,
Collabora Kernel Team, Neil Armstrong
On 12.08.2026 12:06, Boris Brezillon wrote:
> 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.
I remember discussing this with you but I seem to have forgotten to look into this in the TRM.
I'll do it this time and post my findings here. If we actually don't need it, I'll do away
with it in the next iteration;
> > + 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).
I split it into a pre and post perfcnt reset so that I could stop any ongoing
completion waits on a dump as soon as possible, so that it wouldn't somehow
return successfully when the reset is ongoing and produce wrong counter values.
One alternative would be enclosing the completion wait in perfcnt_dup() inside
the reset lock, but I thought this wouldn't be ideal, because I thought resets
should always take preference over performance samples.
> > +a}
> > +
> > +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
> >
Adrian Larumbe
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 11/11] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field
2026-08-11 21:42 [PATCH v5 00/11] RPM, perfcnt and other minor fixes for Panfrost Adrián Larumbe
` (9 preceding siblings ...)
2026-08-11 21:42 ` [PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence Adrián Larumbe
@ 2026-08-11 21:42 ` Adrián Larumbe
10 siblings, 0 replies; 29+ messages in thread
From: Adrián Larumbe @ 2026-08-11 21:42 UTC (permalink / raw)
To: Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Faith Ekstrand, Marty E. Plummer, Tomeu Vizoso, Eric Anholt,
Alyssa Rosenzweig, Robin Murphy, Philipp Zabel
Cc: dri-devel, linux-kernel, Collabora Kernel Team,
Adrián Larumbe, Neil Armstrong
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
^ permalink raw reply related [flat|nested] 29+ messages in thread