Re: [PATCH v12 08/15] drm/panfrost: Move all DRM device initialisation into device_init()

[Steven Price]

On 29/09/2026 04:44, Adrián Larumbe 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.
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx>

Reviewed-by: Steven Price <steven.price@xxxxxxx>

> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 49 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_drv.c    | 52 +-----------------------------
>  2 files changed, 50 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 9f2b1967a398..c6bf3d0663df 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"
> @@ -228,8 +229,15 @@ static int panfrost_pm_domain_init(struct panfrost_device *pfdev)
>  
>  int panfrost_device_init(struct panfrost_device *pfdev)
>  {
> +	bool device_initialised = false;
>  	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;
> +
>  #ifdef CONFIG_DEBUG_FS
>  	mutex_init(&pfdev->debugfs.gems_lock);
>  	INIT_LIST_HEAD(&pfdev->debugfs.gems_list);
> @@ -291,8 +299,35 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  	if (err)
>  		goto out_perfcnt;
>  
> +	device_initialised = true;
> +
> +	/* The reason we must manually set the PM status and usage counter is
> +	 * we have just powered the device up but did not go through the PM
> +	 * runtime resume callback, so we need to update these ourselves.
> +	 */
> +	pm_runtime_get_noresume(pfdev->base.dev);
> +	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_disable_rpm;
> +
> +	pm_runtime_put_autosuspend(pfdev->base.dev);
> +
>  	return 0;
>  
> +err_disable_rpm:
> +	pm_runtime_dont_use_autosuspend(pfdev->base.dev);
> +	pm_runtime_disable(pfdev->base.dev);
> +	panfrost_gem_fini(pfdev);
>  out_perfcnt:
>  	panfrost_perfcnt_fini(pfdev);
>  out_job:
> @@ -311,11 +346,22 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  	panfrost_reset_fini(pfdev);
>  out_pm_domain:
>  	panfrost_pm_domain_fini(pfdev);
> +
> +	if (device_initialised) {
> +		pm_runtime_set_suspended(pfdev->base.dev);
> +		pm_runtime_put_noidle(pfdev->base.dev);
> +	}
> +
>  	return err;
>  }
>  
>  void panfrost_device_fini(struct panfrost_device *pfdev)
>  {
> +	pm_runtime_get_sync(pfdev->base.dev);
> +
> +	pm_runtime_dont_use_autosuspend(pfdev->base.dev);
> +	pm_runtime_disable(pfdev->base.dev);
> +
>  	panfrost_gem_fini(pfdev);
>  	panfrost_perfcnt_fini(pfdev);
>  	panfrost_jm_fini(pfdev);
> @@ -326,6 +372,9 @@ 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);
> +	pm_runtime_put_noidle(pfdev->base.dev);
>  }
>  
>  #define PANFROST_EXCEPTION(id) \
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 907d4a14a0b5..f77780c72a1a 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -830,7 +830,6 @@ static const struct drm_driver panfrost_drm_driver = {
>  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);
> @@ -839,50 +838,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;
> -
> -	err = panfrost_device_init(pfdev);
> -	if (err) {
> -		if (err != -EPROBE_DEFER)
> -			dev_err(&pdev->dev, "Fatal error during GPU init\n");
> -		goto err_out0;
> -	}
> -
> -	/* The reason we must manually set the PM status and usage counter is
> -	 * we have just powered the device up but did not go through the PM
> -	 * runtime resume callback, so we need to update these ourselves.
> -	 */
> -	pm_runtime_get_noresume(pfdev->base.dev);
> -	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;
> -
> -	pm_runtime_put_autosuspend(pfdev->base.dev);
> -
> -	return 0;
> -
> -err_out1:
> -	pm_runtime_dont_use_autosuspend(pfdev->base.dev);
> -	pm_runtime_disable(pfdev->base.dev);
> -	panfrost_device_fini(pfdev);
> -	pm_runtime_set_suspended(pfdev->base.dev);
> -	pm_runtime_put_noidle(pfdev->base.dev);
> -err_out0:
> -	return err;
> +	return panfrost_device_init(pfdev);
>  }
>  
>  static void panfrost_remove(struct platform_device *pdev)
> @@ -891,13 +847,7 @@ static void panfrost_remove(struct platform_device *pdev)
>  
>  	drm_dev_unregister(&pfdev->base);
>  
> -	pm_runtime_get_sync(pfdev->base.dev);
> -	pm_runtime_dont_use_autosuspend(pfdev->base.dev);
> -	pm_runtime_disable(pfdev->base.dev);
>  	panfrost_device_fini(pfdev);
> -	pm_runtime_set_suspended(pfdev->base.dev);
> -	pm_runtime_put_noidle(pfdev->base.dev);
> -
>  }
>  
>  static ssize_t profiling_show(struct device *dev,
>