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

[Rajat Jain]

Hello Hans,

On Tue, Oct 5, 2021 at 1:23 PM Hans de Goede <hdegoede at 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 at gmail.com>
> Reviewed-by: Lyude Paul <lyude at redhat.com>
> Signed-off-by: Hans de Goede <hdegoede at 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 at 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
>