Re: [PATCH v2 1/2] platform/chrome: Add driver for ChromeOS privacy-screen

Hi Dmitry,

Thanks for the review. Please see inline.

On Mon, Dec 20, 2021 at 11:42 AM Dmitry Torokhov dmitry.torokhov@gmail.com 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 rajatja@google.com

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 000000000000..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)

...

@@ -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 <linux/acpi.h> +#include <drm/drm_privacy_screen_driver.h>

+/*

• • 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.

Sure, ack, will do.

...

• if (!priv_scrn)
  

•         return;
  

This should not happen regardless.

Sure, ack, will remove.

...

• handle = acpi_device_handle(to_acpi_device(priv_scrn));
  

• obj = acpi_evaluate_dsm(handle, &chromeos_priv_scrn_dsm_guid,
  

•                         PRIV_SCRN_DSM_REVID,
  

•                         PRIV_SCRN_DSM_FN_GET_STATUS, NULL);
  

• if (!obj) {
  

•         dev_err(priv_scrn, "_DSM failed to get privacy-screen state\n");
  

Can we do something about it? A dedicated status? Also, can we print ACPI-specific error?

Umm ... I don't know. We don't know anything beyond that the ACPI method wasn't able to get us anything. There are no other status other than the ones specified in enum drm_privacy_screen_status. Since that enum was the result of almost 1.5 year of discussion between pekka, Hans and other or drm mailing lists, I don't wat to change that.

...

•         return;
  

• }
  

• if (obj->type != ACPI_TYPE_INTEGER)
  

•         dev_err(priv_scrn, "Bad _DSM to get privacy-screen state\n");
  

Same here.

Just like above, we only know that it couldn't get us what we need. There isn't anything we can do.

...

• else if (obj->integer.value == 1)
  

•         drm_priv_scrn->hw_state = drm_priv_scrn->sw_state =
  

•                 PRIVACY_SCREEN_ENABLED;
  

• else
  

•         drm_priv_scrn->hw_state = drm_priv_scrn->sw_state =
  

•                 PRIVACY_SCREEN_DISABLED;
  

• ACPI_FREE(obj);
  

+}

+static int +chromeos_priv_scrn_set_sw_state(struct drm_privacy_screen *drm_priv_scrn,

•                         enum drm_privacy_screen_status state)
  

+{

• union acpi_object *obj = NULL;
  

• acpi_handle handle;
  

• struct device *priv_scrn = drm_priv_scrn->dev.parent;
  

• if (!priv_scrn)
  

•         return -ENODEV;
  

This should not happen regardless.

Ack, will remove.

...

• handle = acpi_device_handle(to_acpi_device(priv_scrn));
  

• if (state == PRIVACY_SCREEN_DISABLED) {
  

•         obj = acpi_evaluate_dsm(handle, &chromeos_priv_scrn_dsm_guid,
  

•                                 PRIV_SCRN_DSM_REVID,
  

•                                 PRIV_SCRN_DSM_FN_DISABLE, NULL);
  

• } else if (state == PRIVACY_SCREEN_ENABLED) {
  

•         obj = acpi_evaluate_dsm(handle, &chromeos_priv_scrn_dsm_guid,
  

•                                 PRIV_SCRN_DSM_REVID,
  

•                                 PRIV_SCRN_DSM_FN_ENABLE, NULL);
  

• } else {
  

•         dev_err(priv_scrn, "Bad attempt to set privacy-screen status\n");
  

•         return -EINVAL;
  

Print state to aid in tracking bugs?

Ack, will do.

...

• }
  

• if (!obj) {
  

•         dev_err(priv_scrn, "_DSM failed to set privacy-screen state\n");
  

Can we print ACPI-specific error?

AFAICS, we do not know what went wrong. The method gives us a NULL (meaning something went wrong) or a value which means all is OK. I do see that acpi_evaluate_dsm() prints something in case of failure, in case that si what you were looking for..

...

•         return -EIO;
  

• }
  

• drm_priv_scrn->hw_state = drm_priv_scrn->sw_state = state;
  

• ACPI_FREE(obj);
  

• return 0;
  

+}

+static const struct drm_privacy_screen_ops chromeos_priv_scrn_ops = {

• .get_hw_state = chromeos_priv_scrn_get_hw_state,
  

• .set_sw_state = chromeos_priv_scrn_set_sw_state,
  

+};

+static int chromeos_priv_scrn_add(struct acpi_device *adev) +{

• struct drm_privacy_screen *drm_priv_scrn =
  

•         drm_privacy_screen_register(&adev->dev, &chromeos_priv_scrn_ops);
  

• if (IS_ERR(drm_priv_scrn)) {
  

•         dev_err(&adev->dev, "Error registering privacy-screen\n");
  

•         return PTR_ERR(drm_priv_scrn);
  

• }
  

• dev_info(&adev->dev, "registered privacy-screen '%s'\n",
  

•          dev_name(&drm_priv_scrn->dev));
  

I don't think we need be this noisy.

This can help us track the probe deferral related bugs (and whether it is too bad). Yes, we have a probe deferral with these patches. Essentially I wanted these prints because they can be taken as markers to determine how much delay is the probe deferral costing us.

...

• return 0;
  

+}

+static const struct acpi_device_id chromeos_priv_scrn_device_ids[] = {

• {"GOOG0010", 0}, /* Google's electronic privacy screen for eDP-1 */
  

• {}
  

+}; +MODULE_DEVICE_TABLE(acpi, chromeos_priv_scrn_device_ids);

+static struct acpi_driver chromeos_priv_scrn_driver = {

• .name = "chromeos_priv_scrn_drvr",
  

• .class = "ChromeOS",
  

• .ids = chromeos_priv_scrn_device_ids,
  

• .ops = {
  

•         .add = chromeos_priv_scrn_add,
  

We don't need any cleanup?

Ack, will do.

...

• },
  

+};

+module_acpi_driver(chromeos_priv_scrn_driver); +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("ChromeOS ACPI Privacy Screen driver");

+MODULE_AUTHOR("Rajat Jain rajatja@google.com");

2.34.1.307.g9b7440fafd-goog

Thanks.

-- Dmitry