| Message ID | 1471302749-2811-1-git-send-email-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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. > > >
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.
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(-)