[PATCH] drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set

[Daniel Kurtz]

On Jan 31, 2015 5:37 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150131/03a324ce/attachment.html>