diff mbox series

[v2,2/2] drm/privacy_screen_x86: Add entry for ChromeOS privacy-screen

Message ID 20211217202850.1967594-2-rajatja@google.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/2] platform/chrome: Add driver for ChromeOS privacy-screen | expand

Commit Message

Rajat Jain Dec. 17, 2021, 8:28 p.m. UTC
Add a static entry in the x86 table, to detect and wait for
privacy-screen on some ChromeOS platforms.

Please note that this means that if CONFIG_CHROMEOS_PRIVACY_SCREEN is
enabled, and if "GOOG0010" device is found in ACPI, then the i915 probe
shall return EPROBE_DEFER until a platform driver actually registers the
privacy-screen: https://hansdegoede.livejournal.com/25948.html

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: * Use #if instead of #elif
    * Reorder the patches in the series.
    * Rebased on drm-tip

 drivers/gpu/drm/drm_privacy_screen_x86.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Dmitry Torokhov Dec. 20, 2021, 7:50 p.m. UTC | #1
On Fri, Dec 17, 2021 at 12:28:50PM -0800, Rajat Jain wrote:
> Add a static entry in the x86 table, to detect and wait for
> privacy-screen on some ChromeOS platforms.
> 
> Please note that this means that if CONFIG_CHROMEOS_PRIVACY_SCREEN is
> enabled, and if "GOOG0010" device is found in ACPI, then the i915 probe
> shall return EPROBE_DEFER until a platform driver actually registers the
> privacy-screen: https://hansdegoede.livejournal.com/25948.html
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v2: * Use #if instead of #elif
>     * Reorder the patches in the series.
>     * Rebased on drm-tip
> 
>  drivers/gpu/drm/drm_privacy_screen_x86.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> index a2cafb294ca6..0c5699ad70a3 100644
> --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> @@ -47,6 +47,18 @@ static bool __init detect_thinkpad_privacy_screen(void)
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> +static bool __init detect_chromeos_privacy_screen(void)

Does marking this __init work in case there is a deferral? Can it happen
that privacy screen is a module and so will get loaded only after we
discarded __init sections.

> +{
> +	if (!acpi_dev_present("GOOG0010", NULL, -1))
> +		return false;
> +
> +	pr_info("%s: Need to wait for ChromeOS privacy-screen drvr", __func__);

I still do not see how this message is helpful. If it is really desired,
I'd put something into the code that calls into lookups.

> +	return true;
> +
> +}
> +#endif
> +
>  static const struct arch_init_data arch_init_data[] __initconst = {
>  #if IS_ENABLED(CONFIG_THINKPAD_ACPI)
>  	{
> @@ -58,6 +70,16 @@ static const struct arch_init_data arch_init_data[] __initconst = {
>  		.detect = detect_thinkpad_privacy_screen,
>  	},
>  #endif
> +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> +	{
> +		.lookup = {
> +			.dev_id = NULL,
> +			.con_id = NULL,
> +			.provider = "privacy_screen-GOOG0010:00",
> +		},
> +		.detect = detect_chromeos_privacy_screen,
> +	},
> +#endif
>  };
>  
>  void __init drm_privacy_screen_lookup_init(void)
> -- 
> 2.34.1.307.g9b7440fafd-goog
> 

Thanks.
Rajat Jain Dec. 20, 2021, 8:29 p.m. UTC | #2
Hello,

On Mon, Dec 20, 2021 at 11:50 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Fri, Dec 17, 2021 at 12:28:50PM -0800, Rajat Jain wrote:
> > Add a static entry in the x86 table, to detect and wait for
> > privacy-screen on some ChromeOS platforms.
> >
> > Please note that this means that if CONFIG_CHROMEOS_PRIVACY_SCREEN is
> > enabled, and if "GOOG0010" device is found in ACPI, then the i915 probe
> > shall return EPROBE_DEFER until a platform driver actually registers the
> > privacy-screen: https://hansdegoede.livejournal.com/25948.html
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > v2: * Use #if instead of #elif
> >     * Reorder the patches in the series.
> >     * Rebased on drm-tip
> >
> >  drivers/gpu/drm/drm_privacy_screen_x86.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > index a2cafb294ca6..0c5699ad70a3 100644
> > --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> > +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > @@ -47,6 +47,18 @@ static bool __init detect_thinkpad_privacy_screen(void)
> >  }
> >  #endif
> >
> > +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> > +static bool __init detect_chromeos_privacy_screen(void)
>
> Does marking this __init work in case there is a deferral?

Yes, I have verified that for Chromeos case, it is a deferral.

> Can it happen
> that privacy screen is a module and so will get loaded only after we
> discarded __init sections.

Perhaps. But I do not think that  is a problem. All the functions and
data in this file are in __init sections, and this entry is here to
ensure that the drm probe will wait for the privacy screen driver
(whenever it is loaded). That is the reason, ideally we  would want to
somehow restrict the privacy screen to be built into the kernel so as
to minimize the delay if any.


>
> > +{
> > +     if (!acpi_dev_present("GOOG0010", NULL, -1))
> > +             return false;
> > +
> > +     pr_info("%s: Need to wait for ChromeOS privacy-screen drvr", __func__);
>
> I still do not see how this message is helpful. If it is really desired,
> I'd put something into the code that calls into lookups.
>

Ack. This message highlights that the kernel indeed found privacy
screen device in the ACPI and the drm probe will need to wait for it.
I think I agree that this message is not needed here, I think I'll
tweak the existing message in drm_privacy_screen_lookup_init().

> > +     return true;
> > +
> > +}
> > +#endif
> > +
> >  static const struct arch_init_data arch_init_data[] __initconst = {
> >  #if IS_ENABLED(CONFIG_THINKPAD_ACPI)
> >       {
> > @@ -58,6 +70,16 @@ static const struct arch_init_data arch_init_data[] __initconst = {
> >               .detect = detect_thinkpad_privacy_screen,
> >       },
> >  #endif
> > +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> > +     {
> > +             .lookup = {
> > +                     .dev_id = NULL,
> > +                     .con_id = NULL,
> > +                     .provider = "privacy_screen-GOOG0010:00",
> > +             },
> > +             .detect = detect_chromeos_privacy_screen,
> > +     },
> > +#endif
> >  };
> >
> >  void __init drm_privacy_screen_lookup_init(void)
> > --
> > 2.34.1.307.g9b7440fafd-goog
> >
>
> Thanks.

Thanks & Best Regards,

Rajat

>
> --
> Dmitry
Dmitry Torokhov Dec. 20, 2021, 10 p.m. UTC | #3
On Mon, Dec 20, 2021 at 12:29:18PM -0800, Rajat Jain wrote:
> Hello,
> 
> On Mon, Dec 20, 2021 at 11:50 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Fri, Dec 17, 2021 at 12:28:50PM -0800, Rajat Jain wrote:
> > > Add a static entry in the x86 table, to detect and wait for
> > > privacy-screen on some ChromeOS platforms.
> > >
> > > Please note that this means that if CONFIG_CHROMEOS_PRIVACY_SCREEN is
> > > enabled, and if "GOOG0010" device is found in ACPI, then the i915 probe
> > > shall return EPROBE_DEFER until a platform driver actually registers the
> > > privacy-screen: https://hansdegoede.livejournal.com/25948.html
> > >
> > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > ---
> > > v2: * Use #if instead of #elif
> > >     * Reorder the patches in the series.
> > >     * Rebased on drm-tip
> > >
> > >  drivers/gpu/drm/drm_privacy_screen_x86.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > > index a2cafb294ca6..0c5699ad70a3 100644
> > > --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> > > +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> > > @@ -47,6 +47,18 @@ static bool __init detect_thinkpad_privacy_screen(void)
> > >  }
> > >  #endif
> > >
> > > +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> > > +static bool __init detect_chromeos_privacy_screen(void)
> >
> > Does marking this __init work in case there is a deferral?
> 
> Yes, I have verified that for Chromeos case, it is a deferral.
> 
> > Can it happen
> > that privacy screen is a module and so will get loaded only after we
> > discarded __init sections.
> 
> Perhaps. But I do not think that  is a problem. All the functions and
> data in this file are in __init sections, and this entry is here to
> ensure that the drm probe will wait for the privacy screen driver
> (whenever it is loaded).

Ah, OK, we are not leaking detect() pointers outside this module. 

> That is the reason, ideally we  would want to
> somehow restrict the privacy screen to be built into the kernel so as
> to minimize the delay if any.

I understand, but we can not code to the config we expect to use on
Chrome OS, we need to make sure we cover all possibilities.

Thanks.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c b/drivers/gpu/drm/drm_privacy_screen_x86.c
index a2cafb294ca6..0c5699ad70a3 100644
--- a/drivers/gpu/drm/drm_privacy_screen_x86.c
+++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
@@ -47,6 +47,18 @@  static bool __init detect_thinkpad_privacy_screen(void)
 }
 #endif
 
+#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
+static bool __init detect_chromeos_privacy_screen(void)
+{
+	if (!acpi_dev_present("GOOG0010", NULL, -1))
+		return false;
+
+	pr_info("%s: Need to wait for ChromeOS privacy-screen drvr", __func__);
+	return true;
+
+}
+#endif
+
 static const struct arch_init_data arch_init_data[] __initconst = {
 #if IS_ENABLED(CONFIG_THINKPAD_ACPI)
 	{
@@ -58,6 +70,16 @@  static const struct arch_init_data arch_init_data[] __initconst = {
 		.detect = detect_thinkpad_privacy_screen,
 	},
 #endif
+#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
+	{
+		.lookup = {
+			.dev_id = NULL,
+			.con_id = NULL,
+			.provider = "privacy_screen-GOOG0010:00",
+		},
+		.detect = detect_chromeos_privacy_screen,
+	},
+#endif
 };
 
 void __init drm_privacy_screen_lookup_init(void)