Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230712175023.31651-1-maxtram95@gmail.com> <4b7da537-8ec3-95e2-4053-871c56e980ea@redhat.com>
In-Reply-To: <4b7da537-8ec3-95e2-4053-871c56e980ea@redhat.com>
From:   Maxim Mikityanskiy <maxtram95@gmail.com>
Date:   Sat, 15 Jul 2023 20:33:12 +0300
Message-ID: <CAKErNvr+F_6_qFyQi_JeDwKNyUapTWZ=KuNx7-UeNgLJoFPrwg@mail.gmail.com>
Subject: Re: [PATCH] platform/x86/intel/hid: Add HP Dragonfly G2 to DMI quirks
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     Alex Hung <alexhung@gmail.com>, Mark Gross <markgross@kernel.org>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        Richard Hughes <hughsient@gmail.com>,
        Jorge Lopez <jorge.lopez2@hp.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 14 Jul 2023 at 20:00, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Maxim,
>
> On 7/12/23 19:50, Maxim Mikityanskiy wrote:
> > SW_TABLET_MODE reports are broken on BIOS versions newer than 1.9.1 on
> > HP Elite Dragonfly G2. Analysis of SSDT9 shows that the BTNL method has
> > to be called to start getting 0xcc and 0xcd events. Apparently, the
> > button_array_present method used to return true on BIOS 1.9.1 and older=
,
> > but it returns false on newer BIOSes due to HEBC returning 0x000033f3
> > (bits 0x60000 and 0x20000 are not set).
> >
> > Add this laptop to button_array_table to force the BTNL call, and also
> > add it to dmi_vgbs_allow_list to read the initial state and sync VBDS
> > with VBPS, because this laptop has a reliable VGBS method.
> >
> > Tested with BIOS 1.13.1.
> >
> > Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
>
> Thank you for figuring this out and thank you for the patch.
>
> WRT the need to call BTNL, I expect that if one laptop needs this likely =
more models will need it and I expect / hope that this will not cause any i=
ssues when called everywhere.
>
> So I think it would be better to instead of adding this model to the butt=
on_array_table[] it would be better to just always call BTNL (I suspect thi=
s is also what Windows does).
>
> Can you give the attached path to always call BTNL a try ?

Thanks for the patch, I tested it, and it works for me. Also tested on
a non-convertible laptop and didn't see any side effects.

Regarding calling BTNL unconditionally, I guess it's fine to try it,
my only concern is that it might break buttons on some laptops if it
has some weird side effects there, but the change can always be
reverted if you get some bug reports.

I couldn't find any publicly available docs on this interface, so I
don't know whether the driver is expected to call BTNL to get tablet
mode events, or whether such behavior is specific to HP.

> As for adding the model to dmi_vgbs_allow_list[] on many models it seems =
that the EC will send a 0xcc / 0xcd event at boot (now that I think about i=
t I guess this may be in response to the BTNL call) and then on that event =
the SW_TABLET_STATE evdev gets registered and the first event also syncs th=
e state.
>
> Can you check if just using the always call BTNL patch, without the quirk=
 perhaps already makes the SW_TABLET_STATE evdev show up (including the cor=
rect state) ?

Yes, I checked it before, my laptop sends an event on boot (after an
arbitrary delay of about 0.5..1.5 s), but only when started in the
laptop mode. If I turn it on in the tablet mode, there is no initial
event, so I'm afraid the VGBS quirk is needed.


> If no event is send during boot, the n I'm fine with adding the model to =
the dmi_vgbs_allow_list[], but perhaps this is not necessary?
>
> Regards,
>
> Hans
>
>
>
> > ---
> >  drivers/platform/x86/intel/hid.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/platform/x86/intel/hid.c b/drivers/platform/x86/in=
tel/hid.c
> > index 5632bd3c534a..5c78b476ed1e 100644
> > --- a/drivers/platform/x86/intel/hid.c
> > +++ b/drivers/platform/x86/intel/hid.c
> > @@ -128,6 +128,13 @@ static const struct dmi_system_id button_array_tab=
le[] =3D {
> >                       DMI_MATCH(DMI_PRODUCT_NAME, "Surface Go 3"),
> >               },
> >       },
> > +     {
> > +             .ident =3D "HP Elite Dragonfly G2",
> > +             .matches =3D {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> > +                     DMI_MATCH(DMI_PRODUCT_NAME, "HP Elite Dragonfly G=
2 Notebook PC"),
> > +             },
> > +     },
> >       { }
> >  };
> >
> > @@ -150,6 +157,12 @@ static const struct dmi_system_id dmi_vgbs_allow_l=
ist[] =3D {
> >                       DMI_MATCH(DMI_PRODUCT_NAME, "Surface Go"),
> >               },
> >       },
> > +     {
> > +             .matches =3D {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> > +                     DMI_MATCH(DMI_PRODUCT_NAME, "HP Elite Dragonfly G=
2 Notebook PC"),
> > +             },
> > +     },
> >       { }
> >  };
> >