Re: [PATCH v2 1/2] platform/chrome: Add driver for ChromeOS privacy-screen
On Mon, Dec 20, 2021 at 12:21:47PM -0800, Rajat Jain wrote: > Hi Dmitry, > > Thanks for the review. Please see inline. > > On Mon, Dec 20, 2021 at 11:42 AM Dmitry Torokhov > wrote: > > > > Hi Rajat, > > > > On Fri, Dec 17, 2021 at 12:28:49PM -0800, Rajat Jain wrote: > > > This adds the ACPI driver for the ChromeOS privacy screen that is > > > present on some chromeos devices. > > > > > > Note that ideally, we'd want this privacy screen driver to be probed > > > BEFORE the drm probe in order to avoid a drm probe deferral: > > > https://hansdegoede.livejournal.com/25948.html > > > > > > In practise, I found that ACPI drivers are bound to their devices AFTER > > > the drm probe on chromebooks. So on chromebooks with privacy-screen, > > > this patch along with the next one in this series results in a probe > > > deferral of about 250ms for i915 driver. However, it did not result in > > > any user noticeable delay of splash screen in my personal experience. > > > > > > In future if this probe deferral turns out to be an issue, we can > > > consider turning this ACPI driver into something that is probed > > > earlier than the drm drivers. > > > > > > Signed-off-by: Rajat Jain > > > --- > > > v2: * Reword the commit log > > > * Make the Kconfig into a tristate > > > * Reorder the patches in the series. > > > > > > drivers/platform/chrome/Kconfig | 9 ++ > > > drivers/platform/chrome/Makefile | 1 + > > > drivers/platform/chrome/chromeos_priv_scrn.c | 132 +++ > > > 3 files changed, 142 insertions(+) > > > create mode 100644 drivers/platform/chrome/chromeos_priv_scrn.c > > > > > > diff --git a/drivers/platform/chrome/Kconfig > > > b/drivers/platform/chrome/Kconfig > > > index ccc23d8686e8..d1c209a45a62 100644 > > > --- a/drivers/platform/chrome/Kconfig > > > +++ b/drivers/platform/chrome/Kconfig > > > @@ -243,6 +243,15 @@ config CROS_USBPD_NOTIFY > > > To compile this driver as a module, choose M here: the > > > module will be called cros_usbpd_notify. > > > > > > +config CHROMEOS_PRIVACY_SCREEN > > > + tristate "ChromeOS Privacy Screen support" > > > + depends on ACPI > > > + depends on DRM > > > + select DRM_PRIVACY_SCREEN > > > + help > > > + This driver provides the support needed for the in-built > > > electronic > > > + privacy screen that is present on some ChromeOS devices. > > > + > > > source "drivers/platform/chrome/wilco_ec/Kconfig" > > > > > > endif # CHROMEOS_PLATFORMS > > > diff --git a/drivers/platform/chrome/Makefile > > > b/drivers/platform/chrome/Makefile > > > index f901d2e43166..cfa0bb4e9e34 100644 > > > --- a/drivers/platform/chrome/Makefile > > > +++ b/drivers/platform/chrome/Makefile > > > @@ -4,6 +4,7 @@ > > > CFLAGS_cros_ec_trace.o:= -I$(src) > > > > > > obj-$(CONFIG_CHROMEOS_LAPTOP)+= chromeos_laptop.o > > > +obj-$(CONFIG_CHROMEOS_PRIVACY_SCREEN)+= chromeos_priv_scrn.o > > > obj-$(CONFIG_CHROMEOS_PSTORE)+= chromeos_pstore.o > > > obj-$(CONFIG_CHROMEOS_TBMC) += chromeos_tbmc.o > > > obj-$(CONFIG_CROS_EC)+= cros_ec.o > > > diff --git a/drivers/platform/chrome/chromeos_priv_scrn.c > > > b/drivers/platform/chrome/chromeos_priv_scrn.c > > > new file mode 100644 > > > index ..a4cbf5c79c2a > > > --- /dev/null > > > +++ b/drivers/platform/chrome/chromeos_priv_scrn.c > > > > I think we can spare a few more characters :) chromeos_privacy_screen.c > > maybe? > > > > And also see if maybe variables in the code are not that unseemly long > > even if not abbreviated? > > Sure, I can certainly replace "chromeos_priv_scrn" with > "chromeos_privacy_screen" everywhere. Some of the variables may be a > little long, but I think that should be OK (my main concern was > > chromeos_privacy_screen_device_ids > chromeos_privacy_screen_get_hw_state() > > Let me know if that doesn't sound right (in which case, I can probably > omit "chromeos" from the local variable and function names) Another option to go all the way into different direction, and use "cps_" prefix for everything. It is probably just me but combination of "priv" "scrn" just grates on me ;) > > > > > > @@ -0,0 +1,132 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > + > > > +/* > > > + * chromeos_priv_scrn.c - ChromeOS Privacy Screen support > > > > I'd avoid mentioning file name as those tend to change. > > Ack, will do. > > > > > > + * > > > + * Copyright (C) 2022 The Chromium OS Authors > > > > This is not correct copyright for kernel contributions. It should be > > attributed to "Google LLC". Note that it is different from CrOS > > userspace. > > > > Ack, will do. > > > > + * > > > + */ > > > + > > > +#include > > > +#include > > > + > > > +/* > > > + * The DSM (Define Specific Method) constants below are the agreed API > > > with > > > + * the firmware team, on how to control privacy screen using ACPI > > > methods.
Re: [PATCH v2 1/2] platform/chrome: Add driver for ChromeOS privacy-screen
Hi Rajat,
On Fri, Dec 17, 2021 at 12:28:49PM -0800, Rajat Jain wrote:
> This adds the ACPI driver for the ChromeOS privacy screen that is
> present on some chromeos devices.
>
> Note that ideally, we'd want this privacy screen driver to be probed
> BEFORE the drm probe in order to avoid a drm probe deferral:
> https://hansdegoede.livejournal.com/25948.html
>
> In practise, I found that ACPI drivers are bound to their devices AFTER
> the drm probe on chromebooks. So on chromebooks with privacy-screen,
> this patch along with the next one in this series results in a probe
> deferral of about 250ms for i915 driver. However, it did not result in
> any user noticeable delay of splash screen in my personal experience.
>
> In future if this probe deferral turns out to be an issue, we can
> consider turning this ACPI driver into something that is probed
> earlier than the drm drivers.
>
> Signed-off-by: Rajat Jain
> ---
> v2: * Reword the commit log
> * Make the Kconfig into a tristate
> * Reorder the patches in the series.
>
> drivers/platform/chrome/Kconfig | 9 ++
> drivers/platform/chrome/Makefile | 1 +
> drivers/platform/chrome/chromeos_priv_scrn.c | 132 +++
> 3 files changed, 142 insertions(+)
> create mode 100644 drivers/platform/chrome/chromeos_priv_scrn.c
>
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index ccc23d8686e8..d1c209a45a62 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -243,6 +243,15 @@ config CROS_USBPD_NOTIFY
> To compile this driver as a module, choose M here: the
> module will be called cros_usbpd_notify.
>
> +config CHROMEOS_PRIVACY_SCREEN
> + tristate "ChromeOS Privacy Screen support"
> + depends on ACPI
> + depends on DRM
> + select DRM_PRIVACY_SCREEN
> + help
> + This driver provides the support needed for the in-built electronic
> + privacy screen that is present on some ChromeOS devices.
> +
> source "drivers/platform/chrome/wilco_ec/Kconfig"
>
> endif # CHROMEOS_PLATFORMS
> diff --git a/drivers/platform/chrome/Makefile
> b/drivers/platform/chrome/Makefile
> index f901d2e43166..cfa0bb4e9e34 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -4,6 +4,7 @@
> CFLAGS_cros_ec_trace.o:= -I$(src)
>
> obj-$(CONFIG_CHROMEOS_LAPTOP)+= chromeos_laptop.o
> +obj-$(CONFIG_CHROMEOS_PRIVACY_SCREEN)+= chromeos_priv_scrn.o
> obj-$(CONFIG_CHROMEOS_PSTORE)+= chromeos_pstore.o
> obj-$(CONFIG_CHROMEOS_TBMC) += chromeos_tbmc.o
> obj-$(CONFIG_CROS_EC)+= cros_ec.o
> diff --git a/drivers/platform/chrome/chromeos_priv_scrn.c
> b/drivers/platform/chrome/chromeos_priv_scrn.c
> new file mode 100644
> index ..a4cbf5c79c2a
> --- /dev/null
> +++ b/drivers/platform/chrome/chromeos_priv_scrn.c
I think we can spare a few more characters :) chromeos_privacy_screen.c
maybe?
And also see if maybe variables in the code are not that unseemly long
even if not abbreviated?
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * chromeos_priv_scrn.c - ChromeOS Privacy Screen support
I'd avoid mentioning file name as those tend to change.
> + *
> + * Copyright (C) 2022 The Chromium OS Authors
This is not correct copyright for kernel contributions. It should be
attributed to "Google LLC". Note that it is different from CrOS
userspace.
> + *
> + */
> +
> +#include
> +#include
> +
> +/*
> + * The DSM (Define Specific Method) constants below are the agreed API with
> + * the firmware team, on how to control privacy screen using ACPI methods.
> + */
> +#define PRIV_SCRN_DSM_REVID 1 /* DSM version */
> +#define PRIV_SCRN_DSM_FN_GET_STATUS 1 /* Get privacy screen status */
> +#define PRIV_SCRN_DSM_FN_ENABLE 2 /* Enable privacy
> screen */
> +#define PRIV_SCRN_DSM_FN_DISABLE 3 /* Disable privacy screen */
> +
> +static const guid_t chromeos_priv_scrn_dsm_guid =
> + GUID_INIT(0xc7033113, 0x8720, 0x4ceb,
> + 0x90, 0x90, 0x9d, 0x52, 0xb3, 0xe5, 0x2d, 0x73);
> +
> +static void
> +chromeos_priv_scrn_get_hw_state(struct drm_privacy_screen *drm_priv_scrn)
> +{
> + union acpi_object *obj;
> + acpi_handle handle;
> + struct device *priv_scrn = drm_priv_scrn->dev.parent;
This is really bad that we need to poke into internals of
drm_privacy_screen to get to "our" device. I think there is only one
consume of the privacy screen API at the moment, the thinkpad driver, so
maybe it is not too late to change drm_privacy_screen_register() to
either accept instance of struct drm_privacy_screen (which then could be
embedded into something) or accept a void pointer to attach arbitrary
data to it, and then add drm_privacy_screen_get_drvdata() to get to that
pointer.
> +
> + if
