| Message ID | 20190823021216.5862-3-robh@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/8] drm/panfrost: Fix possible suspend in panfrost_remove | expand |
On 23/08/2019 03:12, 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: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > v2: new patch > > 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..f27e3d6aec12 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_use_autosuspend(pfdev->dev); > + pm_runtime_set_autosuspend_delay(pfdev->dev, 0); /* ~3 frames */ Hmm... different timeout, same comment - something seems amiss there, unless perhaps it's all within rounding error anyway? Other than that, the overall change looks sensible to me. Robin. > + pm_runtime_mark_last_busy(pfdev->dev); > + pm_runtime_enable(pfdev->dev); > + > /* > * Register the DRM device with the core and the connectors with > * sysfs >
On Fri, Aug 23, 2019 at 5:54 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 23/08/2019 03:12, 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: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > v2: new patch > > > > 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..f27e3d6aec12 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_use_autosuspend(pfdev->dev); > > + pm_runtime_set_autosuspend_delay(pfdev->dev, 0); /* ~3 frames */ > > Hmm... different timeout, same comment - something seems amiss there, > unless perhaps it's all within rounding error anyway? Leftover debugging to force issues. It should be 50. Rob
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..f27e3d6aec12 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_use_autosuspend(pfdev->dev); + pm_runtime_set_autosuspend_delay(pfdev->dev, 0); /* ~3 frames */ + pm_runtime_mark_last_busy(pfdev->dev); + pm_runtime_enable(pfdev->dev); + /* * Register the DRM device with the core and the connectors with * sysfs
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: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Signed-off-by: Rob Herring <robh@kernel.org> --- v2: new patch drivers/gpu/drm/panfrost/panfrost_device.c | 9 --------- drivers/gpu/drm/panfrost/panfrost_drv.c | 10 ++++++---- 2 files changed, 6 insertions(+), 13 deletions(-)