Re: [PATCH] drm/rockchip: Don't continue trying to enable crtc on failure

From: Daniel Vetter <daniel@ffwll.ch>
To: Sean Paul <seanpaul@chromium.org>
Cc: linux-rockchip@lists.infradead.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/rockchip: Don't continue trying to enable crtc on failure
Date: Sun, 14 Aug 2016 11:42:01 +0200	[thread overview]
Message-ID: <20160814094200.GW6232@phenom.ffwll.local> (raw)
In-Reply-To: <1471036181-14777-1-git-send-email-seanpaul@chromium.org>

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