| Message ID | 20190826223317.28509-2-robh@kernel.org |
|---|---|
| State | Accepted |
| Commit | 635430797d3fccb958e929d3911e39cb9c1ea641 |
| Headers | show |
| Series | panfrost: Locking and runtime PM fixes | expand |
On 26/08/2019 23:33, Rob Herring wrote: > There's a few issues with the runtime PM initialization. > > The documentation states pm_runtime_set_active() should be called before > pm_runtime_enable(). The pm_runtime_put_autosuspend() could suspend the GPU > before panfrost_perfcnt_init() is called which touches the h/w. The > autosuspend delay keeps things from breaking. There's no need explicitly > power off the GPU only to wake back up with pm_runtime_get_sync(). Just > delaying pm_runtime_enable to the end of probe is sufficient. > > Lets move all the runtime PM calls into the probe() function so they are > all in one place and are done after all initialization. Other the nitpick of whether the pm_runtime_enable() call should come at the end after the autosuspend setup, Reviewed-by: Robin Murphy :robin.murphy@arm.com> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Cc: Steven Price <steven.price@arm.com> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > v3: > - Move autosuspend setup after pm_runtime_enable as only autosuspend changes > trigger suspend. > - Fix autosuspend delay to 50ms instead of 0. > > drivers/gpu/drm/panfrost/panfrost_device.c | 9 --------- > drivers/gpu/drm/panfrost/panfrost_drv.c | 10 ++++++---- > 2 files changed, 6 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c > index 4da71bb56c20..73805210834e 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c > @@ -5,7 +5,6 @@ > #include <linux/clk.h> > #include <linux/reset.h> > #include <linux/platform_device.h> > -#include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > > #include "panfrost_device.h" > @@ -166,14 +165,6 @@ int panfrost_device_init(struct panfrost_device *pfdev) > if (err) > goto err_out4; > > - /* runtime PM will wake us up later */ > - panfrost_gpu_power_off(pfdev); > - > - pm_runtime_set_active(pfdev->dev); > - pm_runtime_get_sync(pfdev->dev); > - pm_runtime_mark_last_busy(pfdev->dev); > - pm_runtime_put_autosuspend(pfdev->dev); > - > err = panfrost_perfcnt_init(pfdev); > if (err) > goto err_out5; > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > index d74442d71048..bc2ddeb55f5d 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -523,10 +523,6 @@ static int panfrost_probe(struct platform_device *pdev) > mutex_init(&pfdev->shrinker_lock); > INIT_LIST_HEAD(&pfdev->shrinker_list); > > - pm_runtime_use_autosuspend(pfdev->dev); > - pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */ > - pm_runtime_enable(pfdev->dev); > - > err = panfrost_device_init(pfdev); > if (err) { > if (err != -EPROBE_DEFER) > @@ -541,6 +537,12 @@ static int panfrost_probe(struct platform_device *pdev) > goto err_out1; > } > > + pm_runtime_set_active(pfdev->dev); > + pm_runtime_mark_last_busy(pfdev->dev); > + pm_runtime_enable(pfdev->dev); > + pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */ > + pm_runtime_use_autosuspend(pfdev->dev); > + > /* > * Register the DRM device with the core and the connectors with > * sysfs > -- > 2.20.1 >
On 26/08/2019 23:33, Rob Herring wrote: > There's a few issues with the runtime PM initialization. > > The documentation states pm_runtime_set_active() should be called before > pm_runtime_enable(). The pm_runtime_put_autosuspend() could suspend the GPU > before panfrost_perfcnt_init() is called which touches the h/w. The > autosuspend delay keeps things from breaking. There's no need explicitly > power off the GPU only to wake back up with pm_runtime_get_sync(). Just > delaying pm_runtime_enable to the end of probe is sufficient. > > Lets move all the runtime PM calls into the probe() function so they are > all in one place and are done after all initialization. > > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Cc: Steven Price <steven.price@arm.com> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> > Signed-off-by: Rob Herring <robh@kernel.org> Reviewed-by: Steven Price <steven.price@arm.com> Steve > --- > v3: > - Move autosuspend setup after pm_runtime_enable as only autosuspend changes > trigger suspend. > - Fix autosuspend delay to 50ms instead of 0. > > drivers/gpu/drm/panfrost/panfrost_device.c | 9 --------- > drivers/gpu/drm/panfrost/panfrost_drv.c | 10 ++++++---- > 2 files changed, 6 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c > index 4da71bb56c20..73805210834e 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c > @@ -5,7 +5,6 @@ > #include <linux/clk.h> > #include <linux/reset.h> > #include <linux/platform_device.h> > -#include <linux/pm_runtime.h> > #include <linux/regulator/consumer.h> > > #include "panfrost_device.h" > @@ -166,14 +165,6 @@ int panfrost_device_init(struct panfrost_device *pfdev) > if (err) > goto err_out4; > > - /* runtime PM will wake us up later */ > - panfrost_gpu_power_off(pfdev); > - > - pm_runtime_set_active(pfdev->dev); > - pm_runtime_get_sync(pfdev->dev); > - pm_runtime_mark_last_busy(pfdev->dev); > - pm_runtime_put_autosuspend(pfdev->dev); > - > err = panfrost_perfcnt_init(pfdev); > if (err) > goto err_out5; > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > index d74442d71048..bc2ddeb55f5d 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -523,10 +523,6 @@ static int panfrost_probe(struct platform_device *pdev) > mutex_init(&pfdev->shrinker_lock); > INIT_LIST_HEAD(&pfdev->shrinker_list); > > - pm_runtime_use_autosuspend(pfdev->dev); > - pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */ > - pm_runtime_enable(pfdev->dev); > - > err = panfrost_device_init(pfdev); > if (err) { > if (err != -EPROBE_DEFER) > @@ -541,6 +537,12 @@ static int panfrost_probe(struct platform_device *pdev) > goto err_out1; > } > > + pm_runtime_set_active(pfdev->dev); > + pm_runtime_mark_last_busy(pfdev->dev); > + pm_runtime_enable(pfdev->dev); > + pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */ > + pm_runtime_use_autosuspend(pfdev->dev); > + > /* > * Register the DRM device with the core and the connectors with > * sysfs > -- > 2.20.1 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c index 4da71bb56c20..73805210834e 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.c +++ b/drivers/gpu/drm/panfrost/panfrost_device.c @@ -5,7 +5,6 @@ #include <linux/clk.h> #include <linux/reset.h> #include <linux/platform_device.h> -#include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> #include "panfrost_device.h" @@ -166,14 +165,6 @@ int panfrost_device_init(struct panfrost_device *pfdev) if (err) goto err_out4; - /* runtime PM will wake us up later */ - panfrost_gpu_power_off(pfdev); - - pm_runtime_set_active(pfdev->dev); - pm_runtime_get_sync(pfdev->dev); - pm_runtime_mark_last_busy(pfdev->dev); - pm_runtime_put_autosuspend(pfdev->dev); - err = panfrost_perfcnt_init(pfdev); if (err) goto err_out5; diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index d74442d71048..bc2ddeb55f5d 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -523,10 +523,6 @@ static int panfrost_probe(struct platform_device *pdev) mutex_init(&pfdev->shrinker_lock); INIT_LIST_HEAD(&pfdev->shrinker_list); - pm_runtime_use_autosuspend(pfdev->dev); - pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */ - pm_runtime_enable(pfdev->dev); - err = panfrost_device_init(pfdev); if (err) { if (err != -EPROBE_DEFER) @@ -541,6 +537,12 @@ static int panfrost_probe(struct platform_device *pdev) goto err_out1; } + pm_runtime_set_active(pfdev->dev); + pm_runtime_mark_last_busy(pfdev->dev); + pm_runtime_enable(pfdev->dev); + pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */ + pm_runtime_use_autosuspend(pfdev->dev); + /* * Register the DRM device with the core and the connectors with * sysfs