Re: [PATCH] drm/atomic: document and enforce rules around "spurious" EBUSY

From: Pekka Paalanen <ppaalanen@gmail.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Marius Vlad <marius.vlad@collabora.com>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/atomic: document and enforce rules around "spurious" EBUSY
Date: Thu, 24 Sep 2020 13:10:56 +0300	[thread overview]
Message-ID: <20200924131056.54beb12e@eldfell> (raw)
In-Reply-To: <CAKMK7uHwU0r7Z699qw3Y2HPuvzL3-B12fw3gDbdrxOX1V6iK2w@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 4548 bytes --]

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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel