[PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[Daniel Vetter]

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.

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

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 393114df88a3..0035c3395b30 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;
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[Daniel Vetter]

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.

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

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 393114df88a3..f36750077c12 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;
@@ -959,10 +957,8 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
 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);
-	}
+	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

Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[Daniel Stone]

Hi,

On Thu, 2015-07-02 at 09:23 +0200, Daniel Vetter 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.
> 
> @@ -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(crt
> c, crtc_state);
> -		else
> -			crtc_state->crtc = crtc;

Isn't this line (the crtc_state->crtc) assignment now missing from the
kzalloc branch?

With that fixed:
Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[Daniel Vetter]

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(-)

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[Daniel Stone]

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
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: reset empty state in transitional helpers

[Daniel Vetter]

Transitional drivers might not have all the state frobbing lined up
yet. But since the initial code has been merged a lot more state was
added, so we really need this.

Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_crtc_helper.c  |  8 +++++---
 drivers/gpu/drm/drm_plane_helper.c | 16 ++++++++++------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 93104f3555f5..d3dfb0ebbeb2 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -927,10 +927,12 @@ 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 if (crtc->state)
+	else {
+		if (!crtc->state)
+			drm_atomic_helper_crtc_reset(crtc);
+
 		crtc_state = drm_atomic_helper_crtc_duplicate_state(crtc);
-	else
-		crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
+	}
 
 	if (!crtc_state)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 2f0ed11024eb..b07a213f5655 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -525,10 +525,12 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	if (plane->funcs->atomic_duplicate_state)
 		plane_state = plane->funcs->atomic_duplicate_state(plane);
-	else if (plane->state)
+	else {
+		if (!plane->state)
+			drm_atomic_helper_plane_reset(plane);
+
 		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
-	else
-		plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
+	}
 	if (!plane_state)
 		return -ENOMEM;
 	plane_state->plane = plane;
@@ -572,10 +574,12 @@ int drm_plane_helper_disable(struct drm_plane *plane)
 
 	if (plane->funcs->atomic_duplicate_state)
 		plane_state = plane->funcs->atomic_duplicate_state(plane);
-	else if (plane->state)
+	else {
+		if (!plane->state)
+			drm_atomic_helper_plane_reset(plane);
+
 		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
-	else
-		plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
+	}
 	if (!plane_state)
 		return -ENOMEM;
 	plane_state->plane = plane;
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: reset empty state in transitional helpers

[John Hunter]

[-- Attachment #1.1: Type: text/plain, Size: 3293 bytes --]

On Thu, Jul 2, 2015 at 10:33 PM, Daniel Vetter <daniel.vetter@ffwll.ch>
wrote:

> Transitional drivers might not have all the state frobbing lined up
> yet. But since the initial code has been merged a lot more state was
> added, so we really need this.
>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
Tested-by: Zhao Junwang <zhjwpku@gmail.com>

> ---
>  drivers/gpu/drm/drm_crtc_helper.c  |  8 +++++---
>  drivers/gpu/drm/drm_plane_helper.c | 16 ++++++++++------
>  2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> b/drivers/gpu/drm/drm_crtc_helper.c
> index 93104f3555f5..d3dfb0ebbeb2 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -927,10 +927,12 @@ 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 if (crtc->state)
> +       else {
> +               if (!crtc->state)
> +                       drm_atomic_helper_crtc_reset(crtc);
> +
>                 crtc_state = drm_atomic_helper_crtc_duplicate_state(crtc);
> -       else
> -               crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> +       }
>
>         if (!crtc_state)
>                 return -ENOMEM;
> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> b/drivers/gpu/drm/drm_plane_helper.c
> index 2f0ed11024eb..b07a213f5655 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -525,10 +525,12 @@ int drm_plane_helper_update(struct drm_plane *plane,
> struct drm_crtc *crtc,
>
>         if (plane->funcs->atomic_duplicate_state)
>                 plane_state = plane->funcs->atomic_duplicate_state(plane);
> -       else if (plane->state)
> +       else {
> +               if (!plane->state)
> +                       drm_atomic_helper_plane_reset(plane);
> +
>                 plane_state =
> drm_atomic_helper_plane_duplicate_state(plane);
> -       else
> -               plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
> +       }
>         if (!plane_state)
>                 return -ENOMEM;
>         plane_state->plane = plane;
> @@ -572,10 +574,12 @@ int drm_plane_helper_disable(struct drm_plane *plane)
>
>         if (plane->funcs->atomic_duplicate_state)
>                 plane_state = plane->funcs->atomic_duplicate_state(plane);
> -       else if (plane->state)
> +       else {
> +               if (!plane->state)
> +                       drm_atomic_helper_plane_reset(plane);
> +
>                 plane_state =
> drm_atomic_helper_plane_duplicate_state(plane);
> -       else
> -               plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
> +       }
>         if (!plane_state)
>                 return -ENOMEM;
>         plane_state->plane = plane;
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>



-- 
Best regards
Junwang Zhao
Microprocessor Research and Develop Center
Department of Computer Science &Technology
Peking University
Beijing, 100871, PRC

[-- Attachment #1.2: Type: text/html, Size: 4758 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: reset empty state in transitional helpers

[Daniel Stone]

[-- Attachment #1.1: Type: text/plain, Size: 433 bytes --]

On Thursday, July 2, 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Transitional drivers might not have all the state frobbing lined up
> yet. But since the initial code has been merged a lot more state was
> added, so we really need this.
>
> Cc: Daniel Stone <daniels@collabora.com <javascript:;>>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com <javascript:;>>
>

Reviewed-by: Daniel Stone <daniels@collabora.com>

[-- Attachment #1.2: Type: text/html, Size: 883 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[John Hunter]

[-- Attachment #1.1: Type: text/plain, Size: 3440 bytes --]

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
>



-- 
Best regards
Junwang Zhao
Microprocessor Research and Develop Center
Department of Computer Science &Technology
Peking University
Beijing, 100871, PRC

[-- Attachment #1.2: Type: text/html, Size: 5265 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6709
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -1              287/287              286/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[Daniel Vetter]

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.

v4: Argh, I removed Daniel's fix by accident completely. Reinit the
state properly, even when we allocate it afresh.

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 | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 393114df88a3..280e8581c704 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -927,16 +927,17 @@ 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 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)
+		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;
 	ret = drm_atomic_set_mode_for_crtc(crtc_state, mode);
@@ -957,11 +958,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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/plane-helper: Use init state when freshly allocating them

[Daniel Vetter]

Daniel fixed the same issue for crtc states 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

Follow suite.

Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 2f0ed11024eb..cd367dde7521 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -527,8 +527,11 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		plane_state = plane->funcs->atomic_duplicate_state(plane);
 	else if (plane->state)
 		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
-	else
+	else {
 		plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
+		if (plane_state)
+			__drm_atomic_helper_plane_duplicate_state(plane, plane_state);
+	}
 	if (!plane_state)
 		return -ENOMEM;
 	plane_state->plane = plane;
@@ -574,8 +577,11 @@ int drm_plane_helper_disable(struct drm_plane *plane)
 		plane_state = plane->funcs->atomic_duplicate_state(plane);
 	else if (plane->state)
 		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
-	else
+	else {
 		plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
+		if (plane_state)
+			__drm_atomic_helper_plane_duplicate_state(plane, plane_state);
+	}
 	if (!plane_state)
 		return -ENOMEM;
 	plane_state->plane = plane;
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/plane-helper: Use init state when freshly allocating them

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6710
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -2              287/287              285/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6704
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                                  289/289              289/289
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx