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

[Hans de Goede]

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 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?

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