* [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
@ 2015-07-02 7:23 Daniel Vetter
2015-07-02 7:29 ` Daniel Vetter
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Daniel Vetter @ 2015-07-02 7:23 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Stone, Daniel Vetter, Intel Graphics Development,
Daniel Vetter, John Hunter
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
2015-07-02 7:23 [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set Daniel Vetter
@ 2015-07-02 7:29 ` Daniel Vetter
2015-07-02 9:01 ` Daniel Stone
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2015-07-02 7:29 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Stone, Daniel Vetter, Intel Graphics Development,
Daniel Vetter, John Hunter
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
2015-07-02 7:23 [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set Daniel Vetter
2015-07-02 7:29 ` Daniel Vetter
@ 2015-07-02 9:01 ` Daniel Stone
2015-07-02 13:16 ` Daniel Vetter
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Daniel Stone @ 2015-07-02 9:01 UTC (permalink / raw)
To: Daniel Vetter, DRI Development
Cc: Daniel Vetter, Intel Graphics Development, John Hunter
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
2015-07-02 7:23 [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set Daniel Vetter
2015-07-02 7:29 ` Daniel Vetter
2015-07-02 9:01 ` Daniel Stone
@ 2015-07-02 13:16 ` Daniel Vetter
2015-07-02 13:27 ` Daniel Stone
` (2 more replies)
2015-07-02 14:00 ` [PATCH 1/2] " Daniel Vetter
2015-07-04 10:39 ` [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set shuang.he
4 siblings, 3 replies; 15+ messages in thread
From: Daniel Vetter @ 2015-07-02 13:16 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Stone, Daniel Vetter, Intel Graphics Development,
Daniel Vetter, John Hunter
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
2015-07-02 13:16 ` Daniel Vetter
@ 2015-07-02 13:27 ` Daniel Stone
2015-07-02 14:29 ` Daniel Vetter
2015-07-02 14:33 ` [PATCH] drm: reset empty state in transitional helpers Daniel Vetter
2015-07-02 14:40 ` [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set John Hunter
2015-07-04 16:57 ` shuang.he
2 siblings, 2 replies; 15+ messages in thread
From: Daniel Stone @ 2015-07-02 13:27 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, John Hunter,
DRI Development
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
2015-07-02 7:23 [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set Daniel Vetter
` (2 preceding siblings ...)
2015-07-02 13:16 ` Daniel Vetter
@ 2015-07-02 14:00 ` Daniel Vetter
2015-07-02 14:00 ` [PATCH 2/2] drm/plane-helper: Use init state when freshly allocating them Daniel Vetter
2015-07-04 10:39 ` [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set shuang.he
4 siblings, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2015-07-02 14:00 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Stone, Daniel Vetter, Intel Graphics Development,
Daniel Vetter, John Hunter
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] drm/plane-helper: Use init state when freshly allocating them
2015-07-02 14:00 ` [PATCH 1/2] " Daniel Vetter
@ 2015-07-02 14:00 ` Daniel Vetter
2015-07-04 19:11 ` shuang.he
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2015-07-02 14:00 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Intel Graphics Development, Daniel Stone,
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
2015-07-02 13:27 ` Daniel Stone
@ 2015-07-02 14:29 ` Daniel Vetter
2015-07-02 14:33 ` [PATCH] drm: reset empty state in transitional helpers Daniel Vetter
1 sibling, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2015-07-02 14:29 UTC (permalink / raw)
To: Daniel Stone
Cc: John Hunter, Daniel Vetter, Intel Graphics Development,
DRI Development, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] drm: reset empty state in transitional helpers
2015-07-02 13:27 ` Daniel Stone
2015-07-02 14:29 ` Daniel Vetter
@ 2015-07-02 14:33 ` Daniel Vetter
2015-07-02 14:39 ` John Hunter
2015-07-03 7:10 ` Daniel Stone
1 sibling, 2 replies; 15+ messages in thread
From: Daniel Vetter @ 2015-07-02 14:33 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Intel Graphics Development, Daniel Stone,
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] drm: reset empty state in transitional helpers
2015-07-02 14:33 ` [PATCH] drm: reset empty state in transitional helpers Daniel Vetter
@ 2015-07-02 14:39 ` John Hunter
2015-07-03 7:10 ` Daniel Stone
1 sibling, 0 replies; 15+ messages in thread
From: John Hunter @ 2015-07-02 14:39 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, Daniel Stone,
DRI Development
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
2015-07-02 13:16 ` Daniel Vetter
2015-07-02 13:27 ` Daniel Stone
@ 2015-07-02 14:40 ` John Hunter
2015-07-04 16:57 ` shuang.he
2 siblings, 0 replies; 15+ messages in thread
From: John Hunter @ 2015-07-02 14:40 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, Daniel Stone,
DRI Development, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm: reset empty state in transitional helpers
2015-07-02 14:33 ` [PATCH] drm: reset empty state in transitional helpers Daniel Vetter
2015-07-02 14:39 ` John Hunter
@ 2015-07-03 7:10 ` Daniel Stone
1 sibling, 0 replies; 15+ messages in thread
From: Daniel Stone @ 2015-07-03 7:10 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, Daniel Stone,
DRI Development
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
2015-07-02 7:23 [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set Daniel Vetter
` (3 preceding siblings ...)
2015-07-02 14:00 ` [PATCH 1/2] " Daniel Vetter
@ 2015-07-04 10:39 ` shuang.he
4 siblings, 0 replies; 15+ messages in thread
From: shuang.he @ 2015-07-04 10:39 UTC (permalink / raw)
To: shuang.he, lei.a.liu, intel-gfx, daniel.vetter
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set
2015-07-02 13:16 ` Daniel Vetter
2015-07-02 13:27 ` Daniel Stone
2015-07-02 14:40 ` [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set John Hunter
@ 2015-07-04 16:57 ` shuang.he
2 siblings, 0 replies; 15+ messages in thread
From: shuang.he @ 2015-07-04 16:57 UTC (permalink / raw)
To: shuang.he, lei.a.liu, intel-gfx, daniel.vetter
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/plane-helper: Use init state when freshly allocating them
2015-07-02 14:00 ` [PATCH 2/2] drm/plane-helper: Use init state when freshly allocating them Daniel Vetter
@ 2015-07-04 19:11 ` shuang.he
0 siblings, 0 replies; 15+ messages in thread
From: shuang.he @ 2015-07-04 19:11 UTC (permalink / raw)
To: shuang.he, lei.a.liu, intel-gfx, daniel.vetter
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-07-04 19:11 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-02 7:23 [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set Daniel Vetter
2015-07-02 7:29 ` Daniel Vetter
2015-07-02 9:01 ` Daniel Stone
2015-07-02 13:16 ` Daniel Vetter
2015-07-02 13:27 ` Daniel Stone
2015-07-02 14:29 ` Daniel Vetter
2015-07-02 14:33 ` [PATCH] drm: reset empty state in transitional helpers Daniel Vetter
2015-07-02 14:39 ` John Hunter
2015-07-03 7:10 ` Daniel Stone
2015-07-02 14:40 ` [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set John Hunter
2015-07-04 16:57 ` shuang.he
2015-07-02 14:00 ` [PATCH 1/2] " Daniel Vetter
2015-07-02 14:00 ` [PATCH 2/2] drm/plane-helper: Use init state when freshly allocating them Daniel Vetter
2015-07-04 19:11 ` shuang.he
2015-07-04 10:39 ` [PATCH] drm/crtc-helper: Fixup error handling in drm_helper_crtc_mode_set shuang.he
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox