| Message ID | 20200923151852.2952812-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atomic: document and enforce rules around "spurious" EBUSY | expand |
On Wed, Sep 23, 2020 at 05:18:52PM +0200, Daniel Vetter wrote: > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > pull in arbitrary other resources, including CRTCs (e.g. when > reconfiguring global resources). > > But in nonblocking mode userspace has then no idea this happened, > which can lead to spurious EBUSY calls, both: > - when that other CRTC is currently busy doing a page_flip the > ALLOW_MODESET commit can fail with an EBUSY > - on the other CRTC a normal atomic flip can fail with EBUSY because > of the additional commit inserted by the kernel without userspace's > knowledge > > For blocking commits this isn't a problem, because everyone else will > just block until all the CRTC are reconfigured. Only thing userspace > can notice is the dropped frames without any reason for why frames got > dropped. > > Consensus is that we need new uapi to handle this properly, but no one > has any idea what exactly the new uapi should look like. Since this > has been shipping for years already compositors need to deal no matter > what, so as a first step just try to enforce this across drivers > better with some checks. > > v2: Add comments and a WARN_ON to enforce this only when allowed - we > don't want to silently convert page flips into blocking plane updates > just because the driver is buggy. > > v3: Fix inverted WARN_ON (Pekka). > > v4: Drop the uapi changes, only add a WARN_ON for now to enforce some > rules for drivers. > > v5: Make the WARNING more informative (Daniel) > > v6: Add unconditional debug output for compositor hackers to figure > out what's going on when they get an EBUSY (Daniel) > > References: https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html > Bugzilla: https://gitlab.freedesktop.org/wayland/weston/issues/24#note_9568 > Cc: Daniel Stone <daniel@fooishbar.org> > Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk> > Cc: Simon Ser <contact@emersion.fr> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 58527f151984..f1a912e80846 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free); > * needed. It will also grab the relevant CRTC lock to make sure that the state > * is consistent. > * > + * WARNING: Drivers may only add new CRTC states to a @state if > + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit > + * not created by userspace through an IOCTL call. > + * > * Returns: > * > * Either the allocated state or the error code encoded into the pointer. When > @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > struct drm_crtc_state *new_crtc_state; > struct drm_connector *conn; > struct drm_connector_state *conn_state; > + unsigned requested_crtc = 0; > + unsigned affected_crtc = 0; > int i, ret = 0; > > DRM_DEBUG_ATOMIC("checking %p\n", state); > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > + requested_crtc |= drm_crtc_mask(crtc); > + > for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { > ret = drm_atomic_plane_check(old_plane_state, new_plane_state); > if (ret) { > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > } > } > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > + affected_crtc |= drm_crtc_mask(crtc); > + > + /* > + * For commits that allow modesets drivers can add other CRTCs to the > + * atomic commit, e.g. when they need to reallocate global resources. > + * This can cause spurious EBUSY, which robs compositors of a very > + * effective sanity check for their drawing loop. Therefor only allow > + * drivers to add unrelated CRTC states for modeset commits. > + * > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > + * so compositors know what's going on. > + */ > + if (affected_crtc != requested_crtc) { > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > + requested_crtc, affected_crtc); > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > + requested_crtc, affected_crtc); Previous patch had the warn on state->allow_modeset now is !state->allow_modeset. Is that correct? I haven't followed the entire thread on this matter, but I guess the idea is that somehow the kernel would pass to userspace a CRTC mask of affected_crtc (somehow, we don't know how atm) and with it, userspace can then issue a new commit (this commit blocking) with those? > + } > + > return 0; > } > EXPORT_SYMBOL(drm_atomic_check_only); > -- > 2.28.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 23, 2020 at 9:17 PM Marius Vlad <marius.vlad@collabora.com> wrote: > > On Wed, Sep 23, 2020 at 05:18:52PM +0200, Daniel Vetter wrote: > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > > pull in arbitrary other resources, including CRTCs (e.g. when > > reconfiguring global resources). > > > > But in nonblocking mode userspace has then no idea this happened, > > which can lead to spurious EBUSY calls, both: > > - when that other CRTC is currently busy doing a page_flip the > > ALLOW_MODESET commit can fail with an EBUSY > > - on the other CRTC a normal atomic flip can fail with EBUSY because > > of the additional commit inserted by the kernel without userspace's > > knowledge > > > > For blocking commits this isn't a problem, because everyone else will > > just block until all the CRTC are reconfigured. Only thing userspace > > can notice is the dropped frames without any reason for why frames got > > dropped. > > > > Consensus is that we need new uapi to handle this properly, but no one > > has any idea what exactly the new uapi should look like. Since this > > has been shipping for years already compositors need to deal no matter > > what, so as a first step just try to enforce this across drivers > > better with some checks. > > > > v2: Add comments and a WARN_ON to enforce this only when allowed - we > > don't want to silently convert page flips into blocking plane updates > > just because the driver is buggy. > > > > v3: Fix inverted WARN_ON (Pekka). > > > > v4: Drop the uapi changes, only add a WARN_ON for now to enforce some > > rules for drivers. > > > > v5: Make the WARNING more informative (Daniel) > > > > v6: Add unconditional debug output for compositor hackers to figure > > out what's going on when they get an EBUSY (Daniel) > > > > References: https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html > > Bugzilla: https://gitlab.freedesktop.org/wayland/weston/issues/24#note_9568 > > Cc: Daniel Stone <daniel@fooishbar.org> > > Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk> > > Cc: Simon Ser <contact@emersion.fr> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 58527f151984..f1a912e80846 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free); > > * needed. It will also grab the relevant CRTC lock to make sure that the state > > * is consistent. > > * > > + * WARNING: Drivers may only add new CRTC states to a @state if > > + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit > > + * not created by userspace through an IOCTL call. > > + * > > * Returns: > > * > > * Either the allocated state or the error code encoded into the pointer. When > > @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > struct drm_crtc_state *new_crtc_state; > > struct drm_connector *conn; > > struct drm_connector_state *conn_state; > > + unsigned requested_crtc = 0; > > + unsigned affected_crtc = 0; > > int i, ret = 0; > > > > DRM_DEBUG_ATOMIC("checking %p\n", state); > > > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > > + requested_crtc |= drm_crtc_mask(crtc); > > + > > for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { > > ret = drm_atomic_plane_check(old_plane_state, new_plane_state); > > if (ret) { > > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > } > > } > > > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > > + affected_crtc |= drm_crtc_mask(crtc); > > + > > + /* > > + * For commits that allow modesets drivers can add other CRTCs to the > > + * atomic commit, e.g. when they need to reallocate global resources. > > + * This can cause spurious EBUSY, which robs compositors of a very > > + * effective sanity check for their drawing loop. Therefor only allow > > + * drivers to add unrelated CRTC states for modeset commits. > > + * > > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > > + * so compositors know what's going on. > > + */ > > + if (affected_crtc != requested_crtc) { > > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > > + requested_crtc, affected_crtc); > > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > > + requested_crtc, affected_crtc); > Previous patch had the warn on state->allow_modeset now is > !state->allow_modeset. Is that correct? We need to fire a warning when allow_modeset is _not_ set. An earlier version got that wrong, and yes that would have caused a _ton_ of warnings on any fairly new intel platform. > I haven't followed the entire thread on this matter, but I guess the idea > is that somehow the kernel would pass to userspace a CRTC mask of > affected_crtc (somehow, we don't know how atm) and with it, userspace > can then issue a new commit (this commit blocking) with those? Either that, or just use that to track all the in-flight drm events. Userspace will get events for all the crtc, not just the one it asked to update. If that's easier to do by re-issuing the commit with the full set of crtc, then I guess that's an option. But not required (I think at least, would need to test that to make sure). -Daniel > > + } > > + > > return 0; > > } > > EXPORT_SYMBOL(drm_atomic_check_only); > > -- > > 2.28.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 23 Sep 2020 22:01:25 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Wed, Sep 23, 2020 at 9:17 PM Marius Vlad <marius.vlad@collabora.com> wrote: > > > > On Wed, Sep 23, 2020 at 05:18:52PM +0200, Daniel Vetter wrote: > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > > > pull in arbitrary other resources, including CRTCs (e.g. when > > > reconfiguring global resources). ... > > > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > > } > > > } > > > > > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > > > + affected_crtc |= drm_crtc_mask(crtc); > > > + > > > + /* > > > + * For commits that allow modesets drivers can add other CRTCs to the > > > + * atomic commit, e.g. when they need to reallocate global resources. > > > + * This can cause spurious EBUSY, which robs compositors of a very > > > + * effective sanity check for their drawing loop. Therefor only allow > > > + * drivers to add unrelated CRTC states for modeset commits. > > > + * > > > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > > > + * so compositors know what's going on. > > > + */ > > > + if (affected_crtc != requested_crtc) { > > > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > > > + requested_crtc, affected_crtc); > > > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > > > + requested_crtc, affected_crtc); > > Previous patch had the warn on state->allow_modeset now is > > !state->allow_modeset. Is that correct? > > We need to fire a warning when allow_modeset is _not_ set. An earlier > version got that wrong, and yes that would have caused a _ton_ of > warnings on any fairly new intel platform. > > > I haven't followed the entire thread on this matter, but I guess the idea > > is that somehow the kernel would pass to userspace a CRTC mask of > > affected_crtc (somehow, we don't know how atm) and with it, userspace > > can then issue a new commit (this commit blocking) with those? > > Either that, or just use that to track all the in-flight drm events. > Userspace will get events for all the crtc, not just the one it asked > to update. Wait, does that happen already? Getting CRTC events for CRTCs userspace didn't include in the atomic commit? That could explain why Weston freaks out in https://gitlab.freedesktop.org/wayland/weston/-/issues/435 Thanks, pq > If that's easier to do by re-issuing the commit with the > full set of crtc, then I guess that's an option. But not required (I > think at least, would need to test that to make sure). > -Daniel > > > > + } > > > + > > > return 0; > > > } > > > EXPORT_SYMBOL(drm_atomic_check_only);
On Thu, Sep 24, 2020 at 9:41 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > On Wed, 23 Sep 2020 22:01:25 +0200 > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > On Wed, Sep 23, 2020 at 9:17 PM Marius Vlad <marius.vlad@collabora.com> wrote: > > > > > > On Wed, Sep 23, 2020 at 05:18:52PM +0200, Daniel Vetter wrote: > > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > > > > pull in arbitrary other resources, including CRTCs (e.g. when > > > > reconfiguring global resources). > > ... > > > > > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > > > } > > > > } > > > > > > > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > > > > + affected_crtc |= drm_crtc_mask(crtc); > > > > + > > > > + /* > > > > + * For commits that allow modesets drivers can add other CRTCs to the > > > > + * atomic commit, e.g. when they need to reallocate global resources. > > > > + * This can cause spurious EBUSY, which robs compositors of a very > > > > + * effective sanity check for their drawing loop. Therefor only allow > > > > + * drivers to add unrelated CRTC states for modeset commits. > > > > + * > > > > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > > > > + * so compositors know what's going on. > > > > + */ > > > > + if (affected_crtc != requested_crtc) { > > > > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > > > > + requested_crtc, affected_crtc); > > > > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > > > > + requested_crtc, affected_crtc); > > > Previous patch had the warn on state->allow_modeset now is > > > !state->allow_modeset. Is that correct? > > > > We need to fire a warning when allow_modeset is _not_ set. An earlier > > version got that wrong, and yes that would have caused a _ton_ of > > warnings on any fairly new intel platform. > > > > > I haven't followed the entire thread on this matter, but I guess the idea > > > is that somehow the kernel would pass to userspace a CRTC mask of > > > affected_crtc (somehow, we don't know how atm) and with it, userspace > > > can then issue a new commit (this commit blocking) with those? > > > > Either that, or just use that to track all the in-flight drm events. > > Userspace will get events for all the crtc, not just the one it asked > > to update. > > Wait, does that happen already? Getting CRTC events for CRTCs userspace > didn't include in the atomic commit? Yeah I'm pretty sure. With the affected_crtc mask you could update your internal book-keeping to catch these, which should also prevent all the spurious EBUSY. But I'm not entirely sure, I just read the code, haven't tested. > That could explain why Weston freaks out in > https://gitlab.freedesktop.org/wayland/weston/-/issues/435 Hm it's strange that you first get an EBUSY, and only on the next modeset get the spurious event. You should get one already on the first modeset. -Daniel > > > Thanks, > pq > > > > If that's easier to do by re-issuing the commit with the > > full set of crtc, then I guess that's an option. But not required (I > > think at least, would need to test that to make sure). > > -Daniel > > > > > > + } > > > > + > > > > return 0; > > > > } > > > > EXPORT_SYMBOL(drm_atomic_check_only);
On Thu, 24 Sep 2020 10:04:12 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Thu, Sep 24, 2020 at 9:41 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > On Wed, 23 Sep 2020 22:01:25 +0200 > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > On Wed, Sep 23, 2020 at 9:17 PM Marius Vlad <marius.vlad@collabora.com> wrote: > > > > > > > > On Wed, Sep 23, 2020 at 05:18:52PM +0200, Daniel Vetter wrote: > > > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > > > > > pull in arbitrary other resources, including CRTCs (e.g. when > > > > > reconfiguring global resources). > > > > ... > > > > > > > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > > > > } > > > > > } > > > > > > > > > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > > > > > + affected_crtc |= drm_crtc_mask(crtc); > > > > > + > > > > > + /* > > > > > + * For commits that allow modesets drivers can add other CRTCs to the > > > > > + * atomic commit, e.g. when they need to reallocate global resources. > > > > > + * This can cause spurious EBUSY, which robs compositors of a very > > > > > + * effective sanity check for their drawing loop. Therefor only allow > > > > > + * drivers to add unrelated CRTC states for modeset commits. > > > > > + * > > > > > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > > > > > + * so compositors know what's going on. > > > > > + */ > > > > > + if (affected_crtc != requested_crtc) { > > > > > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > > > > > + requested_crtc, affected_crtc); > > > > > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > > > > > + requested_crtc, affected_crtc); > > > > Previous patch had the warn on state->allow_modeset now is > > > > !state->allow_modeset. Is that correct? > > > > > > We need to fire a warning when allow_modeset is _not_ set. An earlier > > > version got that wrong, and yes that would have caused a _ton_ of > > > warnings on any fairly new intel platform. > > > > > > > I haven't followed the entire thread on this matter, but I guess the idea > > > > is that somehow the kernel would pass to userspace a CRTC mask of > > > > affected_crtc (somehow, we don't know how atm) and with it, userspace > > > > can then issue a new commit (this commit blocking) with those? > > > > > > Either that, or just use that to track all the in-flight drm events. > > > Userspace will get events for all the crtc, not just the one it asked > > > to update. > > > > Wait, does that happen already? Getting CRTC events for CRTCs userspace > > didn't include in the atomic commit? > > Yeah I'm pretty sure. With the affected_crtc mask you could update > your internal book-keeping to catch these, which should also prevent > all the spurious EBUSY. But I'm not entirely sure, I just read the > code, haven't tested. If that actually happens, how does userspace know whether the userdata argument with the event is valid or not? The kernel should expect the userdata argument to be one-shot, because it may be a pointer to a malloc()'d struct that gets freed the moment the originally expected event is handled, so re-using userdata is going to break userspace (ISTR Mutter uses this style with legacy, Weston passes somewhat more persistent pointers with both legacy and atomic). Does the kernel reset it to zero? What if userspace doesn't use a pointer but e.g. an index where 0 may be a legal value but also wrong? Thanks, pq > > That could explain why Weston freaks out in > > https://gitlab.freedesktop.org/wayland/weston/-/issues/435 > > Hm it's strange that you first get an EBUSY, and only on the next > modeset get the spurious event. You should get one already on the > first modeset. > -Daniel > > > > > > > Thanks, > > pq > > > > > > > If that's easier to do by re-issuing the commit with the > > > full set of crtc, then I guess that's an option. But not required (I > > > think at least, would need to test that to make sure). > > > -Daniel > > > > > > > > + } > > > > > + > > > > > return 0; > > > > > } > > > > > EXPORT_SYMBOL(drm_atomic_check_only); > > >
On Thu, Sep 24, 2020 at 01:10:56PM +0300, Pekka Paalanen wrote: > On Thu, 24 Sep 2020 10:04:12 +0200 > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > On Thu, Sep 24, 2020 at 9:41 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > > > On Wed, 23 Sep 2020 22:01:25 +0200 > > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > > On Wed, Sep 23, 2020 at 9:17 PM Marius Vlad <marius.vlad@collabora.com> wrote: > > > > > > > > > > On Wed, Sep 23, 2020 at 05:18:52PM +0200, Daniel Vetter wrote: > > > > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > > > > > > pull in arbitrary other resources, including CRTCs (e.g. when > > > > > > reconfiguring global resources). > > > > > > ... > > > > > > > > > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > > > > > } > > > > > > } > > > > > > > > > > > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > > > > > > + affected_crtc |= drm_crtc_mask(crtc); > > > > > > + > > > > > > + /* > > > > > > + * For commits that allow modesets drivers can add other CRTCs to the > > > > > > + * atomic commit, e.g. when they need to reallocate global resources. > > > > > > + * This can cause spurious EBUSY, which robs compositors of a very > > > > > > + * effective sanity check for their drawing loop. Therefor only allow > > > > > > + * drivers to add unrelated CRTC states for modeset commits. > > > > > > + * > > > > > > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > > > > > > + * so compositors know what's going on. > > > > > > + */ > > > > > > + if (affected_crtc != requested_crtc) { > > > > > > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > > > > > > + requested_crtc, affected_crtc); > > > > > > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > > > > > > + requested_crtc, affected_crtc); > > > > > Previous patch had the warn on state->allow_modeset now is > > > > > !state->allow_modeset. Is that correct? > > > > > > > > We need to fire a warning when allow_modeset is _not_ set. An earlier > > > > version got that wrong, and yes that would have caused a _ton_ of > > > > warnings on any fairly new intel platform. > > > > > > > > > I haven't followed the entire thread on this matter, but I guess the idea > > > > > is that somehow the kernel would pass to userspace a CRTC mask of > > > > > affected_crtc (somehow, we don't know how atm) and with it, userspace > > > > > can then issue a new commit (this commit blocking) with those? > > > > > > > > Either that, or just use that to track all the in-flight drm events. > > > > Userspace will get events for all the crtc, not just the one it asked > > > > to update. > > > > > > Wait, does that happen already? Getting CRTC events for CRTCs userspace > > > didn't include in the atomic commit? > > > > Yeah I'm pretty sure. With the affected_crtc mask you could update > > your internal book-keeping to catch these, which should also prevent > > all the spurious EBUSY. But I'm not entirely sure, I just read the > > code, haven't tested. > > If that actually happens, how does userspace know whether the > userdata argument with the event is valid or not? At some point I was worried about the kernel potentially sending spurious events, but IIRC I managed to convince myself that it shouldn't happen. I think I came to the conclusion the events were populated before the core calls into the driver. But maybe I misanalyzed it, or something has since broken?
On Thu, Sep 24, 2020 at 1:01 PM Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Sep 24, 2020 at 01:10:56PM +0300, Pekka Paalanen wrote: > > On Thu, 24 Sep 2020 10:04:12 +0200 > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > On Thu, Sep 24, 2020 at 9:41 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > > > > > On Wed, 23 Sep 2020 22:01:25 +0200 > > > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > > > > On Wed, Sep 23, 2020 at 9:17 PM Marius Vlad <marius.vlad@collabora.com> wrote: > > > > > > > > > > > > On Wed, Sep 23, 2020 at 05:18:52PM +0200, Daniel Vetter wrote: > > > > > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > > > > > > > pull in arbitrary other resources, including CRTCs (e.g. when > > > > > > > reconfiguring global resources). > > > > > > > > ... > > > > > > > > > > > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > > > > > > > + affected_crtc |= drm_crtc_mask(crtc); > > > > > > > + > > > > > > > + /* > > > > > > > + * For commits that allow modesets drivers can add other CRTCs to the > > > > > > > + * atomic commit, e.g. when they need to reallocate global resources. > > > > > > > + * This can cause spurious EBUSY, which robs compositors of a very > > > > > > > + * effective sanity check for their drawing loop. Therefor only allow > > > > > > > + * drivers to add unrelated CRTC states for modeset commits. > > > > > > > + * > > > > > > > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > > > > > > > + * so compositors know what's going on. > > > > > > > + */ > > > > > > > + if (affected_crtc != requested_crtc) { > > > > > > > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > > > > > > > + requested_crtc, affected_crtc); > > > > > > > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > > > > > > > + requested_crtc, affected_crtc); > > > > > > Previous patch had the warn on state->allow_modeset now is > > > > > > !state->allow_modeset. Is that correct? > > > > > > > > > > We need to fire a warning when allow_modeset is _not_ set. An earlier > > > > > version got that wrong, and yes that would have caused a _ton_ of > > > > > warnings on any fairly new intel platform. > > > > > > > > > > > I haven't followed the entire thread on this matter, but I guess the idea > > > > > > is that somehow the kernel would pass to userspace a CRTC mask of > > > > > > affected_crtc (somehow, we don't know how atm) and with it, userspace > > > > > > can then issue a new commit (this commit blocking) with those? > > > > > > > > > > Either that, or just use that to track all the in-flight drm events. > > > > > Userspace will get events for all the crtc, not just the one it asked > > > > > to update. > > > > > > > > Wait, does that happen already? Getting CRTC events for CRTCs userspace > > > > didn't include in the atomic commit? > > > > > > Yeah I'm pretty sure. With the affected_crtc mask you could update > > > your internal book-keeping to catch these, which should also prevent > > > all the spurious EBUSY. But I'm not entirely sure, I just read the > > > code, haven't tested. > > > > If that actually happens, how does userspace know whether the > > userdata argument with the event is valid or not? > > At some point I was worried about the kernel potentially sending spurious > events, but IIRC I managed to convince myself that it shouldn't happen. > I think I came to the conclusion the events were populated before the > core calls into the driver. But maybe I misanalyzed it, or something > has since broken? Hm right this shouldn't happen, I misread the code. So if userspace wants events for all affected crtc, it needs to add them explicitly to the atomic ioctl (just set an arbitrary property on each crtc to its current value or something like that). That also means that the bug Pekka posted shouldn't have been caused by this stuff here. Note for code readers: There's a retry loop for ww-mutex backoff, but we do completely clear all states, so crtc states shouldn't be able to persist and then get events when userspace didn't ask for them. -Daniel
On Thu, Sep 24, 2020 at 01:13:17PM +0200, Daniel Vetter wrote: > On Thu, Sep 24, 2020 at 1:01 PM Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > > > On Thu, Sep 24, 2020 at 01:10:56PM +0300, Pekka Paalanen wrote: > > > On Thu, 24 Sep 2020 10:04:12 +0200 > > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > > On Thu, Sep 24, 2020 at 9:41 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > > > > > > > On Wed, 23 Sep 2020 22:01:25 +0200 > > > > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > > > > > > > On Wed, Sep 23, 2020 at 9:17 PM Marius Vlad <marius.vlad@collabora.com> wrote: > > > > > > > > > > > > > > On Wed, Sep 23, 2020 at 05:18:52PM +0200, Daniel Vetter wrote: > > > > > > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > > > > > > > > pull in arbitrary other resources, including CRTCs (e.g. when > > > > > > > > reconfiguring global resources). > > > > > > > > > > ... > > > > > > > > > > > > > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > > > > > > > > + affected_crtc |= drm_crtc_mask(crtc); > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * For commits that allow modesets drivers can add other CRTCs to the > > > > > > > > + * atomic commit, e.g. when they need to reallocate global resources. > > > > > > > > + * This can cause spurious EBUSY, which robs compositors of a very > > > > > > > > + * effective sanity check for their drawing loop. Therefor only allow > > > > > > > > + * drivers to add unrelated CRTC states for modeset commits. > > > > > > > > + * > > > > > > > > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > > > > > > > > + * so compositors know what's going on. > > > > > > > > + */ > > > > > > > > + if (affected_crtc != requested_crtc) { > > > > > > > > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > > > > > > > > + requested_crtc, affected_crtc); > > > > > > > > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > > > > > > > > + requested_crtc, affected_crtc); > > > > > > > Previous patch had the warn on state->allow_modeset now is > > > > > > > !state->allow_modeset. Is that correct? > > > > > > > > > > > > We need to fire a warning when allow_modeset is _not_ set. An earlier > > > > > > version got that wrong, and yes that would have caused a _ton_ of > > > > > > warnings on any fairly new intel platform. > > > > > > > > > > > > > I haven't followed the entire thread on this matter, but I guess the idea > > > > > > > is that somehow the kernel would pass to userspace a CRTC mask of > > > > > > > affected_crtc (somehow, we don't know how atm) and with it, userspace > > > > > > > can then issue a new commit (this commit blocking) with those? > > > > > > > > > > > > Either that, or just use that to track all the in-flight drm events. > > > > > > Userspace will get events for all the crtc, not just the one it asked > > > > > > to update. > > > > > > > > > > Wait, does that happen already? Getting CRTC events for CRTCs userspace > > > > > didn't include in the atomic commit? > > > > > > > > Yeah I'm pretty sure. With the affected_crtc mask you could update > > > > your internal book-keeping to catch these, which should also prevent > > > > all the spurious EBUSY. But I'm not entirely sure, I just read the > > > > code, haven't tested. > > > > > > If that actually happens, how does userspace know whether the > > > userdata argument with the event is valid or not? > > > > At some point I was worried about the kernel potentially sending spurious > > events, but IIRC I managed to convince myself that it shouldn't happen. > > I think I came to the conclusion the events were populated before the > > core calls into the driver. But maybe I misanalyzed it, or something > > has since broken? > > Hm right this shouldn't happen, I misread the code. So if userspace > wants events for all affected crtc, it needs to add them explicitly to > the atomic ioctl (just set an arbitrary property on each crtc to its > current value or something like that). Hmm. I thought we wouldn't even need the dummy prop. But looking at the code we do in fact need it. The ioctl structure itself should allow adding an object without any properties, but the code then skips the get_crtc_state() and thus no event for that crtc. I guess it's been like that forever so not much point in trying to change that anymore.
On Wed, 23 Sep 2020 17:18:52 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > pull in arbitrary other resources, including CRTCs (e.g. when > reconfiguring global resources). > > But in nonblocking mode userspace has then no idea this happened, > which can lead to spurious EBUSY calls, both: > - when that other CRTC is currently busy doing a page_flip the > ALLOW_MODESET commit can fail with an EBUSY > - on the other CRTC a normal atomic flip can fail with EBUSY because > of the additional commit inserted by the kernel without userspace's > knowledge > > For blocking commits this isn't a problem, because everyone else will > just block until all the CRTC are reconfigured. Only thing userspace > can notice is the dropped frames without any reason for why frames got > dropped. > > Consensus is that we need new uapi to handle this properly, but no one > has any idea what exactly the new uapi should look like. Since this > has been shipping for years already compositors need to deal no matter > what, so as a first step just try to enforce this across drivers > better with some checks. > > v2: Add comments and a WARN_ON to enforce this only when allowed - we > don't want to silently convert page flips into blocking plane updates > just because the driver is buggy. > > v3: Fix inverted WARN_ON (Pekka). > > v4: Drop the uapi changes, only add a WARN_ON for now to enforce some > rules for drivers. > > v5: Make the WARNING more informative (Daniel) > > v6: Add unconditional debug output for compositor hackers to figure > out what's going on when they get an EBUSY (Daniel) ... gmail workaround ... > --- > drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 58527f151984..f1a912e80846 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free); > * needed. It will also grab the relevant CRTC lock to make sure that the state > * is consistent. > * > + * WARNING: Drivers may only add new CRTC states to a @state if > + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit > + * not created by userspace through an IOCTL call. > + * > * Returns: > * > * Either the allocated state or the error code encoded into the pointer. When > @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > struct drm_crtc_state *new_crtc_state; > struct drm_connector *conn; > struct drm_connector_state *conn_state; > + unsigned requested_crtc = 0; > + unsigned affected_crtc = 0; > int i, ret = 0; > > DRM_DEBUG_ATOMIC("checking %p\n", state); > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > + requested_crtc |= drm_crtc_mask(crtc); Is "old crtc state" the state that userspace is requesting as the new state? > + > for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { > ret = drm_atomic_plane_check(old_plane_state, new_plane_state); > if (ret) { > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > } > } > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > + affected_crtc |= drm_crtc_mask(crtc); And after driver check processing, the "old crtc state" has been modified by the driver to add anything it will necessarily need like other CRTCs? What is "new state" then? > + > + /* > + * For commits that allow modesets drivers can add other CRTCs to the > + * atomic commit, e.g. when they need to reallocate global resources. > + * This can cause spurious EBUSY, which robs compositors of a very > + * effective sanity check for their drawing loop. Therefor only allow > + * drivers to add unrelated CRTC states for modeset commits. > + * > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > + * so compositors know what's going on. > + */ > + if (affected_crtc != requested_crtc) { > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > + requested_crtc, affected_crtc); > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > + requested_crtc, affected_crtc); > + } This hunk looks good to me. Thanks, pq > + > return 0; > } > EXPORT_SYMBOL(drm_atomic_check_only);
On Fri, Sep 25, 2020 at 11:24:46AM +0300, Pekka Paalanen wrote: > On Wed, 23 Sep 2020 17:18:52 +0200 > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to > > pull in arbitrary other resources, including CRTCs (e.g. when > > reconfiguring global resources). > > > > But in nonblocking mode userspace has then no idea this happened, > > which can lead to spurious EBUSY calls, both: > > - when that other CRTC is currently busy doing a page_flip the > > ALLOW_MODESET commit can fail with an EBUSY > > - on the other CRTC a normal atomic flip can fail with EBUSY because > > of the additional commit inserted by the kernel without userspace's > > knowledge > > > > For blocking commits this isn't a problem, because everyone else will > > just block until all the CRTC are reconfigured. Only thing userspace > > can notice is the dropped frames without any reason for why frames got > > dropped. > > > > Consensus is that we need new uapi to handle this properly, but no one > > has any idea what exactly the new uapi should look like. Since this > > has been shipping for years already compositors need to deal no matter > > what, so as a first step just try to enforce this across drivers > > better with some checks. > > > > v2: Add comments and a WARN_ON to enforce this only when allowed - we > > don't want to silently convert page flips into blocking plane updates > > just because the driver is buggy. > > > > v3: Fix inverted WARN_ON (Pekka). > > > > v4: Drop the uapi changes, only add a WARN_ON for now to enforce some > > rules for drivers. > > > > v5: Make the WARNING more informative (Daniel) > > > > v6: Add unconditional debug output for compositor hackers to figure > > out what's going on when they get an EBUSY (Daniel) > > ... gmail workaround ... > > > --- > > drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 58527f151984..f1a912e80846 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free); > > * needed. It will also grab the relevant CRTC lock to make sure that the state > > * is consistent. > > * > > + * WARNING: Drivers may only add new CRTC states to a @state if > > + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit > > + * not created by userspace through an IOCTL call. > > + * > > * Returns: > > * > > * Either the allocated state or the error code encoded into the pointer. When > > @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > struct drm_crtc_state *new_crtc_state; > > struct drm_connector *conn; > > struct drm_connector_state *conn_state; > > + unsigned requested_crtc = 0; > > + unsigned affected_crtc = 0; > > int i, ret = 0; > > > > DRM_DEBUG_ATOMIC("checking %p\n", state); > > > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > > + requested_crtc |= drm_crtc_mask(crtc); > > Is "old crtc state" the state that userspace is requesting as the new > state? It's a dummy iterator variable we don't care about, all we really want is to know which crtc are all part of the update. But everyone else also wants the state. I'll shuffle the patches so the hunk Ville requested is in the right patch. -Daniel > > > + > > for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { > > ret = drm_atomic_plane_check(old_plane_state, new_plane_state); > > if (ret) { > > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) > > } > > } > > > > + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) > > + affected_crtc |= drm_crtc_mask(crtc); > > And after driver check processing, the "old crtc state" has been > modified by the driver to add anything it will necessarily need like > other CRTCs? > > What is "new state" then? > > > + > > + /* > > + * For commits that allow modesets drivers can add other CRTCs to the > > + * atomic commit, e.g. when they need to reallocate global resources. > > + * This can cause spurious EBUSY, which robs compositors of a very > > + * effective sanity check for their drawing loop. Therefor only allow > > + * drivers to add unrelated CRTC states for modeset commits. > > + * > > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output > > + * so compositors know what's going on. > > + */ > > + if (affected_crtc != requested_crtc) { > > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", > > + requested_crtc, affected_crtc); > > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", > > + requested_crtc, affected_crtc); > > + } > > This hunk looks good to me. > > > Thanks, > pq > > > + > > return 0; > > } > > EXPORT_SYMBOL(drm_atomic_check_only); >
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 58527f151984..f1a912e80846 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free); * needed. It will also grab the relevant CRTC lock to make sure that the state * is consistent. * + * WARNING: Drivers may only add new CRTC states to a @state if + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit + * not created by userspace through an IOCTL call. + * * Returns: * * Either the allocated state or the error code encoded into the pointer. When @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state) struct drm_crtc_state *new_crtc_state; struct drm_connector *conn; struct drm_connector_state *conn_state; + unsigned requested_crtc = 0; + unsigned affected_crtc = 0; int i, ret = 0; DRM_DEBUG_ATOMIC("checking %p\n", state); + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) + requested_crtc |= drm_crtc_mask(crtc); + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { ret = drm_atomic_plane_check(old_plane_state, new_plane_state); if (ret) { @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) } } + for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) + affected_crtc |= drm_crtc_mask(crtc); + + /* + * For commits that allow modesets drivers can add other CRTCs to the + * atomic commit, e.g. when they need to reallocate global resources. + * This can cause spurious EBUSY, which robs compositors of a very + * effective sanity check for their drawing loop. Therefor only allow + * drivers to add unrelated CRTC states for modeset commits. + * + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output + * so compositors know what's going on. + */ + if (affected_crtc != requested_crtc) { + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", + requested_crtc, affected_crtc); + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", + requested_crtc, affected_crtc); + } + return 0; } EXPORT_SYMBOL(drm_atomic_check_only);
When doing an atomic modeset with ALLOW_MODESET drivers are allowed to pull in arbitrary other resources, including CRTCs (e.g. when reconfiguring global resources). But in nonblocking mode userspace has then no idea this happened, which can lead to spurious EBUSY calls, both: - when that other CRTC is currently busy doing a page_flip the ALLOW_MODESET commit can fail with an EBUSY - on the other CRTC a normal atomic flip can fail with EBUSY because of the additional commit inserted by the kernel without userspace's knowledge For blocking commits this isn't a problem, because everyone else will just block until all the CRTC are reconfigured. Only thing userspace can notice is the dropped frames without any reason for why frames got dropped. Consensus is that we need new uapi to handle this properly, but no one has any idea what exactly the new uapi should look like. Since this has been shipping for years already compositors need to deal no matter what, so as a first step just try to enforce this across drivers better with some checks. v2: Add comments and a WARN_ON to enforce this only when allowed - we don't want to silently convert page flips into blocking plane updates just because the driver is buggy. v3: Fix inverted WARN_ON (Pekka). v4: Drop the uapi changes, only add a WARN_ON for now to enforce some rules for drivers. v5: Make the WARNING more informative (Daniel) v6: Add unconditional debug output for compositor hackers to figure out what's going on when they get an EBUSY (Daniel) References: https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html Bugzilla: https://gitlab.freedesktop.org/wayland/weston/issues/24#note_9568 Cc: Daniel Stone <daniel@fooishbar.org> Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk> Cc: Simon Ser <contact@emersion.fr> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)