diff mbox series

drm/atomic: document and enforce rules around "spurious" EBUSY

Message ID 20200923151852.2952812-1-daniel.vetter@ffwll.ch
State New
Headers show
Series drm/atomic: document and enforce rules around "spurious" EBUSY | expand

Commit Message

Daniel Vetter Sept. 23, 2020, 3:18 p.m. UTC
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(+)

Comments

Marius Vlad Sept. 23, 2020, 7:17 p.m. UTC | #1
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
Daniel Vetter Sept. 23, 2020, 8:01 p.m. UTC | #2
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
Pekka Paalanen Sept. 24, 2020, 7:41 a.m. UTC | #3
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);
Daniel Vetter Sept. 24, 2020, 8:04 a.m. UTC | #4
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);
Pekka Paalanen Sept. 24, 2020, 10:10 a.m. UTC | #5
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);  
> 
> 
>
Ville Syrjälä Sept. 24, 2020, 11:01 a.m. UTC | #6
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?
Daniel Vetter Sept. 24, 2020, 11:13 a.m. UTC | #7
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
Ville Syrjälä Sept. 24, 2020, 11:32 a.m. UTC | #8
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.
Pekka Paalanen Sept. 25, 2020, 8:24 a.m. UTC | #9
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);
Daniel Vetter Sept. 25, 2020, 8:45 a.m. UTC | #10
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 mbox series

Patch

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);