Re: [PATCH 03/10] drm/privacy-screen: Add X86 specific arch init code

Hello Hans,

On Tue, Oct 5, 2021 at 1:23 PM Hans de Goede hdegoede@redhat.com wrote:

Add X86 specific arch init code, which fills the privacy-screen lookup table by checking for various vendor specific ACPI interfaces for controlling the privacy-screen.

This initial version only checks for the Lenovo Thinkpad specific ACPI methods for privacy-screen control.

Reviewed-by: Emil Velikov emil.l.velikov@gmail.com Reviewed-by: Lyude Paul lyude@redhat.com Signed-off-by: Hans de Goede hdegoede@redhat.com

---

drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/drm_privacy_screen_x86.c | 86 ++++++++++++++++++++++++ include/drm/drm_privacy_screen_machine.h | 5 ++ 3 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_privacy_screen_x86.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 788fc37096f6..12997ca5670d 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -32,7 +32,7 @@ drm-$(CONFIG_OF) += drm_of.o drm-$(CONFIG_PCI) += drm_pci.o drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o -drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o +drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o drm_privacy_screen_x86.o

obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o

diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c new file mode 100644 index 000000000000..a2cafb294ca6 --- /dev/null +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: MIT +/*

• • Copyright (C) 2020 Red Hat, Inc.
• • Authors:
• • Hans de Goede hdegoede@redhat.com
• */

+#include <linux/acpi.h> +#include <drm/drm_privacy_screen_machine.h>

+#ifdef CONFIG_X86 +static struct drm_privacy_screen_lookup arch_lookup;

+struct arch_init_data {

•   struct drm_privacy_screen_lookup lookup;
  

•   bool (*detect)(void);
  

+};

+#if IS_ENABLED(CONFIG_THINKPAD_ACPI) +static acpi_status __init acpi_set_handle(acpi_handle handle, u32 level,

•                                     void *context, void **return_value)
  

+{

•   *(acpi_handle *)return_value = handle;
  

•   return AE_CTRL_TERMINATE;
  

+}

+static bool __init detect_thinkpad_privacy_screen(void) +{

•   union acpi_object obj = { .type = ACPI_TYPE_INTEGER };
  

•   struct acpi_object_list args = { .count = 1, .pointer = &obj, };
  

•   acpi_handle ec_handle = NULL;
  

•   unsigned long long output;
  

•   acpi_status status;
  

•   /* Get embedded-controller handle */
  

•   status = acpi_get_devices("PNP0C09", acpi_set_handle, NULL, &ec_handle);
  

•   if (ACPI_FAILURE(status) || !ec_handle)
  

•           return false;
  

•   /* And call the privacy-screen get-status method */
  

•   status = acpi_evaluate_integer(ec_handle, "HKEY.GSSS", &args, &output);
  

•   if (ACPI_FAILURE(status))
  

•           return false;
  

•   return (output & 0x10000) ? true : false;
  

+} +#endif

+static const struct arch_init_data arch_init_data[] __initconst = { +#if IS_ENABLED(CONFIG_THINKPAD_ACPI)

•   {
  

•           .lookup = {
  

•                   .dev_id = NULL,
  

•                   .con_id = NULL,
  

•                   .provider = "privacy_screen-thinkpad_acpi",
  

•           },
  

•           .detect = detect_thinkpad_privacy_screen,
  

•   },
  

+#endif +};

As I'm trying to add privacy-screen support for my platform, I'm trying to understand if my platform needs to make an entry in this static list.

Do I understand it right that the reason you needed this static list (and this whole file really), instead of just doing a drm_privacy_screen_lookup_add() in the platform code in thinkpad_acpi.c, was because that code was executed AFTER the drm_connectors had already initialized?

In other words, the privacy-screen providers (platform code) need to register a privacy-screen and a lookup structure, BEFORE the drm connectors are initialized. If the platform code that provides a privacy-screen is executed AFTER the drm-connector initializes, then we need an entry in this static list, so that the drm probe (for i915 atleast) is DEFERRED until the privacy-screen provider registers the privacy-screen?

OTOH, if the platform can register a privacy-screen and a lookup function (via drm_privacy_screen_lookup_add()) BEFORE drm probe, then I do not need an entry in this static list.

Is this correct understanding?

Thanks & Best Regards,

Rajat

+void __init drm_privacy_screen_lookup_init(void) +{

•   int i;
  

•   for (i = 0; i < ARRAY_SIZE(arch_init_data); i++) {
  

•           if (!arch_init_data[i].detect())
  

•                   continue;
  

•           pr_info("Found '%s' privacy-screen provider\n",
  

•                   arch_init_data[i].lookup.provider);
  

•           /* Make a copy because arch_init_data is __initconst */
  

•           arch_lookup = arch_init_data[i].lookup;
  

•           drm_privacy_screen_lookup_add(&arch_lookup);
  

•           break;
  

•   }
  

+}

+void drm_privacy_screen_lookup_exit(void) +{

•   if (arch_lookup.provider)
  

•           drm_privacy_screen_lookup_remove(&arch_lookup);
  

+} +#endif /* ifdef CONFIG_X86 */ diff --git a/include/drm/drm_privacy_screen_machine.h b/include/drm/drm_privacy_screen_machine.h index aaa0d38cce92..02e5371904d3 100644 --- a/include/drm/drm_privacy_screen_machine.h +++ b/include/drm/drm_privacy_screen_machine.h @@ -31,11 +31,16 @@ struct drm_privacy_screen_lookup { void drm_privacy_screen_lookup_add(struct drm_privacy_screen_lookup *lookup); void drm_privacy_screen_lookup_remove(struct drm_privacy_screen_lookup *lookup);

+#if IS_ENABLED(CONFIG_DRM_PRIVACY_SCREEN) && IS_ENABLED(CONFIG_X86) +void drm_privacy_screen_lookup_init(void); +void drm_privacy_screen_lookup_exit(void); +#else static inline void drm_privacy_screen_lookup_init(void) { } static inline void drm_privacy_screen_lookup_exit(void) { } +#endif

#endif

2.31.1