[Intel-gfx] [PATCH] drm/atomic: document and enforce rules around "spurious" EBUSY

[Daniel Vetter]

On Thu, Sep 24, 2020 at 1:01 PM Ville Syrjälä
<ville.syrjala at 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 at ffwll.ch> wrote:
> >
> > > On Thu, Sep 24, 2020 at 9:41 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > > >
> > > > On Wed, 23 Sep 2020 22:01:25 +0200
> > > > Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> > > >
> > > > > On Wed, Sep 23, 2020 at 9:17 PM Marius Vlad <marius.vlad at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch