[PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Lv Zheng]

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@lists.freedesktop.org>
Cc: <nouveau@lists.freedesktop.org>
Signed-off-by: Lv Zheng <lv.zheng@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

Re: [Intel-gfx] [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Jani Nikula]

On Tue, 09 May 2017, Lv Zheng <lv.zheng@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@lists.freedesktop.org>
> Cc: <nouveau@lists.freedesktop.org>
> Signed-off-by: Lv Zheng <lv.zheng@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

Re: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Benjamin Tissoires]

On Tue, May 9, 2017 at 9:02 AM, Lv Zheng <lv.zheng@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@lists.freedesktop.org>
> Cc: <nouveau@lists.freedesktop.org>
> Signed-off-by: Lv Zheng <lv.zheng@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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Zheng, Lv]

Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoires@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@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@lists.freedesktop.org>
> > Cc: <nouveau@lists.freedesktop.org>
> > Signed-off-by: Lv Zheng <lv.zheng@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@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Zheng, Lv]

Hi,

If my previous reply is not persuasive enough.
Let me do that in a different way.

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@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@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@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@lists.freedesktop.org>
> > > Cc: <nouveau@lists.freedesktop.org>
> > > Signed-off-by: Lv Zheng <lv.zheng@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@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> \x13��칻\x1c�&�~�&�\x18��+-��ݶ\x17��w��˛���m�b��Zr����^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階
> �ݢj"��\x1a�^[m�����z�ޖ���f���h���~�m�

Re: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Benjamin Tissoires]

Hi Lv,

I am trying to reduce the number of parallel discussion we have on the
same subject, but there is something here I can't let you have.

On Fri, May 12, 2017 at 8:08 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi,
>
> If my previous reply is not persuasive enough.
> Let me do that in a different way.
>
>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@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@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@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

But these two modes are wrong in the absolute case:
- a laptop has no reasons for not being powered up with the lid
physically closed (wake on lan?) -> so open is an approximation
already made on good assumption that there is not a high chance of the
users powering/resuming the laptop with the lid closed
- in the "close" case, this setting works for docked laptops, but if
the laptop can be docked, it can also be undocked. And if you boot
with button.lid_init_state=close, undock your laptop, go into suspend,
resume -> the lid state is now "closed" while it should be opened.

So no, these are just workarounds. i915/nouveau expect the acpi/button
state to be reliable, or they should ignore it. But you can't fake
events when you are not absolutely sure of what is happening.

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

I understand all of that. But my point is still that on some
platforms, the lid acpi state is not reliable, and the code in
i915/nouveau is not made for those platforms. So the ideal solution is
to know which platforms are problematic and take the right decisions
having everything at hands. Just "fixing" the internal API won't help
teaching these drivers to not make the assumption that the _LID acpi
call is always correct.

So yes, nouveau/i915 might need to be fixed, but IMO, fixing
acpi/button to report correct (true, accurate, actual, or physical if
you prefer) is the best, future proof way (minus platform quirks).

Cheers,
Benjamin

>
> Thanks,
> Lv
>
>>
>> Cheers,
>> Lv
>>
>> >
>> > Cheers,
>> > Benjamin
>> >
>> > >
>> > > Cc: <intel-gfx@lists.freedesktop.org>
>> > > Cc: <nouveau@lists.freedesktop.org>
>> > > Signed-off-by: Lv Zheng <lv.zheng@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@vger.kernel.org
>> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>  ��칻 �&�~�&� ��+-��ݶ ��w��˛���m�b��Zr����^n�r���z� ��h����&�� �G���h� (�階
>> �ݢj"�� � m�����z�ޖ���f���h���~�m�

RE: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Zheng, Lv]

Hi, Benjamin

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Benjamin
> Tissoires
> Subject: Re: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations
> 
> Hi Lv,
> 
> I am trying to reduce the number of parallel discussion we have on the
> same subject, but there is something here I can't let you have.

OK. So let's stop talking in PATCH 4-5.
They are actually cleanups from my point of view.

In fact, I don't see any big conflicts between us.

My point of view is:
There is a gap between (BIOS ensured/Windows expected) acpi control method lid device behavior and Linux user space expected acpi control method lid device behavior.
Button driver default behavior should be: button.lid_init_state=ignore
If user space programs have special needs, they can fix problems on their own, via the following mean:
 echo -n "open" > /sys/modules/button/parameters/lid_init_state
 echo -n "close" > /sys/modules/button/parameters/lid_init_state
Keeping open/close modes is because I don't think there is any bug in button driver.
So I need to prepare quirk modes from button driver's point of view and use them as a response to related bug reports reported in acpi community.
Your point of view is:
There is a gap between (BIOS ensured/Windows expected) acpi control method lid device behavior and Linux user space expected acpi control method lid device behavior.
Button driver default behavior should be (not 100% sure if this is your opinion): button.lid_init_state=method
If user space programs have special needs, they can fix them on their own, via the following mean:
 libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
From this point of view, we actually don't need open/close modes.

It seems we just need to determine the following first:
1. Who should be responsible for solving bugs triggered by the conflict between bios and linux user space expectations:
   button driver? libinput? Some other user space programs? Users?
2. What should be the default button driver behavior?
   button.lid_init_state=ignore? button.lid_init_state=method?
3. If non button driver quirks are working, button driver quirk modes are useless.
   The question is: Should button.lid_init_state=open/close be kept?
4. From button driver's point of view, button.lid_init_state=ignore seems to be always correct, so we won't abandon it.
   If we can use libinput to manage platform quirks, then button.lid_init_state=method also looks useless.
   The question is: Should button.lid_init_state=method be kept?

I should also let you know my preference:
1. using button.lid_init_state=ignore and button.lid_init_state=method as default behavior is ok for me if answer to 1 is not button driver, otherwise using button.lid_init_state=method is not ok for me
2. deletion of button.lid_init_state=open/close is ok for me if answer to 1 is not button driver, otherwise deletion of button.lid_init_state=open/close is not ok for me
3. deletion of button.lid_init_state=method is always ok for me

See the base line from my side is very clear:
If acpi community need to handle such bug reports, button.lid_init_state=method cannot be the default behavior.
We are just using a different default behavior than "method" to drive things to reach the final root caused solution.

Could you let me know your preference so that we can figure out an agreement between us.
Though I don't know if end users will buy it (they may keep on filing regression reports in ACPI community).

Can this make the discussion simpler?

So before determining whether we should keep button.lid_init_state=open/close or not.
We obviously should stop talking here.
You can copy above questions to PATCH 1-2 discussion and reply in order to stop this.
We can revisit PATCH 4-5 when the basic questions are answered. :)

> 
> On Fri, May 12, 2017 at 8:08 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> > Hi,
> >
> > If my previous reply is not persuasive enough.
> > Let me do that in a different way.
> >
> >> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@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@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@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
> 
> But these two modes are wrong in the absolute case:
> - a laptop has no reasons for not being powered up with the lid
> physically closed (wake on lan?) -> so open is an approximation
> already made on good assumption that there is not a high chance of the
> users powering/resuming the laptop with the lid closed
> - in the "close" case, this setting works for docked laptops, but if
> the laptop can be docked, it can also be undocked. And if you boot
> with button.lid_init_state=close, undock your laptop, go into suspend,
> resume -> the lid state is now "closed" while it should be opened.
> 
> So no, these are just workarounds. i915/nouveau expect the acpi/button
> state to be reliable, or they should ignore it. But you can't fake
> events when you are not absolutely sure of what is happening.
> 
> > 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.
> 
> I understand all of that. But my point is still that on some
> platforms, the lid acpi state is not reliable, and the code in
> i915/nouveau is not made for those platforms. So the ideal solution is
> to know which platforms are problematic and take the right decisions
> having everything at hands. Just "fixing" the internal API won't help
> teaching these drivers to not make the assumption that the _LID acpi
> call is always correct.
> 
> So yes, nouveau/i915 might need to be fixed, but IMO, fixing
> acpi/button to report correct (true, accurate, actual, or physical if
> you prefer) is the best, future proof way (minus platform quirks).

This leads to endless discussion.
Let's make agreement on above questions first.

Cheers,
Lv

> 
> Cheers,
> Benjamin
> 
> >
> > Thanks,
> > Lv
> >
> >>
> >> Cheers,
> >> Lv
> >>
> >> >
> >> > Cheers,
> >> > Benjamin
> >> >
> >> > >
> >> > > Cc: <intel-gfx@lists.freedesktop.org>
> >> > > Cc: <nouveau@lists.freedesktop.org>
> >> > > Signed-off-by: Lv Zheng <lv.zheng@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@vger.kernel.org
> >> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>  ��칻 �&�~�&� ��+-��ݶ ��w��˛���m�b��Zr����^n�r���z� ��h����&�� �G���h� (�階
> >> �ݢj"�� � m�����z�ޖ���f���h���~�m�
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Benjamin Tissoires]

Hi Lv,

On Mon, May 15, 2017 at 5:55 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi, Benjamin
>
>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Benjamin
>> Tissoires
>> Subject: Re: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations
>>
>> Hi Lv,
>>
>> I am trying to reduce the number of parallel discussion we have on the
>> same subject, but there is something here I can't let you have.
>
> OK. So let's stop talking in PATCH 4-5.
> They are actually cleanups from my point of view.
>
> In fact, I don't see any big conflicts between us.
>
> My point of view is:
> There is a gap between (BIOS ensured/Windows expected) acpi control method lid device behavior and Linux user space expected acpi control method lid device behavior.
> Button driver default behavior should be: button.lid_init_state=ignore
> If user space programs have special needs, they can fix problems on their own, via the following mean:
>  echo -n "open" > /sys/modules/button/parameters/lid_init_state
>  echo -n "close" > /sys/modules/button/parameters/lid_init_state
> Keeping open/close modes is because I don't think there is any bug in button driver.
> So I need to prepare quirk modes from button driver's point of view and use them as a response to related bug reports reported in acpi community.
> Your point of view is:
> There is a gap between (BIOS ensured/Windows expected) acpi control method lid device behavior and Linux user space expected acpi control method lid device behavior.
> Button driver default behavior should be (not 100% sure if this is your opinion): button.lid_init_state=method
> If user space programs have special needs, they can fix them on their own, via the following mean:
>  libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
>   LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> From this point of view, we actually don't need open/close modes.
>
> It seems we just need to determine the following first:
> 1. Who should be responsible for solving bugs triggered by the conflict between bios and linux user space expectations:
>    button driver? libinput? Some other user space programs? Users?
> 2. What should be the default button driver behavior?
>    button.lid_init_state=ignore? button.lid_init_state=method?
> 3. If non button driver quirks are working, button driver quirk modes are useless.
>    The question is: Should button.lid_init_state=open/close be kept?
> 4. From button driver's point of view, button.lid_init_state=ignore seems to be always correct, so we won't abandon it.
>    If we can use libinput to manage platform quirks, then button.lid_init_state=method also looks useless.
>    The question is: Should button.lid_init_state=method be kept?

I'll answer everything in the other thread, where there are slightly
more other points raised:
https://lkml.org/lkml/2017/5/15/10

>
> I should also let you know my preference:
> 1. using button.lid_init_state=ignore and button.lid_init_state=method as default behavior is ok for me if answer to 1 is not button driver, otherwise using button.lid_init_state=method is not ok for me
> 2. deletion of button.lid_init_state=open/close is ok for me if answer to 1 is not button driver, otherwise deletion of button.lid_init_state=open/close is not ok for me
> 3. deletion of button.lid_init_state=method is always ok for me
>
> See the base line from my side is very clear:
> If acpi community need to handle such bug reports, button.lid_init_state=method cannot be the default behavior.
> We are just using a different default behavior than "method" to drive things to reach the final root caused solution.
>
> Could you let me know your preference so that we can figure out an agreement between us.
> Though I don't know if end users will buy it (they may keep on filing regression reports in ACPI community).
>
> Can this make the discussion simpler?

I really hope so :)

Cheers,
Benjamin

>
> So before determining whether we should keep button.lid_init_state=open/close or not.
> We obviously should stop talking here.
> You can copy above questions to PATCH 1-2 discussion and reply in order to stop this.
> We can revisit PATCH 4-5 when the basic questions are answered. :)
>
>>
>> On Fri, May 12, 2017 at 8:08 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
>> > Hi,
>> >
>> > If my previous reply is not persuasive enough.
>> > Let me do that in a different way.
>> >
>> >> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@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@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@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
>>
>> But these two modes are wrong in the absolute case:
>> - a laptop has no reasons for not being powered up with the lid
>> physically closed (wake on lan?) -> so open is an approximation
>> already made on good assumption that there is not a high chance of the
>> users powering/resuming the laptop with the lid closed
>> - in the "close" case, this setting works for docked laptops, but if
>> the laptop can be docked, it can also be undocked. And if you boot
>> with button.lid_init_state=close, undock your laptop, go into suspend,
>> resume -> the lid state is now "closed" while it should be opened.
>>
>> So no, these are just workarounds. i915/nouveau expect the acpi/button
>> state to be reliable, or they should ignore it. But you can't fake
>> events when you are not absolutely sure of what is happening.
>>
>> > 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.
>>
>> I understand all of that. But my point is still that on some
>> platforms, the lid acpi state is not reliable, and the code in
>> i915/nouveau is not made for those platforms. So the ideal solution is
>> to know which platforms are problematic and take the right decisions
>> having everything at hands. Just "fixing" the internal API won't help
>> teaching these drivers to not make the assumption that the _LID acpi
>> call is always correct.
>>
>> So yes, nouveau/i915 might need to be fixed, but IMO, fixing
>> acpi/button to report correct (true, accurate, actual, or physical if
>> you prefer) is the best, future proof way (minus platform quirks).
>
> This leads to endless discussion.
> Let's make agreement on above questions first.
>
> Cheers,
> Lv
>
>>
>> Cheers,
>> Benjamin
>>
>> >
>> > Thanks,
>> > Lv
>> >
>> >>
>> >> Cheers,
>> >> Lv
>> >>
>> >> >
>> >> > Cheers,
>> >> > Benjamin
>> >> >
>> >> > >
>> >> > > Cc: <intel-gfx@lists.freedesktop.org>
>> >> > > Cc: <nouveau@lists.freedesktop.org>
>> >> > > Signed-off-by: Lv Zheng <lv.zheng@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@vger.kernel.org
>> >> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>  ��칻 �&�~�&� ��+-��ݶ ��w��˛���m�b��Zr����^n�r���z� ��h����&�� �G���h� (�階
>> >> �ݢj"�� � m�����z�ޖ���f���h���~�m�
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Jani Nikula]

On Mon, 15 May 2017, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote:
> I'll answer everything in the other thread, where there are slightly
> more other points raised: https://lkml.org/lkml/2017/5/15/10

If you are discussing changes impacting i915, please keep intel-gfx list
in the loop.

Thanks,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Benjamin Tissoires]

On Mon, May 15, 2017 at 10:42 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Mon, 15 May 2017, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote:
>> I'll answer everything in the other thread, where there are slightly
>> more other points raised: https://lkml.org/lkml/2017/5/15/10
>
> If you are discussing changes impacting i915, please keep intel-gfx list
> in the loop.
>

I can add intel-gfx to the other thread if you want, but this will IMO
just add more noise to your list.
The question is whether or not the kernel should provide a fake state
for the _LID acpi call, and until we reach an agreement on how to
handle things, there is no point changing the currently working code
in i915.

It is true that there is an issue in i915 regarding the fact that
intel_lid_notify() doesn't use the provided value but calls
acpi_lid_open(), but this is something that can be solved in
https://bugs.freedesktop.org/show_bug.cgi?id=100923, when the
situation clarifies.

Cheers,
Benjamin

Re: [Intel-gfx] [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations

[Jani Nikula]

On Mon, 15 May 2017, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote:
> On Mon, May 15, 2017 at 10:42 AM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>> On Mon, 15 May 2017, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote:
>>> I'll answer everything in the other thread, where there are slightly
>>> more other points raised: https://lkml.org/lkml/2017/5/15/10
>>
>> If you are discussing changes impacting i915, please keep intel-gfx list
>> in the loop.
>>
>
> I can add intel-gfx to the other thread if you want, but this will IMO
> just add more noise to your list.
> The question is whether or not the kernel should provide a fake state
> for the _LID acpi call, and until we reach an agreement on how to
> handle things, there is no point changing the currently working code
> in i915.

Fair enough.

> It is true that there is an issue in i915 regarding the fact that
> intel_lid_notify() doesn't use the provided value but calls
> acpi_lid_open(), but this is something that can be solved in
> https://bugs.freedesktop.org/show_bug.cgi?id=100923, when the
> situation clarifies.

The snarky reply here might be that we're just following the
documentation of acpi_lid_notifier_register(), acpi_lid_open(), and
friends. ;)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

[RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value

[Lv Zheng]

Both nouveau and i915, the only 2 kernel space lid notification listeners,
invoke acpi_lid_open() API to obtain _LID returning value instead of using
the notified value.

So this patch moves this logic from listeners to lid driver, always notify
kernel space listeners using _LID returning value.

This is a no-op cleanup, but facilitates administrators to configure to
notify kernel drivers with faked lid init states via command line
"button.lid_notify_init_state=Y".

Cc: <intel-gfx@lists.freedesktop.org>
Cc: <nouveau@lists.freedesktop.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/button.c             | 16 ++++++++++++++--
 drivers/gpu/drm/i915/intel_lvds.c |  2 +-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 4abf8ae..e047d34 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -119,6 +119,9 @@ static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
 static unsigned long lid_report_interval __read_mostly = 500;
 module_param(lid_report_interval, ulong, 0644);
 MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events");
+static bool lid_notify_init_state __read_mostly = false;
+module_param(lid_notify_init_state, bool, 0644);
+MODULE_PARM_DESC(lid_notify_init_state, "Notify init lid state to kernel drivers after boot/resume");
 
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
@@ -224,6 +227,15 @@ static void acpi_lid_notify_state(struct acpi_device *device,
 	if (state)
 		pm_wakeup_event(&device->dev, 0);
 
+	if (!lid_notify_init_state) {
+		/*
+		 * There are cases "state" is not a _LID return value, so
+		 * correct it before notification.
+		 */
+		if (!bios_notify &&
+		    lid_init_state != ACPI_BUTTON_LID_INIT_METHOD)
+			state = acpi_lid_evaluate_state(device);
+	}
 	acpi_lid_notifier_call(device, state);
 }
 
@@ -572,10 +584,10 @@ static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
 
 	if (!strncmp(val, "open", sizeof("open") - 1)) {
 		lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
-		pr_info("Notify initial lid state as open\n");
+		pr_info("Notify initial lid state to users space as open and kernel drivers with _LID return value\n");
 	} else if (!strncmp(val, "method", sizeof("method") - 1)) {
 		lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
-		pr_info("Notify initial lid state with _LID return value\n");
+		pr_info("Notify initial lid state to user/kernel space with _LID return value\n");
 	} else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
 		lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
 		pr_info("Do not notify initial lid state\n");
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

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value

[Benjamin Tissoires]

Hi Lv,

On May 27 2017 or thereabouts, Lv Zheng wrote:
> Both nouveau and i915, the only 2 kernel space lid notification listeners,
> invoke acpi_lid_open() API to obtain _LID returning value instead of using
> the notified value.
> 
> So this patch moves this logic from listeners to lid driver, always notify
> kernel space listeners using _LID returning value.
> 
> This is a no-op cleanup, but facilitates administrators to configure to
> notify kernel drivers with faked lid init states via command line
> "button.lid_notify_init_state=Y".
> 
> Cc: <intel-gfx@lists.freedesktop.org>
> Cc: <nouveau@lists.freedesktop.org>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/button.c             | 16 ++++++++++++++--
>  drivers/gpu/drm/i915/intel_lvds.c |  2 +-
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 4abf8ae..e047d34 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -119,6 +119,9 @@ static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
>  static unsigned long lid_report_interval __read_mostly = 500;
>  module_param(lid_report_interval, ulong, 0644);
>  MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events");
> +static bool lid_notify_init_state __read_mostly = false;
> +module_param(lid_notify_init_state, bool, 0644);
> +MODULE_PARM_DESC(lid_notify_init_state, "Notify init lid state to kernel drivers after boot/resume");
>  
>  /* --------------------------------------------------------------------------
>                                FS Interface (/proc)
> @@ -224,6 +227,15 @@ static void acpi_lid_notify_state(struct acpi_device *device,
>  	if (state)
>  		pm_wakeup_event(&device->dev, 0);
>  
> +	if (!lid_notify_init_state) {
> +		/*
> +		 * There are cases "state" is not a _LID return value, so
> +		 * correct it before notification.
> +		 */
> +		if (!bios_notify &&
> +		    lid_init_state != ACPI_BUTTON_LID_INIT_METHOD)
> +			state = acpi_lid_evaluate_state(device);
> +	}
>  	acpi_lid_notifier_call(device, state);
>  }
>  
> @@ -572,10 +584,10 @@ static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
>  
>  	if (!strncmp(val, "open", sizeof("open") - 1)) {
>  		lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> -		pr_info("Notify initial lid state as open\n");
> +		pr_info("Notify initial lid state to users space as open and kernel drivers with _LID return value\n");
>  	} else if (!strncmp(val, "method", sizeof("method") - 1)) {
>  		lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> -		pr_info("Notify initial lid state with _LID return value\n");
> +		pr_info("Notify initial lid state to user/kernel space with _LID return value\n");
>  	} else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
>  		lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
>  		pr_info("Do not notify initial lid state\n");
> 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;

This last hunk should really be in its own patch because the intel GPU
folks would need to apply the rest of the series for their CI suite, and
also because there is no reason for this change to be alongside any
other acpi/button.c change.

Cheers,
Benjamin

> -- 
> 2.7.4
> 

RE: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value

[Zheng, Lv]

Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Subject: Re: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value
> 
> Hi Lv,
> 
> On May 27 2017 or thereabouts, Lv Zheng wrote:
> > Both nouveau and i915, the only 2 kernel space lid notification listeners,
> > invoke acpi_lid_open() API to obtain _LID returning value instead of using
> > the notified value.
> >
> > So this patch moves this logic from listeners to lid driver, always notify
> > kernel space listeners using _LID returning value.
> >
> > This is a no-op cleanup, but facilitates administrators to configure to
> > notify kernel drivers with faked lid init states via command line
> > "button.lid_notify_init_state=Y".
> >
> > Cc: <intel-gfx@lists.freedesktop.org>
> > Cc: <nouveau@lists.freedesktop.org>
> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Cc: Peter Hutterer <peter.hutterer@who-t.net>
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > ---
> >  drivers/acpi/button.c             | 16 ++++++++++++++--
> >  drivers/gpu/drm/i915/intel_lvds.c |  2 +-
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 4abf8ae..e047d34 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -119,6 +119,9 @@ static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> >  static unsigned long lid_report_interval __read_mostly = 500;
> >  module_param(lid_report_interval, ulong, 0644);
> >  MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events");
> > +static bool lid_notify_init_state __read_mostly = false;
> > +module_param(lid_notify_init_state, bool, 0644);
> > +MODULE_PARM_DESC(lid_notify_init_state, "Notify init lid state to kernel drivers after
> boot/resume");
> >
> >  /* --------------------------------------------------------------------------
> >                                FS Interface (/proc)
> > @@ -224,6 +227,15 @@ static void acpi_lid_notify_state(struct acpi_device *device,
> >  	if (state)
> >  		pm_wakeup_event(&device->dev, 0);
> >
> > +	if (!lid_notify_init_state) {
> > +		/*
> > +		 * There are cases "state" is not a _LID return value, so
> > +		 * correct it before notification.
> > +		 */
> > +		if (!bios_notify &&
> > +		    lid_init_state != ACPI_BUTTON_LID_INIT_METHOD)
> > +			state = acpi_lid_evaluate_state(device);
> > +	}
> >  	acpi_lid_notifier_call(device, state);
> >  }
> >
> > @@ -572,10 +584,10 @@ static int param_set_lid_init_state(const char *val, struct kernel_param *kp)
> >
> >  	if (!strncmp(val, "open", sizeof("open") - 1)) {
> >  		lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> > -		pr_info("Notify initial lid state as open\n");
> > +		pr_info("Notify initial lid state to users space as open and kernel drivers with _LID
> return value\n");
> >  	} else if (!strncmp(val, "method", sizeof("method") - 1)) {
> >  		lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> > -		pr_info("Notify initial lid state with _LID return value\n");
> > +		pr_info("Notify initial lid state to user/kernel space with _LID return value\n");
> >  	} else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) {
> >  		lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE;
> >  		pr_info("Do not notify initial lid state\n");
> > 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;
> 
> This last hunk should really be in its own patch because the intel GPU
> folks would need to apply the rest of the series for their CI suite, and
> also because there is no reason for this change to be alongside any
> other acpi/button.c change.

OK, I'll drop i915 related changes.
However I can see cleanup chances in button.c.
I feel I should at least do minimal tunings in button driver to allow future improvements.

Cheers,
Lv

> Cheers,
> Benjamin