New sysfs interface for privacy screens

New sysfs interface for privacy screens

[Mat King]

Resending in plain text mode

I have been looking into adding Linux support for electronic privacy
screens which is a feature on some new laptops which is built into the
display and allows users to turn it on instead of needing to use a
physical privacy filter. In discussions with my colleagues the idea of
using either /sys/class/backlight or /sys/class/leds but this new
feature does not seem to quite fit into either of those classes.

I am proposing adding a class called "privacy_screen" to interface
with these devices. The initial API would be simple just a single
property called "privacy_state" which when set to 1 would mean that
privacy is enabled and 0 when privacy is disabled.

Current known use cases will use ACPI _DSM in order to interface with
the privacy screens, but this class would allow device driver authors
to use other interfaces as well.

Example:

# get privacy screen state
cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
enabled 0: privacy disabled

# set privacy enabled
echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state

 Does this approach seem to be reasonable?

Re: New sysfs interface for privacy screens

[Greg KH]

On Tue, Oct 01, 2019 at 10:09:46AM -0600, Mat King wrote:
> Resending in plain text mode
> 
> I have been looking into adding Linux support for electronic privacy
> screens which is a feature on some new laptops which is built into the
> display and allows users to turn it on instead of needing to use a
> physical privacy filter. In discussions with my colleagues the idea of
> using either /sys/class/backlight or /sys/class/leds but this new
> feature does not seem to quite fit into either of those classes.
> 
> I am proposing adding a class called "privacy_screen" to interface
> with these devices. The initial API would be simple just a single
> property called "privacy_state" which when set to 1 would mean that
> privacy is enabled and 0 when privacy is disabled.
> 
> Current known use cases will use ACPI _DSM in order to interface with
> the privacy screens, but this class would allow device driver authors
> to use other interfaces as well.
> 
> Example:
> 
> # get privacy screen state
> cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
> enabled 0: privacy disabled
> 
> # set privacy enabled
> echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state

What is "cros_privacy" here?

>  Does this approach seem to be reasonable?

Seems sane to me, do you have any code that implements this so we can
see it?

thanks,

greg k-h

Re: New sysfs interface for privacy screens

[Mat King]

On Tue, Oct 1, 2019 at 10:27 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Oct 01, 2019 at 10:09:46AM -0600, Mat King wrote:
> > Resending in plain text mode
> >
> > I have been looking into adding Linux support for electronic privacy
> > screens which is a feature on some new laptops which is built into the
> > display and allows users to turn it on instead of needing to use a
> > physical privacy filter. In discussions with my colleagues the idea of
> > using either /sys/class/backlight or /sys/class/leds but this new
> > feature does not seem to quite fit into either of those classes.
> >
> > I am proposing adding a class called "privacy_screen" to interface
> > with these devices. The initial API would be simple just a single
> > property called "privacy_state" which when set to 1 would mean that
> > privacy is enabled and 0 when privacy is disabled.
> >
> > Current known use cases will use ACPI _DSM in order to interface with
> > the privacy screens, but this class would allow device driver authors
> > to use other interfaces as well.
> >
> > Example:
> >
> > # get privacy screen state
> > cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
> > enabled 0: privacy disabled
> >
> > # set privacy enabled
> > echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state
>
> What is "cros_privacy" here?

This would be set by the device driver. This example would be for a
Chrome OS privacy screen device, but the driver would set the name
just like in the backlight class.

>
> >  Does this approach seem to be reasonable?
>
> Seems sane to me, do you have any code that implements this so we can
> see it?

It is still early in the implementation so there is no code quite yet.
I wanted to get some general feedback on the approach first. As soon
as I have code to share I will post it.

>
> thanks,
>
> greg k-h

Thank you for the feedback.

Re: New sysfs interface for privacy screens

[Jani Nikula]

On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
> Resending in plain text mode
>
> I have been looking into adding Linux support for electronic privacy
> screens which is a feature on some new laptops which is built into the
> display and allows users to turn it on instead of needing to use a
> physical privacy filter. In discussions with my colleagues the idea of
> using either /sys/class/backlight or /sys/class/leds but this new
> feature does not seem to quite fit into either of those classes.
>
> I am proposing adding a class called "privacy_screen" to interface
> with these devices. The initial API would be simple just a single
> property called "privacy_state" which when set to 1 would mean that
> privacy is enabled and 0 when privacy is disabled.
>
> Current known use cases will use ACPI _DSM in order to interface with
> the privacy screens, but this class would allow device driver authors
> to use other interfaces as well.
>
> Example:
>
> # get privacy screen state
> cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
> enabled 0: privacy disabled
>
> # set privacy enabled
> echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state
>
>  Does this approach seem to be reasonable?

What part of the userspace would be managing the privacy screen? Should
there be a connection between the display and the privacy screen that
covers the display? How would the userspace make that connection if it's
a sysfs interface?

I don't know how the privacy screen operates, but if it draws any power,
you'll want to disable it when you switch off the display it covers.

If the privacy screen control was part of the graphics subsystem (say, a
DRM connector property, which feels somewhat natural), I think it would
make it easier for userspace to have policies such as enabling the
privacy screen automatically depending on the content you're viewing,
but only if the content is on the display that has a privacy screen.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: New sysfs interface for privacy screens

[Daniel Thompson]

On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
> > Resending in plain text mode
> >
> > I have been looking into adding Linux support for electronic privacy
> > screens which is a feature on some new laptops which is built into the
> > display and allows users to turn it on instead of needing to use a
> > physical privacy filter. In discussions with my colleagues the idea of
> > using either /sys/class/backlight or /sys/class/leds but this new
> > feature does not seem to quite fit into either of those classes.
> >
> > I am proposing adding a class called "privacy_screen" to interface
> > with these devices. The initial API would be simple just a single
> > property called "privacy_state" which when set to 1 would mean that
> > privacy is enabled and 0 when privacy is disabled.
> >
> > Current known use cases will use ACPI _DSM in order to interface with
> > the privacy screens, but this class would allow device driver authors
> > to use other interfaces as well.
> >
> > Example:
> >
> > # get privacy screen state
> > cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
> > enabled 0: privacy disabled
> >
> > # set privacy enabled
> > echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state
> >
> >  Does this approach seem to be reasonable?
> 
> What part of the userspace would be managing the privacy screen? Should
> there be a connection between the display and the privacy screen that
> covers the display? How would the userspace make that connection if it's
> a sysfs interface?
> 
> I don't know how the privacy screen operates, but if it draws any power,
> you'll want to disable it when you switch off the display it covers.
> 
> If the privacy screen control was part of the graphics subsystem (say, a
> DRM connector property, which feels somewhat natural), I think it would
> make it easier for userspace to have policies such as enabling the
> privacy screen automatically depending on the content you're viewing,
> but only if the content is on the display that has a privacy screen.

Connectors versus sysfs came up on a backlight thread recently.

Daniel Vetter wrote an excellent summary on why it has been (and still
is) difficult to migrate backlight controls towards the DRM connector
interface:
https://lkml.org/lkml/2019/8/20/752

Many of the backlight legacy problems do not apply to privacy screens
but I do suggest reading this post and some of the neighbouring parts
of the thread. In particular the ACPI driver versus real driver issues
Daniel mentioned could occur again. Hopefully not though, I mean how
wrong can a 1-bit control go? (actually no... don't answer that).

It would definitely be a shame to build up an unnecessary sysfs legacy
for privacy screens so definitely worth seeing if this can use DRM
connector properties.


Daniel.

Re: New sysfs interface for privacy screens

[Jani Nikula]

On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
>> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
>> > Resending in plain text mode
>> >
>> > I have been looking into adding Linux support for electronic privacy
>> > screens which is a feature on some new laptops which is built into the
>> > display and allows users to turn it on instead of needing to use a
>> > physical privacy filter. In discussions with my colleagues the idea of
>> > using either /sys/class/backlight or /sys/class/leds but this new
>> > feature does not seem to quite fit into either of those classes.
>> >
>> > I am proposing adding a class called "privacy_screen" to interface
>> > with these devices. The initial API would be simple just a single
>> > property called "privacy_state" which when set to 1 would mean that
>> > privacy is enabled and 0 when privacy is disabled.
>> >
>> > Current known use cases will use ACPI _DSM in order to interface with
>> > the privacy screens, but this class would allow device driver authors
>> > to use other interfaces as well.
>> >
>> > Example:
>> >
>> > # get privacy screen state
>> > cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
>> > enabled 0: privacy disabled
>> >
>> > # set privacy enabled
>> > echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state
>> >
>> >  Does this approach seem to be reasonable?
>> 
>> What part of the userspace would be managing the privacy screen? Should
>> there be a connection between the display and the privacy screen that
>> covers the display? How would the userspace make that connection if it's
>> a sysfs interface?
>> 
>> I don't know how the privacy screen operates, but if it draws any power,
>> you'll want to disable it when you switch off the display it covers.
>> 
>> If the privacy screen control was part of the graphics subsystem (say, a
>> DRM connector property, which feels somewhat natural), I think it would
>> make it easier for userspace to have policies such as enabling the
>> privacy screen automatically depending on the content you're viewing,
>> but only if the content is on the display that has a privacy screen.
>
> Connectors versus sysfs came up on a backlight thread recently.
>
> Daniel Vetter wrote an excellent summary on why it has been (and still
> is) difficult to migrate backlight controls towards the DRM connector
> interface:
> https://lkml.org/lkml/2019/8/20/752
>
> Many of the backlight legacy problems do not apply to privacy screens
> but I do suggest reading this post and some of the neighbouring parts
> of the thread. In particular the ACPI driver versus real driver issues
> Daniel mentioned could occur again. Hopefully not though, I mean how
> wrong can a 1-bit control go? (actually no... don't answer that).
>
> It would definitely be a shame to build up an unnecessary sysfs legacy
> for privacy screens so definitely worth seeing if this can use DRM
> connector properties.

Indeed. I'm painfully aware of the issues Daniel describes, and that's
part of the motivation for writing this.

Obviously the problem with associating the privacy screen with the DRM
connector is that then the kernel has to make the connection, somehow,
instead of just making it a userspace problem.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: New sysfs interface for privacy screens

[Mat King]

On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
> >> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
> >> > Resending in plain text mode
> >> >
> >> > I have been looking into adding Linux support for electronic privacy
> >> > screens which is a feature on some new laptops which is built into the
> >> > display and allows users to turn it on instead of needing to use a
> >> > physical privacy filter. In discussions with my colleagues the idea of
> >> > using either /sys/class/backlight or /sys/class/leds but this new
> >> > feature does not seem to quite fit into either of those classes.
> >> >
> >> > I am proposing adding a class called "privacy_screen" to interface
> >> > with these devices. The initial API would be simple just a single
> >> > property called "privacy_state" which when set to 1 would mean that
> >> > privacy is enabled and 0 when privacy is disabled.
> >> >
> >> > Current known use cases will use ACPI _DSM in order to interface with
> >> > the privacy screens, but this class would allow device driver authors
> >> > to use other interfaces as well.
> >> >
> >> > Example:
> >> >
> >> > # get privacy screen state
> >> > cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
> >> > enabled 0: privacy disabled
> >> >
> >> > # set privacy enabled
> >> > echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state
> >> >
> >> >  Does this approach seem to be reasonable?
> >>
> >> What part of the userspace would be managing the privacy screen? Should
> >> there be a connection between the display and the privacy screen that
> >> covers the display? How would the userspace make that connection if it's
> >> a sysfs interface?
> >>
> >> I don't know how the privacy screen operates, but if it draws any power,
> >> you'll want to disable it when you switch off the display it covers.
> >>
> >> If the privacy screen control was part of the graphics subsystem (say, a
> >> DRM connector property, which feels somewhat natural), I think it would
> >> make it easier for userspace to have policies such as enabling the
> >> privacy screen automatically depending on the content you're viewing,
> >> but only if the content is on the display that has a privacy screen.
> >
> > Connectors versus sysfs came up on a backlight thread recently.
> >
> > Daniel Vetter wrote an excellent summary on why it has been (and still
> > is) difficult to migrate backlight controls towards the DRM connector
> > interface:
> > https://lkml.org/lkml/2019/8/20/752
> >
> > Many of the backlight legacy problems do not apply to privacy screens
> > but I do suggest reading this post and some of the neighbouring parts
> > of the thread. In particular the ACPI driver versus real driver issues
> > Daniel mentioned could occur again. Hopefully not though, I mean how
> > wrong can a 1-bit control go? (actually no... don't answer that).
> >
> > It would definitely be a shame to build up an unnecessary sysfs legacy
> > for privacy screens so definitely worth seeing if this can use DRM
> > connector properties.
>
> Indeed. I'm painfully aware of the issues Daniel describes, and that's
> part of the motivation for writing this.
>
> Obviously the problem with associating the privacy screen with the DRM
> connector is that then the kernel has to make the connection, somehow,
> instead of just making it a userspace problem.
>
> BR,
> Jani.
>
> --
> Jani Nikula, Intel Open Source Graphics Center

I am not familiar with the DRM connector interface and I don't quite
understand how it would work in this case. How would the connector
provide control to userspace? Is there documentation or example code
somewhere that you could point me to?

Thanks,

Mat King

Re: New sysfs interface for privacy screens

[Jonathan Corbet]

On Tue, 1 Oct 2019 10:09:46 -0600
Mat King <mathewk@google.com> wrote:

> I have been looking into adding Linux support for electronic privacy
> screens which is a feature on some new laptops which is built into the
> display and allows users to turn it on instead of needing to use a
> physical privacy filter. In discussions with my colleagues the idea of
> using either /sys/class/backlight or /sys/class/leds but this new
> feature does not seem to quite fit into either of those classes.

FWIW, it seems that you're not alone in this; 5.4 got some support for
such screens if I understand things correctly:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=110ea1d833ad

jon

Re: New sysfs interface for privacy screens

[Mat King]

On Wed, Oct 2, 2019 at 9:46 AM Jonathan Corbet <corbet@lwn.net> wrote:
>
> On Tue, 1 Oct 2019 10:09:46 -0600
> Mat King <mathewk@google.com> wrote:
>
> > I have been looking into adding Linux support for electronic privacy
> > screens which is a feature on some new laptops which is built into the
> > display and allows users to turn it on instead of needing to use a
> > physical privacy filter. In discussions with my colleagues the idea of
> > using either /sys/class/backlight or /sys/class/leds but this new
> > feature does not seem to quite fit into either of those classes.
>
> FWIW, it seems that you're not alone in this; 5.4 got some support for
> such screens if I understand things correctly:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=110ea1d833ad
>
> jon

Thanks Jon, I had seen that as well and I should have mentioned it in
my original post. That patch exposes the privacy screen using
/proc/acpi which does not seem ideal when adding more privacy screen
support into the kernel.

Re: New sysfs interface for privacy screens

[Jani Nikula]

On Wed, 02 Oct 2019, Jonathan Corbet <corbet@lwn.net> wrote:
> On Tue, 1 Oct 2019 10:09:46 -0600
> Mat King <mathewk@google.com> wrote:
>
>> I have been looking into adding Linux support for electronic privacy
>> screens which is a feature on some new laptops which is built into the
>> display and allows users to turn it on instead of needing to use a
>> physical privacy filter. In discussions with my colleagues the idea of
>> using either /sys/class/backlight or /sys/class/leds but this new
>> feature does not seem to quite fit into either of those classes.
>
> FWIW, it seems that you're not alone in this; 5.4 got some support for
> such screens if I understand things correctly:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=110ea1d833ad

Oh, I didn't realize it got merged already, I thought this was
related...

So we've already replicated the backlight sysfs interface problem for
privacy screens. :(

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: New sysfs interface for privacy screens

[Jani Nikula]

On Wed, 02 Oct 2019, Mat King <mathewk@google.com> wrote:
> On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
>> >> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
>> >> > Resending in plain text mode
>> >> >
>> >> > I have been looking into adding Linux support for electronic privacy
>> >> > screens which is a feature on some new laptops which is built into the
>> >> > display and allows users to turn it on instead of needing to use a
>> >> > physical privacy filter. In discussions with my colleagues the idea of
>> >> > using either /sys/class/backlight or /sys/class/leds but this new
>> >> > feature does not seem to quite fit into either of those classes.
>> >> >
>> >> > I am proposing adding a class called "privacy_screen" to interface
>> >> > with these devices. The initial API would be simple just a single
>> >> > property called "privacy_state" which when set to 1 would mean that
>> >> > privacy is enabled and 0 when privacy is disabled.
>> >> >
>> >> > Current known use cases will use ACPI _DSM in order to interface with
>> >> > the privacy screens, but this class would allow device driver authors
>> >> > to use other interfaces as well.
>> >> >
>> >> > Example:
>> >> >
>> >> > # get privacy screen state
>> >> > cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
>> >> > enabled 0: privacy disabled
>> >> >
>> >> > # set privacy enabled
>> >> > echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state
>> >> >
>> >> >  Does this approach seem to be reasonable?
>> >>
>> >> What part of the userspace would be managing the privacy screen? Should
>> >> there be a connection between the display and the privacy screen that
>> >> covers the display? How would the userspace make that connection if it's
>> >> a sysfs interface?
>> >>
>> >> I don't know how the privacy screen operates, but if it draws any power,
>> >> you'll want to disable it when you switch off the display it covers.
>> >>
>> >> If the privacy screen control was part of the graphics subsystem (say, a
>> >> DRM connector property, which feels somewhat natural), I think it would
>> >> make it easier for userspace to have policies such as enabling the
>> >> privacy screen automatically depending on the content you're viewing,
>> >> but only if the content is on the display that has a privacy screen.
>> >
>> > Connectors versus sysfs came up on a backlight thread recently.
>> >
>> > Daniel Vetter wrote an excellent summary on why it has been (and still
>> > is) difficult to migrate backlight controls towards the DRM connector
>> > interface:
>> > https://lkml.org/lkml/2019/8/20/752
>> >
>> > Many of the backlight legacy problems do not apply to privacy screens
>> > but I do suggest reading this post and some of the neighbouring parts
>> > of the thread. In particular the ACPI driver versus real driver issues
>> > Daniel mentioned could occur again. Hopefully not though, I mean how
>> > wrong can a 1-bit control go? (actually no... don't answer that).
>> >
>> > It would definitely be a shame to build up an unnecessary sysfs legacy
>> > for privacy screens so definitely worth seeing if this can use DRM
>> > connector properties.
>>
>> Indeed. I'm painfully aware of the issues Daniel describes, and that's
>> part of the motivation for writing this.
>>
>> Obviously the problem with associating the privacy screen with the DRM
>> connector is that then the kernel has to make the connection, somehow,
>> instead of just making it a userspace problem.
>>
>> BR,
>> Jani.
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
>
> I am not familiar with the DRM connector interface and I don't quite
> understand how it would work in this case. How would the connector
> provide control to userspace? Is there documentation or example code
> somewhere that you could point me to?

Here are some links, from the general to more specific. Don't get
overwhelmed. ;)

https://www.kernel.org/doc/html/latest/gpu/index.html
https://www.kernel.org/doc/html/latest/gpu/drm-kms.html
https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#kms-properties

The kms userspace tests have some example code. Likely pretty far from
what a nice userspace would actually look like, but you get the idea.

https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/tests/kms_properties.c

Finally, the larger point all along in exposing this via connector
properties is that this could be integrated to some graphics userspace
for a nice user experience, instead of scattering a bunch of userspace
APIs for the same feature across the kernel, and then desperately trying
to gather them to a coherent experience in userspace.

In fact, to that end we have rather more strict requirements for
userspace APIs in drm than perhaps the rest of the kernel:

https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements

Just shoving this into sysfs or procfs to get the kernel part done is
technical debt that ultimately has to be paid by userspace. The
backlight sysfs interface is ancient, and we didn't know better. We
don't have that excuse anymore.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: New sysfs interface for privacy screens

[Daniel Thompson]

On Thu, Oct 03, 2019 at 11:19:13AM +0300, Jani Nikula wrote:
> On Wed, 02 Oct 2019, Jonathan Corbet <corbet@lwn.net> wrote:
> > On Tue, 1 Oct 2019 10:09:46 -0600
> > Mat King <mathewk@google.com> wrote:
> >
> >> I have been looking into adding Linux support for electronic privacy
> >> screens which is a feature on some new laptops which is built into the
> >> display and allows users to turn it on instead of needing to use a
> >> physical privacy filter. In discussions with my colleagues the idea of
> >> using either /sys/class/backlight or /sys/class/leds but this new
> >> feature does not seem to quite fit into either of those classes.
> >
> > FWIW, it seems that you're not alone in this; 5.4 got some support for
> > such screens if I understand things correctly:
> >
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=110ea1d833ad
> 
> Oh, I didn't realize it got merged already, I thought this was
> related...
> 
> So we've already replicated the backlight sysfs interface problem for
> privacy screens. :(

I guess... although the Thinkpad code hasn't added any standard
interfaces (no other laptop should be placing controls for a privacy
screen in /proc/acpi/ibm/... ). Maybe its not too late.


Daniel.

Re: New sysfs interface for privacy screens

[Mat King]

On Thu, Oct 3, 2019 at 2:59 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Wed, 02 Oct 2019, Mat King <mathewk@google.com> wrote:
> > On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>
> >> On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> >> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
> >> >> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
> >> >> > Resending in plain text mode
> >> >> >
> >> >> > I have been looking into adding Linux support for electronic privacy
> >> >> > screens which is a feature on some new laptops which is built into the
> >> >> > display and allows users to turn it on instead of needing to use a
> >> >> > physical privacy filter. In discussions with my colleagues the idea of
> >> >> > using either /sys/class/backlight or /sys/class/leds but this new
> >> >> > feature does not seem to quite fit into either of those classes.
> >> >> >
> >> >> > I am proposing adding a class called "privacy_screen" to interface
> >> >> > with these devices. The initial API would be simple just a single
> >> >> > property called "privacy_state" which when set to 1 would mean that
> >> >> > privacy is enabled and 0 when privacy is disabled.
> >> >> >
> >> >> > Current known use cases will use ACPI _DSM in order to interface with
> >> >> > the privacy screens, but this class would allow device driver authors
> >> >> > to use other interfaces as well.
> >> >> >
> >> >> > Example:
> >> >> >
> >> >> > # get privacy screen state
> >> >> > cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
> >> >> > enabled 0: privacy disabled
> >> >> >
> >> >> > # set privacy enabled
> >> >> > echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state
> >> >> >
> >> >> >  Does this approach seem to be reasonable?
> >> >>
> >> >> What part of the userspace would be managing the privacy screen? Should
> >> >> there be a connection between the display and the privacy screen that
> >> >> covers the display? How would the userspace make that connection if it's
> >> >> a sysfs interface?
> >> >>
> >> >> I don't know how the privacy screen operates, but if it draws any power,
> >> >> you'll want to disable it when you switch off the display it covers.
> >> >>
> >> >> If the privacy screen control was part of the graphics subsystem (say, a
> >> >> DRM connector property, which feels somewhat natural), I think it would
> >> >> make it easier for userspace to have policies such as enabling the
> >> >> privacy screen automatically depending on the content you're viewing,
> >> >> but only if the content is on the display that has a privacy screen.
> >> >
> >> > Connectors versus sysfs came up on a backlight thread recently.
> >> >
> >> > Daniel Vetter wrote an excellent summary on why it has been (and still
> >> > is) difficult to migrate backlight controls towards the DRM connector
> >> > interface:
> >> > https://lkml.org/lkml/2019/8/20/752
> >> >
> >> > Many of the backlight legacy problems do not apply to privacy screens
> >> > but I do suggest reading this post and some of the neighbouring parts
> >> > of the thread. In particular the ACPI driver versus real driver issues
> >> > Daniel mentioned could occur again. Hopefully not though, I mean how
> >> > wrong can a 1-bit control go? (actually no... don't answer that).
> >> >
> >> > It would definitely be a shame to build up an unnecessary sysfs legacy
> >> > for privacy screens so definitely worth seeing if this can use DRM
> >> > connector properties.
> >>
> >> Indeed. I'm painfully aware of the issues Daniel describes, and that's
> >> part of the motivation for writing this.
> >>
> >> Obviously the problem with associating the privacy screen with the DRM
> >> connector is that then the kernel has to make the connection, somehow,
> >> instead of just making it a userspace problem.
> >>
> >> BR,
> >> Jani.
> >>
> >> --
> >> Jani Nikula, Intel Open Source Graphics Center
> >
> > I am not familiar with the DRM connector interface and I don't quite
> > understand how it would work in this case. How would the connector
> > provide control to userspace? Is there documentation or example code
> > somewhere that you could point me to?
>
> Here are some links, from the general to more specific. Don't get
> overwhelmed. ;)
>
> https://www.kernel.org/doc/html/latest/gpu/index.html
> https://www.kernel.org/doc/html/latest/gpu/drm-kms.html
> https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#kms-properties
>
> The kms userspace tests have some example code. Likely pretty far from
> what a nice userspace would actually look like, but you get the idea.
>
> https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/tests/kms_properties.c
>
> Finally, the larger point all along in exposing this via connector
> properties is that this could be integrated to some graphics userspace
> for a nice user experience, instead of scattering a bunch of userspace
> APIs for the same feature across the kernel, and then desperately trying
> to gather them to a coherent experience in userspace.
>
> In fact, to that end we have rather more strict requirements for
> userspace APIs in drm than perhaps the rest of the kernel:
>
> https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
>
> Just shoving this into sysfs or procfs to get the kernel part done is
> technical debt that ultimately has to be paid by userspace. The
> backlight sysfs interface is ancient, and we didn't know better. We
> don't have that excuse anymore.
>
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Graphics Center

Thanks Jani, the DRM connector picture is much more clear now after
reading through those.

So my proposal would now be to add a new standard property to
drm_connector called "privacy_screen" this property would be an enum
which can take one of three values.

PRIVACY_UNSUPPORTED - Privacy is not available for this connector
PRIVACY_DISABLED - Privacy is available but turned off
PRIVACY_ENABLED - Privacy is available and turned on

When the connector is initized the privacy screen property is set to
PRIVACY_UNSUPPORTED and cannot be changed unless a drm_privacy_screen
is registered to the connector. drm_privacy_screen will look something
like

struct drm_privacy_screen_ops {
    int (*get_privacy_state)(struct drm_privacy_screen *);
    int (*set_privacy_state)(struct drm_privacy_screen *, int);
}

struct drm_privacy_screen {
    /* The privacy screen device */
    struct device *dev;

    /* The connector that the privacy screen is attached */
    struct drm_connector *connector;

    /* Ops to get and set the privacy screen state */
    struct drm_privacy_screen_ops *ops;

    /* The current state of the privacy screen */
    int state;
}

Privacy screen device drivers will call a function to register the
privacy screen with the connector.

int drm_privacy_screen_register(struct drm_privacy_screen_ops *ops,
struct device *dev, struct drm_connector *);

Calling this will set a new field on the connector "struct
drm_privacy_screen *privacy_screen" and change the value of the
property to ops->get_privacy_state(). When
drm_mode_connector_set_obj_prop() is called with the
privacy_screen_proptery if a privacy_screen is registered to the
connector the ops->set_privacy_state() will be called with the new
value.

Setting of this property (and all drm properties) is done in user
space using ioctrl.

Registering the privacy screen with a connector may be tricky because
the driver for the privacy screen will need to be able to identify
which connector it belongs to and we will have to deal with connectors
being added both before and after the privacy screen device is added
by it's driver.

How does that sound? I will work on a patch if that all sounds about right.

One question I still have is there a way to not accept a value that is
passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
screen is not registered the property must stay PRIVACY_UNSUPPORTED
and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
never be set.

Re: New sysfs interface for privacy screens

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

On Tue 2019-10-01 10:09:46, Mat King wrote:
> Resending in plain text mode
> 
> I have been looking into adding Linux support for electronic privacy
> screens which is a feature on some new laptops which is built into the
> display and allows users to turn it on instead of needing to use a
> physical privacy filter. In discussions with my colleagues the idea of
> using either /sys/class/backlight or /sys/class/leds but this new
> feature does not seem to quite fit into either of those classes.

Thank you for not trying to push it as a LED ;-).

> I am proposing adding a class called "privacy_screen" to interface
> with these devices. The initial API would be simple just a single
> property called "privacy_state" which when set to 1 would mean that
> privacy is enabled and 0 when privacy is disabled.
> 
> Current known use cases will use ACPI _DSM in order to interface with
> the privacy screens, but this class would allow device driver authors
> to use other interfaces as well.
> 
> Example:
> 
> # get privacy screen state
> cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
> enabled 0: privacy disabled
> 
> # set privacy enabled
> echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state
> 
>  Does this approach seem to be reasonable?

Not really. How does the userland know which displays this will
affect?

This sounds like something that should go through drm drivers,
probably to be selected by xrandr, rather than separate file somewhere
in sysfs.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: New sysfs interface for privacy screens

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

On Wed 2019-10-02 09:46:50, Jonathan Corbet wrote:
> On Tue, 1 Oct 2019 10:09:46 -0600
> Mat King <mathewk@google.com> wrote:
> 
> > I have been looking into adding Linux support for electronic privacy
> > screens which is a feature on some new laptops which is built into the
> > display and allows users to turn it on instead of needing to use a
> > physical privacy filter. In discussions with my colleagues the idea of
> > using either /sys/class/backlight or /sys/class/leds but this new
> > feature does not seem to quite fit into either of those classes.
> 
> FWIW, it seems that you're not alone in this; 5.4 got some support for
> such screens if I understand things correctly:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=110ea1d833ad

Hmm. We may want to revert that one because it does more damage :-(.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: New sysfs interface for privacy screens

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

Hi!

> > >> I have been looking into adding Linux support for electronic privacy
> > >> screens which is a feature on some new laptops which is built into the
> > >> display and allows users to turn it on instead of needing to use a
> > >> physical privacy filter. In discussions with my colleagues the idea of
> > >> using either /sys/class/backlight or /sys/class/leds but this new
> > >> feature does not seem to quite fit into either of those classes.
> > >
> > > FWIW, it seems that you're not alone in this; 5.4 got some support for
> > > such screens if I understand things correctly:
> > >
> > >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=110ea1d833ad
> > 
> > Oh, I didn't realize it got merged already, I thought this was
> > related...
> > 
> > So we've already replicated the backlight sysfs interface problem for
> > privacy screens. :(
> 
> I guess... although the Thinkpad code hasn't added any standard
> interfaces (no other laptop should be placing controls for a privacy
> screen in /proc/acpi/ibm/... ). Maybe its not too late.

There's new interface for controlling privacyguard... but perhaps we
need better solution than what went in 5.4. Perhaps it should be
reconsidered?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: New sysfs interface for privacy screens

[Jingoo Han]

> On 10/6/19, 7:04 AM, Pavel Machek wrote:
>
> Hi!
>
> > > >> I have been looking into adding Linux support for electronic privacy
> > > >> screens which is a feature on some new laptops which is built into the
> > > >> display and allows users to turn it on instead of needing to use a
> > > >> physical privacy filter. In discussions with my colleagues the idea of
> > > >> using either /sys/class/backlight or /sys/class/leds but this new
> > > >> feature does not seem to quite fit into either of those classes.
> > > >
> > > > FWIW, it seems that you're not alone in this; 5.4 got some support for
> > > > such screens if I understand things correctly:
> > > >
> > > >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=110ea1d833ad
> > > 
> > > Oh, I didn't realize it got merged already, I thought this was
> > > related...
> > > 
> > > So we've already replicated the backlight sysfs interface problem for
> > > privacy screens. :(
> > 
> > I guess... although the Thinkpad code hasn't added any standard
> > interfaces (no other laptop should be placing controls for a privacy
> > screen in /proc/acpi/ibm/... ). Maybe its not too late.
>
> There's new interface for controlling privacyguard... but perhaps we
> need better solution than what went in 5.4. Perhaps it should be
> reconsidered?

I agree with your opinion. I also think that better solution can be considered,
although there is already something in 5.4.

Best regards,
Jingoo Han

>
> Best regards,
>									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: New sysfs interface for privacy screens

[Henrique de Moraes Holschuh]

On Thu, 03 Oct 2019, Daniel Thompson wrote:
> I guess... although the Thinkpad code hasn't added any standard
> interfaces (no other laptop should be placing controls for a privacy
> screen in /proc/acpi/ibm/... ). Maybe its not too late.

As far as I am concerned, it is *not* too late.  And you can always have
a driver-private way of messing with something, and a more generic way
of doing the same thing.

thinkpad-acpi will always welcome patches switching to the new generic
way (as long as we can have a deprecation period *for long-time-used
facilities* -- which is not the case of the privacy screen, no
deprecation period need there).

The privacy thing is too new, feel free to design a generic one and
send in a patch *switching* thinkpad-acpi to the new generic one.

I would ACK that.  If the subsystem maintainer also agrees, (and nobody
*seriously* complain about it from userspace), the private interface
would be gone just like that.

-- 
  Henrique Holschuh

Re: New sysfs interface for privacy screens

[Rajat Jain]

Hi,

Me and Mat are working on this together, and I had a followup to
something Mat asked earlier.

On Thu, Oct 3, 2019 at 12:57 PM Mat King <mathewk@google.com> wrote:
>
> On Thu, Oct 3, 2019 at 2:59 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > On Wed, 02 Oct 2019, Mat King <mathewk@google.com> wrote:
> > > On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > >>
> > >> On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > >> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
> > >> >> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
> > >> >> > Resending in plain text mode
> > >> >> >
> One question I still have is there a way to not accept a value that is
> passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
> screen is not registered the property must stay PRIVACY_UNSUPPORTED
> and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
> never be set.
> One question I still have is there a way to not accept a value that is
> passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
> screen is not registered the property must stay PRIVACY_UNSUPPORTED
> and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
> never be set.


Any guidance here on this?

Essentially I think we are looking for a way to
1) expose the "capability" of having a privacy screen feature from
kernel to userland, and
2) then a way to "control" (enable / disable) the feature from
userland to kernel. I understand DRM connector property is the
suggested way to do this.

But I was not clear if DRM connector properties a recommended way to
do (1) also. If yes, then Mat's posted a proposal
(https://lkml.org/lkml/2019/10/3/2068) but I did not see any comment
specifically if that idea looked reasonable. Also looking for any
guidance to Mat's question above.

Thanks,

Rajat

>
> > >> >> > I have been looking into adding Linux support for electronic privacy
> > >> >> > screens which is a feature on some new laptops which is built into the
> > >> >> > display and allows users to turn it on instead of needing to use a
> > >> >> > physical privacy filter. In discussions with my colleagues the idea of
> > >> >> > using either /sys/class/backlight or /sys/class/leds but this new
> > >> >> > feature does not seem to quite fit into either of those classes.
> > >> >> >
> > >> >> > I am proposing adding a class called "privacy_screen" to interface
> > >> >> > with these devices. The initial API would be simple just a single
> > >> >> > property called "privacy_state" which when set to 1 would mean that
> > >> >> > privacy is enabled and 0 when privacy is disabled.
> > >> >> >
> > >> >> > Current known use cases will use ACPI _DSM in order to interface with
> > >> >> > the privacy screens, but this class would allow device driver authors
> > >> >> > to use other interfaces as well.
> > >> >> >
> > >> >> > Example:
> > >> >> >
> > >> >> > # get privacy screen state
> > >> >> > cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
> > >> >> > enabled 0: privacy disabled
> > >> >> >
> > >> >> > # set privacy enabled
> > >> >> > echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state
> > >> >> >
> > >> >> >  Does this approach seem to be reasonable?
> > >> >>
> > >> >> What part of the userspace would be managing the privacy screen? Should
> > >> >> there be a connection between the display and the privacy screen that
> > >> >> covers the display? How would the userspace make that connection if it's
> > >> >> a sysfs interface?
> > >> >>
> > >> >> I don't know how the privacy screen operates, but if it draws any power,
> > >> >> you'll want to disable it when you switch off the display it covers.
> > >> >>
> > >> >> If the privacy screen control was part of the graphics subsystem (say, a
> > >> >> DRM connector property, which feels somewhat natural), I think it would
> > >> >> make it easier for userspace to have policies such as enabling the
> > >> >> privacy screen automatically depending on the content you're viewing,
> > >> >> but only if the content is on the display that has a privacy screen.
> > >> >
> > >> > Connectors versus sysfs came up on a backlight thread recently.
> > >> >
> > >> > Daniel Vetter wrote an excellent summary on why it has been (and still
> > >> > is) difficult to migrate backlight controls towards the DRM connector
> > >> > interface:
> > >> > https://lkml.org/lkml/2019/8/20/752
> > >> >
> > >> > Many of the backlight legacy problems do not apply to privacy screens
> > >> > but I do suggest reading this post and some of the neighbouring parts
> > >> > of the thread. In particular the ACPI driver versus real driver issues
> > >> > Daniel mentioned could occur again. Hopefully not though, I mean how
> > >> > wrong can a 1-bit control go? (actually no... don't answer that).
> > >> >
> > >> > It would definitely be a shame to build up an unnecessary sysfs legacy
> > >> > for privacy screens so definitely worth seeing if this can use DRM
> > >> > connector properties.
> > >>
> > >> Indeed. I'm painfully aware of the issues Daniel describes, and that's
> > >> part of the motivation for writing this.
> > >>
> > >> Obviously the problem with associating the privacy screen with the DRM
> > >> connector is that then the kernel has to make the connection, somehow,
> > >> instead of just making it a userspace problem.
> > >>
> > >> BR,
> > >> Jani.
> > >>
> > >> --
> > >> Jani Nikula, Intel Open Source Graphics Center
> > >
> > > I am not familiar with the DRM connector interface and I don't quite
> > > understand how it would work in this case. How would the connector
> > > provide control to userspace? Is there documentation or example code
> > > somewhere that you could point me to?
> >
> > Here are some links, from the general to more specific. Don't get
> > overwhelmed. ;)
> >
> > https://www.kernel.org/doc/html/latest/gpu/index.html
> > https://www.kernel.org/doc/html/latest/gpu/drm-kms.html
> > https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#kms-properties
> >
> > The kms userspace tests have some example code. Likely pretty far from
> > what a nice userspace would actually look like, but you get the idea.
> >
> > https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/tests/kms_properties.c
> >
> > Finally, the larger point all along in exposing this via connector
> > properties is that this could be integrated to some graphics userspace
> > for a nice user experience, instead of scattering a bunch of userspace
> > APIs for the same feature across the kernel, and then desperately trying
> > to gather them to a coherent experience in userspace.
> >
> > In fact, to that end we have rather more strict requirements for
> > userspace APIs in drm than perhaps the rest of the kernel:
> >
> > https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
> >
> > Just shoving this into sysfs or procfs to get the kernel part done is
> > technical debt that ultimately has to be paid by userspace. The
> > backlight sysfs interface is ancient, and we didn't know better. We
> > don't have that excuse anymore.
> >
> >
> > BR,
> > Jani.
> >
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center
>
> Thanks Jani, the DRM connector picture is much more clear now after
> reading through those.
>
> So my proposal would now be to add a new standard property to
> drm_connector called "privacy_screen" this property would be an enum
> which can take one of three values.
>
> PRIVACY_UNSUPPORTED - Privacy is not available for this connector
> PRIVACY_DISABLED - Privacy is available but turned off
> PRIVACY_ENABLED - Privacy is available and turned on
>
> When the connector is initized the privacy screen property is set to
> PRIVACY_UNSUPPORTED and cannot be changed unless a drm_privacy_screen
> is registered to the connector. drm_privacy_screen will look something
> like
>
> struct drm_privacy_screen_ops {
>     int (*get_privacy_state)(struct drm_privacy_screen *);
>     int (*set_privacy_state)(struct drm_privacy_screen *, int);
> }
>
> struct drm_privacy_screen {
>     /* The privacy screen device */
>     struct device *dev;
>
>     /* The connector that the privacy screen is attached */
>     struct drm_connector *connector;
>
>     /* Ops to get and set the privacy screen state */
>     struct drm_privacy_screen_ops *ops;
>
>     /* The current state of the privacy screen */
>     int state;
> }
>
> Privacy screen device drivers will call a function to register the
> privacy screen with the connector.
>
> int drm_privacy_screen_register(struct drm_privacy_screen_ops *ops,
> struct device *dev, struct drm_connector *);
>
> Calling this will set a new field on the connector "struct
> drm_privacy_screen *privacy_screen" and change the value of the
> property to ops->get_privacy_state(). When
> drm_mode_connector_set_obj_prop() is called with the
> privacy_screen_proptery if a privacy_screen is registered to the
> connector the ops->set_privacy_state() will be called with the new
> value.
>
> Setting of this property (and all drm properties) is done in user
> space using ioctrl.
>
> Registering the privacy screen with a connector may be tricky because
> the driver for the privacy screen will need to be able to identify
> which connector it belongs to and we will have to deal with connectors
> being added both before and after the privacy screen device is added
> by it's driver.
>
> How does that sound? I will work on a patch if that all sounds about right.
>
> One question I still have is there a way to not accept a value that is
> passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
> screen is not registered the property must stay PRIVACY_UNSUPPORTED
> and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
> never be set.

Re: New sysfs interface for privacy screens

[Jani Nikula]

On Sun, 06 Oct 2019, Rajat Jain <rajatja@google.com> wrote:
> Hi,
>
> Me and Mat are working on this together, and I had a followup to
> something Mat asked earlier.
>
> On Thu, Oct 3, 2019 at 12:57 PM Mat King <mathewk@google.com> wrote:
>>
>> On Thu, Oct 3, 2019 at 2:59 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> >
>> > On Wed, 02 Oct 2019, Mat King <mathewk@google.com> wrote:
>> > > On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> > >>
>> > >> On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>> > >> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
>> > >> >> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
>> > >> >> > Resending in plain text mode
>> > >> >> >
>> One question I still have is there a way to not accept a value that is
>> passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
>> screen is not registered the property must stay PRIVACY_UNSUPPORTED
>> and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
>> never be set.
>> One question I still have is there a way to not accept a value that is
>> passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
>> screen is not registered the property must stay PRIVACY_UNSUPPORTED
>> and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
>> never be set.
>
>
> Any guidance here on this?
>
> Essentially I think we are looking for a way to
> 1) expose the "capability" of having a privacy screen feature from
> kernel to userland, and
> 2) then a way to "control" (enable / disable) the feature from
> userland to kernel. I understand DRM connector property is the
> suggested way to do this.
>
> But I was not clear if DRM connector properties a recommended way to
> do (1) also. If yes, then Mat's posted a proposal
> (https://lkml.org/lkml/2019/10/3/2068) but I did not see any comment
> specifically if that idea looked reasonable. Also looking for any
> guidance to Mat's question above.

I'd say the capability would be indicated by the existence of the
property. Only add the property if you can control it. Userspace will
have to check this no matter what. Then you use the property to
enable/disable it.

BR,
Jani.




>
> Thanks,
>
> Rajat
>
>>
>> > >> >> > I have been looking into adding Linux support for electronic privacy
>> > >> >> > screens which is a feature on some new laptops which is built into the
>> > >> >> > display and allows users to turn it on instead of needing to use a
>> > >> >> > physical privacy filter. In discussions with my colleagues the idea of
>> > >> >> > using either /sys/class/backlight or /sys/class/leds but this new
>> > >> >> > feature does not seem to quite fit into either of those classes.
>> > >> >> >
>> > >> >> > I am proposing adding a class called "privacy_screen" to interface
>> > >> >> > with these devices. The initial API would be simple just a single
>> > >> >> > property called "privacy_state" which when set to 1 would mean that
>> > >> >> > privacy is enabled and 0 when privacy is disabled.
>> > >> >> >
>> > >> >> > Current known use cases will use ACPI _DSM in order to interface with
>> > >> >> > the privacy screens, but this class would allow device driver authors
>> > >> >> > to use other interfaces as well.
>> > >> >> >
>> > >> >> > Example:
>> > >> >> >
>> > >> >> > # get privacy screen state
>> > >> >> > cat /sys/class/privacy_screen/cros_privacy/privacy_state # 1: privacy
>> > >> >> > enabled 0: privacy disabled
>> > >> >> >
>> > >> >> > # set privacy enabled
>> > >> >> > echo 1 > /sys/class/privacy_screen/cros_privacy/privacy_state
>> > >> >> >
>> > >> >> >  Does this approach seem to be reasonable?
>> > >> >>
>> > >> >> What part of the userspace would be managing the privacy screen? Should
>> > >> >> there be a connection between the display and the privacy screen that
>> > >> >> covers the display? How would the userspace make that connection if it's
>> > >> >> a sysfs interface?
>> > >> >>
>> > >> >> I don't know how the privacy screen operates, but if it draws any power,
>> > >> >> you'll want to disable it when you switch off the display it covers.
>> > >> >>
>> > >> >> If the privacy screen control was part of the graphics subsystem (say, a
>> > >> >> DRM connector property, which feels somewhat natural), I think it would
>> > >> >> make it easier for userspace to have policies such as enabling the
>> > >> >> privacy screen automatically depending on the content you're viewing,
>> > >> >> but only if the content is on the display that has a privacy screen.
>> > >> >
>> > >> > Connectors versus sysfs came up on a backlight thread recently.
>> > >> >
>> > >> > Daniel Vetter wrote an excellent summary on why it has been (and still
>> > >> > is) difficult to migrate backlight controls towards the DRM connector
>> > >> > interface:
>> > >> > https://lkml.org/lkml/2019/8/20/752
>> > >> >
>> > >> > Many of the backlight legacy problems do not apply to privacy screens
>> > >> > but I do suggest reading this post and some of the neighbouring parts
>> > >> > of the thread. In particular the ACPI driver versus real driver issues
>> > >> > Daniel mentioned could occur again. Hopefully not though, I mean how
>> > >> > wrong can a 1-bit control go? (actually no... don't answer that).
>> > >> >
>> > >> > It would definitely be a shame to build up an unnecessary sysfs legacy
>> > >> > for privacy screens so definitely worth seeing if this can use DRM
>> > >> > connector properties.
>> > >>
>> > >> Indeed. I'm painfully aware of the issues Daniel describes, and that's
>> > >> part of the motivation for writing this.
>> > >>
>> > >> Obviously the problem with associating the privacy screen with the DRM
>> > >> connector is that then the kernel has to make the connection, somehow,
>> > >> instead of just making it a userspace problem.
>> > >>
>> > >> BR,
>> > >> Jani.
>> > >>
>> > >> --
>> > >> Jani Nikula, Intel Open Source Graphics Center
>> > >
>> > > I am not familiar with the DRM connector interface and I don't quite
>> > > understand how it would work in this case. How would the connector
>> > > provide control to userspace? Is there documentation or example code
>> > > somewhere that you could point me to?
>> >
>> > Here are some links, from the general to more specific. Don't get
>> > overwhelmed. ;)
>> >
>> > https://www.kernel.org/doc/html/latest/gpu/index.html
>> > https://www.kernel.org/doc/html/latest/gpu/drm-kms.html
>> > https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#kms-properties
>> >
>> > The kms userspace tests have some example code. Likely pretty far from
>> > what a nice userspace would actually look like, but you get the idea.
>> >
>> > https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/tests/kms_properties.c
>> >
>> > Finally, the larger point all along in exposing this via connector
>> > properties is that this could be integrated to some graphics userspace
>> > for a nice user experience, instead of scattering a bunch of userspace
>> > APIs for the same feature across the kernel, and then desperately trying
>> > to gather them to a coherent experience in userspace.
>> >
>> > In fact, to that end we have rather more strict requirements for
>> > userspace APIs in drm than perhaps the rest of the kernel:
>> >
>> > https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
>> >
>> > Just shoving this into sysfs or procfs to get the kernel part done is
>> > technical debt that ultimately has to be paid by userspace. The
>> > backlight sysfs interface is ancient, and we didn't know better. We
>> > don't have that excuse anymore.
>> >
>> >
>> > BR,
>> > Jani.
>> >
>> >
>> > --
>> > Jani Nikula, Intel Open Source Graphics Center
>>
>> Thanks Jani, the DRM connector picture is much more clear now after
>> reading through those.
>>
>> So my proposal would now be to add a new standard property to
>> drm_connector called "privacy_screen" this property would be an enum
>> which can take one of three values.
>>
>> PRIVACY_UNSUPPORTED - Privacy is not available for this connector
>> PRIVACY_DISABLED - Privacy is available but turned off
>> PRIVACY_ENABLED - Privacy is available and turned on
>>
>> When the connector is initized the privacy screen property is set to
>> PRIVACY_UNSUPPORTED and cannot be changed unless a drm_privacy_screen
>> is registered to the connector. drm_privacy_screen will look something
>> like
>>
>> struct drm_privacy_screen_ops {
>>     int (*get_privacy_state)(struct drm_privacy_screen *);
>>     int (*set_privacy_state)(struct drm_privacy_screen *, int);
>> }
>>
>> struct drm_privacy_screen {
>>     /* The privacy screen device */
>>     struct device *dev;
>>
>>     /* The connector that the privacy screen is attached */
>>     struct drm_connector *connector;
>>
>>     /* Ops to get and set the privacy screen state */
>>     struct drm_privacy_screen_ops *ops;
>>
>>     /* The current state of the privacy screen */
>>     int state;
>> }
>>
>> Privacy screen device drivers will call a function to register the
>> privacy screen with the connector.
>>
>> int drm_privacy_screen_register(struct drm_privacy_screen_ops *ops,
>> struct device *dev, struct drm_connector *);
>>
>> Calling this will set a new field on the connector "struct
>> drm_privacy_screen *privacy_screen" and change the value of the
>> property to ops->get_privacy_state(). When
>> drm_mode_connector_set_obj_prop() is called with the
>> privacy_screen_proptery if a privacy_screen is registered to the
>> connector the ops->set_privacy_state() will be called with the new
>> value.
>>
>> Setting of this property (and all drm properties) is done in user
>> space using ioctrl.
>>
>> Registering the privacy screen with a connector may be tricky because
>> the driver for the privacy screen will need to be able to identify
>> which connector it belongs to and we will have to deal with connectors
>> being added both before and after the privacy screen device is added
>> by it's driver.
>>
>> How does that sound? I will work on a patch if that all sounds about right.
>>
>> One question I still have is there a way to not accept a value that is
>> passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
>> screen is not registered the property must stay PRIVACY_UNSUPPORTED
>> and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
>> never be set.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: New sysfs interface for privacy screens

[Sean Paul]

On Thu, Oct 3, 2019 at 3:57 PM Mat King <mathewk@google.com> wrote:
>
> On Thu, Oct 3, 2019 at 2:59 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > On Wed, 02 Oct 2019, Mat King <mathewk@google.com> wrote:
> > > On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > >>
> > >> On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > >> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
> > >> >> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
> > >> >> > Resending in plain text mode

/snip

>
> So my proposal would now be to add a new standard property to
> drm_connector called "privacy_screen" this property would be an enum
> which can take one of three values.
>
> PRIVACY_UNSUPPORTED - Privacy is not available for this connector
> PRIVACY_DISABLED - Privacy is available but turned off
> PRIVACY_ENABLED - Privacy is available and turned on

Agree with Jani, use the property presence to determine if it's supported

>
> When the connector is initized the privacy screen property is set to
> PRIVACY_UNSUPPORTED and cannot be changed unless a drm_privacy_screen
> is registered to the connector. drm_privacy_screen will look something
> like
>
> struct drm_privacy_screen_ops {
>     int (*get_privacy_state)(struct drm_privacy_screen *);
>     int (*set_privacy_state)(struct drm_privacy_screen *, int);
> }
>
> struct drm_privacy_screen {
>     /* The privacy screen device */
>     struct device *dev;
>
>     /* The connector that the privacy screen is attached */
>     struct drm_connector *connector;
>
>     /* Ops to get and set the privacy screen state */
>     struct drm_privacy_screen_ops *ops;
>
>     /* The current state of the privacy screen */
>     int state;
> }
>
> Privacy screen device drivers will call a function to register the
> privacy screen with the connector.

Do we actually need dedicated drivers for privacy screen? It seems
like something that is panel-specific hardware, so I'd suggest just
using the panel driver.

Sean

>
> int drm_privacy_screen_register(struct drm_privacy_screen_ops *ops,
> struct device *dev, struct drm_connector *);
>
> Calling this will set a new field on the connector "struct
> drm_privacy_screen *privacy_screen" and change the value of the
> property to ops->get_privacy_state(). When
> drm_mode_connector_set_obj_prop() is called with the
> privacy_screen_proptery if a privacy_screen is registered to the
> connector the ops->set_privacy_state() will be called with the new
> value.
>
> Setting of this property (and all drm properties) is done in user
> space using ioctrl.
>
> Registering the privacy screen with a connector may be tricky because
> the driver for the privacy screen will need to be able to identify
> which connector it belongs to and we will have to deal with connectors
> being added both before and after the privacy screen device is added
> by it's driver.
>
> How does that sound? I will work on a patch if that all sounds about right.
>
> One question I still have is there a way to not accept a value that is
> passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
> screen is not registered the property must stay PRIVACY_UNSUPPORTED
> and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
> never be set.

Re: New sysfs interface for privacy screens

[Mat King]

On Mon, Oct 7, 2019 at 7:09 AM Sean Paul <seanpaul@chromium.org> wrote:
>
> On Thu, Oct 3, 2019 at 3:57 PM Mat King <mathewk@google.com> wrote:
> >
> > On Thu, Oct 3, 2019 at 2:59 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > >
> > > On Wed, 02 Oct 2019, Mat King <mathewk@google.com> wrote:
> > > > On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > > >>
> > > >> On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > >> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
> > > >> >> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
> > > >> >> > Resending in plain text mode
>
> /snip
>
> >
> > So my proposal would now be to add a new standard property to
> > drm_connector called "privacy_screen" this property would be an enum
> > which can take one of three values.
> >
> > PRIVACY_UNSUPPORTED - Privacy is not available for this connector
> > PRIVACY_DISABLED - Privacy is available but turned off
> > PRIVACY_ENABLED - Privacy is available and turned on
>
> Agree with Jani, use the property presence to determine if it's supported

That makes sense; just to confirm can a property be added or removed
after the connector is registered?

>
> >
> > When the connector is initized the privacy screen property is set to
> > PRIVACY_UNSUPPORTED and cannot be changed unless a drm_privacy_screen
> > is registered to the connector. drm_privacy_screen will look something
> > like
> >
> > struct drm_privacy_screen_ops {
> >     int (*get_privacy_state)(struct drm_privacy_screen *);
> >     int (*set_privacy_state)(struct drm_privacy_screen *, int);
> > }
> >
> > struct drm_privacy_screen {
> >     /* The privacy screen device */
> >     struct device *dev;
> >
> >     /* The connector that the privacy screen is attached */
> >     struct drm_connector *connector;
> >
> >     /* Ops to get and set the privacy screen state */
> >     struct drm_privacy_screen_ops *ops;
> >
> >     /* The current state of the privacy screen */
> >     int state;
> > }
> >
> > Privacy screen device drivers will call a function to register the
> > privacy screen with the connector.
>
> Do we actually need dedicated drivers for privacy screen? It seems
> like something that is panel-specific hardware, so I'd suggest just
> using the panel driver.

The privacy screen is physically part of the display but the control
interface, at least in all current use cases, is ACPI. Is there a way
to control an ACPI device with the panel driver?

>
> Sean
>
> >
> > int drm_privacy_screen_register(struct drm_privacy_screen_ops *ops,
> > struct device *dev, struct drm_connector *);
> >
> > Calling this will set a new field on the connector "struct
> > drm_privacy_screen *privacy_screen" and change the value of the
> > property to ops->get_privacy_state(). When
> > drm_mode_connector_set_obj_prop() is called with the
> > privacy_screen_proptery if a privacy_screen is registered to the
> > connector the ops->set_privacy_state() will be called with the new
> > value.
> >
> > Setting of this property (and all drm properties) is done in user
> > space using ioctrl.
> >
> > Registering the privacy screen with a connector may be tricky because
> > the driver for the privacy screen will need to be able to identify
> > which connector it belongs to and we will have to deal with connectors
> > being added both before and after the privacy screen device is added
> > by it's driver.
> >
> > How does that sound? I will work on a patch if that all sounds about right.
> >
> > One question I still have is there a way to not accept a value that is
> > passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
> > screen is not registered the property must stay PRIVACY_UNSUPPORTED
> > and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
> > never be set.

Re: New sysfs interface for privacy screens

[Rajat Jain]

On Mon, Oct 7, 2019 at 9:19 AM Mat King <mathewk@google.com> wrote:
>
> On Mon, Oct 7, 2019 at 7:09 AM Sean Paul <seanpaul@chromium.org> wrote:
> >
> > On Thu, Oct 3, 2019 at 3:57 PM Mat King <mathewk@google.com> wrote:
> > >
> > > On Thu, Oct 3, 2019 at 2:59 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > > >
> > > > On Wed, 02 Oct 2019, Mat King <mathewk@google.com> wrote:
> > > > > On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > > > >>
> > > > >> On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > > >> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
> > > > >> >> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
> > > > >> >> > Resending in plain text mode
> >
> > /snip
> >
> > >
> > > So my proposal would now be to add a new standard property to
> > > drm_connector called "privacy_screen" this property would be an enum
> > > which can take one of three values.
> > >
> > > PRIVACY_UNSUPPORTED - Privacy is not available for this connector
> > > PRIVACY_DISABLED - Privacy is available but turned off
> > > PRIVACY_ENABLED - Privacy is available and turned on
> >
> > Agree with Jani, use the property presence to determine if it's supported
>
> That makes sense; just to confirm can a property be added or removed
> after the connector is registered?
>
> >
> > >
> > > When the connector is initized the privacy screen property is set to
> > > PRIVACY_UNSUPPORTED and cannot be changed unless a drm_privacy_screen
> > > is registered to the connector. drm_privacy_screen will look something
> > > like
> > >
> > > struct drm_privacy_screen_ops {
> > >     int (*get_privacy_state)(struct drm_privacy_screen *);
> > >     int (*set_privacy_state)(struct drm_privacy_screen *, int);
> > > }
> > >
> > > struct drm_privacy_screen {
> > >     /* The privacy screen device */
> > >     struct device *dev;
> > >
> > >     /* The connector that the privacy screen is attached */
> > >     struct drm_connector *connector;
> > >
> > >     /* Ops to get and set the privacy screen state */
> > >     struct drm_privacy_screen_ops *ops;
> > >
> > >     /* The current state of the privacy screen */
> > >     int state;
> > > }
> > >
> > > Privacy screen device drivers will call a function to register the
> > > privacy screen with the connector.
> >
> > Do we actually need dedicated drivers for privacy screen? It seems
> > like something that is panel-specific hardware, so I'd suggest just
> > using the panel driver.
>
> The privacy screen is physically part of the display but the control
> interface, at least in all current use cases, is ACPI. Is there a way
> to control an ACPI device with the panel driver?

I feel that doing it in a dedicated driver has the advantage that if
we can standardise the control interface, it can be used across
different panels. So a new panel can be supported using the existing
driver by merely instantiating the right ACPI HID "privacy screen"
device as a child device of the parent display / panel device. This
parent-child relation would also give the kernel the connection needed
about "which display does this privacy screen attach to". In future,if
non-x86 platforms need the feature using a different control interface
(say via a GPIO driver), the privacy screen driver can be updated to
support that also.

Thanks,

Rajat

>
> >
> > Sean
> >
> > >
> > > int drm_privacy_screen_register(struct drm_privacy_screen_ops *ops,
> > > struct device *dev, struct drm_connector *);
> > >
> > > Calling this will set a new field on the connector "struct
> > > drm_privacy_screen *privacy_screen" and change the value of the
> > > property to ops->get_privacy_state(). When
> > > drm_mode_connector_set_obj_prop() is called with the
> > > privacy_screen_proptery if a privacy_screen is registered to the
> > > connector the ops->set_privacy_state() will be called with the new
> > > value.
> > >
> > > Setting of this property (and all drm properties) is done in user
> > > space using ioctrl.
> > >
> > > Registering the privacy screen with a connector may be tricky because
> > > the driver for the privacy screen will need to be able to identify
> > > which connector it belongs to and we will have to deal with connectors
> > > being added both before and after the privacy screen device is added
> > > by it's driver.
> > >
> > > How does that sound? I will work on a patch if that all sounds about right.
> > >
> > > One question I still have is there a way to not accept a value that is
> > > passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
> > > screen is not registered the property must stay PRIVACY_UNSUPPORTED
> > > and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
> > > never be set.

Re: New sysfs interface for privacy screens

[Sean Paul]

On Mon, Oct 07, 2019 at 12:31:08PM -0700, Rajat Jain wrote:
> On Mon, Oct 7, 2019 at 9:19 AM Mat King <mathewk@google.com> wrote:
> >
> > On Mon, Oct 7, 2019 at 7:09 AM Sean Paul <seanpaul@chromium.org> wrote:
> > >
> > > On Thu, Oct 3, 2019 at 3:57 PM Mat King <mathewk@google.com> wrote:
> > > >
> > > > On Thu, Oct 3, 2019 at 2:59 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > > > >
> > > > > On Wed, 02 Oct 2019, Mat King <mathewk@google.com> wrote:
> > > > > > On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > > > > >>
> > > > > >> On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> > > > > >> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
> > > > > >> >> On Tue, 01 Oct 2019, Mat King <mathewk@google.com> wrote:
> > > > > >> >> > Resending in plain text mode
> > >
> > > /snip
> > >
> > > >
> > > > So my proposal would now be to add a new standard property to
> > > > drm_connector called "privacy_screen" this property would be an enum
> > > > which can take one of three values.
> > > >
> > > > PRIVACY_UNSUPPORTED - Privacy is not available for this connector
> > > > PRIVACY_DISABLED - Privacy is available but turned off
> > > > PRIVACY_ENABLED - Privacy is available and turned on
> > >
> > > Agree with Jani, use the property presence to determine if it's supported
> >
> > That makes sense; just to confirm can a property be added or removed
> > after the connector is registered?
> >
> > >
> > > >
> > > > When the connector is initized the privacy screen property is set to
> > > > PRIVACY_UNSUPPORTED and cannot be changed unless a drm_privacy_screen
> > > > is registered to the connector. drm_privacy_screen will look something
> > > > like
> > > >
> > > > struct drm_privacy_screen_ops {
> > > >     int (*get_privacy_state)(struct drm_privacy_screen *);
> > > >     int (*set_privacy_state)(struct drm_privacy_screen *, int);
> > > > }
> > > >
> > > > struct drm_privacy_screen {
> > > >     /* The privacy screen device */
> > > >     struct device *dev;
> > > >
> > > >     /* The connector that the privacy screen is attached */
> > > >     struct drm_connector *connector;
> > > >
> > > >     /* Ops to get and set the privacy screen state */
> > > >     struct drm_privacy_screen_ops *ops;
> > > >
> > > >     /* The current state of the privacy screen */
> > > >     int state;
> > > > }
> > > >
> > > > Privacy screen device drivers will call a function to register the
> > > > privacy screen with the connector.
> > >
> > > Do we actually need dedicated drivers for privacy screen? It seems
> > > like something that is panel-specific hardware, so I'd suggest just
> > > using the panel driver.
> >
> > The privacy screen is physically part of the display but the control
> > interface, at least in all current use cases, is ACPI. Is there a way
> > to control an ACPI device with the panel driver?
> 
> I feel that doing it in a dedicated driver has the advantage that if
> we can standardise the control interface, it can be used across
> different panels. So a new panel can be supported using the existing
> driver by merely instantiating the right ACPI HID "privacy screen"
> device as a child device of the parent display / panel device. This
> parent-child relation would also give the kernel the connection needed
> about "which display does this privacy screen attach to". In future,if
> non-x86 platforms need the feature using a different control interface
> (say via a GPIO driver), the privacy screen driver can be updated to
> support that also.


I might be misunderstanding the scope of this, but if everything is controlled
via drm properties, you could just use a helper function to toggle it on/off? We
have helper libraries for a plethora of optional hardware features already.

Sean

> 
> Thanks,
> 
> Rajat
> 
> >
> > >
> > > Sean
> > >
> > > >
> > > > int drm_privacy_screen_register(struct drm_privacy_screen_ops *ops,
> > > > struct device *dev, struct drm_connector *);
> > > >
> > > > Calling this will set a new field on the connector "struct
> > > > drm_privacy_screen *privacy_screen" and change the value of the
> > > > property to ops->get_privacy_state(). When
> > > > drm_mode_connector_set_obj_prop() is called with the
> > > > privacy_screen_proptery if a privacy_screen is registered to the
> > > > connector the ops->set_privacy_state() will be called with the new
> > > > value.
> > > >
> > > > Setting of this property (and all drm properties) is done in user
> > > > space using ioctrl.
> > > >
> > > > Registering the privacy screen with a connector may be tricky because
> > > > the driver for the privacy screen will need to be able to identify
> > > > which connector it belongs to and we will have to deal with connectors
> > > > being added both before and after the privacy screen device is added
> > > > by it's driver.
> > > >
> > > > How does that sound? I will work on a patch if that all sounds about right.
> > > >
> > > > One question I still have is there a way to not accept a value that is
> > > > passed to drm_mode_connector_set_obj_prop()? In this case if a privacy
> > > > screen is not registered the property must stay PRIVACY_UNSUPPORTED
> > > > and if a privacy screen is registered then PRIVACY_UNSUPPORTED must
> > > > never be set.

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: New sysfs interface for privacy screens

[Jani Nikula]

On Mon, 07 Oct 2019, Mat King <mathewk@google.com> wrote:
> That makes sense; just to confirm can a property be added or removed
> after the connector is registered?

You need to create the property before registering the drm device. You
can attach/detach the property later, but I should think you know by the
time you're registering the connector whether it supports the privacy
screen or not.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: New sysfs interface for privacy screens

[Rajat Jain]

On Tue, Oct 22, 2019 at 5:14 PM Rajat Jain <rajatja@google.com> wrote:
>
> Hi Folks,
>
> On Mon, Oct 7, 2019 at 11:13 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Mon, 07 Oct 2019, Mat King <mathewk@google.com> wrote:
>> > That makes sense; just to confirm can a property be added or removed
>> > after the connector is registered?
>>
>> You need to create the property before registering the drm device. You
>> can attach/detach the property later, but I should think you know by the
>> time you're registering the connector whether it supports the privacy
>> screen or not.
>
>

I just posted a patch for this here:

https://lkml.org/lkml/2019/10/22/967

Would appreciate review and comments.

Thanks & Best Regards,

Rajat
>
>>
>>
>> BR,
>> Jani.
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center

Re: New sysfs interface for privacy screens

[Daniel Vetter]

On Tue, Oct 08, 2019 at 09:13:51AM +0300, Jani Nikula wrote:
> On Mon, 07 Oct 2019, Mat King <mathewk@google.com> wrote:
> > That makes sense; just to confirm can a property be added or removed
> > after the connector is registered?
> 
> You need to create the property before registering the drm device. You
> can attach/detach the property later, but I should think you know by the
> time you're registering the connector whether it supports the privacy
> screen or not.

I don't think you can add/remove a property after the object you're
adding/removing the prop from has gone public (either with
drm_dev_register or drm_connector_register for hotplugged connectors).

I guess another gap we should cover with some WARN_ON?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch