Re: Keyboard regression by intel-vbtn

[Hans de Goede]

Hi,

On 9/30/20 5:12 PM, Limonciello, Mario wrote:
> > -----Original Message-----
> > From: Hans de Goede <hdegoede@xxxxxxxxxx>
> > Sent: Wednesday, September 30, 2020 8:28
> > To: Limonciello, Mario; Barnabás Pőcze; Andy Shevchenko
> > Cc: platform-driver-x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Takashi
> > Iwai
> > Subject: Re: Keyboard regression by intel-vbtn
> >
> >
> > [EXTERNAL EMAIL]
> >
> > Hi,
> >
> > On 9/29/20 10:47 PM, Limonciello, Mario wrote:
> > > > I requested on the Ubuntu bug for someone to provide these.
> > >
> > > Joe Barnett was kind enough to share two ACPI dumps to compare.
> > > Not affected:
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394/+attachment/54153
> > 18/+files/1.2.0.acpidump
> > > Affected:
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394/+attachment/54154
> > 05/+files/1.13.0.acpidump
> >
> > Thank you, I took a look at these (before completing my allow-list fix),
> > but there is not really much which stands out. The only related thing which
> > stands out is that the 1.13.0 dsdt.dsl has this new bit:
> >
> >
> >                               Case (0x08)
> >                               {
> >                                   Return (^^PCI0.LPCB.H_EC.VGBI.VGBS ())
> >                               }
> >
> > Inside the _DSM of the HIDD / INT33D5 device.
> >
> >               Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
> >               {
> >                   If ((Arg0 == ToUUID ("eeec56b3-4442-408f-a792-
> > 4edd4d758054")))
> >
> >
> > What is interesting here is that the PCI0.LPCB.H_EC.VGBI.VGBS object/method
> > does not actually exist the correct path is:
> >
> > ^^PCI0.LPCB.ECDV.VGBI.VGBS
> >
> > So this does suggest that something around the VGBS handling changed
> > (and since it points to a non existing ACPI object, possibly broke)
> > in the newer BIOS version. But what exactly is going on on this XPS 2-in-1
> > cannot really be derived from the acpidumps.
> >
> > Regards,
> >
> > Hans
>
> Looking through some publicly found content I think I might have figured out what
> bight be the missing link.
>
> https://software.intel.com/sites/default/files/detecting-slate-clamshell-mode-and-screen-orientation-in-convertible-pc-1.pdf
>
> You can see that the device with CID PNP0C60 is supposed to indicate the presence
> of a convertible hinge.  We don't currently have anything that matches that _CID or _HID
> in intel-vbtn.
>
> In the DSDT dump you can see that the status method for the INT33D3 device returns
> 0x0F on 2-in-1.s
>
>          Device (CIND)
>          {
>              Name (_HID, "INT33D3" /* Intel GPIO Buttons */)  // _HID: Hardware ID
>              Name (_CID, "PNP0C60" /* Display Sensor Device */)  // _CID: Compatible ID
>              Method (_STA, 0, Serialized)  // _STA: Status
>              {
>                  If ((OSYS >= 0x07DC))
>                  {
>                      Return (0x0F)
>                  }
>
>                  Return (Zero)
>              }
>          }
>
> On a non 2-in-1 device I don't see this present.  So I think we should have intel-vbtn
> look for that INT33D3/PNP0C60 device to decide whether to offer the switch.
>
> Similarly as mentioned in that document I think that we should not be showing the
> docking switch only when INT33D4/PNP0C70 is present and returns 0xF.

That is a good find, thank you for digging into this and finding this.

But it seems we have a typical case of the microsoft/intel example DSDT code being
blindly copied everywhere here too:

The chassis-type check was originally introduced by you in:
commit de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode switch on 2-in-1's")

    Some laptops such as the XPS 9360 support the intel-vbtn INT33D6
    interface but don't initialize the bit that intel-vbtn uses to
    represent switching tablet mode.

    By running this only on real 2-in-1's it shouldn't cause false
    positives.

    Fixes: 30323fb6d5 ("Support tablet mode switch")

I have a XPS 9360 (which is not 2-in-1) acpidump and that has:

        Device (CIND)
        {
            Name (_HID, "INT33D3" /* Intel GPIO Buttons */)  // _HID: Hardware ID
            Name (_CID, "PNP0C60" /* Display Sensor Device */)  // _CID: Compatible ID
            Method (_STA, 0, Serialized)  // _STA: Status
            {
                If ((OSYS >= 0x07DC))
                {
                    Return (0x0F)
                }

                Return (Zero)
            }
        }

And on an asus e200ha (also not a 2-in-1), where we were seeing
similar problems, but then using asus custom WMI interface for
getting SW_TABLET_MODE I see:

        Device (CIND)
        {
            Name (_HID, "INT33D3" /* Intel GPIO Buttons */)  // _HID: Hardware ID
            Name (_CID, "PNP0C60" /* Display Sensor Device */)  // _CID: Compatible ID
            Method (_STA, 0, Serialized)  // _STA: Status
            {
                Return (0x0F)
            }
        }

I have quite a few DSDTs (mostly byt/cht tablets or 2-in-1s though) and
65 of them define a "PNP0C60" device and only 1 unconditionally
returns 0 from the _STA method for this device. Most others have
an OSYS check. Some also check for some other, presumably BIOS
configured variable, but by far most always return 0x0F, or do
so after an OSYS check which will be true in our case.

So I'm afraid that almost all devices which have the intel-vbtn (INT33D6)
ACPI device will also have a PNP0C60 device with the exact same
_STA method as found on the XPS 9360 and that this thus is not
helpful.

Regards,

Hans