[v2] drm/rockchip: Don't continue trying to enable crtc on failure
diff mbox

Message ID 1471302749-2811-1-git-send-email-seanpaul@chromium.org
State New
Headers show

Commit Message

Sean Paul Aug. 15, 2016, 11:12 p.m. UTC
If vop_enable fails, don't continue on, it causes system hangs.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---

This patch uses the new DRM_DEV_ERROR logging, so it should be applied on
top of "[PATCH 2/2] drm/rockchip: Use DRM_DEV_ERROR in vop".

Changes in v2:
	- Escalate dev_err to WARN_ON for clk_enable failures (Daniel Vetter)

Sean


 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 31 ++++++++++++++++-------------
 1 file changed, 17 insertions(+), 14 deletions(-)

Comments

Yakir Yang Aug. 17, 2016, 7:28 a.m. UTC | #1
Sean,

On 08/16/2016 07:12 AM, Sean Paul wrote:
> If vop_enable fails, don't continue on, it causes system hangs.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Also meet this problem on my Rk3399 Kevin board. VOP just failed to get 
the pm_runtime at resume time, but driver still just continue without 
anything enable rightly, oops, and then system crashed :(

So this patch looks good to me, and also fix my problem, thanks:

Reviewed-by: Yakir Yang <ykk@rock-chips.com>
Tested-by: Yakir Yang <ykk@rock-chips.com>

Thanks,
- Yakir
> ---
>
> This patch uses the new DRM_DEV_ERROR logging, so it should be applied on
> top of "[PATCH 2/2] drm/rockchip: Use DRM_DEV_ERROR in vop".
>
> Changes in v2:
> 	- Escalate dev_err to WARN_ON for clk_enable failures (Daniel Vetter)
>
> Sean
>
>
>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 31 ++++++++++++++++-------------
>   1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index ec8ad00..a176d03 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -428,7 +428,7 @@ static void vop_dsp_hold_valid_irq_disable(struct vop *vop)
>   	spin_unlock_irqrestore(&vop->irq_lock, flags);
>   }
>   
> -static void vop_enable(struct drm_crtc *crtc)
> +static int vop_enable(struct drm_crtc *crtc)
>   {
>   	struct vop *vop = to_vop(crtc);
>   	int ret;
> @@ -436,26 +436,20 @@ static void vop_enable(struct drm_crtc *crtc)
>   	ret = pm_runtime_get_sync(vop->dev);
>   	if (ret < 0) {
>   		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
> -		return;
> +		goto err_put_pm_runtime;
>   	}
>   
>   	ret = clk_enable(vop->hclk);
> -	if (ret < 0) {
> -		dev_err(vop->dev, "failed to enable hclk - %d\n", ret);
> -		return;
> -	}
> +	if (WARN_ON(ret < 0))
> +		goto err_put_pm_runtime;
>   
>   	ret = clk_enable(vop->dclk);
> -	if (ret < 0) {
> -		dev_err(vop->dev, "failed to enable dclk - %d\n", ret);
> +	if (WARN_ON(ret < 0))
>   		goto err_disable_hclk;
> -	}
>   
>   	ret = clk_enable(vop->aclk);
> -	if (ret < 0) {
> -		dev_err(vop->dev, "failed to enable aclk - %d\n", ret);
> +	if (WARN_ON(ret < 0))
>   		goto err_disable_dclk;
> -	}
>   
>   	/*
>   	 * Slave iommu shares power, irq and clock with vop.  It was associated
> @@ -485,7 +479,7 @@ static void vop_enable(struct drm_crtc *crtc)
>   
>   	drm_crtc_vblank_on(crtc);
>   
> -	return;
> +	return 0;
>   
>   err_disable_aclk:
>   	clk_disable(vop->aclk);
> @@ -493,6 +487,9 @@ err_disable_dclk:
>   	clk_disable(vop->dclk);
>   err_disable_hclk:
>   	clk_disable(vop->hclk);
> +err_put_pm_runtime:
> +	pm_runtime_put_sync(vop->dev);
> +	return ret;
>   }
>   
>   static void vop_crtc_disable(struct drm_crtc *crtc)
> @@ -912,10 +909,16 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
>   	u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
>   	u16 vact_end = vact_st + vdisplay;
>   	uint32_t val;
> +	int ret;
>   
>   	WARN_ON(vop->event);
>   
> -	vop_enable(crtc);
> +	ret = vop_enable(crtc);
> +	if (ret) {
> +		DRM_DEV_ERROR(vop->dev, "Failed to enable vop (%d)\n", ret);
> +		return;
> +	}
> +
>   	/*
>   	 * If dclk rate is zero, mean that scanout is stop,
>   	 * we don't need wait any more.
Sean Paul Aug. 23, 2016, 3:36 p.m. UTC | #2
On Wed, Aug 17, 2016 at 3:28 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> Sean,
>
> On 08/16/2016 07:12 AM, Sean Paul wrote:
>>
>> If vop_enable fails, don't continue on, it causes system hangs.
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>
>
> Also meet this problem on my Rk3399 Kevin board. VOP just failed to get the
> pm_runtime at resume time, but driver still just continue without anything
> enable rightly, oops, and then system crashed :(
>
> So this patch looks good to me, and also fix my problem, thanks:
>
> Reviewed-by: Yakir Yang <ykk@rock-chips.com>
> Tested-by: Yakir Yang <ykk@rock-chips.com>
>

Applied to -misc

Sean

> Thanks,
> - Yakir
>
>> ---
>>
>> This patch uses the new DRM_DEV_ERROR logging, so it should be applied on
>> top of "[PATCH 2/2] drm/rockchip: Use DRM_DEV_ERROR in vop".
>>
>> Changes in v2:
>>         - Escalate dev_err to WARN_ON for clk_enable failures (Daniel
>> Vetter)
>>
>> Sean
>>
>>
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 31
>> ++++++++++++++++-------------
>>   1 file changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index ec8ad00..a176d03 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -428,7 +428,7 @@ static void vop_dsp_hold_valid_irq_disable(struct vop
>> *vop)
>>         spin_unlock_irqrestore(&vop->irq_lock, flags);
>>   }
>>   -static void vop_enable(struct drm_crtc *crtc)
>> +static int vop_enable(struct drm_crtc *crtc)
>>   {
>>         struct vop *vop = to_vop(crtc);
>>         int ret;
>> @@ -436,26 +436,20 @@ static void vop_enable(struct drm_crtc *crtc)
>>         ret = pm_runtime_get_sync(vop->dev);
>>         if (ret < 0) {
>>                 dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
>> -               return;
>> +               goto err_put_pm_runtime;
>>         }
>>         ret = clk_enable(vop->hclk);
>> -       if (ret < 0) {
>> -               dev_err(vop->dev, "failed to enable hclk - %d\n", ret);
>> -               return;
>> -       }
>> +       if (WARN_ON(ret < 0))
>> +               goto err_put_pm_runtime;
>>         ret = clk_enable(vop->dclk);
>> -       if (ret < 0) {
>> -               dev_err(vop->dev, "failed to enable dclk - %d\n", ret);
>> +       if (WARN_ON(ret < 0))
>>                 goto err_disable_hclk;
>> -       }
>>         ret = clk_enable(vop->aclk);
>> -       if (ret < 0) {
>> -               dev_err(vop->dev, "failed to enable aclk - %d\n", ret);
>> +       if (WARN_ON(ret < 0))
>>                 goto err_disable_dclk;
>> -       }
>>         /*
>>          * Slave iommu shares power, irq and clock with vop.  It was
>> associated
>> @@ -485,7 +479,7 @@ static void vop_enable(struct drm_crtc *crtc)
>>         drm_crtc_vblank_on(crtc);
>>   -     return;
>> +       return 0;
>>     err_disable_aclk:
>>         clk_disable(vop->aclk);
>> @@ -493,6 +487,9 @@ err_disable_dclk:
>>         clk_disable(vop->dclk);
>>   err_disable_hclk:
>>         clk_disable(vop->hclk);
>> +err_put_pm_runtime:
>> +       pm_runtime_put_sync(vop->dev);
>> +       return ret;
>>   }
>>     static void vop_crtc_disable(struct drm_crtc *crtc)
>> @@ -912,10 +909,16 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
>>         u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
>>         u16 vact_end = vact_st + vdisplay;
>>         uint32_t val;
>> +       int ret;
>>         WARN_ON(vop->event);
>>   -     vop_enable(crtc);
>> +       ret = vop_enable(crtc);
>> +       if (ret) {
>> +               DRM_DEV_ERROR(vop->dev, "Failed to enable vop (%d)\n",
>> ret);
>> +               return;
>> +       }
>> +
>>         /*
>>          * If dclk rate is zero, mean that scanout is stop,
>>          * we don't need wait any more.
>
>
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index ec8ad00..a176d03 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -428,7 +428,7 @@  static void vop_dsp_hold_valid_irq_disable(struct vop *vop)
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
 }
 
-static void vop_enable(struct drm_crtc *crtc)
+static int vop_enable(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
 	int ret;
@@ -436,26 +436,20 @@  static void vop_enable(struct drm_crtc *crtc)
 	ret = pm_runtime_get_sync(vop->dev);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
-		return;
+		goto err_put_pm_runtime;
 	}
 
 	ret = clk_enable(vop->hclk);
-	if (ret < 0) {
-		dev_err(vop->dev, "failed to enable hclk - %d\n", ret);
-		return;
-	}
+	if (WARN_ON(ret < 0))
+		goto err_put_pm_runtime;
 
 	ret = clk_enable(vop->dclk);
-	if (ret < 0) {
-		dev_err(vop->dev, "failed to enable dclk - %d\n", ret);
+	if (WARN_ON(ret < 0))
 		goto err_disable_hclk;
-	}
 
 	ret = clk_enable(vop->aclk);
-	if (ret < 0) {
-		dev_err(vop->dev, "failed to enable aclk - %d\n", ret);
+	if (WARN_ON(ret < 0))
 		goto err_disable_dclk;
-	}
 
 	/*
 	 * Slave iommu shares power, irq and clock with vop.  It was associated
@@ -485,7 +479,7 @@  static void vop_enable(struct drm_crtc *crtc)
 
 	drm_crtc_vblank_on(crtc);
 
-	return;
+	return 0;
 
 err_disable_aclk:
 	clk_disable(vop->aclk);
@@ -493,6 +487,9 @@  err_disable_dclk:
 	clk_disable(vop->dclk);
 err_disable_hclk:
 	clk_disable(vop->hclk);
+err_put_pm_runtime:
+	pm_runtime_put_sync(vop->dev);
+	return ret;
 }
 
 static void vop_crtc_disable(struct drm_crtc *crtc)
@@ -912,10 +909,16 @@  static void vop_crtc_enable(struct drm_crtc *crtc)
 	u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
 	u16 vact_end = vact_st + vdisplay;
 	uint32_t val;
+	int ret;
 
 	WARN_ON(vop->event);
 
-	vop_enable(crtc);
+	ret = vop_enable(crtc);
+	if (ret) {
+		DRM_DEV_ERROR(vop->dev, "Failed to enable vop (%d)\n", ret);
+		return;
+	}
+
 	/*
 	 * If dclk rate is zero, mean that scanout is stop,
 	 * we don't need wait any more.