diff mbox series

[v3,1/8] drm/panfrost: Rework runtime PM initialization

Message ID 20190826223317.28509-2-robh@kernel.org
State Accepted
Commit 635430797d3fccb958e929d3911e39cb9c1ea641
Headers show
Series panfrost: Locking and runtime PM fixes | expand

Commit Message

Rob Herring Aug. 26, 2019, 10:33 p.m. UTC
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>
---
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(-)

--
2.20.1

Comments

Robin Murphy Aug. 27, 2019, 10:59 a.m. UTC | #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.

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
>
Steven Price Aug. 28, 2019, 10:39 a.m. UTC | #2
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 mbox series

Patch

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