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

[Ville Syrjälä]

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

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.

-- 
Ville Syrjälä
Intel