On Tue, Apr 21, 2020 at 02:37:41PM +0200, Hans de Goede wrote: > Hi, > > On 4/17/20 1:55 PM, Jani Nikula wrote: > > On Fri, 17 Apr 2020, Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote: > > > On Wed, 15 Apr 2020 17:40:46 +0200 > > > Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > > > > > > Hi, > > > > > > > > On 4/15/20 5:28 PM, Jani Nikula wrote: > > > > > On Wed, 15 Apr 2020, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > > > > > ii. Currently the "privacy-screen" property added by Rajat's > > > > > > patch-set is an enum with 2 possible values: > > > > > > "Enabled" > > > > > > "Disabled" > > > > > > > > > > > > We could add a third value "Not Available", which would be the > > > > > > default and then for internal panels always add the property > > > > > > so that we avoid the problem that detecting if the laptop has > > > > > > an internal privacy screen needs to be done before the connector > > > > > > is registered. Then we can add some hooks which allow an > > > > > > lcdshadow-driver to register itself against a connector later > > > > > > (which is non trivial wrt probe order, but lets ignore that for now). > > > > > > > > > > I regret dropping the ball on Rajat's series (sorry!). > > > > > > > > > > I do think having the connector property for this is the way to go. > > > > > > > > I 100% agree. > > > > > > > > > Even > > > > > if we couldn't necessarily figure out all the details on the kernel > > > > > internal connections, can we settle on the property though, so we could > > > > > move forward with Rajat's series? > > > > > > > > Yes please, this will also allow us to move forward with userspace > > > > support even if for testing that we do some hacks for the kernel's > > > > internal connections for now. > > > > > > > > > Moreover, do we actually need two properties, one which could indicate > > > > > userspace's desire for the property, and another that tells the hardware > > > > > state? > > > > > > > > No I do not think so. I would expect there to just be one property, > > > > I guess that if the state is (partly) firmware controlled then there > > > > might be a race, but we will need a notification mechanism (*) for > > > > firmware triggered state changes anyways, so shortly after loosing > > > > the race userspace will process the notification and it will know > > > > about it. > > > > > > > > One thing which might be useful is a way to signal that the property > > > > is read-only in case we ever hit hw where that is the case. > > > > > > > > > I'd so very much like to have no in-kernel/in-firmware shortcuts > > > > > to enable/disable the privacy screen, and instead have any hardware > > > > > buttons just be events that the userspace could react to. However I > > > > > don't think that'll be the case unfortunately. > > > > > > > > In my experience with keyboard-backlight support, we will (unfortunately) > > > > see a mix and in some case we will get a notification that the firmware > > > > has adjusted the state, rather then just getting a keypress and > > > > dealing with that ourselves. In some cases we may even be able to > > > > choose, so the fw will deal with it by default but we can ask it > > > > to just send a key-press. But I do believe that we can *not* expect > > > > that we will always just get a keypress for userspace to deal with. > > > > > > Hi, > > > > > > let's think about how userspace uses atomic KMS UAPI. The simplest way > > > to use atomic correctly is that userspace will for every update send the > > > full, complete set of all properties that exist, both known and unknown > > > to userspace (to recover from temporarily VT-switching to another KMS > > > program that changes unknown properties). Attempting to track which > > > properties already have their correct values in the kernel is extra > > > work for just extra bugs. > > > > > > Assuming the property is userspace-writable: if kernel goes and > > > changes the property value on its own, it will very likely be just > > > overwritten by userspace right after if userspace does not manage to > > > process the uevent first. If that happens and userspace later > > > processes the uevent, userspace queries the kernel for the current > > > proprerty state which is now what userspace wrote, not what firmware > > > set. > > > > > > Therefore you end up with the firmware hotkey working only randomly. > > > > > > It would be much better to have the hotkey events delivered to > > > userspace so that userspace can control the privacy screen and > > > everything will be reliable, both the hotkeys and any GUI for it. > > > > I'd like this too. However I fear this is out of our control, and OEMs > > have and will anyway fiddle with the privacy screen directly no matter > > what we say, and we can't prevent that. From their POV it's easier for > > them to do their value-add in components they have total control over. I > > emphatize with that view, even if it's counter-productive from the Linux > > ecosystem POV. > > > > So we'll just have to deal with it. > > Ack, at least that is the case for the current generation Lenovo devices. > > > > The other reliable option is that userspace must never be able to > > > change privacy screen state, only the hardware hotkeys can. > > > > That, in turn, discourages anyone from doing the right thing, and blocks > > us from adding any nice additional features for privacy screens that > > only the userspace is capable of managing. For example, controlling > > privacy screen based on content, which seems like an obvious feature. > > Right. > > So we have the case here were both the firmware and userspace may change > the privacyscreen state (on/off) at any time. > > This means that the atomic API use described by Pekka for this, where > userspace keeps all properties in memory, updates the one which it > wants to change and then commits simply cannot work here. > > Userspace should only include this property in the transaction if > it actually wants to change it. Or it should do a read of it > and update its internal state before committing that state unless > it wants to change it. But always blindly committing the last value > used by userspace will simply not work, because then there is no > way for the kernel to know if userspace is just repeating itself > or actually wants to change the property. > > As for the one vs two properties. For the current hw userspace can > always change the state at any time. But I can envision devices > where there is a hardware override forcing the privacy screen to > be on. So I guess that a r/w "software state" + a ro "hardware state" > property would make sense. Note that with the current gen. Lenovo > devices the hw-state will simply be a mirror of the sw-state and > the sw-state will be toggled by the firmware independently of > the last value requested by userspace. > > I guess we could add a third ro firmware-state property, which > would reflect the last firmware requested value, but I cannot > really come up with clear semantics for this; nor can I come > up with how this actually helps. > > TL;DR: Yes there will be races, because of both userspace + > the firmware having; and potentially using r/w access to > the privacy-screen state. But in practice I expect these > to not really be an issue. Important here is that userspace > only commits the property in a transaction to commit if > it actually intends to change the property so as to not > needlessly create a situation where we might hit the race. > > As for 1 vs 2 properties for this I guess that in preparation > for potential devices where the state is locked, having a > r/w sw-state + a ro hw-state property makes sense. > > So I suggest that we replace the current "privacy-screen" property > from Rajat's patch-set with 2 props named: > > "privacy-screen-sw-state" (r/w) > "privacy-screen-hw-state" (ro) > > Where for current gen hardware the privacy-screen-hw-state is > just a mirror of the sw-state. Yup. I think internally we should have some kind of explicit update function perhaps, that's at least how immutable (by userspace) properties work. Or we'll change that to a callback, so that there's no inversion going on. -Daniel > > Regards, > > Hans > > > > > > > -- 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
