Re: New sysfs interface for privacy screens

[Mat King]

On Mon, Oct 7, 2019 at 7:09 AM Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
>
> On Thu, Oct 3, 2019 at 3:57 PM Mat King <mathewk@xxxxxxxxxx> wrote:
> >
> > On Thu, Oct 3, 2019 at 2:59 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, 02 Oct 2019, Mat King <mathewk@xxxxxxxxxx> wrote:
> > > > On Wed, Oct 2, 2019 at 4:46 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> > > >>
> > > >> On Wed, 02 Oct 2019, Daniel Thompson <daniel.thompson@xxxxxxxxxx> wrote:
> > > >> > On Wed, Oct 02, 2019 at 12:30:05PM +0300, Jani Nikula wrote:
> > > >> >> On Tue, 01 Oct 2019, Mat King <mathewk@xxxxxxxxxx> 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.