Skip to contentSkip to conversation

drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set

PATCH In review Heiko Stübner 2 participants 5 messages Jan 30, 2015 – Feb 2, 2015

Subsystems (4)
DRM drivers DRM misc Arm Rockchip DRM drivers for Rockchip
lore
Share & tools
lore

Heiko Stübner

author maintainer
#0 lore raw

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

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 <[email protected]>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +10 -8
index 04b619a..c0387f7 100644
@@ -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)
Signature · 7 lines
2.1.1


_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
#1 lore raw

Hi Heiko,

Quoted 3 lines · Heiko Stübner

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.

Quoted 45 lines

ret_clk is introduced to hold the clk_enable result and not mangle the original error code.

Signed-off-by Heiko Stuebner <[email protected]>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +4 -3
index 04b619a..c0387f7 100644
@@ -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);

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_crtc_mode_set(struct drm_crtc *crtc, · line 916

Quoted 10 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +5 -4
        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?

Quoted 9 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +1 -2
        }
 
-       return 0;
+       return ret;
 }
 
 static void vop_crtc_commit(struct drm_crtc *crtc)
--

2.1.1

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

author maintainer
#2 lore raw

Am Samstag, 31. Januar 2015, 13:43:23 schrieb Daniel Kurtz:

Quoted 73 lines

Hi Heiko,

Quoted 3 lines · Heiko Stübner

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.

Quoted 62 lines

ret_clk is introduced to hold the clk_enable result and not mangle the original error code.

Signed-off-by Heiko Stuebner <[email protected]>

---

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 04b619a..c0387f7 100644

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +1 -1
@@ -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;
        
        /*
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +2 -1
@@ -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);
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +1 -1
@@ -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

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -0
@@ -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);
Quoted 13 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_crtc_mode_set(struct drm_crtc *crtc, · line 916

Quoted 10 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +5 -4
-       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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Reviewed-by
#3 lore raw
Quoted 6 lines · Heiko Stübner

Am Samstag, 31. Januar 2015, 13:43:23 schrieb Daniel Kurtz:

Quoted 4 lines

Hi Heiko,

Quoted 1 line · Heiko Stübner

The function disables the dclk at the beginning, so don't simply

return

Quoted 17 lines
Quoted 17 lines
Quoted 2 lines

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.

Quoted 10 lines

ret_clk is introduced to hold the clk_enable result and not mangle the original error code.

Signed-off-by Heiko Stuebner <[email protected]>

---

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
Quoted 6 lines
Quoted 6 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_crtc_mode_set(struct drm_crtc

Quoted 6 lines
(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 04b619a..c0387f7 100644

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -0
@@ -852,7 +852,7 @@ static int vop_crtc_mode_set(struct drm_crtc

*crtc,

Quoted 4 lines
Quoted 4 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_crtc_mode_set(struct drm_crtc · line 854

Quoted 4 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -0
        u16 vsync_len = adjusted_mode->vsync_end -
        adjusted_mode->vsync_start;
        u16 vact_st = adjusted_mode->vtotal -

adjusted_mode->vsync_start;

Quoted 10 lines
Quoted 10 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_crtc_mode_set(struct drm_crtc

Quoted 10 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +1 -1
        u16 vact_end = vact_st + vdisplay;
 
-       int ret;
+       int ret, ret_clk;
 

        uint32_t val;

        /*
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -0
@@ -874,7 +874,8 @@ static int vop_crtc_mode_set(struct drm_crtc

*crtc,

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -0
        default:
                DRM_ERROR("unsupport connector_type[%d]\n",
 
                          vop->connector_type);
Quoted 8 lines
Quoted 8 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_crtc_mode_set(struct drm_crtc

Quoted 8 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +2 -1
-               return -EINVAL;
+               ret = -EINVAL;
+               goto out;
 

        };
        VOP_CTRL_SET(vop, out_mode, vop->connector_out_mode);
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -0
@@ -897,7 +898,7 @@ static int vop_crtc_mode_set(struct drm_crtc

*crtc,

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +1 -1
        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

Quoted 3 lines
Quoted 3 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_crtc_mode_set(struct drm_crtc

Quoted 3 lines

in

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -0
@@ -908,13 +909,14 @@ static int vop_crtc_mode_set(struct drm_crtc

*crtc,

Quoted 8 lines
Quoted 8 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_crtc_mode_set(struct drm_crtc *crtc,

Quoted 8 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -3
        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);

Quoted 11 lines
Quoted 10 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_crtc_mode_set(struct drm_crtc *crtc, · line 916

Quoted 7 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +5 -1
-               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?

Quoted 6 lines

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

Quoted 1 line

where clk_enable simply returns 0 .

Yeah, that makes more sense :-).

Reviewed-by Daniel Kurtz <[email protected]>
Quoted 3 lines

Heiko

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Reviewed-by
#4 lore raw
In reply to Heiko Stübner
Quoted 88 lines · Heiko Stübner

Am Samstag, 31. Januar 2015, 13:43:23 schrieb Daniel Kurtz:

Quoted 73 lines

Hi Heiko,

Quoted 3 lines · Heiko Stübner

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.

Quoted 62 lines

ret_clk is introduced to hold the clk_enable result and not mangle the original error code.

Signed-off-by Heiko Stuebner <[email protected]>

---

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 04b619a..c0387f7 100644

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +1 -1
@@ -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;

        /*
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +2 -1
@@ -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);
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +1 -1
@@ -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

drivers/gpu/drm/rockchip/rockchip_drm_vop.c +0 -0
@@ -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);
Quoted 13 lines

drivers/gpu/drm/rockchip/rockchip_drm_vop.c static int vop_crtc_mode_set(struct drm_crtc *crtc, · line 916

Quoted 10 lines
drivers/gpu/drm/rockchip/rockchip_drm_vop.c +5 -4
-       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?

Quoted 7 lines

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 <[email protected]>
Quoted 3 lines

Heiko

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel