Hi Lv, On Mon, May 15, 2017 at 5:55 AM, Zheng, Lv <lv.zheng@xxxxxxxxx> wrote: > Hi, Benjamin > >> From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-owner@xxxxxxxxxxxxxxx] 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@xxxxxxxxx> wrote: >> > Hi, >> > >> > If my previous reply is not persuasive enough. >> > Let me do that in a different way. >> > >> >> From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-owner@xxxxxxxxxxxxxxx] 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@xxxxxxxxx] >> >> > 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> >> >> > > Cc: <nouveau@xxxxxxxxxxxxxxxxxxxxx> >> >> > > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx> >> >> > > --- >> >> > > 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@xxxxxxxxxxxxxxx >> >> > > 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@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
