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@xxxxxxxx> 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@xxxxxxxx> wrote: > > > > > > > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote: > > > > > > > > > > On Tue, 14 May 2019 13:02:09 +0200 > > > > > Daniel Vetter <daniel.vetter@xxxxxxxx> wrote: > > > > > > > > > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon <simon.ser@xxxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
