Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 28 Oct 2019 10:12:23 +0200
From:   Pekka Paalanen <ppaalanen@gmail.com>
To:     Rajat Jain <rajatja@google.com>
Cc:     Daniel Vetter <daniel@ffwll.ch>,
        Thierry Reding <thierry.reding@gmail.com>,
        Sean Paul <seanpaul@google.com>,
        David Airlie <airlied@linux.ie>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Daniel Thompson <daniel.thompson@linaro.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Jesse Barnes <jsbarnes@google.com>,
        Rajat Jain <rajatxjain@gmail.com>,
        intel-gfx <intel-gfx@lists.freedesktop.org>,
        Mat King <mathewk@google.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        =?UTF-8?B?Sm9zw6k=?= Roberto de Souza <jose.souza@intel.com>,
        Sean Paul <sean@poorly.run>,
        Duncan Laurie <dlaurie@google.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Pavel Machek <pavel@denx.de>
Subject: Re: [PATCH] drm: Add support for integrated privacy screens
Message-ID: <20191028101223.24da4d78@eldfell.localdomain>
In-Reply-To: <CAKMK7uGscg0YDgNZh61PAhnkF8xnASepo2HK2Y51wROPSkqJLA@mail.gmail.com>
References: <20191023001206.15741-1-rajatja@google.com>
        <20191024112040.GE2825247@ulmo>
        <CAA93t1ozojwgVoLCZ=AWx72yddQoiaZCMFG35gQg3mQL9n9Z2w@mail.gmail.com>
        <20191025113609.GB928835@ulmo>
        <CAKMK7uGscg0YDgNZh61PAhnkF8xnASepo2HK2Y51wROPSkqJLA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
 boundary="Sig_/NJcM6CAa7BRFczfzsRNLDUV"; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--Sig_/NJcM6CAa7BRFczfzsRNLDUV
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable

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: =20
> > > 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: =20
> > > >
> > > > On Tue, Oct 22, 2019 at 05:12:06PM -0700, Rajat Jain wrote: =20
> > > > > 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 i=
dea
> > > > > was discussed here:
> > > > >
> > > > > https://lkml.org/lkml/2019/10/1/786
> > > > >
> > > > > ACPI methods are used to identify, query and control privacy scre=
en:
> > > > >
> > > > > * 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_Jan=
30.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 abi=
lity
> > > > > 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 avai=
lable
> > > > > (e.g. external monitors supporting integrated privacy screens?).
> > > > >
> > > > > Also, this code can be extended in future to support non-ACPI met=
hods
> > > > > (e.g. using a kernel GPIO driver to toggle a gpio that controls t=
he
> > > > > 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 =20
> > > >
> > > > I like this much better than the prior proposal to use sysfs. Howev=
er
> > > > the support currently looks a bit tangled. I realize that we only h=
ave 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.
> > > > =20
> > > > > 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       :=3D      drm_auth.o drm_cache.o \
> > > > >               drm_syncobj.o drm_lease.o drm_writeback.o drm_clien=
t.o \
> > > > >               drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > > > >
> > > > > +drm-$(CONFIG_ACPI) +=3D drm_privacy_screen.o
> > > > >  drm-$(CONFIG_DRM_LEGACY) +=3D drm_legacy_misc.o drm_bufs.o drm_c=
ontext.o drm_dma.o drm_scatter.o drm_lock.o
> > > > >  drm-$(CONFIG_DRM_LIB_RANDOM) +=3D lib/drm_random.o
> > > > >  drm-$(CONFIG_DRM_VM) +=3D 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 =3D=3D connector->max_bpc_property) {
> > > > >               state->max_requested_bpc =3D val;
> > > > > +     } else if (property =3D=3D config->privacy_screen_property)=
 {
> > > > > +             drm_privacy_screen_set_val(connector, val); =20
> > > >
> > > > This doesn't look right. Shouldn't you store the value in the conne=
ctor
> > > > state and then leave it up to the connector driver to set it
> > > > appropriately? I think that also has the advantage of untangling th=
is
> > > > support a little. =20
> > >
> > > Hopefully this gets answered in my explanations below.
> > > =20
> > > > =20
> > > > >       } else if (connector->funcs->atomic_set_property) {
> > > > >               return connector->funcs->atomic_set_property(connec=
tor,
> > > > >                               state, property, val);
> > > > > @@ -842,6 +845,8 @@ drm_atomic_connector_get_property(struct drm_=
connector *connector,
> > > > >               *val =3D 0;
> > > > >       } else if (property =3D=3D connector->max_bpc_property) {
> > > > >               *val =3D state->max_requested_bpc;
> > > > > +     } else if (property =3D=3D config->privacy_screen_property)=
 {
> > > > > +             *val =3D drm_privacy_screen_get_val(connector); =20
> > > >
> > > > Similarly, I think this can just return the atomic state's value for
> > > > this. =20
> > >
> > > 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. =20
> >
> > 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. =20
>=20
> We have atomic properties that the driver also updates, not just userspac=
e:
> - 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

--Sig_/NJcM6CAa7BRFczfzsRNLDUV
Content-Type: application/pgp-signature
Content-Description: OpenPGP digital signature

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEJQjwWQChkWOYOIONI1/ltBGqqqcFAl22oucACgkQI1/ltBGq
qqdk0BAAh6bjj6VS7FwzG/UXCTEYTxnhogIOjkTbODjM2xFAH7RHyGev/3hxnJi+
eXQ2Xu+rFa4BLH5m4Jmu1BW9U3dyTrhJ5JT8me86JYFfP333VdIxS9GMkznTiBvT
hM0e4hyki5qQnxQhNmF/0puxe7yIn/r/WN+jZAACQScEoJCG3me07uemT7wEwsa6
n+ONF0Y/efWK3s2qL39pWfa2dw1Z1ZAaXbUqkumnfrNopjw1t7G6z2zuFoVnAyS3
93RjS5XE/xNEPK3TdHPICGy2vzU+PGSYYg4WSQWHSVNq8ciVSi+dVsAz24t7jagX
mSSAmTwQgT9LegzF/WQ/rwcPO7AXXmKyD6acJXRq2XJk8Z3skFDcKqmokRJqeTbM
5IV8WKRZeI0K9VU9bSrRpciD8gTx5yfNsMJDzFl5txymPrJ2bvBauTiSRKdRsfyt
5P3fbwnWCIPoX9TOWB8KC5UBpdZGrdC3XOe6+ltY4EDdUUcpA8esnn166dEXFGCo
hbKRAD/gQvoRQ98V0C/N9B300/Cmqi6jLPZW87QScl52NFgF7HvJNacovdITWZJm
XIsiAz/oTkHSu0+7d9kdyY4yIZdqyxskV1f89dmbNwE6XW7TwHwKIDRbNVFXwHH8
Jv3Vc/8AaxSXV73hrK2a7//+NUvA7mGx+gxgLv1UYmy2FILjlOY=
=KOpg
-----END PGP SIGNATURE-----

--Sig_/NJcM6CAa7BRFczfzsRNLDUV--