On Fri, 28 Aug 2026 21:56:42 +0100 Adrián Larumbe <[email protected]> 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 <[email protected]> > --- > 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, >
