Bug 207433 - intel-hid: add support for SW_TABLET_MODE
Summary: intel-hid: add support for SW_TABLET_MODE
Status: RESOLVED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Platform_x86 (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: drivers_platform_x86@kernel-bugs.osdl.org
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-25 14:35 UTC by Elia Devito
Modified: 2021-02-15 11:48 UTC (History)
5 users (show)

See Also:
Kernel Version: 5.6.7
Subsystem:
Regression: No
Bisected commit-id:


Attachments
add support for SW_TABLET_MODE (2.97 KB, patch)
2020-04-26 18:23 UTC, Elia Devito
Details | Diff
add support for SW_TABLET_MODE (5.58 KB, patch)
2020-10-07 18:11 UTC, Elia Devito
Details | Diff
acpidump from latitute 7410 2in1 (1.69 MB, text/plain)
2020-10-08 04:22 UTC, Benjamin Hennion
Details
add support for SW_TABLET_MODE (5.69 KB, patch)
2020-10-22 16:29 UTC, Elia Devito
Details | Diff
add support for SW_TABLET_MODE (6.93 KB, patch)
2020-10-24 10:35 UTC, Elia Devito
Details | Diff
add support for SW_TABLET_MODE (6.98 KB, patch)
2020-10-25 18:42 UTC, Elia Devito
Details | Diff
add support for SW_TABLET_MODE (6.74 KB, patch)
2020-11-11 16:15 UTC, Elia Devito
Details | Diff

Description Elia Devito 2020-04-25 14:35:06 UTC
Hello,
I have noted on my macchine (HP Spectre x360 Convertible 15-df0xxx) that intel-hid driver emit two event when switch from/to tablet/laptop mode similar to intel-vbtn

evtest output:
Input driver version is 1.0.1
Input device ID: bus 0x19 vendor 0x0 product 0x0 version 0x0
Input device name: "Intel HID 5 button array"
Supported events:
  Event type 0 (EV_SYN)
  Event type 1 (EV_KEY)
    Event code 114 (KEY_VOLUMEDOWN)
    Event code 115 (KEY_VOLUMEUP)
    Event code 116 (KEY_POWER)
    Event code 125 (KEY_LEFTMETA)
    Event code 240 (KEY_UNKNOWN)
    Event code 561 (?)
  Event type 4 (EV_MSC)
    Event code 4 (MSC_SCAN)
Properties:
Testing ... (interrupt to exit)
Event: time 1587822368.146020, type 4 (EV_MSC), code 4 (MSC_SCAN), value cc
Event: time 1587822368.146020, type 1 (EV_KEY), code 240 (KEY_UNKNOWN), value 1
Event: time 1587822368.146020, -------------- SYN_REPORT ------------
Event: time 1587822368.146061, type 1 (EV_KEY), code 240 (KEY_UNKNOWN), value 0
Event: time 1587822368.146061, -------------- SYN_REPORT ------------
Event: time 1587822374.663964, type 4 (EV_MSC), code 4 (MSC_SCAN), value cd
Event: time 1587822374.663964, type 1 (EV_KEY), code 240 (KEY_UNKNOWN), value 1
Event: time 1587822374.663964, -------------- SYN_REPORT ------------
Event: time 1587822374.664006, type 1 (EV_KEY), code 240 (KEY_UNKNOWN), value 0
Event: time 1587822374.664006, -------------- SYN_REPORT ------------

as intel-vbtn driver, event code 0xcc is emitted when entering tablet mode, and 0xcd on return to laptop mode.
I'm not able to write a patch to fix this, but remain available to give more info and test
Comment 1 Elia Devito 2020-04-26 18:23:24 UTC
Created attachment 288739 [details]
add support for SW_TABLET_MODE

Contrary to what I said, I managed to make a patch that fix the issue, it's basically a copy, paste and adapt from intel-vbtn driver.

I attached the patch here, if someone more expert than me can see it and eventually merge it.

I tested it on my pc and it seem to work correctly
Comment 2 Benjamin Hennion 2020-09-28 09:16:33 UTC
Hello,
I have the same behaviour on a Dell Latitude 7410 2-in-1, probably equipped with the same sensor. 
Dist.: Arch Linux
Kernel: 5.8.11

My evtest output is exactly the same as Elia Devito's, and the patch solves the problem. libinput and tools based on it now detect properly when I flip the screen and use the device in tablet mode.
Comment 3 Elia Devito 2020-09-28 09:27:07 UTC
Hello Benjamin,
have you possibility to test the patch that I have attached?

I'm very bus at work this period but in this week (or next) I would like to rework  my patch and make a pull request to kernel devs
Comment 4 Borislav Petkov 2020-09-28 09:34:59 UTC
> and make a pull request to kernel devs

Kernel patches are not submitted through pull requests but sent to a mailing list and the respective maintainers. In your case:

$ ./scripts/get_maintainer.pl /tmp/intel-hid-add-support-for-SW_TABLET_MODE.patch 
Alex Hung <alex.hung@canonical.com> (maintainer:INTEL HID EVENT DRIVER)
Darren Hart <dvhart@infradead.org> (odd fixer:X86 PLATFORM DRIVERS)
Andy Shevchenko <andy@infradead.org> (odd fixer:X86 PLATFORM DRIVERS)
platform-driver-x86@vger.kernel.org (open list:INTEL HID EVENT DRIVER)
linux-kernel@vger.kernel.org (open list)

See here for more info:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

HTH.
Comment 5 Benjamin Hennion 2020-09-28 11:01:08 UTC
> have you possibility to test the patch that I have attached?

Yes, I tested your patch, and it worked flowlessly.
Comment 6 Benjamin Hennion 2020-10-04 14:26:01 UTC
@Elia Devito
After using your patch for a while, I found a weird bug.

When booting on AC (but not on battery), the keyboard and touchpad do not work on various graphical environments. After some time, I understood that for some reason, when booting on AC, the SW_TABLET_MODE state is 1 (even though the laptop is in laptop mode - see the evtest output below).
Flipping the screen to tablet mode and back issues a SW_TABLET_MODE=0 event (preceeded by a SW_TABLET_MODE=1) and enables the keyboard and touchpad.

Here is the evtest output with the patch, obtained with the following protocol:
-Boot to shell with AC plugged in, log in, run evtest /dev/input/event5
-Flip the screen to enter tablet mode
-Flip the screen back to return to laptop mode

Input driver version is 1.0.1
Input device ID: bus 0x19 vendor 0x0 product 0x0 version 0x0
Input device name: "Intel HID 5 button array"
Supported events:
  Event type 0 (EV_SYN)
  Event type 1 (EV_KEY)
    Event code 114 (KEY_VOLUMEDOWN)
    Event code 115 (KEY_VOLUMEUP)
    Event code 116 (KEY_POWER)
    Event code 125 (KEY_LEFTMETA)
    Event code 240 (KEY_UNKNOWN)
    Event code 561 (?)
  Event type 4 (EV_MSC)
    Event code 4 (MSC_SCAN)
  Event type 5 (EV_SW)
    Event code 1 (SW_TABLET_MODE) state 1
Properties:
Testing ... (interrupt to exit)
Event: time 1601820116.610071, type 5 (EV_SW), code 1 (SW_TABLET_MODE), value 1
Event: time 1601820116.610071, -------------- SYN_REPORT ------------
Event: time 1601820119.048729, type 5 (EV_SW), code 1 (SW_TABLET_MODE), value 0
Event: time 1601820119.048729, -------------- SYN_REPORT ------------


When booting on battery, the same protocol gives me the same output except for the line
    Event code 1 (SW_TABLET_MODE) state 1
which becomes the expected
    Event code 1 (SW_TABLET_MODE) state 0

I don't know why the AC cable influences the behaviour, and the bug may come from elsewhere (firmware?). Any idea?
Comment 7 Elia Devito 2020-10-07 18:11:00 UTC
Created attachment 292891 [details]
add support for SW_TABLET_MODE

new patch version
Comment 8 Elia Devito 2020-10-07 18:11:30 UTC
Hi Benjamin,
If you attach acpidump of your pc I can try to understand how to solve this.

I have attached a new versione of the patch, can you test it?
Comment 9 Benjamin Hennion 2020-10-08 04:22:46 UTC
Created attachment 292899 [details]
acpidump from latitute 7410 2in1

Here is my acpidump. Anything else you'd need?

I'll test your new patch and report asap.
Comment 10 Benjamin Hennion 2020-10-08 16:29:37 UTC
Hi,
I've tested your new patch. It works as well as the first one, with the same bug when booting on AC.

This is arguably irrelevent, but there is one thing that seems to have changed: with the same protocal as in my comment 6 (boot to shell with AC plugged in, launch evtest, flip the screen to tablet mode and back), I get a slightly different output: the SW_TABLET_MODE=1 event in not issued anymore (see below). It does not change the problem that the initial state of SW_TABLET_MODE is wrong.

By the way, thanks a lot for looking a this!
Benjamin


The evtest output with the version 2 of the patch:

Input driver version is 1.0.1
Input device ID: bus 0x19 vendor 0x0 product 0x0 version 0x0
Input device name: "Intel HID 5 button array"
Supported events:
  Event type 0 (EV_SYN)
  Event type 1 (EV_KEY)
    Event code 114 (KEY_VOLUMEDOWN)
    Event code 115 (KEY_VOLUMEUP)
    Event code 116 (KEY_POWER)
    Event code 125 (KEY_LEFTMETA)
    Event code 240 (KEY_UNKNOWN)
    Event code 561 (?)
  Event type 4 (EV_MSC)
    Event code 4 (MSC_SCAN)
  Event type 5 (EV_SW)
    Event code 1 (SW_TABLET_MODE) state 1
Properties:
Testing ... (interrupt to exit)
Event: time 1602173283.332107, type 5 (EV_SW), code 1 (SW_TABLET_MODE), value 0
Event: time 1602173283.332107, -------------- SYN_REPORT ------------
Comment 11 Elia Devito 2020-10-22 16:29:24 UTC
Created attachment 293127 [details]
add support for SW_TABLET_MODE

added more output
Comment 12 Elia Devito 2020-10-22 16:30:06 UTC
Hi Benjamin,
I have have added to the patch some output to help us understand what's going on.

Can you apply it and tell me the output of "dmesg | grep intel-hid"
after booting (with AC and without AC) and flipping the screen to tablet mode and back
Comment 13 Benjamin Hennion 2020-10-22 18:42:14 UTC
Hi,
Here are the results. You can easily spot my flipping the screen back and forth in both cases.

With AC: 
[    7.203800] intel-hid INT33D5:00: platform supports 5 button array
[    7.203905] intel-hid INT33D5:00: vgbs: 0x0 --- mode: 1
[   95.771247] intel-hid INT33D5:00: event 0xcc
[   96.017609] intel-hid INT33D5:00: event 0xcc
[   97.358649] intel-hid INT33D5:00: event 0xcc
[   98.945310] intel-hid INT33D5:00: event 0xcd

On battery:
[    6.908082] intel-hid INT33D5:00: platform supports 5 button array
[    6.908197] intel-hid INT33D5:00: vgbs: 0x40 --- mode: 0
[    7.663100] intel-hid INT33D5:00: event 0xcd
[   36.568153] intel-hid INT33D5:00: event 0xcc
[   37.177498] intel-hid INT33D5:00: event 0xcc
[   38.644747] intel-hid INT33D5:00: event 0xcd
Comment 14 Elia Devito 2020-10-23 13:14:49 UTC
Hi Benjamin,
apparently this seem a firmware bug, have you bios updated to latest version?
I can add exception for you notebook model, can you tell me the output of this commands:

dmidecode -s system-manufacturer
dmidecode -s system-product-name
dmidecode -s bios-version
Comment 15 Elia Devito 2020-10-24 10:35:49 UTC
Created attachment 293169 [details]
add support for SW_TABLET_MODE
Comment 16 Elia Devito 2020-10-24 10:36:15 UTC
Hi Benjamin,
I have added to the patch a workaround to try to solve the problem with your notebook firmware,
to enable it you need to load intel-hid module with parameter tablet_mode_alt_flag=1
(to make this step automatic I need the outputs that I asked you in the previous message) 

can you test the patch with the new parameter enabled?

specifically, you should check the correct recognition of the tablet mode in the following cases:

- Boot in laptop mode witch AC connected
- Boot in tablet mode witch AC connected
- Boot in laptop mode on battery
- Boot in tablet mode on battery
Comment 17 Benjamin Hennion 2020-10-25 11:16:50 UTC
Hi Elia,
First of all, thanks a lot for your work!

Testing your last patch, I realized my bug only occured when booting on AC after having shutdown on AC too. If I shut down on battery, plug the AC in, and boot, it works nominally. I agree this seems like a BIOS issue.


Here are my dmidecode outputs:

$ sudo dmidecode -s system-manufacturer
Dell Inc.
$ sudo dmidecode -s system-product-name
Latitude 7410
$ sudo dmidecode -s bios-version
1.3.1


Now, I tested you last patch, with intel-hid.tablet_mode_alt_flag=1 in my cmdline. You will find below the various dmesg outputs. Every time, I boot, flip to tablet mode (if not already in it) and go (back) to laptop mode.

We can notice two things:
1- your alternate flag does not seem to fix the issue
2- with AC+tablet an event 0xcc is issued at the 25th second without me moving anything. It more or less coincides with the Xserver's finishing its initialization. I'll test some more.

Since on battery, a 0xcc or 0xcd event is always issued right after your module's initialization (why is that?), it may be enough, in my laptop's case, to always assume we have booted in laptop mode and only change when receiving a 0xcc event. This may have been the idea behing your alternate flag, but the line
#define TABLET_MODE_FLAG_ALT (TABLET_MODE_FLAG | 0x0)
just sets TABLET_MODE_FLAG_ALT to TABLET_MODE_FLAG.

AC+laptop: 
[    4.539031] intel-hid INT33D5:00: platform supports 5 button array
[    4.539141] intel-hid INT33D5:00: use alternative tablet mode flag
[    4.539142] intel-hid INT33D5:00: vgbs: 0x0 --- mode: 1
[   37.231210] intel-hid INT33D5:00: event 0xcc
[   37.476059] intel-hid INT33D5:00: event 0xcc
[   38.820977] intel-hid INT33D5:00: event 0xcd

AC+tablet:
[    4.536992] intel-hid INT33D5:00: platform supports 5 button array
[    4.537106] intel-hid INT33D5:00: use alternative tablet mode flag
[    4.537107] intel-hid INT33D5:00: vgbs: 0x0 --- mode: 1
[   25.096174] intel-hid INT33D5:00: event 0xcc
[   65.620308] intel-hid INT33D5:00: event 0xcc
[   72.094802] intel-hid INT33D5:00: event 0xcc
[   75.020589] intel-hid INT33D5:00: event 0xcd

Battery+laptop:
[    4.578901] intel-hid INT33D5:00: platform supports 5 button array
[    4.579014] intel-hid INT33D5:00: use alternative tablet mode flag
[    4.579017] intel-hid INT33D5:00: vgbs: 0x40 --- mode: 0
[    5.233264] intel-hid INT33D5:00: event 0xcd
[   36.489287] intel-hid INT33D5:00: event 0xcc
[   37.952181] intel-hid INT33D5:00: event 0xcd

Battery+tablet:
[    4.650280] intel-hid INT33D5:00: platform supports 5 button array
[    4.650345] intel-hid INT33D5:00: use alternative tablet mode flag
[    4.650345] intel-hid INT33D5:00: vgbs: 0x0 --- mode: 1
[    5.360908] intel-hid INT33D5:00: event 0xcc
[   69.815671] intel-hid INT33D5:00: event 0xcc
[   71.892741] intel-hid INT33D5:00: event 0xcd
Comment 18 Elia Devito 2020-10-25 18:42:06 UTC
Created attachment 293185 [details]
add support for SW_TABLET_MODE
Comment 19 Elia Devito 2020-10-25 18:42:22 UTC
Hi Benjamin,

(In reply to Benjamin Hennion from comment #17)
> Hi Elia,
> First of all, thanks a lot for your work!
It's a pleasure, thanks for your collaboration.

> Since on battery, a 0xcc or 0xcd event is always issued right after your
> module's initialization (why is that?), it may be enough, in my laptop's
> case, to always assume we have booted in laptop mode and only change when
> receiving a 0xcc event. This may have been the idea behing your alternate
> flag, but the line
yes this behaviour is behind my idea

> #define TABLET_MODE_FLAG_ALT (TABLET_MODE_FLAG | 0x0)
> just sets TABLET_MODE_FLAG_ALT to TABLET_MODE_FLAG.

effectively this is wrong as the alternative flag idea itsefl.

I have removed tablet_mode_alt_flag parameter in favour of ignore_vgbs_return 
that simply ignores the vgbs method return and use only the events to set tablet mode switch.

The new parameter should be automatically enabled on you notebook model, you can check this of dmesg output.

I'm waiting for your feedback
Comment 20 Benjamin Hennion 2020-10-26 17:55:02 UTC
Hi Elia,

So, I tested your last patch. All in all, even if the behaviour is not entirely nominal, this is much better. For starters, I don't get stuck with no keyboard on my DM anymore :-).

1- my laptop is well recognized and the module indeed ignores the VGBS return. 
2- the behaviour on battery is unchanged. It still works as expected
3- on AC: it works well when I boot in laptop mode. In tablet mode, the behaviour is erratic: it boots as if in laptop mode, until some 0xcc event is issued at a seemingly random time.

In this instance:
[    4.672082] kernel: intel-hid INT33D5:00: platform supports 5 button array
[    4.672190] kernel: intel-hid INT33D5:00: Ignore vgbs return
[   63.084592] kernel: intel-hid INT33D5:00: event 0xcc
[   73.583117] kernel: intel-hid INT33D5:00: event 0xcc
you can see two events were triggered more than a minute after boot. This was long after I reached my desktop, and I did not move my lid. In another instance, those events were simple never issued (for at least 3 minutes):
[    4.642453] kernel: intel-hid INT33D5:00: platform supports 5 button array
[    4.642556] kernel: intel-hid INT33D5:00: Ignore vgbs return

I would love to understand if something triggers those erratic events. They are also sometimes issued in laptop-mode (on AC, of course). It explains why sometimes (rarely), with previous versions of the patch, the touchpad/keyboard started working out of the blue after some time. I don't know how I could investigate this further though.
Comment 21 Benjamin Hennion 2020-10-26 18:01:42 UTC
Hey,

I also made more advanced testing with suspension and hibernation (on AC). The device is waken up from suspension by the tablet-mode switch, and the event is passed to the module: this works flawlessly. For hibernation, it also works. For instance, I got the following dmesg by hibernating in laptop-mode, flipping the screen to tablet-mode and pressing the power button to resume:

[ 2388.989418] kernel: intel-hid INT33D5:00: event 0xcc
[ 2388.989545] kernel: intel-hid INT33D5:00: event 0xce
[ 2388.989547] kernel: intel-hid INT33D5:00: event 0xcf
[ 2388.989548] kernel: intel-hid INT33D5:00: unknown event 0xcf

0xcc is the tablet mode (great!), the other events correspond to the power button. The unknown event does not seem to matter (= power button release, I think).

This is pretty much anything you can hope for.
Comment 22 Benjamin Hennion 2020-10-26 19:06:35 UTC
By the way, maybe a parameter to force the use of VGBS values (for devices on the blacklist) would be nice, for easy testing whenever the firmware is updated.
Comment 23 Elia Devito 2020-11-11 16:15:33 UTC
Created attachment 293627 [details]
add support for SW_TABLET_MODE
Comment 24 Elia Devito 2020-11-11 16:16:43 UTC
Hi Benjamin,
I updated the patch to prepare it for submission for kernel merge, now it offer
two parameter:
- tablet_mode_switch (0: disable, 1: enable, -1: auto[default])
- ignore_vgbs_return

by default your notebook model has tablet_mode_switch disabled,
you can manually enable it via the parameters above (I think this is the best
solutions until we can get everything working properly).

I have noted that dell has released a new bios version (1.4.1) for your notebook,
can you test it with this new patch a tell me is if something has changed?
Comment 25 Benjamin Hennion 2020-11-12 09:27:16 UTC
Hi Elia,

I updated my BIOS and applied your patch. After enabling the tablet_mode_switch, I see no change in behaviour from the previous version. So your patch still works and Dell's firmware is still buggy.
Comment 26 Hans de Goede 2021-02-15 11:48:44 UTC
Elia's patches for this are available in the just released 5.12 kernel, so this bug can be closed.

Note You need to log in before you can comment on or make changes to this bug.