RFC: Drm-connector properties managed by another driver / privacy screen support

Tue Apr 21 12:40:35 UTC 2020

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 at gmail.com> wrote:
> > > On Wed, 15 Apr 2020 17:40:46 +0200
> > > Hans de Goede <hdegoede at redhat.com> wrote:
> > > 
> > > > Hi,
> > > > 
> > > > On 4/15/20 5:28 PM, Jani Nikula wrote:
> > > > > On Wed, 15 Apr 2020, Hans de Goede <hdegoede at redhat.com> 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