Re: [PATCH] drm: Add support for integrated privacy screens

[Pekka Paalanen <ppaalanen@gmail.com>]

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

On Fri, 25 Oct 2019 21:03:12 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Oct 25, 2019 at 1:36 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Thu, Oct 24, 2019 at 01:45:16PM -0700, Rajat Jain wrote:  
> > > Hi,
> > >
> > > Thanks for your review and comments. Please see inline below.
> > >
> > > On Thu, Oct 24, 2019 at 4:20 AM Thierry Reding <thierry.reding@gmail.com> wrote:  
> > > >
> > > > On Tue, Oct 22, 2019 at 05:12:06PM -0700, Rajat Jain wrote:  
> > > > > Certain laptops now come with panels that have integrated privacy
> > > > > screens on them. This patch adds support for such panels by adding
> > > > > a privacy-screen property to the drm_connector for the panel, that
> > > > > the userspace can then use to control and check the status. The idea
> > > > > was discussed here:
> > > > >
> > > > > https://lkml.org/lkml/2019/10/1/786
> > > > >
> > > > > ACPI methods are used to identify, query and control privacy screen:
> > > > >
> > > > > * Identifying an ACPI object corresponding to the panel: The patch
> > > > > follows ACPI Spec 6.3 (available at
> > > > > https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf).
> > > > > Pages 1119 - 1123 describe what I believe, is a standard way of
> > > > > identifying / addressing "display panels" in the ACPI tables, thus
> > > > > allowing kernel to attach ACPI nodes to the panel. IMHO, this ability
> > > > > to identify and attach ACPI nodes to drm connectors may be useful for
> > > > > reasons other privacy-screens, in future.
> > > > >
> > > > > * Identifying the presence of privacy screen, and controlling it, is done
> > > > > via ACPI _DSM methods.
> > > > >
> > > > > Currently, this is done only for the Intel display ports. But in future,
> > > > > this can be done for any other ports if the hardware becomes available
> > > > > (e.g. external monitors supporting integrated privacy screens?).
> > > > >
> > > > > Also, this code can be extended in future to support non-ACPI methods
> > > > > (e.g. using a kernel GPIO driver to toggle a gpio that controls the
> > > > > privacy-screen).
> > > > >
> > > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > > ---
> > > > >  drivers/gpu/drm/Makefile                |   1 +
> > > > >  drivers/gpu/drm/drm_atomic_uapi.c       |   5 +
> > > > >  drivers/gpu/drm/drm_connector.c         |  38 +++++
> > > > >  drivers/gpu/drm/drm_privacy_screen.c    | 176 ++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/display/intel_dp.c |   3 +
> > > > >  include/drm/drm_connector.h             |  18 +++
> > > > >  include/drm/drm_mode_config.h           |   7 +
> > > > >  include/drm/drm_privacy_screen.h        |  33 +++++
> > > > >  8 files changed, 281 insertions(+)
> > > > >  create mode 100644 drivers/gpu/drm/drm_privacy_screen.c
> > > > >  create mode 100644 include/drm/drm_privacy_screen.h  
> > > >
> > > > I like this much better than the prior proposal to use sysfs. However
> > > > the support currently looks a bit tangled. I realize that we only have a
> > > > single implementation for this in hardware right now, so there's no use
> > > > in over-engineering things, but I think we can do a better job from the
> > > > start without getting into too many abstractions. See below.
> > > >  
> > > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > > index 82ff826b33cc..e1fc33d69bb7 100644
> > > > > --- a/drivers/gpu/drm/Makefile
> > > > > +++ b/drivers/gpu/drm/Makefile
> > > > > @@ -19,6 +19,7 @@ drm-y       :=      drm_auth.o drm_cache.o \
> > > > >               drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > > > >               drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > > > >
> > > > > +drm-$(CONFIG_ACPI) += drm_privacy_screen.o
> > > > >  drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
> > > > >  drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > > > >  drm-$(CONFIG_DRM_VM) += drm_vm.o
> > > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > > index 7a26bfb5329c..44131165e4ea 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > > @@ -30,6 +30,7 @@
> > > > >  #include <drm/drm_atomic.h>
> > > > >  #include <drm/drm_print.h>
> > > > >  #include <drm/drm_drv.h>
> > > > > +#include <drm/drm_privacy_screen.h>
> > > > >  #include <drm/drm_writeback.h>
> > > > >  #include <drm/drm_vblank.h>
> > > > >
> > > > > @@ -766,6 +767,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> > > > >                                                  fence_ptr);
> > > > >       } else if (property == connector->max_bpc_property) {
> > > > >               state->max_requested_bpc = val;
> > > > > +     } else if (property == config->privacy_screen_property) {
> > > > > +             drm_privacy_screen_set_val(connector, val);  
> > > >
> > > > This doesn't look right. Shouldn't you store the value in the connector
> > > > state and then leave it up to the connector driver to set it
> > > > appropriately? I think that also has the advantage of untangling this
> > > > support a little.  
> > >
> > > Hopefully this gets answered in my explanations below.
> > >  
> > > >  
> > > > >       } else if (connector->funcs->atomic_set_property) {
> > > > >               return connector->funcs->atomic_set_property(connector,
> > > > >                               state, property, val);
> > > > > @@ -842,6 +845,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> > > > >               *val = 0;
> > > > >       } else if (property == connector->max_bpc_property) {
> > > > >               *val = state->max_requested_bpc;
> > > > > +     } else if (property == config->privacy_screen_property) {
> > > > > +             *val = drm_privacy_screen_get_val(connector);  
> > > >
> > > > Similarly, I think this can just return the atomic state's value for
> > > > this.  
> > >
> > > I did think about having a state variable in software to get and set
> > > this. However, I think it is not very far fetched that some platforms
> > > may have "hardware kill" switches that allow hardware to switch
> > > privacy-screen on and off directly, in addition to the software
> > > control that we are implementing. Privacy is a touchy subject in
> > > enterprise, and anything that reduces the possibility of having any
> > > inconsistency between software state and hardware state is desirable.
> > > So in this case, I chose to not have a state in software about this -
> > > we just report the hardware state everytime we are asked for it.  
> >
> > So this doesn't really work with atomic KMS, then. The main idea behind
> > atomic KMS is that you apply a configuration either completely or not at
> > all. So at least for setting this property you'd have to go through the
> > state object.
> >
> > Now, for reading out the property you might be able to get away with the
> > above. I'm not sure if that's enough to keep the state up-to-date,
> > though. Is there some way for a kill switch to trigger an interrupt or
> > other event of some sort so that the state could be kept up-to-date?
> >
> > Daniel (or anyone else), do you know of any precedent for state that
> > might get modified behind the atomic helpers' back? Seems to me like we
> > need to find some point where we can actually read back the current
> > "hardware value" of this privacy screen property and store that back
> > into the state.  
> 
> We have atomic properties that the driver also updates, not just userspace:
> - link status
> - hdcp machinery

Hi,

just a note about properties. Please, do not use the HDCP "Content
Protection" as an example of a good property design. A property that is
writable by both userspace and kernel is a hard one to use right in my
opinion.

For privacy screens, I suggest defining two optional and separate
properties:

Software control on/off: userspace writable, kernel immutable

Hardware kill switch on/off: userspace immutable, kernel writable

The semantics of these should be fairly clear: if hardware kill switch
exists and is on, then the privacy screen is on. (Does this match
hardware and expected behaviour?) Otherwise, if the software control
exists, it can be used to control the privacy screen.

For delivering change events for the hardware kill switch, please
search for the proposal to enhance hotplug uevents with property ids.
This was discussed and implemented(?) for delivering HDCP "Content
Protection" changes to userspace when implementing the "HDCP Content
Type" property.


Thanks,
pq

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