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