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

[Rajat Jain <rajatja@google.com>]

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.

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?

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

>
> Hi,
>
> this sounds like a good plan to me, assuming the kernel writes only to
> the ro property and never to the r/w property.
>
> I understand that as long as firmware hotkeys will toggle actual state,
> there is no design that could work reliably if userspace will always
> commit all KMS state even when it is not necessary. But not committing
> KMS state unless it is actually necessary is really For implementing the "privacy-screen-sw-state".a new requirement on
> userspace, so that needs to be documented before it's too late.
>
> It's not enough to document "don't set it unless you want to
> overwrite/change it" for privacy screen properties. It needs to be
> documented as a general rule that userspace must follow with *unknown*
> properties as well. "Do not restore unrecognized properties unless the
> kernel KMS state might be incorrect compared to what you used to have."
>
> This means that with a display server that does not understand privacy
> screen properties, the end user will lose the privacy screen state on
> every VT-switch back to the display server.
>
> However, if we had a way to query the kernel for the default state to
> reset unknown properties to, the kernel implementation could return the
> current value of the privacy screen property instead of "off" to not
> lose the firmware state. Assuming firmware hotkeys exist, but if they
> don't then return just "off". The point is that the kernel who knows
> all the properties makes the decision what a sane reset value is.
> Userspace can always override the reset value for the properties
> it recognizes.
>
>
> Thanks,For implementing the "privacy-screen-sw-state".
> pq
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel