Re: [PATCH v7 02/17] drm/panfrost: Move all DRM device initialisation into device_init()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Fri, 28 Aug 2026 21:56:42 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Ideally the probe() function will do as little as possible, and all device
> initialisation and registration should happen inside the panfrost device
> subsystem, just like it's done in Panthor. This also simplifies resource
> unwinding in the error path.
> 
> Do the same thing for DRM driver remove, as in, sweep most of the action
> into panfrost_device_fini(), just like we did for device probe.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 33 ++++++++++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_drv.c    | 44 +-----------------------------
>  2 files changed, 34 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 05c40d5a20b5..d2d2830f11a7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -8,6 +8,7 @@
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
> +#include <drm/drm_drv.h>
>  
>  #include "panfrost_device.h"
>  #include "panfrost_devfreq.h"
> @@ -216,6 +217,15 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  {
>  	int err;
>  
> +	pfdev->comp = of_device_get_match_data(pfdev->base.dev);
> +	if (!pfdev->comp)
> +		return -ENODEV;
> +
> +	pfdev->coherent = device_get_dma_attr(pfdev->base.dev) == DEV_DMA_COHERENT;
> +
> +	mutex_init(&pfdev->shrinker_lock);
> +	INIT_LIST_HEAD(&pfdev->shrinker_list);
> +
>  	mutex_init(&pfdev->sched_lock);
>  	INIT_LIST_HEAD(&pfdev->as_lru_list);
>  
> @@ -284,8 +294,25 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  	if (err)
>  		goto out_perfcnt;
>  
> +	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);
> +
> +	/*
> +	 * Register the DRM device with the core and the connectors with
> +	 * sysfs
> +	 */
> +	err = drm_dev_register(&pfdev->base, 0);
> +	if (err < 0)
> +		goto out_devreg;
> +
>  	return 0;
>  
> +out_devreg:

Not a huge fan of labels that describe where this is jumped from instead
of what is done under the label (that gets particularly confusing when
you start multiple locations jumping to the same label). So I'd suggest
renaming that one err_disable_rpm.

> +	pm_runtime_disable(pfdev->base.dev);

I think you need a pm_runtime_dont_use_autosuspend() call before
pm_runtime_disable().

> +	panfrost_gem_fini(pfdev);
>  out_perfcnt:
>  	panfrost_perfcnt_fini(pfdev);
>  out_job:
> @@ -304,11 +331,15 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  	panfrost_reset_fini(pfdev);
>  out_pm_domain:
>  	panfrost_pm_domain_fini(pfdev);
> +	pm_runtime_set_suspended(pfdev->base.dev);

Do we have a good reason for not flagging the device suspended just
after the pm_runtime_disable() call in the error path? I mean, sure
it's not truly suspended until the clks/regulators have been turned
off, but it also wasn't suspended the before the initial
pm_runtime_set_active() call, and I like the idea of undoing things in
reverse init order.

>  	return err;
>  }
>  
>  void panfrost_device_fini(struct panfrost_device *pfdev)
>  {
> +	pm_runtime_get_sync(pfdev->base.dev);

	pm_runtime_dont_use_autosuspend();

I see it fixed in patch 9, just like a few other issues that are made
more apparent by this code motion change. I guess it's fine but it
confused me, so it might be worth mentioning in the commit message.


> +	pm_runtime_disable(pfdev->base.dev);
> +
>  	panfrost_gem_fini(pfdev);
>  	panfrost_perfcnt_fini(pfdev);
>  	panfrost_jm_fini(pfdev);
> @@ -319,6 +350,8 @@ void panfrost_device_fini(struct panfrost_device *pfdev)
>  	panfrost_clk_fini(pfdev);
>  	panfrost_reset_fini(pfdev);
>  	panfrost_pm_domain_fini(pfdev);
> +
> +	pm_runtime_set_suspended(pfdev->base.dev);

Fixed in patch 9, but the RPM ref you acquire at the beginning of the
function is never returned, so you end up with an unbalanced get/put.
This is a pre-existing issue, I know, this catches the eye of the
reviewer so we better mention that existing issues around PM are
preserved and will be fixed later.

>  }
>  
>  #define PANFROST_EXCEPTION(id) \
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 9882a3ede75f..80996e311a9d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -964,7 +964,6 @@ MODULE_PARM_DESC(transparent_hugepage, "Use a dedicated tmpfs mount point with T
>  static int panfrost_probe(struct platform_device *pdev)
>  {
>  	struct panfrost_device *pfdev;
> -	int err;
>  
>  	pfdev = devm_drm_dev_alloc(&pdev->dev, &panfrost_drm_driver,
>  				   struct panfrost_device, base);
> @@ -973,45 +972,7 @@ static int panfrost_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, pfdev);
>  
> -	pfdev->comp = of_device_get_match_data(&pdev->dev);
> -	if (!pfdev->comp)
> -		return -ENODEV;
> -
> -	pfdev->coherent = device_get_dma_attr(&pdev->dev) == DEV_DMA_COHERENT;
> -
> -	mutex_init(&pfdev->shrinker_lock);
> -	INIT_LIST_HEAD(&pfdev->shrinker_list);
> -
> -	err = panfrost_device_init(pfdev);
> -	if (err) {
> -		if (err != -EPROBE_DEFER)
> -			dev_err(&pdev->dev, "Fatal error during GPU init\n");
> -		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);
> -
> -	/*
> -	 * Register the DRM device with the core and the connectors with
> -	 * sysfs
> -	 */
> -	err = drm_dev_register(&pfdev->base, 0);
> -	if (err < 0)
> -		goto err_out1;
> -
> -
> -	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;
> +	return panfrost_device_init(pfdev);
>  }
>  
>  static void panfrost_remove(struct platform_device *pdev)
> @@ -1020,10 +981,7 @@ 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);
>  	panfrost_device_fini(pfdev);
> -	pm_runtime_set_suspended(pfdev->base.dev);
>  }
>  
>  static ssize_t profiling_show(struct device *dev,
>