[Intel-gfx] [PATCH v2 5/5] ACPI: button: Obselete acpi_lid_open() invocations
Benjamin Tissoires
benjamin.tissoires at gmail.com
Mon May 15 07:12:02 UTC 2017
Hi Lv,
On Mon, May 15, 2017 at 5:55 AM, Zheng, Lv <lv.zheng at intel.com> wrote:
> Hi, Benjamin
>
>> From: linux-acpi-owner at vger.kernel.org [mailto:linux-acpi-owner at 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 at intel.com> wrote:
>> > 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
>>
>> 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 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
>> >> ��칻 �&�~�&� ��+-��ݶ ��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 at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the Intel-gfx
mailing list