[Intel-gfx] [PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Maarten Lankhorst]

Op 27-08-15 om 14:19 schreef Daniel Stone:
> Hi,
>
> On 4 August 2015 at 12:34, Maarten Lankhorst
> <maarten.lankhorst at 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.
Even testing with PAGE_FLIP_EVENT would be useful because
event && !crtc_state->active should not be allowed. In that case test
could succeed but commit could fail.

Though I guess you're right and it's currently not allowed.
>> @@ -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.
If our bookkeeping is correct then it won't be harmful to fixup old_fb.
Cleaning up more old_fb for more planes than initially had fb updates can always
happen anyway, because a modeset will add all affected planes.

How about the below patch? Apply with git am --scissors

-------->8------
Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
"drm/atomic: Cleanup on error properly in the atomic ioctl."
cleaned up some error paths, but allowed -EDEADLK to
leak vblank events.

Additionally check_only was updating plane->fb, which should
not be done when checking a new configuration only.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
---
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c2448f42480f..78ffb4965548 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1542,7 +1542,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;
@@ -1564,10 +1565,11 @@ retry:
 	}
 
 	if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
+		/*
+		 * Unlike commit, check_only does not clean up state.
+		 * Below we call drm_atomic_state_free for it.
+		 */
 		ret = drm_atomic_check_only(state);
-		/* _check_only() does not free state, unlike _commit() */
-		if (!ret)
-			drm_atomic_state_free(state);
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
 		ret = drm_atomic_async_commit(state);
 	} else {
@@ -1594,25 +1596,24 @@ out:
 		plane->old_fb = NULL;
 	}
 
+	if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+			if (!crtc_state->event)
+				continue;
+
+			destroy_vblank_event(dev, file_priv,
+					     crtc_state->event);
+		}
+	}
+
 	if (ret == -EDEADLK) {
 		drm_atomic_state_clear(state);
 		drm_modeset_backoff(&ctx);
 		goto retry;
 	}
 
-	if (ret) {
-		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
-			for_each_crtc_in_state(state, crtc, crtc_state, i) {
-				if (!crtc_state->event)
-					continue;
-
-				destroy_vblank_event(dev, file_priv,
-						     crtc_state->event);
-			}
-		}
-
+	if (ret || arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
 		drm_atomic_state_free(state);
-	}
 
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);