drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
diff mbox

Message ID 1435842999-25699-1-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter July 2, 2015, 1:16 p.m. UTC
In

commit 9f658b7b62e7aefc1ee067136126eca3f58cabfd
Author: Daniel Stone <daniels@collabora.com>
Date:   Fri May 22 13:34:45 2015 +0100

    drm/crtc_helper: Replace open-coded CRTC state helpers

error handling code was broken, resulting in the first path not being
checked correctly. Fix this by using the same pattern as in the
transitional plane helper function drm_plane_helper_update.

v2: Simplify the cleanup code while at it too.

v3: After some debugging with John we realized that the above patch
from Daniel also accidentally removed the if (crtc_state) check. This
is legal when transitioning to atomic, when the initial state reset
isn't all wired up yet properly. Reinstate that check to fix the bug
John has hit.

Cc: Daniel Stone <daniels@collabora.com>
CC: Sean Paul <seanpaul@chromium.org>
Cc: John Hunter <zhaojunwang@pku.edu.cn>
Reported-by: John Hunter <zhaojunwang@pku.edu.cn>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc_helper.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Comments

Daniel Stone July 2, 2015, 1:27 p.m. UTC | #1
Hi,

> On 2 Jul 2015, at 2:16 pm, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> In
> 
> commit 9f658b7b62e7aefc1ee067136126eca3f58cabfd
> Author: Daniel Stone <daniels@collabora.com>
> Date:   Fri May 22 13:34:45 2015 +0100
> 
>    drm/crtc_helper: Replace open-coded CRTC state helpers
> 
> error handling code was broken, resulting in the first path not being
> checked correctly. Fix this by using the same pattern as in the
> transitional plane helper function drm_plane_helper_update.
> 
> v2: Simplify the cleanup code while at it too.
> 
> v3: After some debugging with John we realized that the above patch
> from Daniel also accidentally removed the if (crtc_state) check. This
> is legal when transitioning to atomic, when the initial state reset
> isn't all wired up yet properly. Reinstate that check to fix the bug
> John has hit.

Still misses the crtc_state->crtc assignment in the kzalloc path.

Cheers,
Daniel

> Cc: Daniel Stone <daniels@collabora.com>
> CC: Sean Paul <seanpaul@chromium.org>
> Cc: John Hunter <zhaojunwang@pku.edu.cn>
> Reported-by: John Hunter <zhaojunwang@pku.edu.cn>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/gpu/drm/drm_crtc_helper.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 393114df88a3..93104f3555f5 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -927,15 +927,13 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
> 
>    if (crtc->funcs->atomic_duplicate_state)
>        crtc_state = crtc->funcs->atomic_duplicate_state(crtc);
> -    else {
> +    else if (crtc->state)
> +        crtc_state = drm_atomic_helper_crtc_duplicate_state(crtc);
> +    else
>        crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> -        if (!crtc_state)
> -            return -ENOMEM;
> -        if (crtc->state)
> -            __drm_atomic_helper_crtc_duplicate_state(crtc, crtc_state);
> -        else
> -            crtc_state->crtc = crtc;
> -    }
> +
> +    if (!crtc_state)
> +        return -ENOMEM;
> 
>    crtc_state->planes_changed = true;
>    crtc_state->mode_changed = true;
> @@ -957,11 +955,11 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
>    ret = drm_helper_crtc_mode_set_base(crtc, x, y, old_fb);
> 
> out:
> -    if (crtc->funcs->atomic_destroy_state)
> -        crtc->funcs->atomic_destroy_state(crtc, crtc_state);
> -    else {
> -        __drm_atomic_helper_crtc_destroy_state(crtc, crtc_state);
> -        kfree(crtc_state);
> +    if (crtc_state) {
> +        if (crtc->funcs->atomic_destroy_state)
> +            crtc->funcs->atomic_destroy_state(crtc, crtc_state);
> +        else
> +            drm_atomic_helper_crtc_destroy_state(crtc, crtc_state);
>    }
> 
>    return ret;
> -- 
> 2.1.4
>
Daniel Vetter July 2, 2015, 2:29 p.m. UTC | #2
On Thu, Jul 02, 2015 at 02:27:30PM +0100, Daniel Stone wrote:
> Hi,
> 
> > On 2 Jul 2015, at 2:16 pm, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > 
> > In
> > 
> > commit 9f658b7b62e7aefc1ee067136126eca3f58cabfd
> > Author: Daniel Stone <daniels@collabora.com>
> > Date:   Fri May 22 13:34:45 2015 +0100
> > 
> >    drm/crtc_helper: Replace open-coded CRTC state helpers
> > 
> > error handling code was broken, resulting in the first path not being
> > checked correctly. Fix this by using the same pattern as in the
> > transitional plane helper function drm_plane_helper_update.
> > 
> > v2: Simplify the cleanup code while at it too.
> > 
> > v3: After some debugging with John we realized that the above patch
> > from Daniel also accidentally removed the if (crtc_state) check. This
> > is legal when transitioning to atomic, when the initial state reset
> > isn't all wired up yet properly. Reinstate that check to fix the bug
> > John has hit.
> 
> Still misses the crtc_state->crtc assignment in the kzalloc path.

Yeah I was random-walking over that code badly. Please disregard v4 too.
I'll follow up with a patch to use the reset helper if crtc->state isnt'
set both here and for plane transitional helpers too.
-Daniel
John Hunter July 2, 2015, 2:40 p.m. UTC | #3
On Thu, Jul 2, 2015 at 9:16 PM, Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> In
>
> commit 9f658b7b62e7aefc1ee067136126eca3f58cabfd
> Author: Daniel Stone <daniels@collabora.com>
> Date:   Fri May 22 13:34:45 2015 +0100
>
>     drm/crtc_helper: Replace open-coded CRTC state helpers
>
> error handling code was broken, resulting in the first path not being
> checked correctly. Fix this by using the same pattern as in the
> transitional plane helper function drm_plane_helper_update.
>
> v2: Simplify the cleanup code while at it too.
>
> v3: After some debugging with John we realized that the above patch
> from Daniel also accidentally removed the if (crtc_state) check. This
> is legal when transitioning to atomic, when the initial state reset
> isn't all wired up yet properly. Reinstate that check to fix the bug
> John has hit.
>
> Cc: Daniel Stone <daniels@collabora.com>
> CC: Sean Paul <seanpaul@chromium.org>
> Cc: John Hunter <zhaojunwang@pku.edu.cn>
> Reported-by: John Hunter <zhaojunwang@pku.edu.cn>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
Tested-by: Zhao Junwang <zhjwpku@gmail.com>

> ---
>  drivers/gpu/drm/drm_crtc_helper.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> b/drivers/gpu/drm/drm_crtc_helper.c
> index 393114df88a3..93104f3555f5 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -927,15 +927,13 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc,
> struct drm_display_mode *mod
>
>         if (crtc->funcs->atomic_duplicate_state)
>                 crtc_state = crtc->funcs->atomic_duplicate_state(crtc);
> -       else {
> +       else if (crtc->state)
> +               crtc_state = drm_atomic_helper_crtc_duplicate_state(crtc);
> +       else
>                 crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> -               if (!crtc_state)
> -                       return -ENOMEM;
> -               if (crtc->state)
> -                       __drm_atomic_helper_crtc_duplicate_state(crtc,
> crtc_state);
> -               else
> -                       crtc_state->crtc = crtc;
> -       }
> +
> +       if (!crtc_state)
> +               return -ENOMEM;
>
>         crtc_state->planes_changed = true;
>         crtc_state->mode_changed = true;
> @@ -957,11 +955,11 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc,
> struct drm_display_mode *mod
>         ret = drm_helper_crtc_mode_set_base(crtc, x, y, old_fb);
>
>  out:
> -       if (crtc->funcs->atomic_destroy_state)
> -               crtc->funcs->atomic_destroy_state(crtc, crtc_state);
> -       else {
> -               __drm_atomic_helper_crtc_destroy_state(crtc, crtc_state);
> -               kfree(crtc_state);
> +       if (crtc_state) {
> +               if (crtc->funcs->atomic_destroy_state)
> +                       crtc->funcs->atomic_destroy_state(crtc,
> crtc_state);
> +               else
> +                       drm_atomic_helper_crtc_destroy_state(crtc,
> crtc_state);
>         }
>
>         return ret;
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 393114df88a3..93104f3555f5 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -927,15 +927,13 @@  int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
 
 	if (crtc->funcs->atomic_duplicate_state)
 		crtc_state = crtc->funcs->atomic_duplicate_state(crtc);
-	else {
+	else if (crtc->state)
+		crtc_state = drm_atomic_helper_crtc_duplicate_state(crtc);
+	else
 		crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
-		if (!crtc_state)
-			return -ENOMEM;
-		if (crtc->state)
-			__drm_atomic_helper_crtc_duplicate_state(crtc, crtc_state);
-		else
-			crtc_state->crtc = crtc;
-	}
+
+	if (!crtc_state)
+		return -ENOMEM;
 
 	crtc_state->planes_changed = true;
 	crtc_state->mode_changed = true;
@@ -957,11 +955,11 @@  int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
 	ret = drm_helper_crtc_mode_set_base(crtc, x, y, old_fb);
 
 out:
-	if (crtc->funcs->atomic_destroy_state)
-		crtc->funcs->atomic_destroy_state(crtc, crtc_state);
-	else {
-		__drm_atomic_helper_crtc_destroy_state(crtc, crtc_state);
-		kfree(crtc_state);
+	if (crtc_state) {
+		if (crtc->funcs->atomic_destroy_state)
+			crtc->funcs->atomic_destroy_state(crtc, crtc_state);
+		else
+			drm_atomic_helper_crtc_destroy_state(crtc, crtc_state);
 	}
 
 	return ret;