connector/edid probing races

[Sean Paul]

On Sat, Mar 11, 2017 at 11:53:04AM +0100, Daniel Vetter wrote:
> On Fri, Mar 10, 2017 at 01:13:12PM -0500, Sean Paul wrote:
> > On Fri, Mar 10, 2017 at 05:52:34PM +0100, Daniel Vetter wrote:
> > > On Fri, Mar 10, 2017 at 11:01:54AM -0500, Sean Paul wrote:
> > > > On Fri, Mar 10, 2017 at 02:12:14PM +1000, Dave Airlie wrote:
> > > > > Talk to Jonas (jadahl) on irc, he had 3 mutters running and on hotplug
> > > > > all 3 of them were diving into the connector getting code. Now I think
> > > > > we can avoid this in userspace by not probing when not owning the VT,
> > > > > but it's still messy behaviour.
> > > > > 
> > > > > It looks like one thread does a getconnector, this fills out the EDID
> > > > > blob with a specific blob id,
> > > > > then second thread does getconnector, which replaces the EDID blob
> > > > > with another one
> > > > > the first thread calls get property blob, but the blob id it is
> > > > > looking for is destroyed.
> > > > > 
> > > > > Ideas?
> > > > 
> > > > I think you could probably fix this by introducing a new
> > > > drm_property_overwrite_global_blob() function. If the size of the current blob is
> > > > the same as the new blob, you could just overwrite the data and preserve the id
> > > > and references. You could then replace the drm_property_replace_global_blob() call in
> > > > drm_mode_connector_update_edid_property() to call the new function. Of course,
> > > > you would need to fall back on *replace_global_blob() if size differed, but it
> > > > would probably solve the issue above.
> > > 
> > > blobs are imutable (not that userspace can use that much since we can
> > > reuse ids), 
> > 
> > Pedantic distinction: blobs themselves aren't inherantly immutable. However, the
> > EDID property is defined as immutable. I suppose there could conceivably be some
> > userspace somewhere that relies on this. Bummer.
> 
> It's more tricky: The property is labelled as IMMUTABLE, but that's a lie,
> it essentially only means that userspace isn't allowed to change it. The
> kernel is.
> 
> But blobs themselves are immutable, this is the same semantics as with
> framebuffer objects, and we carried that over when we started to allow
> userspace to create blobs (for atomic). The only benefit is for drivers,
> as long as you hold a life reference you can just compare pointers and if
> they're equal, you know nothing changed. 

Ah, right. I had conveniently forgotten this detail. 

> Due to id reuse, userspace
> unfortunately can't do this (and yes we've had clever userspace who got
> this wrong).
> 
> > > I think if we do this we should compare the contents of the
> > > blob.
> > > 
> > > > That said, I'm not convinced it should be kernel's responsibility to really
> > > > solve this problem. How much effort should we put into mitigating the effects of
> > > > a racey and wasteful userspace?
> > > 
> > > It's a tradeoff between "why should the kernel care" and "every compositor
> > > will get this wrong". Maybe something like libweston will fix this
> > > eventually ....
> > 
> > So the question is whether it's worth it to penalize well-behaved compositors
> > with superfluous memcmp on EDID fetches to make up for the ill-behaved ones. I
> > don't think it is, but perhaps there are other benefits to preserving the single
> > blob for the lifetime of the connection.
> 
> If you can profile that memcmp in a 25ms edid read, I'll buy this argument

Sure. It just sucks to add extra code/complexity to cover such a basic faux pas.


> :-) I think the better argument against is that there's still cases where
> userspace must recover from similar issues (getconnector racing with a hpd
> which can also update the edid), but those are even rarer. It might be
> good to keep the easier-to-hit races open, to force better error handling
> in userspace.

Ok, we're saying the same thing, you just have better reasoning :-)

Sean

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel