Re: New sysfs interface for privacy screens
On Thu, Oct 3, 2019 at 2:59 AM Jani Nikula wrote: > > On Wed, 02 Oct 2019, Mat King wrote: > > On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula > > wrote: > >> > >> On Wed, 02 Oct 2019, Daniel Thompson wrote: > >> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote: > >> >> On Tue, 01 Oct 2019, 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 > >> >> > > >> >> > 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 O
Re: New sysfs interface for privacy screens
On Mon, Oct 7, 2019 at 7:09 AM Sean Paul wrote:
>
> On Thu, Oct 3, 2019 at 3:57 PM Mat King wrote:
> >
> > On Thu, Oct 3, 2019 at 2:59 AM Jani Nikula
> > wrote:
> > >
> > > On Wed, 02 Oct 2019, Mat King wrote:
> > > > On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula
> > > > wrote:
> > > >>
> > > >> On Wed, 02 Oct 2019, Daniel Thompson
> > > >> wrote:
> > > >> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
> > > >> >> On Tue, 01 Oct 2019, Mat King 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.
New sysfs interface for privacy screens
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
On Tue, Oct 1, 2019 at 10:27 AM Greg KH 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. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: New sysfs interface for privacy screens
On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula wrote: > > On Wed, 02 Oct 2019, Daniel Thompson wrote: > > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote: > >> On Tue, 01 Oct 2019, 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 > >> > > >> > 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
On Wed, Oct 2, 2019 at 9:46 AM Jonathan Corbet wrote: > > On Tue, 1 Oct 2019 10:09:46 -0600 > Mat King 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.
