drm/atomic: Fix bookkeeping with TEST_ONLY.

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.

drivers/gpu/drm/drm_atomic.c +1 -1

@@ -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 {

Hi,

Quoted 5 lines · Maarten Lankhorst

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.

Quoted 12 lines

(quoted diff) +1 -1

@@ -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:
        }
 

Review out: · line 1570

Quoted 2 lines

(quoted diff) +1 -0

        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.

Op 27-08-15 om 14:19 schreef Daniel Stone:

Quoted 9 lines

Hi,

Quoted 5 lines · Maarten Lankhorst

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.

Quoted 5 lines

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.

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/drm_atomic.c +1 -1

@@ -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:
        }
 

Quoted 7 lines

Review out: · line 1570

Quoted 2 lines

(quoted diff) +1 -0

        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 <[email protected]>

---

drivers/gpu/drm/drm_atomic.c +17 -16

index c2448f42480f..78ffb4965548 100644
@@ -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);

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 15 lines

Op 27-08-15 om 14:19 schreef Daniel Stone:

Quoted 14 lines

Hi,

Quoted 5 lines · Maarten Lankhorst

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.

Quoted 3 lines

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.

Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.

Op 27-08-15 om 14:48 schreef Ville Syrjälä:

Quoted 16 lines · Maarten Lankhorst

Quoted 15 lines

Op 27-08-15 om 14:19 schreef Daniel Stone:

Quoted 14 lines

Hi,

Quoted 5 lines · Maarten Lankhorst

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.

Quoted 7 lines

Quoted 3 lines

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.

Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.

You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.

Quoted 20 lines

Op 27-08-15 om 14:48 schreef Ville Syrjälä:

Quoted 19 lines · Maarten Lankhorst

Quoted 18 lines

Op 27-08-15 om 14:19 schreef Daniel Stone:

Quoted 9 lines

Hi,

Quoted 5 lines · Maarten Lankhorst

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.

Quoted 5 lines

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.

Quoted 5 lines

Quoted 4 lines

Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.

You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.

What's so special about the event here? Just send it out as soon as the state has been swapped.

Op 27-08-15 om 14:52 schreef Ville Syrjälä:

Quoted 21 lines · Maarten Lankhorst

Quoted 20 lines

Op 27-08-15 om 14:48 schreef Ville Syrjälä:

Quoted 19 lines · Maarten Lankhorst

Quoted 18 lines

Op 27-08-15 om 14:19 schreef Daniel Stone:

Quoted 9 lines

Hi,

Quoted 5 lines · Maarten Lankhorst

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.

Quoted 5 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 4 lines

Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.

You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.

What's so special about the event here? Just send it out as soon as the state has been swapped.

Previously this has been disallowed for legacy page flips.

I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.

Quoted 26 lines

Op 27-08-15 om 14:52 schreef Ville Syrjälä:

Quoted 25 lines · Maarten Lankhorst

Quoted 24 lines

Op 27-08-15 om 14:48 schreef Ville Syrjälä:

Quoted 23 lines · Maarten Lankhorst

Quoted 10 lines

Op 27-08-15 om 14:19 schreef Daniel Stone:

Quoted 9 lines

Hi,

Quoted 5 lines · Maarten Lankhorst

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.

Quoted 8 lines

Quoted 5 lines

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.

Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.

What's so special about the event here? Just send it out as soon as the state has been swapped.

Previously this has been disallowed for legacy page flips.

I don't think so. Speaking for i915, I think we've just rejected legacy page flips entirely with the pipe is off on account of drm_vblank_get() failing.

Quoted 1 line

I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.

We could stick the last vbl count/timestamp in there.

Not allowing means userspace is forced to consider the dpms state whenever it wants to call the atomic ioctl.

Op 27-08-15 om 15:50 schreef Ville Syrjälä:

Quoted 27 lines · Maarten Lankhorst

Quoted 26 lines

Op 27-08-15 om 14:52 schreef Ville Syrjälä:

Quoted 25 lines · Maarten Lankhorst

Quoted 24 lines

Op 27-08-15 om 14:48 schreef Ville Syrjälä:

Quoted 23 lines · Maarten Lankhorst

Quoted 10 lines

Op 27-08-15 om 14:19 schreef Daniel Stone:

Quoted 9 lines

Hi,

Quoted 5 lines · Maarten Lankhorst

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.

Quoted 8 lines

Quoted 5 lines

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.

Why would commit fail when the we're in DPMS off? I would suggest it should be allowed. The operation would just a be a nop from a HW point of view, all the calculation/checks would still be performed.

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

You can commit, just not with PAGE_FLIP_EVENT set when crtc is inactive.

What's so special about the event here? Just send it out as soon as the state has been swapped.

Previously this has been disallowed for legacy page flips.

I don't think so. Speaking for i915, I think we've just rejected legacy page flips entirely with the pipe is off on account of drm_vblank_get() failing.

No atomic driver handles this case correctly. You can't get vblank events with the crtc off.

Quoted 5 lines

Quoted 1 line

I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.

We could stick the last vbl count/timestamp in there.

Not allowing means userspace is forced to consider the dpms state whenever it wants to call the atomic ioctl.

Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)

Quoted 4 lines · Maarten Lankhorst

Op 27-08-15 om 15:50 schreef Ville Syrjälä:

Quoted 2 lines

I don't think so. Speaking for i915, I think we've just rejected legacy page flips entirely with the pipe is off on account of drm_vblank_get() failing.

No atomic driver handles this case correctly. You can't get vblank events with the crtc off.

I don't understand what you're saying. Should there be a comma after "No" ? If not, then I'm not sure what they don't handle.

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.

We could stick the last vbl count/timestamp in there.

Not allowing means userspace is forced to consider the dpms state whenever it wants to call the atomic ioctl.

Quoted 1 line

Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)

Meh. Much simpler to write code when you don't have to worry about such details.

In the kernel it should amount to
if (!pipe_active)
send_event

We anyway need something like that for the crtc getting disabled case, don't we?

Hi,

Quoted 8 lines · Ville Syrjälä

Quoted 4 lines · Maarten Lankhorst

Op 27-08-15 om 15:50 schreef Ville Syrjälä:

Quoted 2 lines

I don't think so. Speaking for i915, I think we've just rejected legacy page flips entirely with the pipe is off on account of drm_vblank_get() failing.

No atomic driver handles this case correctly. You can't get vblank events with the crtc off.

I don't understand what you're saying. Should there be a comma after "No" ? If not, then I'm not sure what they don't handle.

I think you're arguing over the definition of 'correctly' perhaps?

Quoted 10 lines

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

I don't see why this should be relaxed. It just complicates things and you have nothing to stick in for the vblank counter.

We could stick the last vbl count/timestamp in there.

Not allowing means userspace is forced to consider the dpms state whenever it wants to call the atomic ioctl.

Userspace was the one turning off the crtc in the first place; it shouldn't continue flipping but preserve power. :-)

Meh. Much simpler to write code when you don't have to worry about such details.

Quoted 3 lines

In the kernel it should amount to
if (!pipe_active)
send_event

No, thankyou. Asking for an event, having the request succeed, and never getting an event, is a deathtrap.

PAGE_FLIP_EVENT should mean
that either an event gets delivered for every CRTC in crtc_state, or the request getting rejected. Nothing else.

Quoted 2 lines

We anyway need something like that for the crtc getting disabled case, don't we?

Yes, which I was getting at previously. We know when the CRTC gets disabled - we have to, in order to sensibly unpin etc - so that's when the event gets sent.

Cheers,
Daniel

Quoted 7 lines · Daniel Stone

Quoted 3 lines · Ville Syrjälä

In the kernel it should amount to
if (!pipe_active)
send_event

No, thankyou. Asking for an event, having the request succeed, and never getting an event, is a deathtrap.

Obviously the event gets sent if the operation succeeds. I never suggested anything else.

Op 27-08-15 om 14:19 schreef Daniel Stone:

Quoted 5 lines

On 4 August 2015 at 12:34, Maarten Lankhorst <[email protected]> wrote:
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.

Quoted 5 lines

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 <[email protected]>

---

drivers/gpu/drm/drm_atomic.c +6 -4

index c2448f42480f..78ffb4965548 100644
@@ -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;
        }