RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi,

> From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-owner@xxxxxxxxxxxxxxx] On Behalf Of Zheng,
> Lv
> Subject: RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"
> 
> Hi, Guys
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoires@xxxxxxxxxx]
> > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"
> >
> > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
> > > <benjamin.tissoires@xxxxxxxxxx> wrote:
> > > > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > >> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> > > >> <benjamin.tissoires@xxxxxxxxxx> wrote:
> > > >> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > >> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> > > >> >> > Hi,
> > > >> >> >
> > > >> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoires@xxxxxxxxxx]
> > > >> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to
> > lid_init_state=open"
> > > >> >> > >
> > > >> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > > >> >> > > > Hi,
> > > >> >> > > >
> > > >> >> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@xxxxxxxxxx]
> > > >> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior to
> > lid_init_state=open"
> > > >> >> > > > >
> > > >> >> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > > >> >> > > > >
> > > >> >> > > > > Even if the method implementation can be buggy on some platform,
> > > >> >> > > > > the "open" choice is worse. It breaks docking stations basically
> > > >> >> > > > > and there is no way to have a user-space hwdb to fix that.
> > > >> >> > > > >
> > > >> >> > > > > On the contrary, it's rather easy in user-space to have a hwdb
> > > >> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> > > >> >> > > > > the state of the LID switch for us: you need to set the udev
> > > >> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > > >> >> > > > >
> > > >> >> > > > > When libinput detects internal keyboard events, it will
> > > >> >> > > > > overwrite the state of the switch to open, making it reliable
> > > >> >> > > > > again. Given that logind only checks the LID switch value after
> > > >> >> > > > > a timeout, we can assume the user will use the internal keyboard
> > > >> >> > > > > before this timeout expires.
> > > >> >> > > > >
> > > >> >> > > > > For example, such a hwdb entry is:
> > > >> >> > > > >
> > > >> >> > > > > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > > >> >> > > >
> > > >> >
> > > >> > [...]
> > > >> >
> > > >> >>
> > > >> >> Well, if it worked in a specific way that users depended on before the commit in
> > > >> >> question and now it works differently, then it does break things.
> > > >> >>
> > > >> >> Benjamin, my understanding is that this is the case, is it correct?
> > > >> >
> > > >> > That is correct. This patch I reverted introduces regression for professional
> > > >> > laptops that expect the LID switch to be reported accurately.
> > > >>
> > > >> And from a user's perspective, what does not work any more?
> > > >
> > > > If you boot or resume your laptop with the lid closed on a docking
> > > > station while using an external monitor connected to it, both internal
> > > > and external displays will light on, while only the external should.
> > > >
> > > > There is a design choice in gdm to only provide the greater on the
> > > > internal display when lit on, so users only see a gray area on the
> > > > external monitor. Also, the cursor will not show up as it's by default
> > > > on the internal display too.
> > > >
> > > > To "fix" that, users have to open the laptop once and close it once
> > > > again to sync the state of the switch with the hardware state.
> > >
> > > OK
> > >
> > > Yeah, that sucks.
> > >
> > > So without the Lv's patch the behavior (on the systems in question) is
> > > as expected, right?
> > >
> >
> > Yes, reverting these 2 patches restores the pre v4.11 kernel behavior.
> 
> I would make an argument that:
> A. Is this necessarily a button driver regression?
> 1. Users already configured to not using internal display, why gdm need to determine it again instead
> of users choice?
> 2. Can gdm/graphics driver saves state before suspend, and restores saved state after resume?
>    If users didn't change state during suspend, then everything should be correct.
>    If users changed state during suspend, it should be acceptable for users to change it again to make
> the state correct.
> See, this is obviously a case that is not so strictly related to ACPI button driver.
> Why do we need to force button driver to marry external monitors.
> B. Bug reporters are all ok with using quirk modes as boot parameters to work this around.
> Why should we change our default behavior aimlessly?

I have one more concern:
In button.lid_init_state=method mode,
Is that possible for libinput to work things around if _LID return value is not correct?
How libinput ensures correct timing of overwriting the input node value?
Will button driver faked event value overwrites what libinput has written?

From this point of view, button.lid_init_state=ignore might be a better choice than button.lid_init_state=method to allow libinput to deal with all kind of cases.

Thanks and best regards
Lv