drm/atomic: Fix bookkeeping with TEST_ONLY.
diff mbox

Message ID 55C0A330.6050404@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst Aug. 4, 2015, 11:34 a.m. UTC
Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
"drm/atomic: Cleanup on error properly in the atomic ioctl."
cleaned up some error paths, but didn't fix the TEST_ONLY path.
In the check only case plane->fb shouldn't be updated, and
the vblank events should be cleared as on failure.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---

Comments

Daniel Stone Aug. 27, 2015, 12:19 p.m. UTC | #1
Hi,

On 4 August 2015 at 12:34, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> cleaned up some error paths, but didn't fix the TEST_ONLY path.
> In the check only case plane->fb shouldn't be updated, and
> the vblank events should be cleared as on failure.

Bikeshedding a bit ...

An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
to mention this in the commit message; in this case, the main change
is about plane->{,old_}fb.

> @@ -1532,7 +1533,7 @@ retry:
>                 ret = drm_atomic_check_only(state);
>                 /* _check_only() does not free state, unlike _commit() */
>                 if (!ret)
> -                       drm_atomic_state_free(state);
> +                       goto free;
>         } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>                 ret = drm_atomic_async_commit(state);
>         } else {
> @@ -1566,6 +1567,7 @@ out:
>         }
>
>         if (ret) {
> +free:

This is a bit nasty. Can we please move the label above the
conditional and change the condition to (ret || flags & TEST_ONLY)?
Doing that, you could also move the label above the (ret == -EDEADLK)
check, which would cover ->atomic_check needing to grab other states
(global resources?) and failing.

Cheers,
Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d719ce0b10a0..03672a782e84 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1507,7 +1507,8 @@  retry:
 			copied_props++;
 		}
 
-		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) {
+		if (obj->type == DRM_MODE_OBJECT_PLANE && count_props &&
+		    !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
 			plane = obj_to_plane(obj);
 			plane_mask |= (1 << drm_plane_index(plane));
 			plane->old_fb = plane->fb;
@@ -1532,7 +1533,7 @@  retry:
 		ret = drm_atomic_check_only(state);
 		/* _check_only() does not free state, unlike _commit() */
 		if (!ret)
-			drm_atomic_state_free(state);
+			goto free;
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
 		ret = drm_atomic_async_commit(state);
 	} else {
@@ -1566,6 +1567,7 @@  out:
 	}
 
 	if (ret) {
+free:
 		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
 			for_each_crtc_in_state(state, crtc, crtc_state, i) {
 				if (!crtc_state->event)