| Message ID | 20211005202322.700909-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add privacy-screen class and connector properties | expand |
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 >
Hi Rajat, On 11/17/21 15:13, Rajat Jain wrote: > 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? Yes, this is all here to deal with probe-ordering. On a platform where the link between connectors and device-tree is available in something like devicetree this all becomes much easier. The i915 code does a: privacy_screen = drm_privacy_screen_get(&pdev->dev, NULL); if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -EPROBE_DEFER) return true; Early on to determine if there is a privacy_screen device it needs to wait for. With devicetree you can just scan for connector fwnodes under pdev->dev.fwnode and if any of those declare a link to a privacy_screen check if the provider if that screen is ready and if not return -EPROBE_DEFER. Rather then hardcoding "eDP-1" as con_id as I suggested before I guess it would be good to have a generic: int drm_privacy_screen_providers_ready(struct device *dev); helper which always returns either 0 or -EPROBE_DEFER. For devicetree this could check all connector fwnodes and for non-devicetree fallsback to the current: privacy_screen = drm_privacy_screen_get(&pdev->dev, NULL); if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -EPROBE_DEFER) return -EPROBE_DEFER; code. Then there is no need to hardcode "eDP-1" here. This way you won't even have to ensure that the privacy_screen gets registered first, instead you are correctly having the i915 probe defer until the privacy_screen(s) get registered. Either way you definitely should NOT need to add entries to this static table on a devicetree based platform. I hope this helps, if anything is not clear please keep asking questions. Regards, Hans
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 +}; + +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