Hi Dmitry, Thanks for the review. Please see inline. On Mon, Dec 20, 2021 at 11:42 AM Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx>"); > > -- > > 2.34.1.307.g9b7440fafd-goog > > > > Thanks. > > -- > Dmitry
