From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pekka Paalanen Subject: Re: [PATCH v7 09/11] drm: uevent for connector status change Date: Wed, 15 May 2019 10:37:31 +0300 Message-ID: <20190515103731.16855195@eldfell.localdomain> References: <20190507162745.25600-1-ramalingam.c@intel.com> <20190507162745.25600-10-ramalingam.c@intel.com> <31dad9a323382628911c5301a6eec179855aa815.camel@bootlin.com> <8aa3980a6948b9b2b989c237f8453ca54e72ad95.camel@bootlin.com> <20190514110242.6f6ba4b0@eldfell.localdomain> <9b6386239ecae396fc4f5cc4467f8e76721f2c83.camel@intel.com> <20190514163602.7d252b12@eldfell.localdomain> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0134721523==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: "Ser, Simon" , "maxime.ripard@bootlin.com" , "intel-gfx@lists.freedesktop.org" , "dri-devel@lists.freedesktop.org" , "Mun, Gwan-gyeong" , "paul.kocialkowski@bootlin.com" , "airlied@linux.ie" , "thomas.petazzoni@bootlin.com" , "Vetter, Daniel" , "sean@poorly.run" List-Id: dri-devel@lists.freedesktop.org --===============0134721523== Content-Type: multipart/signed; micalg=pgp-sha256; boundary="Sig_/yrwTOwUg7dkdhYf/CxHS1AV"; protocol="application/pgp-signature" --Sig_/yrwTOwUg7dkdhYf/CxHS1AV Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Tue, 14 May 2019 16:34:01 +0200 Daniel Vetter wrote: > On Tue, May 14, 2019 at 3:36 PM Pekka Paalanen wrot= e: > > > > On Tue, 14 May 2019 13:02:09 +0200 > > Daniel Vetter wrote: > > =20 > > > On Tue, May 14, 2019 at 10:18 AM Ser, Simon wro= te: =20 > > > > > > > > On Tue, 2019-05-14 at 11:02 +0300, Pekka Paalanen wrote: =20 ... > > > > > 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 run= time > > > > > (the undesired/desired/enabled tristate). Userspace must be notif= ied > > > > > when it changes, I do not want userspace have to poll that proper= ty > > > > > with a timer. > > > > > > > > > > When that property alone changes, and userspace is prepared to ha= ndle > > > > > 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 w= ith 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 necessar= y. > > > > > This is complementary to indicating to userspace that only some > > > > > connectors need to be reprobed instead of everything. =20 > > > > > > > > Can't you use the PROPERTY hint? If PROPERTY is the HDCP one, skip = the > > > > reprobing. Would that work? =20 > > > > 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". > > =20 > > > 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. =20 > > > > Right. > > =20 > > > 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. =20 > > > > What changed? This sounds very much what Paul suggested. Looking at it > > from userspace side: =20 >=20 > This sounds solid, some refinements below: >=20 > > HOTPLUG=3D1 CONNECTOR=3Dxx PROPERTY=3Dyy > > > > - 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. =20 >=20 >=20 > 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? 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=3D1 CONNECTOR=3Dxx PROPERTY=3Dyy - yy denotes which connector xx property changed. - Userspace does not need to do drmModeGetConnector(), it only needs to drmModeObjectGetProperties() on the connector to receive the new updated property values. - 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. > > HOTPLUG=3D1 CONNECTOR=3Dxx > > > > - Needs to drmModeGetConnector() on the one connector, no need to probe > > others. Implies that one needs to re-get the connector properties as > > well. =20 >=20 > Sounds good. >=20 > > HOTPLUG=3D1 > > > > - Need to do drmModeGetResouces() to discover new/disappeared > > connectors, and need to drmModeGetConnector to re-probe every > > connector. (As always.) =20 >=20 > 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... > 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. > > That should be also backwards-compatible: any userspace that doesn't > > understand CONNECTOR will see HOTPLUG=3D1 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. =20 >=20 > Agreed, that should work. Cool. The epoch exception you worded seems to fit backward-compatible as well. >=20 > > 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. =20 >=20 > 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. Thanks, pq --Sig_/yrwTOwUg7dkdhYf/CxHS1AV Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEJQjwWQChkWOYOIONI1/ltBGqqqcFAlzbwbwACgkQI1/ltBGq qqeFOg/7BAClCiJltqjfT+E55wAk890KhTN8nz2/oNf/1XyXoQVISuwcvV559ReQ YurjtWZX9k00ZuctWPBEHCpqUG8D4fm84wRB+zO89tyVspCqjf5Zg4cvcdkqenjF A4gOvXJeZtk1vO1lYpkeHXkzizwOhb7VLZqiuOu6fpNazu/vx8103i3fvnAjV0yY q74J1axXenIZR4VaH7O3sEkWQFdxYwE1EEJjWTmfaUFqdk64fHlh9hRCwl2YA5po usb5YrEiDV3o3tcwaA2rycaplaL2KR/8uJRV6+chNvdKDANq+fFyDdxvwY9Eu1Xa uXS8KgH9OYIILKZfgF5Z5HcewmZBaIKFyJPF7lpkcCf1wfHIfo3nuvyhx58Wd4KJ NGWWqjjqupn92wrUzpR9HtQ3g2Bo8ftbNQ8sX92b6O8vRuso3YhfK9O791xtoesi RmGOjxmJcVUV3+XtJr5AFDS2KSwqb9COEglIBvEn9h1EB2+w57FaYEitv1GtVx2V tDu1pAdZJQbI3tR8NLOi8SIqaYBK8WExKlv+kD4nVX2H30Vmvcpv7deq4GxHiI8s nRkZxXlGSf8v/5OvpeHGOGunZmBAX3r4d2mJenyyemc8ffMM/5CHdyFe49xfYWRe zQoJh+8T2Vesw7pvhf2C1bisfuD4N/FJ5ilwNyFJ+/TgpxE9zsA= =hWOr -----END PGP SIGNATURE----- --Sig_/yrwTOwUg7dkdhYf/CxHS1AV-- --===============0134721523== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs --===============0134721523==--