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 --- 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)
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
Op 27-08-15 om 14:19 schreef Daniel Stone:
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.
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@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);
On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:19 schreef Daniel Stone:
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.
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.
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@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.*/
/* _check_only() does not free state, unlike _commit() */if (!ret)drm_atomic_state_free(state);@@ -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 dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 27-08-15 om 14:48 schreef Ville Syrjälä:
On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:19 schreef Daniel Stone:
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.
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.
~Maarten
On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:48 schreef Ville Syrjälä:
On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:19 schreef Daniel Stone:
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.
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.
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ä:
On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:48 schreef Ville Syrjälä:
On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:19 schreef Daniel Stone:
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.
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.
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.
~Maarten
On Thu, Aug 27, 2015 at 03:05:38PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:52 schreef Ville Syrjälä:
On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:48 schreef Ville Syrjälä:
On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:19 schreef Daniel Stone:
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.
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.
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.
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ä:
On Thu, Aug 27, 2015 at 03:05:38PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:52 schreef Ville Syrjälä:
On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:48 schreef Ville Syrjälä:
On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:19 schreef Daniel Stone: > 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. 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.
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.
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. :-)
~Maarten
On Thu, Aug 27, 2015 at 04:00:05PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 15:50 schreef Ville Syrjälä:
On Thu, Aug 27, 2015 at 03:05:38PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:52 schreef Ville Syrjälä:
On Thu, Aug 27, 2015 at 02:50:34PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 14:48 schreef Ville Syrjälä:
On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote: > Op 27-08-15 om 14:19 schreef Daniel Stone: >> 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. > 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.
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.
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 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.
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,
On 27 August 2015 at 15:09, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Thu, Aug 27, 2015 at 04:00:05PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 15:50 schreef Ville Syrjälä:
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?
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.
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.
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
On Thu, Aug 27, 2015 at 03:28:53PM +0100, Daniel Stone wrote:
Hi,
On 27 August 2015 at 15:09, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Thu, Aug 27, 2015 at 04:00:05PM +0200, Maarten Lankhorst wrote:
Op 27-08-15 om 15:50 schreef Ville Syrjälä:
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?
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.
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.
PAGE_FLIP_EVENT should mean that either an event gets delivered for every CRTC in crtc_state, or the request getting rejected. Nothing else.
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
On 27 August 2015 at 15:34, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Thu, Aug 27, 2015 at 03:28:53PM +0100, Daniel Stone wrote:
On 27 August 2015 at 15:09, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
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.
Er, yeah. Totally missed the ! in the condition. As you were.
Cheers, Daniel
Hi,
On 27 August 2015 at 13:43, Maarten Lankhorst maarten.lankhorst@linux.intel.com wrote:
Op 27-08-15 om 14:19 schreef Daniel Stone:
On 4 August 2015 at 12:34, Maarten Lankhorst maarten.lankhorst@linux.intel.com 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.
Will reply to that in the (very old) thread.
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.
It won't happen: plane_mask will always be 0, because it's only set in the branch which is now !TEST_ONLY. So yeah, it's safe to just skip the label completely.
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@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);}}
Yeah, moving this looks good for fixing the -EDEADLK leak. So, great. Can you please add a comment above though noting that we don't need to call this branch on (ret == 0 && flags & DRM_MODE_ATOMIC_TEST_ONLY), as we do for freeing the state, because TEST_ONLY and PAGE_FLIP_EVENT are mutually exclusive? Otherwise the next person to come along and have the same idea of allowing them is probably going to break this. :P
With that though, feel free to send it directly with: Reviewed-by: Daniel Stone daniels@collabora.com
Cheers, Daniel
dri-devel@lists.freedesktop.org
-
Daniel Stone -
Maarten Lankhorst -
Ville Syrjälä