[PATCH v2] drm/msm/adreno: adreno_gpu: Use suspend() instead of idle() on load error

[PATCH v2] drm/msm/adreno: adreno_gpu: Use suspend() instead of idle() on load error

[Konrad Dybcio]

The adreno_load_gpu() path is guarded by an error check on
adreno_load_fw(). This function is responsible for loading
Qualcomm-only-signed binaries (e.g. SQE and GMU FW for A6XX), but it
does not take the vendor-signed ZAP blob into account.

By embedding the SQE (and GMU, if necessary) firmware into the
initrd/kernel, we can trigger and unfortunate path that would not bail
out early and proceed with gpu->hw_init(). That will fail, as the ZAP
loader path will not find the firmware and return back to
adreno_load_gpu().

This error path involves pm_runtime_put_sync() which then calls idle()
instead of suspend(). This is suboptimal, as it means that we're not
going through the clean shutdown sequence. With at least A619_holi, this
makes the GPU not wake up until it goes through at least one more
start-fail-stop cycle. The pm_runtime_put_sync that appears in the error
path actually does not guarantee that because of the earlier enabling of
runtime autosuspend.

Fix that by using pm_runtime_put_sync_suspend to force a clean shutdown.

Test cases:
1. All firmware baked into kernel
2. error loading ZAP fw in initrd -> load from rootfs at DE start

Both succeed on A619_holi (SM6375) and A630 (SDM845).

Fixes: 0d997f95b70f ("drm/msm/adreno: fix runtime PM imbalance at gpu load")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
v1 -> v2:
- Improve the commit message and the reasoning within

 drivers/gpu/drm/msm/adreno/adreno_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index f61896629be6..59f3302e8167 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -477,7 +477,7 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
 	return gpu;
 
 err_put_rpm:
-	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_put_sync_suspend(&pdev->dev);
 err_disable_rpm:
 	pm_runtime_disable(&pdev->dev);
 
-- 
2.40.0

Re: [PATCH v2] drm/msm/adreno: adreno_gpu: Use suspend() instead of idle() on load error

[Johan Hovold]

On Fri, Mar 31, 2023 at 01:15:16AM +0200, Konrad Dybcio wrote:
> The adreno_load_gpu() path is guarded by an error check on
> adreno_load_fw(). This function is responsible for loading
> Qualcomm-only-signed binaries (e.g. SQE and GMU FW for A6XX), but it
> does not take the vendor-signed ZAP blob into account.
> 
> By embedding the SQE (and GMU, if necessary) firmware into the
> initrd/kernel, we can trigger and unfortunate path that would not bail
> out early and proceed with gpu->hw_init(). That will fail, as the ZAP
> loader path will not find the firmware and return back to
> adreno_load_gpu().
> 
> This error path involves pm_runtime_put_sync() which then calls idle()
> instead of suspend(). This is suboptimal, as it means that we're not
> going through the clean shutdown sequence. With at least A619_holi, this
> makes the GPU not wake up until it goes through at least one more
> start-fail-stop cycle. The pm_runtime_put_sync that appears in the error
> path actually does not guarantee that because of the earlier enabling of
> runtime autosuspend.
> 
> Fix that by using pm_runtime_put_sync_suspend to force a clean shutdown.
> 
> Test cases:
> 1. All firmware baked into kernel
> 2. error loading ZAP fw in initrd -> load from rootfs at DE start
> 
> Both succeed on A619_holi (SM6375) and A630 (SDM845).
> 
> Fixes: 0d997f95b70f ("drm/msm/adreno: fix runtime PM imbalance at gpu load")

As this one is marked for stable, you also need:

Cc: stable@vger.kernel.org	# 6.0

> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

> ---
> v1 -> v2:
> - Improve the commit message and the reasoning within
> 
>  drivers/gpu/drm/msm/adreno/adreno_device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index f61896629be6..59f3302e8167 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -477,7 +477,7 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
>  	return gpu;
>  
>  err_put_rpm:
> -	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_put_sync_suspend(&pdev->dev);
>  err_disable_rpm:
>  	pm_runtime_disable(&pdev->dev);

Johan