* [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 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. _______________________________________________ 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 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. > > > _______________________________________________ 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.