[PATCH] drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set
Daniel Kurtz
djkurtz at chromium.org
Sun Feb 1 18:11:58 PST 2015
On Sat, Jan 31, 2015 at 5:43 PM, Heiko Stübner <heiko at sntech.de> wrote:
> Am Samstag, 31. Januar 2015, 13:43:23 schrieb Daniel Kurtz:
>> Hi Heiko,
>>
>> On Sat, Jan 31, 2015 at 3:28 AM, Heiko Stübner <heiko at sntech.de> wrote:
>> > The function disables the dclk at the beginning, so don't simply return
>> > when an error happens, but instead enable the clock again, so that
>> > enable and disable calls are balanced.
>>
>> This function is a bit of a disaster, and needs fixing some day.
>> AFAICT, the dclk_rst is actually ignored if dclk is disabled, so that
>> part doesn't even work.
>>
>> > ret_clk is introduced to hold the clk_enable result and not mangle the
>> > original error code.
>> >
>> > Signed-off-by: Heiko Stuebner <heiko at sntech.de>
>> >
>> > ---
>> >
>> > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ++++++++++--------
>> > 1 file changed, 10 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> > b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 04b619a..c0387f7
>> > 100644
>> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> > @@ -852,7 +852,7 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc,
>> >
>> > u16 vsync_len = adjusted_mode->vsync_end -
>> > adjusted_mode->vsync_start;
>> > u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start;
>> > u16 vact_end = vact_st + vdisplay;
>> >
>> > - int ret;
>> > + int ret, ret_clk;
>> >
>> > uint32_t val;
>> >
>> > /*
>> >
>> > @@ -874,7 +874,8 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc,
>> >
>> > default:
>> > DRM_ERROR("unsupport connector_type[%d]\n",
>> >
>> > vop->connector_type);
>> >
>> > - return -EINVAL;
>> > + ret = -EINVAL;
>> > + goto out;
>> >
>> > };
>> > VOP_CTRL_SET(vop, out_mode, vop->connector_out_mode);
>> >
>> > @@ -897,7 +898,7 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc,
>> >
>> > ret = vop_crtc_mode_set_base(crtc, x, y, fb);
>> > if (ret)
>> >
>> > - return ret;
>> > + goto out;
>> >
>> > /*
>> >
>> > * reset dclk, take all mode config affect, so the clk would run
>> > in
>> >
>> > @@ -908,13 +909,14 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc,
>> >
>> > reset_control_deassert(vop->dclk_rst);
>> >
>> > clk_set_rate(vop->dclk, adjusted_mode->clock * 1000);
>> >
>> > - ret = clk_enable(vop->dclk);
>> > - if (ret < 0) {
>> > - dev_err(vop->dev, "failed to enable dclk - %d\n", ret);
>> > - return ret;
>> > +out:
>> > + ret_clk = clk_enable(vop->dclk);
>> > + if (ret_clk < 0) {
>> > + dev_err(vop->dev, "failed to enable dclk - %d\n",
>> > ret_clk);
>> > + return ret_clk;
>>
>> Doesn't this swallow ret ? I thought the point was to still return
>> the original error?
>
> I think if even the reenabling of the clock fails, there must be something
> _really_ wrong with the system [enabled before and all] - so if this also
> returns an error after the core functionality failed already, it doesn't
> really matter anymore which error we return :-) .
>
> The original point was to not overwrite the actual error (in ret) in the case
> where clk_enable simply returns 0 .
Yeah, that makes more sense :-).
Reviewed-by: Daniel Kurtz <djkurtz at chromium.org>
>
>
> Heiko
More information about the dri-devel
mailing list