| Message ID | 55DF05F7.1010308@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + */ > 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 > 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
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
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
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);