Nouveau - May 2017 - [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

Since notification side has been changed to always notify kernel listeners
using _LID returning value. Now listeners needn't invoke acpi_lid_open(),
it should use a spec suggested control method lid device usage model:
register lid notification and use the notified value instead, which is the
only way to ensure the value is correct for
"button.lid_init_state=ignore"
mode or other modes with "button.lid_fake_events=N" specified.

This patch fixes i915 driver to drop acpi_lid_open() user. It's not
possible to change nouveau_connector.c user using a simple way now. So this
patch only marks acpi_lid_open() as deprecated to accelerate this process
by indicating this change to the nouveau developers.

Cc: <intel-gfx at lists.freedesktop.org>
Cc: <nouveau at lists.freedesktop.org>
Signed-off-by: Lv Zheng <lv.zheng at intel.com>
---
 drivers/acpi/button.c             | 7 ++++++-
 drivers/gpu/drm/i915/intel_lvds.c | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 7ff3a75..50d7898 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -348,7 +348,12 @@ int acpi_lid_notifier_unregister(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(acpi_lid_notifier_unregister);
 
-int acpi_lid_open(void)
+/*
+ * The intentional usage model is to register a lid notifier and use the
+ * notified value instead. Directly evaluating _LID without seeing a
+ * Notify(lid, 0x80) is not reliable.
+ */
+int __deprecated acpi_lid_open(void)
 {
 	if (!lid_device)
 		return -ENODEV;
diff --git a/drivers/gpu/drm/i915/intel_lvds.c
b/drivers/gpu/drm/i915/intel_lvds.c
index 9ca4dc4..8ca9080 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block *nb,
unsigned long val,
 	/* Don't force modeset on machines where it causes a GPU lockup */
 	if (dmi_check_system(intel_no_modeset_on_lid))
 		goto exit;
-	if (!acpi_lid_open()) {
+	if (!val) {
 		/* do modeset on next lid open event */
 		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
 		goto exit;
-- 
2.7.4

On Tue, 09 May 2017, Lv Zheng <lv.zheng at intel.com>
wrote:
> Since notification side has been changed to always notify kernel listeners
> using _LID returning value. Now listeners needn't invoke
acpi_lid_open(),
> it should use a spec suggested control method lid device usage model:
> register lid notification and use the notified value instead, which is the
> only way to ensure the value is correct for
"button.lid_init_state=ignore"
> mode or other modes with "button.lid_fake_events=N" specified.
>
> This patch fixes i915 driver to drop acpi_lid_open() user. It's not
> possible to change nouveau_connector.c user using a simple way now. So this
> patch only marks acpi_lid_open() as deprecated to accelerate this process
> by indicating this change to the nouveau developers.
Where's the rest of the series? Please send it all to intel-gfx so our
CI can crunch through it. And I could take a peek too.

This seems relevant:

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100923


BR,
Jani.
>
> Cc: <intel-gfx at lists.freedesktop.org>
> Cc: <nouveau at lists.freedesktop.org>
> Signed-off-by: Lv Zheng <lv.zheng at intel.com>
> ---
>  drivers/acpi/button.c             | 7 ++++++-
>  drivers/gpu/drm/i915/intel_lvds.c | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 7ff3a75..50d7898 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -348,7 +348,12 @@ int acpi_lid_notifier_unregister(struct notifier_block
*nb)
>  }
>  EXPORT_SYMBOL(acpi_lid_notifier_unregister);
>  
> -int acpi_lid_open(void)
> +/*
> + * The intentional usage model is to register a lid notifier and use the
> + * notified value instead. Directly evaluating _LID without seeing a
> + * Notify(lid, 0x80) is not reliable.
> + */
> +int __deprecated acpi_lid_open(void)
>  {
>  	if (!lid_device)
>  		return -ENODEV;
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c
b/drivers/gpu/drm/i915/intel_lvds.c
> index 9ca4dc4..8ca9080 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block *nb,
unsigned long val,
>  	/* Don't force modeset on machines where it causes a GPU lockup */
>  	if (dmi_check_system(intel_no_modeset_on_lid))
>  		goto exit;
> -	if (!acpi_lid_open()) {
> +	if (!val) {
>  		/* do modeset on next lid open event */
>  		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
>  		goto exit;
-- 
Jani Nikula, Intel Open Source Technology Center

On Tue, May 9, 2017 at 9:02 AM, Lv Zheng <lv.zheng at intel.com>
wrote:
> Since notification side has been changed to always notify kernel listeners
> using _LID returning value. Now listeners needn't invoke
acpi_lid_open(),
> it should use a spec suggested control method lid device usage model:
> register lid notification and use the notified value instead, which is the
> only way to ensure the value is correct for
"button.lid_init_state=ignore"
> mode or other modes with "button.lid_fake_events=N" specified.
>
> This patch fixes i915 driver to drop acpi_lid_open() user. It's not
> possible to change nouveau_connector.c user using a simple way now. So this
> patch only marks acpi_lid_open() as deprecated to accelerate this process
> by indicating this change to the nouveau developers.
If  the only 2 users of acpi_lid_open() are intel and nouveau, and if
they should rely on the value stored in the input node, not the one
exported by the _LID method (which can be wrong on some platforms),
how about simply changing the implementation of acpi_lid_open() to
fetch the value from the input node directly?

Cheers,
Benjamin
>
> Cc: <intel-gfx at lists.freedesktop.org>
> Cc: <nouveau at lists.freedesktop.org>
> Signed-off-by: Lv Zheng <lv.zheng at intel.com>
> ---
>  drivers/acpi/button.c             | 7 ++++++-
>  drivers/gpu/drm/i915/intel_lvds.c | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 7ff3a75..50d7898 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -348,7 +348,12 @@ int acpi_lid_notifier_unregister(struct notifier_block
*nb)
>  }
>  EXPORT_SYMBOL(acpi_lid_notifier_unregister);
>
> -int acpi_lid_open(void)
> +/*
> + * The intentional usage model is to register a lid notifier and use the
> + * notified value instead. Directly evaluating _LID without seeing a
> + * Notify(lid, 0x80) is not reliable.
> + */
> +int __deprecated acpi_lid_open(void)
>  {
>         if (!lid_device)
>                 return -ENODEV;
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c
b/drivers/gpu/drm/i915/intel_lvds.c
> index 9ca4dc4..8ca9080 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block *nb,
unsigned long val,
>         /* Don't force modeset on machines where it causes a GPU lockup
*/
>         if (dmi_check_system(intel_no_modeset_on_lid))
>                 goto exit;
> -       if (!acpi_lid_open()) {
> +       if (!val) {
>                 /* do modeset on next lid open event */
>                 dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
>                 goto exit;
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-acpi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi,
> From: Benjamin Tissoires [mailto:benjamin.tissoires at gmail.com]
> Subject: Re: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open()
invocations
> 
> On Tue, May 9, 2017 at 9:02 AM, Lv Zheng <lv.zheng at intel.com>
wrote:
> > Since notification side has been changed to always notify kernel
listeners
> > using _LID returning value. Now listeners needn't invoke
acpi_lid_open(),
> > it should use a spec suggested control method lid device usage model:
> > register lid notification and use the notified value instead, which is
the
> > only way to ensure the value is correct for
"button.lid_init_state=ignore"
> > mode or other modes with "button.lid_fake_events=N"
specified.
> >
> > This patch fixes i915 driver to drop acpi_lid_open() user. It's
not
> > possible to change nouveau_connector.c user using a simple way now. So
this
> > patch only marks acpi_lid_open() as deprecated to accelerate this
process
> > by indicating this change to the nouveau developers.
> 
> If  the only 2 users of acpi_lid_open() are intel and nouveau, and if
> they should rely on the value stored in the input node, not the one
> exported by the _LID method (which can be wrong on some platforms),
> how about simply changing the implementation of acpi_lid_open() to
> fetch the value from the input node directly?
See my reply of PATCH 4.
It seems they trust _LID return value more than what we send to them.

We can actually send faked "open/close" to them when
button.lid_init_state=open/close is specified.

So these 2 patches [PATCH 4-5/5] are used to do a small cleanup for lid event
notifier APIs.
I think they are not strictly related to the lid issues we are talking about.

Cheers,
Lv
> 
> Cheers,
> Benjamin
> 
> >
> > Cc: <intel-gfx at lists.freedesktop.org>
> > Cc: <nouveau at lists.freedesktop.org>
> > Signed-off-by: Lv Zheng <lv.zheng at intel.com>
> > ---
> >  drivers/acpi/button.c             | 7 ++++++-
> >  drivers/gpu/drm/i915/intel_lvds.c | 2 +-
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 7ff3a75..50d7898 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -348,7 +348,12 @@ int acpi_lid_notifier_unregister(struct
notifier_block *nb)
> >  }
> >  EXPORT_SYMBOL(acpi_lid_notifier_unregister);
> >
> > -int acpi_lid_open(void)
> > +/*
> > + * The intentional usage model is to register a lid notifier and use
the
> > + * notified value instead. Directly evaluating _LID without seeing a
> > + * Notify(lid, 0x80) is not reliable.
> > + */
> > +int __deprecated acpi_lid_open(void)
> >  {
> >         if (!lid_device)
> >                 return -ENODEV;
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c
b/drivers/gpu/drm/i915/intel_lvds.c
> > index 9ca4dc4..8ca9080 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block
*nb, unsigned long val,
> >         /* Don't force modeset on machines where it causes a GPU
lockup */
> >         if (dmi_check_system(intel_no_modeset_on_lid))
> >                 goto exit;
> > -       if (!acpi_lid_open()) {
> > +       if (!val) {
> >                 /* do modeset on next lid open event */
> >                 dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> >                 goto exit;
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
linux-acpi" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html