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 <[email protected]>
---
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
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * chromeos_priv_scrn.c - ChromeOS Privacy Screen support
+ *
+ * Copyright (C) 2022 The Chromium OS Authors
+ *
+ */
+
+#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;
+
+ if (!priv_scrn)
+ return;
+
+ 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");
+ return;
+ }
+
+ if (obj->type != ACPI_TYPE_INTEGER)
+ dev_err(priv_scrn, "Bad _DSM to get privacy-screen state\n");
+ 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;
+
+ 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;
+ }
+
+ if (!obj) {
+ dev_err(priv_scrn, "_DSM failed to set privacy-screen state\n");
+ 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));
+
+ 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,
+ },
+};
+
+module_acpi_driver(chromeos_priv_scrn_driver);
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("ChromeOS ACPI Privacy Screen driver");
+MODULE_AUTHOR("Rajat Jain <[email protected]>");
--
2.34.1.307.g9b7440fafd-goog
Add a static entry in the x86 table, to detect and wait for
privacy-screen on some ChromeOS platforms.
Please note that this means that if CONFIG_CHROMEOS_PRIVACY_SCREEN is
enabled, and if "GOOG0010" device is found in ACPI, then the i915 probe
shall return EPROBE_DEFER until a platform driver actually registers the
privacy-screen: https://hansdegoede.livejournal.com/25948.html
Signed-off-by: Rajat Jain <[email protected]>
---
v2: * Use #if instead of #elif
* Reorder the patches in the series.
* Rebased on drm-tip
drivers/gpu/drm/drm_privacy_screen_x86.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
index a2cafb294ca6..0c5699ad70a3 100644
--- a/drivers/gpu/drm/drm_privacy_screen_x86.c
+++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
@@ -47,6 +47,18 @@ static bool __init detect_thinkpad_privacy_screen(void)
}
#endif
+#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
+static bool __init detect_chromeos_privacy_screen(void)
+{
+ if (!acpi_dev_present("GOOG0010", NULL, -1))
+ return false;
+
+ pr_info("%s: Need to wait for ChromeOS privacy-screen drvr", __func__);
+ return true;
+
+}
+#endif
+
static const struct arch_init_data arch_init_data[] __initconst = {
#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
{
@@ -58,6 +70,16 @@ static const struct arch_init_data arch_init_data[] __initconst = {
.detect = detect_thinkpad_privacy_screen,
},
#endif
+#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
+ {
+ .lookup = {
+ .dev_id = NULL,
+ .con_id = NULL,
+ .provider = "privacy_screen-GOOG0010:00",
+ },
+ .detect = detect_chromeos_privacy_screen,
+ },
+#endif
};
void __init drm_privacy_screen_lookup_init(void)
--
2.34.1.307.g9b7440fafd-goog
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 <[email protected]>
> ---
> 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?
> @@ -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 <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.
> +
> + if (!priv_scrn)
> + return;
This should not happen regardless.
> +
> + 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?
> + return;
> + }
> +
> + if (obj->type != ACPI_TYPE_INTEGER)
> + dev_err(priv_scrn, "Bad _DSM to get privacy-screen state\n");
Same here.
> + 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.
> +
> + 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?
> + }
> +
> + if (!obj) {
> + dev_err(priv_scrn, "_DSM failed to set privacy-screen state\n");
Can we print ACPI-specific error?
> + 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.
> +
> + 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?
> + },
> +};
> +
> +module_acpi_driver(chromeos_priv_scrn_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("ChromeOS ACPI Privacy Screen driver");
> +MODULE_AUTHOR("Rajat Jain <[email protected]>");
> --
> 2.34.1.307.g9b7440fafd-goog
>
Thanks.
--
Dmitry
On Fri, Dec 17, 2021 at 12:28:50PM -0800, Rajat Jain wrote:
> Add a static entry in the x86 table, to detect and wait for
> privacy-screen on some ChromeOS platforms.
>
> Please note that this means that if CONFIG_CHROMEOS_PRIVACY_SCREEN is
> enabled, and if "GOOG0010" device is found in ACPI, then the i915 probe
> shall return EPROBE_DEFER until a platform driver actually registers the
> privacy-screen: https://hansdegoede.livejournal.com/25948.html
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v2: * Use #if instead of #elif
> * Reorder the patches in the series.
> * Rebased on drm-tip
>
> drivers/gpu/drm/drm_privacy_screen_x86.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> index a2cafb294ca6..0c5699ad70a3 100644
> --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> @@ -47,6 +47,18 @@ static bool __init detect_thinkpad_privacy_screen(void)
> }
> #endif
>
> +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> +static bool __init detect_chromeos_privacy_screen(void)
Does marking this __init work in case there is a deferral? Can it happen
that privacy screen is a module and so will get loaded only after we
discarded __init sections.
> +{
> + if (!acpi_dev_present("GOOG0010", NULL, -1))
> + return false;
> +
> + pr_info("%s: Need to wait for ChromeOS privacy-screen drvr", __func__);
I still do not see how this message is helpful. If it is really desired,
I'd put something into the code that calls into lookups.
> + return true;
> +
> +}
> +#endif
> +
> static const struct arch_init_data arch_init_data[] __initconst = {
> #if IS_ENABLED(CONFIG_THINKPAD_ACPI)
> {
> @@ -58,6 +70,16 @@ static const struct arch_init_data arch_init_data[] __initconst = {
> .detect = detect_thinkpad_privacy_screen,
> },
> #endif
> +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> + {
> + .lookup = {
> + .dev_id = NULL,
> + .con_id = NULL,
> + .provider = "privacy_screen-GOOG0010:00",
> + },
> + .detect = detect_chromeos_privacy_screen,
> + },
> +#endif
> };
>
> void __init drm_privacy_screen_lookup_init(void)
> --
> 2.34.1.307.g9b7440fafd-goog
>
Thanks.
--
Dmitry
Hi Dmitry,
Thanks for the review. Please see inline.
On Mon, Dec 20, 2021 at 11:42 AM Dmitry Torokhov
<[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>");
> > --
> > 2.34.1.307.g9b7440fafd-goog
> >
>
> Thanks.
>
> --
> Dmitry
Hello,
On Mon, Dec 20, 2021 at 11:50 AM Dmitry Torokhov
<[email protected]> wrote:
>
> On Fri, Dec 17, 2021 at 12:28:50PM -0800, Rajat Jain wrote:
> > Add a static entry in the x86 table, to detect and wait for
> > privacy-screen on some ChromeOS platforms.
> >
> > Please note that this means that if CONFIG_CHROMEOS_PRIVACY_SCREEN is
> > enabled, and if "GOOG0010" device is found in ACPI, then the i915 probe
> > shall return EPROBE_DEFER until a platform driver actually registers the
> > privacy-screen: https://hansdegoede.livejournal.com/25948.html
> >
> > Signed-off-by: Rajat Jain <[email protected]>
> > ---
> > v2: * Use #if instead of #elif
> > * Reorder the patches in the series.
> > * Rebased on drm-tip
> >
> > drivers/gpu/drm/drm_privacy_screen_x86.c | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > index a2cafb294ca6..0c5699ad70a3 100644
> > --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> > +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > @@ -47,6 +47,18 @@ static bool __init detect_thinkpad_privacy_screen(void)
> > }
> > #endif
> >
> > +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> > +static bool __init detect_chromeos_privacy_screen(void)
>
> Does marking this __init work in case there is a deferral?
Yes, I have verified that for Chromeos case, it is a deferral.
> Can it happen
> that privacy screen is a module and so will get loaded only after we
> discarded __init sections.
Perhaps. But I do not think that is a problem. All the functions and
data in this file are in __init sections, and this entry is here to
ensure that the drm probe will wait for the privacy screen driver
(whenever it is loaded). That is the reason, ideally we would want to
somehow restrict the privacy screen to be built into the kernel so as
to minimize the delay if any.
>
> > +{
> > + if (!acpi_dev_present("GOOG0010", NULL, -1))
> > + return false;
> > +
> > + pr_info("%s: Need to wait for ChromeOS privacy-screen drvr", __func__);
>
> I still do not see how this message is helpful. If it is really desired,
> I'd put something into the code that calls into lookups.
>
Ack. This message highlights that the kernel indeed found privacy
screen device in the ACPI and the drm probe will need to wait for it.
I think I agree that this message is not needed here, I think I'll
tweak the existing message in drm_privacy_screen_lookup_init().
> > + return true;
> > +
> > +}
> > +#endif
> > +
> > static const struct arch_init_data arch_init_data[] __initconst = {
> > #if IS_ENABLED(CONFIG_THINKPAD_ACPI)
> > {
> > @@ -58,6 +70,16 @@ static const struct arch_init_data arch_init_data[] __initconst = {
> > .detect = detect_thinkpad_privacy_screen,
> > },
> > #endif
> > +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> > + {
> > + .lookup = {
> > + .dev_id = NULL,
> > + .con_id = NULL,
> > + .provider = "privacy_screen-GOOG0010:00",
> > + },
> > + .detect = detect_chromeos_privacy_screen,
> > + },
> > +#endif
> > };
> >
> > void __init drm_privacy_screen_lookup_init(void)
> > --
> > 2.34.1.307.g9b7440fafd-goog
> >
>
> Thanks.
Thanks & Best Regards,
Rajat
>
> --
> Dmitry
On Mon, Dec 20, 2021 at 12:29:18PM -0800, Rajat Jain wrote:
> Hello,
>
> On Mon, Dec 20, 2021 at 11:50 AM Dmitry Torokhov
> <[email protected]> wrote:
> >
> > On Fri, Dec 17, 2021 at 12:28:50PM -0800, Rajat Jain wrote:
> > > Add a static entry in the x86 table, to detect and wait for
> > > privacy-screen on some ChromeOS platforms.
> > >
> > > Please note that this means that if CONFIG_CHROMEOS_PRIVACY_SCREEN is
> > > enabled, and if "GOOG0010" device is found in ACPI, then the i915 probe
> > > shall return EPROBE_DEFER until a platform driver actually registers the
> > > privacy-screen: https://hansdegoede.livejournal.com/25948.html
> > >
> > > Signed-off-by: Rajat Jain <[email protected]>
> > > ---
> > > v2: * Use #if instead of #elif
> > > * Reorder the patches in the series.
> > > * Rebased on drm-tip
> > >
> > > drivers/gpu/drm/drm_privacy_screen_x86.c | 22 ++++++++++++++++++++++
> > > 1 file changed, 22 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > > index a2cafb294ca6..0c5699ad70a3 100644
> > > --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> > > +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > > @@ -47,6 +47,18 @@ static bool __init detect_thinkpad_privacy_screen(void)
> > > }
> > > #endif
> > >
> > > +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> > > +static bool __init detect_chromeos_privacy_screen(void)
> >
> > Does marking this __init work in case there is a deferral?
>
> Yes, I have verified that for Chromeos case, it is a deferral.
>
> > Can it happen
> > that privacy screen is a module and so will get loaded only after we
> > discarded __init sections.
>
> Perhaps. But I do not think that is a problem. All the functions and
> data in this file are in __init sections, and this entry is here to
> ensure that the drm probe will wait for the privacy screen driver
> (whenever it is loaded).
Ah, OK, we are not leaking detect() pointers outside this module.
> That is the reason, ideally we would want to
> somehow restrict the privacy screen to be built into the kernel so as
> to minimize the delay if any.
I understand, but we can not code to the config we expect to use on
Chrome OS, we need to make sure we cover all possibilities.
Thanks.
--
Dmitry
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
> <[email protected]> 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 <[email protected]>
> > > ---
> > > 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)
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 <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.
OK, I guess we need to chose a fine default here, probably assume
privacy screen is inoperable/disabled.
>
> > > + 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.
I'd force the status to disabled here as well.
>
> >
> > > + 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..
I see that acpi_evaluate_dsm() already provides diagnostic, so you can
ignore me here.
>
> >
> > > + 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.
I am sure you can get boot timeline with debug initcalls option. Then
you will see when this module was initialized vs i915.
Thanks.
--
Dmitry
Hi Dmitry,
Thanks for your comments. I posted a v3 that addresses most of your
comments here:
https://patchwork.kernel.org/project/platform-driver-x86/patch/[email protected]/
Please also see inline below.
On Mon, Dec 20, 2021 at 2:08 PM Dmitry Torokhov
<[email protected]> wrote:
>
> 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
> > <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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?
Done in v3.
> > >
> > > And also see if maybe variables in the code are not that unseemly long
> > > even if not abbreviated?
Done in v3.
> >
> > 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 ;)
Done in v3.
>
> >
> > >
> > > > @@ -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.
Done in v3.
> >
> > >
> > > > + *
> > > > + * 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.
Done in v3.
> >
> > > > + *
> > > > + */
> > > > +
> > > > +#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.
Done in v3.
> >
> > > > +
> > > > + if (!priv_scrn)
> > > > + return;
> > >
> > > This should not happen regardless.
> > >
> >
> > Sure, ack, will remove.
Done in v3.
> >
> > > > +
> > > > + 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.
>
> OK, I guess we need to chose a fine default here, probably assume
> privacy screen is inoperable/disabled.
On error conditions (both for read and write) I'd prefer that we leave
the state unchanged. The reason being that this state, the associated
state machine, as well as the semantics (who is allowed to change the
value - kernel, userspace etc, and cache / reuse the value etc, and
under what conditions etc) were very long debated before a conclusion
and agreement was reached. A value of PRIVACY_SCREEN_DISABLED_LOCKED
might mean that the user space / kernel may not attempt to change it
again (assuming the presence of physical slider switch), and the
driver is expected to notify the kernel if that switch slider changes
again. I don't understand it very well, and Chromebooks don't have
such a slider switch, so I'm hesitant to change it, especially since
it provides no real advantage in our case, and perhaps may cause
confusion.
I'm OK to change it though if you are still not convinced.
>
> >
> > > > + 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.
>
> I'd force the status to disabled here as well.
>
> >
> > >
> > > > + 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.
Done in v3
> >
> > >
> > > > +
> > > > + 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.
Done in v3.
> >
> > >
> > > > + }
> > > > +
> > > > + 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..
>
> I see that acpi_evaluate_dsm() already provides diagnostic, so you can
> ignore me here.
>
> >
> > >
> > > > + 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.
>
> I am sure you can get boot timeline with debug initcalls option. Then
> you will see when this module was initialized vs i915.
It isn't about module initialization and / or ACPI driver registration
timeline, but rather about the timeline of chromeos_priv_scrn_add()
getting called. I did some experiments in the earlier version of my
patches where I was using subsys initcall I think, to register my
driver early on. I found that even though the ACPI driver is
registered early on (i.e. before drm probe), the
chromeos_priv_scrn_add() gets called much later (after drm probe has
already returned EPROBE_DEFER). So debugging with initcalls may not
help. I'm saying this because this probe deferral did cause me a lot
of confusion while I was working on this, and I feel such a print
would have saved time, rather than resorting to hand debugging.
Another point to consider is that this print helps monitor and bisect
vanila (golden eye) Chromium OS images for regressions - i.e. to
determine if the probe deferral delay (currently ~250ms) has regressed
beyond a certain time threshold (say >500ms and causes a noticeable
delay in the splash screen).
What do you think?
Thanks & Best Regards,
Rajat
>
> Thanks.
>
> --
> Dmitry