* [PATCH] drm/rockchip: Don't continue trying to enable crtc on failure
@ 2016-08-12 21:09 Sean Paul
2016-08-14 9:42 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Sean Paul @ 2016-08-12 21:09 UTC (permalink / raw)
To: dri-devel, linux-rockchip
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".
Sean
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index ec8ad00..d0ffe59 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,13 +436,13 @@ 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;
+ goto err_put_pm_runtime;
}
ret = clk_enable(vop->dclk);
@@ -485,7 +485,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 +493,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 +915,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.
--
2.8.0.rc3.226.g39d4020
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/rockchip: Don't continue trying to enable crtc on failure
2016-08-12 21:09 [PATCH] drm/rockchip: Don't continue trying to enable crtc on failure Sean Paul
@ 2016-08-14 9:42 ` Daniel Vetter
2016-08-15 23:12 ` [PATCH v2] " Sean Paul
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2016-08-14 9:42 UTC (permalink / raw)
To: Sean Paul; +Cc: linux-rockchip, dri-devel
On Fri, Aug 12, 2016 at 05:09:41PM -0400, Sean Paul wrote:
> If vop_enable fails, don't continue on, it causes system hangs.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Hm, I thought there's clk_prepare which then makes sure that clk_enable
actually works? In that case I'd go with a WARN_ON or similar bad levels
of noise.
If this can indeed happen then ugh, someone needs to make the clk
framework more atomic ;-)
-Daniel
> ---
>
> 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".
>
> Sean
>
>
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index ec8ad00..d0ffe59 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,13 +436,13 @@ 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;
> + goto err_put_pm_runtime;
> }
>
> ret = clk_enable(vop->dclk);
> @@ -485,7 +485,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 +493,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 +915,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.
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] drm/rockchip: Don't continue trying to enable crtc on failure
2016-08-14 9:42 ` Daniel Vetter
@ 2016-08-15 23:12 ` Sean Paul
2016-08-17 7:28 ` Yakir Yang
0 siblings, 1 reply; 5+ messages in thread
From: Sean Paul @ 2016-08-15 23:12 UTC (permalink / raw)
To: dri-devel, linux-rockchip, mark.yao, daniel
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(-)
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.
--
2.8.0.rc3.226.g39d4020
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/rockchip: Don't continue trying to enable crtc on failure
2016-08-15 23:12 ` [PATCH v2] " Sean Paul
@ 2016-08-17 7:28 ` Yakir Yang
2016-08-23 15:36 ` Sean Paul
0 siblings, 1 reply; 5+ messages in thread
From: Yakir Yang @ 2016-08-17 7:28 UTC (permalink / raw)
To: Sean Paul; +Cc: linux-rockchip, dri-devel
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 ｄｒｉｖer 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:
Ｒｅviewed-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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/rockchip: Don't continue trying to enable crtc on failure
2016-08-17 7:28 ` Yakir Yang
@ 2016-08-23 15:36 ` Sean Paul
0 siblings, 0 replies; 5+ messages in thread
From: Sean Paul @ 2016-08-23 15:36 UTC (permalink / raw)
To: Yakir Yang; +Cc: linux-rockchip, dri-devel
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 ｄｒｉｖer 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:
>
> Ｒｅviewed-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.
>
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-23 15:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-12 21:09 [PATCH] drm/rockchip: Don't continue trying to enable crtc on failure Sean Paul
2016-08-14 9:42 ` Daniel Vetter
2016-08-15 23:12 ` [PATCH v2] " Sean Paul
2016-08-17 7:28 ` Yakir Yang
2016-08-23 15:36 ` Sean Paul
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.