On Fri, Apr 17, 2020 at 1:55 PM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> 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. > > > 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. > > I suppose rf-kill is a bit similar. > > You'd have a userspace controlled property to state what the userspace > wants, and a kernel controlled property to show the hardware > state. Userspace can ask for one or the other, and usually this happens, > but the hardware hotkey bypasses the whole thing. > > It's not perfect. But I think just one property changed nilly-willy by > both the kernel and userspace (especially when it's really not in the > kernel's full control either) is going to be a PITA. Yeah that's what we've done in other cases where both kernel and userspace can change stuff. These where just tri-states, but this here sounds like we need all for, so best to just have 2 properties. -Daniel > > BR, > Jani. > > > > > > >> > >> Regards, > >> > >> Hans > >> > >> > >> *) Some udev event I guess, I sorta assume there already is a > >> notification mechanism for property change notifications ? > > > > Yes, such mechanism has been discussed before, see the thread containing > > https://lists.freedesktop.org/archives/dri-devel/2019-May/217588.html > > > > TL;DR: the mechanism exists and is called the hotplug event. But the > > problem with the hotplug event is that it carries no information about > > what changed, so userspace is forced re-discover everything about the > > DRM device. That thread discusses extending the hotplug event to be > > able to signal changes in individual properties rather than "something > > maybe changed, you figure out what". > > > > > > Thanks, > > pq > > -- > Jani Nikula, Intel Open Source Graphics Center -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
