Nouveau - May 2017 - [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?

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,

If my previous reply is not persuasive enough.
Let me do that in a different way.
> From: linux-acpi-owner at vger.kernel.org [mailto:linux-acpi-owner at
vger.kernel.org] On Behalf Of Zheng,
> Lv
> Subject: RE: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open()
invocations
> 
> 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?
If acpi_lid_open() returns stored value in input node,
then it actually returns the value we notified to the input layer.
While i915 and nouveau explicitly do not trust that value and invoke
acpi_lid_open().

For button.lid_init_state=method, PATCH 4/5 is useless as the values are same.

However in my reply of PATCH 2.
I explained the difference of method/close, for the same reason, we can also
sense the difference of method/open.
The difference then can explain the usefulness of open/close modes.

Given open/close modes are all useful:
button.lid_init_state=open
1. button driver sends open to input layer after boot/resume
2. i915/nouveau uses _LID return value after boot/resume
If _LID return value after boot/resume is "close", they are different.

button.lid_init_state=close
1. button driver sends close to input layer after boot/resume
2. i915/nouveau uses _LID return value after boot/resume
If _LID return value after boot/resume is "open", they are different.

The only way to be consistent to old i915/nouveau behavior is:
button.lid_init_state=open/close
1. button driver sends open/close to input layer after boot/resume
2. button driver sends _LID return value to i915 after boot/resume (PATCH 4)
So that i915 can just use the notified value in this patch (PATCH 5).
For nouveau, no change has been made for now, and as a concequence,
acpi_lid_open() is still kept but marked as deprecated.
> 
> 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.
See Documentation/acpi/acpi-lid.txt:
The _LID control method is described to return the "current" lid
state.
However the word of "current" has ambiguity, some buggy AML tables
return
the lid state upon the last lid notification instead of returning the lid
state upon the last _LID evaluation.

In order to have acpi lid event notifier API compliant to the above mentioned
both "buggy" and "nonbuggy" implementation.
The ensured lid event model interface should be:
1. Lid event notifier listeners invokes acpi_lid_notifier_register().
2. And in the callback, uses _LID return value.
This is also the only possible way to combine "receiving lid notify"
and "evaluate _LID" into 1 single atomic operation.

So I trend to remove acpi_lid_open() and enforce the new API.

As a conclusion, PATCH 4-5
1. makes a hidden logic explicit - the lid event listeners always use _LID
return value. Less hidden logics should leave less chances of bugs.
2. is an implementation for our documented ACPI lid event model.
And the implementation is done in a regression safe way.

Thanks,
Lv
> 
> 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
> &~&+-wmbZr^nrzh&Gh(
> j"mzfh~m