* [PATCH] drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set
@ 2015-01-30 19:28 Heiko Stübner
2015-01-31 5:43 ` Daniel Kurtz
0 siblings, 1 reply; 5+ messages in thread
From: Heiko Stübner @ 2015-01-30 19:28 UTC (permalink / raw)
To: mark.yao, Dave Airlie; +Cc: linux-rockchip, Daniel Kurtz, dri-devel
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.
ret_clk is introduced to hold the clk_enable result and not mangle the
original error code.
Signed-off-by: Heiko Stuebner <heiko@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;
}
- return 0;
+ return ret;
}
static void vop_crtc_commit(struct drm_crtc *crtc)
--
2.1.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set
2015-01-30 19:28 [PATCH] drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set Heiko Stübner
@ 2015-01-31 5:43 ` Daniel Kurtz
2015-01-31 9:43 ` Heiko Stübner
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Kurtz @ 2015-01-31 5:43 UTC (permalink / raw)
To: Heiko Stübner; +Cc: open list:ARM/Rockchip SoC..., dri-devel, mark.yao
Hi Heiko,
On Sat, Jan 31, 2015 at 3:28 AM, Heiko Stübner <heiko@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@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?
> }
>
> - return 0;
> + return ret;
> }
>
> static void vop_crtc_commit(struct drm_crtc *crtc)
> --
> 2.1.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set
2015-01-31 5:43 ` Daniel Kurtz
@ 2015-01-31 9:43 ` Heiko Stübner
2015-01-31 10:20 ` Daniel Kurtz
2015-02-02 2:11 ` Daniel Kurtz
0 siblings, 2 replies; 5+ messages in thread
From: Heiko Stübner @ 2015-01-31 9:43 UTC (permalink / raw)
To: Daniel Kurtz; +Cc: open list:ARM/Rockchip SoC..., dri-devel, mark.yao
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@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@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 .
Heiko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set
2015-01-31 9:43 ` Heiko Stübner
@ 2015-01-31 10:20 ` Daniel Kurtz
2015-02-02 2:11 ` Daniel Kurtz
1 sibling, 0 replies; 5+ messages in thread
From: Daniel Kurtz @ 2015-01-31 10:20 UTC (permalink / raw)
To: Heiko Stuebner; +Cc: open list:ARM/Rockchip SoC..., dri-devel, mark.yao
[-- Attachment #1.1: Type: text/plain, Size: 3771 bytes --]
On Jan 31, 2015 5:37 PM, "Heiko Stübner" <heiko@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@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@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@chromium.org>
>
>
> Heiko
[-- Attachment #1.2: Type: text/html, Size: 5488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set
2015-01-31 9:43 ` Heiko Stübner
2015-01-31 10:20 ` Daniel Kurtz
@ 2015-02-02 2:11 ` Daniel Kurtz
1 sibling, 0 replies; 5+ messages in thread
From: Daniel Kurtz @ 2015-02-02 2:11 UTC (permalink / raw)
To: Heiko Stübner; +Cc: open list:ARM/Rockchip SoC..., dri-devel, mark.yao
On Sat, Jan 31, 2015 at 5:43 PM, Heiko Stübner <heiko@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@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@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@chromium.org>
>
>
> Heiko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-02-02 2:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-30 19:28 [PATCH] drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set Heiko Stübner
2015-01-31 5:43 ` Daniel Kurtz
2015-01-31 9:43 ` Heiko Stübner
2015-01-31 10:20 ` Daniel Kurtz
2015-02-02 2:11 ` Daniel Kurtz
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.