[Intel-gfx] [PATCH v7 09/11] drm: uevent for connector status change

Thu May 16 12:24:55 UTC 2019

On Thu, May 16, 2019 at 11:22:11AM +0300, Pekka Paalanen wrote:
> On Wed, 15 May 2019 10:24:49 +0200
> Daniel Vetter <daniel at ffwll.ch> wrote:
> 
> > On Wed, May 15, 2019 at 10:37:31AM +0300, Pekka Paalanen wrote:
> > > On Tue, 14 May 2019 16:34:01 +0200
> > > Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> > >   
> > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen <ppaalanen at gmail.com> wrote:  
> > > > >
> > > > > On Tue, 14 May 2019 13:02:09 +0200
> > > > > Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> > > > >    
> > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon <simon.ser at intel.com> wrote:    
> > > > > > >
> > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote:    
> > > 
> > > ...
> > >   
> > > > > > > > Hi Daniel,
> > > > > > > >
> > > > > > > > just to clarify the first case, specific to one very particular
> > > > > > > > property:
> > > > > > > >
> > > > > > > > With HDCP, there is a property that may change dynamically at runtime
> > > > > > > > (the undesired/desired/enabled tristate). Userspace must be notified
> > > > > > > > when it changes, I do not want userspace have to poll that property
> > > > > > > > with a timer.
> > > > > > > >
> > > > > > > > When that property alone changes, and userspace is prepared to handle
> > > > > > > > that property changing alone, it must not trigger a reprobe of the
> > > > > > > > connector. There is no reason to reprobe at that point AFAIU.
> > > > > > > >
> > > > > > > > How do you ensure that userspace can avoid triggering a reprobe with the
> > > > > > > > epoch approach or with any alternate uevent design?
> > > > > > > >
> > > > > > > > We need an event to userspace that indicates that re-reading the
> > > > > > > > properties is enough and reprobe of the connector is not necessary.
> > > > > > > > This is complementary to indicating to userspace that only some
> > > > > > > > connectors need to be reprobed instead of everything.    
> > > > > > >
> > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip the
> > > > > > > reprobing. Would that work?    
> > > > >
> > > > > Hi,
> > > > >
> > > > > yes, that would work, if it was acceptable to DRM upstream. The replies
> > > > > to Paul seemed to be going south so fast that I thought we wouldn't get
> > > > > any new uevent fields in favour of "epoch counters".
> > > > >    
> > > > > > Yes that's the idea, depending upon which property you get you know
> > > > > > it's a sink change (needs full reprobe) or something else like hdcp
> > > > > > state machinery update.    
> > > > >
> > > > > Right.
> > > > >    
> > > > > > Wrt avoiding the full reprobe for sink changes: I think we should
> > > > > > indeed decouple that from the per-connector event for sink changes.
> > > > > > That along is a good win already, since you know for which connector
> > > > > > you need to call drmGetConnector (which forces the reprobe). It would
> > > > > > be nice to only call drmGetConnectorCurrent (avoids the reprobe), but
> > > > > > historically speaking every time we tried to rely on this we ended up
> > > > > > regretting things.    
> > > > >
> > > > > What changed? This sounds very much what Paul suggested. Looking at it
> > > > > from userspace side:    
> > > > 
> > > > This sounds solid, some refinements below:
> > > >   
> > > > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy
> > > > >
> > > > > - If yy is "Content Protection", no need to drmModeGetConnector(), just
> > > > >   re-get the connector properties.
> > > > >
> > > > > - Kernel probably shouldn't bother sending this for properties where
> > > > >   re-probe could be necessary, and send the below instead.    
> > > > 
> > > > 
> > > > I think we should assert that the kernel can get the new property
> > > > values using drmModeGetConnectorCurrent for this case, i.e. the kernel
> > > > does not expect a full reprobe. I.e. upgrade your idea from "should"
> > > > to "must"  
> > > 
> > > Hi Daniel,
> > > 
> > > ok, that's good.
> > >   
> > > > Furthermore different property can indicate different kind of updates,
> > > > e.g. hdcp vs general sink change vs. whatever else might come in the
> > > > future.  
> > > 
> > > What do you mean by different kinds of updates?  
> > 
> > Atm we're discussing two:
> > 
> > - "Content Protection"
> > - "sink changed, but you don't need to reprobe" this would be quite a bit
> >   a catch all from the output detection. Paul thinks differently, but I'm
> >   not sold on splitting this up more, at least not right now. This would
> >   include connector status (and related things returned by drmGetConnector
> >   which currently aren't a property), EDID, the mst path id, that kind of
> >   stuff.
> > 
> > Ime once we have 2, there's more bound to come :-)
> 
> Hi Daniel,
> 
> I don't understand what the "sink changed" thing could be, but sure,
> there can be more.

So if you have a repeater (hdmi or dp) and you change the thing you plug
into that, then on the computer you don't get a full hotplug, because the
repeater was always connected. Instead you get a short pulse hotplug,
indicating that something with the sink has changed. This could be a
slightly adjusted EDID (e.g. different eld in the audio section), or
something else. That's what I mean with "sink changed". Similar thing can
happen if you unplug and then plug in something else real quick (usually
over suspend/resume), where connector->status stays the same, but the
actual thing plugged in is different. I think for hdmi this is just the
EDID, but we do parse a bunch of things out of the EDID that have further
effects (color space, max clock). With DP there's also dp aux stuff, e.g.
if you switch from a 2 lane to a 4 lane cable then with same screen more
modes can work.

Clearer?

I guess for the documentation for this new uapi we need to make an
exhaustive list of all the things that might have changed for a "sink
changed" event, whatever we actually agree on what that should look like.
Or the PROPERTY=0 fallback you mention below as a fallback idea.

> > > Btw. I started thinking, maybe we should completely leave out the "If
> > > yy is "Content Protection"" and require the kernel to guarantee, that
> > > if PROPERTY is set, then drmModeGetConnector() (probing) must not be
> > > necessary based on this event alone.
> > > 
> > > Writing it down again:
> > > 
> > > HOTPLUG=1 CONNECTOR=xx PROPERTY=yy
> > > 
> > > - yy denotes which connector xx property changed.  
> > 
> > Maybe yy denotes which group of properties changed, and part of the uapi
> > is picking the canonical one. E.g. content protection might also gain more
> > properties in the future (there's patches, but the userspace won't be open
> > sourced). And for that case I don't think we should then send an even for
> > every single individual property, but just for the lead property.
> > 
> > Maybe we should change it to UPDATE_TYPE=<some-unique-string>, but it felt
> > better to use the property id we already have for this.
> 
> Indeed, it is not really necessary to identify the exact property.
> 
> We could even just use PROPERTY=0 to indicate "something may have
> changed, you should re-get the properties, but no need to probe I
> promise".
> 
> Or like you said, whatever. I don't really care as long as the
> semantics are the same.
> 
> > > - Userspace does not need to do drmModeGetConnector(), it only needs to
> > >   drmModeObjectGetProperties() on the connector to receive the new
> > >   updated property values.  
> > 
> > drmModeGetConnector(Current) also supplies all the properties already.
> > This is special with connectors, since the predate the "properties on
> > everything" design. I'd just mention this function here, and ignore
> > drmModeObjectGetProperties.
> 
> To avoid confusion, it would be best to mention all three functions
> then. It is very easy to forget about legacy uAPI like properties
> through GetConnector.
> 
> > > - Kernel must not send this event for changes that may require probing
> > >   for correct results, exceptional conditions (buggy hardware, etc.)
> > >   included. Instead, the kernel must send one of the below events.
> > > 
> > > Is there actually anything interesting that
> > > drmModeGetConnectorCurrent() could guaranteed correctly return that is
> > > not a property already? I'd probably leave this consideration out
> > > completely, and just say do one of the needs-probing events if anything
> > > there changed.  
> > 
> > That's why I'm suggesting the PROPERTY=<epoch_prop_id> would indicate all
> > sink related stuff, including the not-properperty-fied stuff is updated,
> > and will be reported correctly by GetConnectorCurrent.
> 
> Just because GetConnectorCurrent returns the same properties as
> drmModeObjectGetProperties? Ok then. Personally I'm quite firmly in the
> land where drmModeObjectGetProperties exists, so don't really care
> about the legacy stuff.

So from a quick skimming GetConnectorCurrent == GetProperties, except you
don't get the non-propertified stuff like mode list, ->status, and a few
other things we parse out from various sources. So for connectors you need
to use GetConnector/Current anyway I think, if you rely on GetProperties
only, you're missing stuff.

Agreed that we need to spell this all out.

> > > > > HOTPLUG=1 CONNECTOR=xx
> > > > >
> > > > > - Needs to drmModeGetConnector() on the one connector, no need to probe
> > > > >   others. Implies that one needs to re-get the connector properties as
> > > > >   well.    
> > > > 
> > > > Sounds good.
> > > >   
> > > > > HOTPLUG=1
> > > > >
> > > > > - Need to do drmM odeGetResouces() to discover new/disappeared
> > > > >   connectors, and need to drmModeGetConnector to re-probe every
> > > > >   connector. (As always.)    
> > > > 
> > > > Maybe we should clarify that this is also what you get when an entire
> > > > connector appears/disappears (for dp mst hotplug).  
> > > 
> > > Yes, that's what I wrote. :-)
> > > 
> > > Weston implements the discovery of appearing/disappearing connectors
> > > (as opposed to connecting/disconnecting connectors). Not sure anyone
> > > has ever tested it though...  
> > 
> > From what -modesetting and X drivers do: Expect surprises in real world
> > usage :-/
> 
> I don't know what they do, but sure, always. :-)
> 
> As long as no-one uses untested Weston code to scream "kernel
> regression"...
> 
> > > > Maybe we could make an additional rule that if a connector has the
> > > > EPOCH property, then it does _not_ need to be reprobe for the global
> > > > events. For that case userspace should only check whether there's
> > > > new/removed connectors, and then probe the new ones (and disable the
> > > > removed ones as needed). We can also use some other flag to indicate
> > > > this if we don't add the epoch proprty.  
> > > 
> > > Sounds fine to me, though I'm not too clear what the epoch property
> > > is designed to achieve. Is it about avoiding re-probing when re-gaining
> > > DRM master after having let it go, e.g. VT-switching back from another
> > > VT? That would be nice.  
> > 
> > Yup, pretty much. Plus I think we need the epoch internally in the kernel
> > anyway, to figure out what has changed without having to rewrite endless
> > amounts of output detection code in all drivers to pass up a new status
> > change return code. Because atm we totally fail to track sink-related
> > changes from short pulse hpd (i.e. stays connected, but e.g. edid
> > changed).
> 
> I do not care at all what you might need internally. ;-)
> 
> I am solely interested in the uAPI, and will not look at kernel code. I
> just don't have the time for that.
> 
> > > > > That should be also backwards-compatible: any userspace that doesn't
> > > > > understand CONNECTOR will see HOTPLUG=1 and re-probe everything. Any
> > > > > userspace that doesn't understand PROPERTY or the property it refers to
> > > > > will fall back to probing either the connector or everything.    
> > > > 
> > > > Agreed, that should work.  
> > > 
> > > Cool. The epoch exception you worded seems to fit backward-compatible
> > > as well.
> > >   
> > > >   
> > > > > I would be happy to get that behaviour into Weston, particularly as the
> > > > > HDCP feature is brewing for Weston too.
> > > > >
> > > > > --------
> > > > >
> > > > > When discussing this in IRC, I had the concern about how uevents are
> > > > > delivered in userspace. Is there a possibility that they might be
> > > > > overwritten, contain stale attributes, or get squashed together?
> > > > >
> > > > > Particularly if a display server is current on the VT and active and
> > > > > monitoring udev, but stuck doing something and cannot service uevents
> > > > > very fast, and the kernel sends more than one event before the process
> > > > > gets back to dispatching. The terminology in libudev API confused me as
> > > > > an event is a device. Squashing together would make sense if the
> > > > > uevent were just updating a device attribute list. Previously when we
> > > > > had just a single kind of uevent, that would not have made a
> > > > > difference, but if we gain different kinds of uevents like here, it
> > > > > starts to matter.
> > > > >
> > > > > However, Paul came to the conclusion that we will be ok as long as the
> > > > > events come via netlink.    
> > > > 
> > > > Yeah netlink shouldn't drop events on the floor I think. It might
> > > > still happen, but then I think you should get an indication of that
> > > > error, and you just treat it as a general hotplug event like on older
> > > > kernels.  
> > > 
> > > Alright, although reading Paul it sounds like there is another
> > > (fallback?) method as well that wouldn't work. Should userspace worry
> > > about that?
> > > 
> > > Hmm, get an indication of an error... I don't know how that would be
> > > presented in libudev API and I can't point to any code in Weston that
> > > would deal with it. Does anyone have a clue about that?
> > > 
> > > Userspace cannot really start taking advantage of any new fine-grained
> > > hotplug events until it can rely on the event delivery. Granted, this
> > > seems purely a userspace issue, but I bet it could be formulated as a
> > > kernel regression: things stop working after upgrading the kernel while
> > > having always used new userspace which was ready for detailed hotplug
> > > events but didn't ensure the delivery in userspace.  
> > 
> > You have this already (if it's really an issue with netlink reliability,
> > tbh no idea), you can already miss a global uevent. It's easier to catch
> > up if you do miss it, since you're forcing a reprobe on everything. That's
> > why I think the EPOCH thing would be good, userspace could be defensive
> > and always call GetConnectorCurrent on all connectors if it gets any
> > hotplug uevent, and if it gets an EPOCH change, force a reprobe. But I'm
> > not sure that's really required (aside from VT switching).
> 
> No, my concern is not an issue with netlink reliability. It is a
> potential issue when userspace chooses to not use netlink, and uses
> something else instead. I'm not sure what that else is but Paul says
> there is code in libudev and that is completely outside the control of
> KMS apps like display servers.

afaik this other path only exists because it's the older one, for uapi
backwards compatibility with older userspace. Shouldn't be used for
anything.

> Can you explain how one could miss a global hotplug event when
> userspace is using netlink to watch for events? I thought the netlink
> path through libudev was reliable. And how does userspace realize it
> missed an event?

I thought netlink is supposed to be reliable, but then if you send
bazillion of events and userspace is stuck, eventually you will run out of
memory. I have no idea how netlink signals that, and how that's forwarded
or not in libudev. Also not sure whether we should actually care about
this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch