On Fri, Jan 22, 2021 at 01:15:05AM +0100, Hans de Goede wrote: > Hi, > > On 1/22/21 12:48 AM, mark gross wrote: > > On Wed, Jan 20, 2021 at 01:49:41PM +0100, Hans de Goede wrote: > >> Recently userspace has started making more use of SW_TABLET_MODE > >> (when an input-dev reports this). > >> > >> Specifically recent GNOME3 versions will: > >> > >> 1. When SW_TABLET_MODE is reported and is reporting 0: > >> 1.1 Disable accelerometer-based screen auto-rotation > >> 1.2 Disable automatically showing the on-screen keyboard when a > >> text-input field is focussed > >> > >> 2. When SW_TABLET_MODE is reported and is reporting 1: > >> 2.1 Ignore input-events from the builtin keyboard and touchpad > >> (this is for 360° hinges style 2-in-1s where the keyboard and > >> touchpads are accessible on the back of the tablet when folded > >> into tablet-mode) > >> > >> This means that claiming to support SW_TABLET_MODE when it does not > >> actually work / reports correct values has bad side-effects. > > did you mean "reports incorrect values"? > > Yes and no, I meant this to be read as "does not (...) report correct values" > but your suggestion is better I will fix this for v2. > > > > >> > >> The check in the hp-wmi code which is used to decide if the input-dev > >> should claim SW_TABLET_MODE support, only checks if the > >> HPWMI_HARDWARE_QUERY is supported. It does *not* check if the hardware > >> actually is capable of reporting SW_TABLET_MODE. > >> > >> This leads to the hp-wmi input-dev claming SW_TABLET_MODE support, > >> while in reality it will always report 0 as SW_TABLET_MODE value. > >> This has been seen on a "HP ENVY x360 Convertible 15-cp0xxx" and > >> this likely is the case on a whole lot of other HP models. > >> > >> This problem causes both auto-rotation and on-screen keyboard > >> support to not work on affected x360 models. > >> > >> There is no easy fix for this, but since userspace expects > >> SW_TABLET_MODE reporting to be reliable when advertised it is > >> better to not claim/report SW_TABLET_MODE support at all, then > > than > >> to claim to support it while it does not work. > >> > >> To avoid the mentioned problems, add a new enable_tablet_mode_sw > >> module-parameter which defaults to false. > >> > >> Note I've made this an int using the standard -1=auto, 0=off, 1=on > >> tripplet, with the hope that in the future we can come up with a > >> better way to detect SW_TABLET_MODE support. ATM the default > >> auto option just does the same as off. > >> > >> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1918255 > >> Cc: Stefan Brüns <stefan.bruens@xxxxxxxxxxxxxx> > >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > >> --- > >> drivers/platform/x86/hp-wmi.c | 14 ++++++++++---- > >> 1 file changed, 10 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > >> index 18bf8aeb5f87..ff028587cd21 100644 > >> --- a/drivers/platform/x86/hp-wmi.c > >> +++ b/drivers/platform/x86/hp-wmi.c > >> @@ -32,6 +32,10 @@ MODULE_LICENSE("GPL"); > >> MODULE_ALIAS("wmi:95F24279-4D7B-4334-9387-ACCDC67EF61C"); > >> MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4"); > >> > >> +static int enable_tablet_mode_sw = -1; > > So busted HW gets the default while working HW will need to add a boot time > > parameter. If there are no working tablet_mode devices I guess its ok but, if > > I had a working platform I'd be a little miffed at the choice to make my life > > harder (by forcing me to add a enable_tablet_mode_sw=1 to my kernel > > command line) while making life easier for those with busted hardware. > > > > I'm not saying change it but, it should be considered. > > Until recently userspace pretty much ignored SW_TABLET_MODE, so reporting > it while it did not work was not a big deal, but as I tried to explain > in the commit message always reporting SW_TABLET_MODE=0 does cause some > real issues: > > >> 1. When SW_TABLET_MODE is reported and is reporting 0: > >> 1.1 Disable accelerometer-based screen auto-rotation > >> 1.2 Disable automatically showing the on-screen keyboard when a > >> text-input field is focussed > > By defaulting to not reporting SW_TABLET_MODE at all we go back to the > (slightly) older userspace behavior of always doing auto-rotation and > always popping up the onscreen-keyboard on text-field focus (on devices > with a touchscreen). > > So basically the bad side-effects of reporting SW_TABLET_MODE while it > is not working are much worse then the bad side-effects of not reporting > it on devices where it does work. > > More in general the way userspace uses SW_TABLET_MODE means that if > we report it, then it MUST be reliable. If it is not reliable then it > is better to not support it at all. Yeah, thats all too true as well. Making user mode work correctly takes priority over my sense of fairness. Acked-by: Mark Gross <mgross@xxxxxxxxxxxxxxx> --mark > > For this reason the intel-vbtn, intel-hid and asus-wmi driver have all > 3 already been moved over to using a DMI based whitelist. And now I > guess it is hp-wmi's turn to follow in their footsteps. > > Ideally there is some WMI query other then the HPWMI_HARDWARE_QUERY > which actually tells us which bits in the HPWMI_HARDWARE_QUERY result > are valid and which bits are simply always 0. I hope someone who > actually has this hardware can spend some time figuring this out. > > In the mean time disabling SW_TABLET_MODE reporting is the safe > fallback option; and if people come forward where this does work > then we can do a DMI based whitelist (*). > > Regards, > > Hans > > *) Which will hopefully be a temporary solution but there is absolutely > no documentation for all this crap and most vendors don't seem to care > about helping us with this, so... > > > > > > > > > > > > > > --mark > > > > > >> +module_param(enable_tablet_mode_sw, int, 0444); > >> +MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=auto, 0=no, 1=yes)"); > >> + > >> #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C" > >> #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4" > >> > >> @@ -654,10 +658,12 @@ static int __init hp_wmi_input_setup(void) > >> } > >> > >> /* Tablet mode */ > >> - val = hp_wmi_hw_state(HPWMI_TABLET_MASK); > >> - if (!(val < 0)) { > >> - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); > >> - input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val); > >> + if (enable_tablet_mode_sw > 0) { > >> + val = hp_wmi_hw_state(HPWMI_TABLET_MASK); > >> + if (!(val < 0)) { > >> + __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); > >> + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val); > >> + } > >> } > >> > >> err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL); > >> -- > >> 2.28.0 > >> > > >
