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@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@ffwll.ch wrote:
On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen ppaalanen@gmail.com wrote:
On Tue, 14 May 2019 13:02:09 +0200 Daniel Vetter daniel.vetter@ffwll.ch wrote:
On Tue, May 14, 2019 at 10:18 AM Ser, Simon simon.ser@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