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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5138 bytes --]

On Thu, 23 Apr 2020 11:21:47 -0700
Rajat Jain <rajatja@google.com> wrote:

> On Tue, Apr 21, 2020 at 7:46 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Tue, 21 Apr 2020 14:37:41 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >  
> > > 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.  
> 
> Just to make sure I understand the semantics correctly:
> 
> - The  "privacy-screen-hw-state" shall be read-only, and can be modified by:
>       - Hardware (e.g. HW kill switch).
>       - Firmware.
>       - (Potentially) needs a notification/irq to the kernel when this
> changes (or may be kernel can read it only when userspace queries for
> it).
> 
> - The "privacy-screen-sw-state" shall be read-write, and can only be
> modified by user space.
>         - If user space toggles it, the kernel will attempt to
> "request" the change to hardware.
>         - Whether the request to hardware was successful or not, the
> "privacy-screen-sw-state" will always reflect the latest value
> userspace wrote.
>         - If the request to hardware was successful, the
> "privacy-screen-hw-state" will also change (probably via a separate
> notification/irq from HW).
>         - We expect the user space to write to
> "privacy-screen-sw-state" only if it really wants to toggle the value.

Hi,

yes, to my understanding, that seems to be the correct idea from this
thread. The hw-state property must reflect the actual hardware state at
all times.

However, when userspace sets "privacy-screen-sw-state", the driver
should attempt to change hardware state regardless of whether the
"privacy-screen-sw-state" value changes compared to its old value or
not. Otherwise userspace cannot intentionally override a hardware
hotkey setting if possible (or would need two atomic commits to do it).

Mind, the above paragraph is only what I interpreted from this email
thread here. Previously I did not think that with atomic KMS, setting a
property to a value it already has could trigger anything. But I guess
it can?

This design is based on that it can.

> What is not clear to me is if any change to"privacy-screen-hw-state"
> shall be propagated to "privacy-screen-sw-state"?
>  - If yes, then I think we are not solving any problems of single property.
>  - If no, then why do we require userspace to write to sw state only
> if something has changed?

No. As already written, the kernel must not change the value of
"privacy-screen-sw-state", only userspace can.

Let's assume that you have a firmware-implemented hardware hotkey for
toggling the shield. The driver also successfully implements
"privacy-screen-sw-state" meaning that writing to it will set the
hardware shield state. If userspace was writing
"privacy-screen-sw-state" even when it does not intend to change
hardware state, it would almost immediately override any state set by
the hardware hotkey, making the hardware hotkey (randomly) not work.

This assumes that the hardware hotkey is a momentary switch that does
not stop software from controlling the shield too.

If the hardware hotkey can stop software from changing the shield
state, then it might not be necessary for userspace to avoid unneeded
setting of the property. But that depends on which way the hotkey works
and which way users want to use it, so it's still best for userspace to
not set the property unless it really intends to apply a change.

If possible, it would be good to make this case the prime example of
how to correctly implement KMS properties for a hardware feature that
can be controlled (and fought over) by both userspace and
hardware/firmware. It seems like the same design can also work with
hardware switches that force the hardware state to be one or the other,
stopping userspace from changing it. Therefore I'd avoid incorporating
any specific shield use cases in the design, e.g. "if hw switch is set
to shield-on, userspace cannot turn shield off".


Thanks,
pq


> 
> Also, it seems to me that in my current patchset, the property I have
> already behaves like "privacy-screen-sw-state". Do I just need to
> rename it?
> 
> Thanks,
> 
> Rajat

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel