Bug 103749 - SW_TABLET_MODE disables monitor bezel button in tablet mode
Summary: SW_TABLET_MODE disables monitor bezel button in tablet mode
Status: RESOLVED FIXED
Alias: None
Product: Wayland
Classification: Unclassified
Component: libinput (show other bugs)
Version: unspecified
Hardware: Other Linux (All)
: medium normal
Assignee: Davide Depau
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-15 00:37 UTC by Phoebe
Modified: 2018-06-03 22:21 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
evemu-record output on /dev/input/event4 (10.24 KB, text/plain)
2017-11-15 03:35 UTC, Phoebe
Details
xinput list-props for the device (417 bytes, text/plain)
2017-11-15 03:38 UTC, Phoebe
Details
0001-evdev-don-t-disable-the-keyboard-on-the-Yoga-1-in-ta.patch (2.45 KB, patch)
2017-12-18 05:34 UTC, Peter Hutterer
Details | Splinter Review
libinput debug-events --verbose with patch, ThinkPad X1 Yoga 1st (6.83 KB, text/plain)
2018-03-19 08:43 UTC, Davide Depau
Details
libinput debug-events --verbose with patch on ThinkPad X60 Tablet (5.00 KB, text/plain)
2018-03-19 13:52 UTC, Michał Kopeć
Details
0001-evdev-don-t-suspend-keyboard-on-the-X1-Yoga-1st-DEPAU.patch (3.08 KB, patch)
2018-03-20 16:20 UTC, Davide Depau
Details | Splinter Review
email garbage (4.56 KB, text/html)
2018-04-03 15:30 UTC, Davide Depau
Details
0001-evdev-don-t-suspend-keyboard-on-ThinkPad-X1-Yoga-1st.patch (2.89 KB, patch)
2018-04-17 13:25 UTC, Davide Depau
Details | Splinter Review
Same as previous patch, fixed style (2.89 KB, patch)
2018-04-17 13:28 UTC, Davide Depau
Details | Splinter Review
evemu-describe report on keyboard device of ThinkPad X41t (6.44 KB, text/plain)
2018-06-03 12:36 UTC, Sergey Koziakov
Details

Description Phoebe 2017-11-15 00:37:09 UTC
Component/OS/Platform Detail: 

- libinput 1.9.1 on Xorg. Arch Linux (4.13.11-1)
- X1 Yoga laptop
- Hotkey handling with shxkd (can be reproduced with xev)

Description:

Converting the laptop to tablet mode disables the capacitive touch (windows) button on the laptop's lower screen bezel. I believe this is not the intended behaviour, that button should continue to behave as usual (since it is still "user facing") in tablet mode.

Ordinarily (in laptop mode) the bezel button can be mapped via shxkd with keycode 133 (Super_L), but in tablet mode these bindings stop working.

Steps to reproduce:

- In laptop mode, run xev (filtering KeyPress events)
$ xev | awk -F'[ )]+' '/^KeyPress/ { a[NR+2] } NR in a { printf "%-3s %s\n", $5, $8 }'

- tapping the bezel button repeatedly produces:
133 Super_L
133 Super_L
133 Super_L

- with xev still running, switch to tablet mode
- tapping the windows button produces no output.
Comment 1 Phoebe 2017-11-15 00:50:00 UTC
Moved from Wayland to xorg, didn't realise libinput had a component listing under it.
Comment 2 Peter Hutterer 2017-11-15 01:25:00 UTC
The xorg Input/libinput component is for the xf86-input-libinput driver, your first guess was correct :)


I'll need an evemu-record of the device node that provides that button, thanks.
Comment 3 Phoebe 2017-11-15 03:35:58 UTC
Created attachment 135477 [details]
evemu-record output on /dev/input/event4

Ah, whoops!

I've attached a recording from evemu-record. I tapped the bezel button twice, so there are two sets of events.
Comment 4 Phoebe 2017-11-15 03:38:40 UTC
Created attachment 135478 [details]
xinput list-props for the device
Comment 5 Peter Hutterer 2017-11-15 03:54:23 UTC
well, great. this is apparently wired up through the normal keyboard and pretends to be a windows key. this is going to get a bit more complicated than I expected because libinput needs to:
* have a hw quirk to note which event codes are permitted on this device even when tablet mode is on
* have libinput handle this particular case accordingly
* have test cases for it

It's not technically hard, just needs the code to be written. If you want to have a crack at it let me know because I'm pretty swamped right now.
Comment 6 Phoebe 2017-11-15 04:06:03 UTC
Sure, I'll have a crack at it.

Do you know off hand of any similar issues that have been solved in the past that might serve as a reference? I'm open to any guidance or suggestions you might have.
Comment 7 Peter Hutterer 2017-11-15 04:16:00 UTC
not for this particular thing. the todo list is:
* write a test case, tablet_mode_disable_keyboard should serve as good template. That way you can quickly test and iterate. Make yourself familiar with the test suite, see https://wayland.freedesktop.org/libinput/doc/latest/test-suite.html
* the hw quirk will need a hwdb entry, LIBINPUT_ATTR_TABLET_MODE_EXCLUDED_KEYS or so. look at the history of the .hwdb entry to get a feel for how these are added. Don't forget the parsing tests and the parser test
* for a device without that property empty keep the current code, might as well keep it fully disabled (less wakeups)
* for devices with a property, set some state and discard any events except the quirked ones.

I think that should be about it.
Comment 8 Phoebe 2017-11-15 07:30:20 UTC
Point me elsewhere if this is the wrong place to ask this stuff, but-

I'm having trouble getting the full test suite to run properly on the unmodified code. Is there some clever evocation to ignore a test group? 

I've followed the instructions to avoid tests interfering with xorg, but something in (as far as I can tell) the lid:buggy group is triggering a system shutdown.
Comment 9 Peter Hutterer 2017-11-15 07:49:16 UTC
there's nothing to ignore a group, sorry. short of just #if 0  in the code :)

The issue is likely one of the tests that triggers an SW_LID which logind takes as signal to suspend. /etc/systemd/logind.conf has HandleLidSwitch which you can set to ignore. That should do the trick.
Comment 10 Phoebe 2017-11-18 05:29:46 UTC
Thank you, updating logind.conf did the trick.

Just thought I'd drop a comment to say that I'm making modest progress on this fix. (on wrapping my head around libinput to some extent, anyway..)

It might be of some interest that on these particular laptops the keyboard is mechanically disabled in tablet mode, I wonder if there are other devices in the wild that share this win-key design but do not have a built in mechanism to disable the keyboard.

It is tempting to just bypass the suspend/resume keyboard calls during tablet mode.

Does it make sense to add a keyfilter? Might it just be unnecessary complexity if no other manufacturers are using the internal keyboard to proxy the bezel button?
Comment 11 Peter Hutterer 2017-11-18 10:38:44 UTC
oh, good point. yes, if the keyboard is disabled anyway then it's much better to just have a hw flag for this model and skip the whole code that manually disables the keyboard.
Comment 12 Peter Hutterer 2017-11-29 02:48:41 UTC
Hey Phoebe, just wondering what the status of this is?
Comment 13 Phoebe 2017-12-02 23:22:12 UTC
Hi Peter,

Life got a bit busy so I haven't been had time to tidy this up to submit a patch.

I also ran into some issues trying to find a way to actually identify the machine(/keyboard) to write the appropriate udev rule. I was planning on posting back for advice once I had a spare evening to get back to this.

Let me know if you'd rather take over and if there's anything I can contribute to help that along.
Comment 14 Peter Hutterer 2017-12-03 01:20:32 UTC
To identify the machine, use a DMI match, see http://wayland.freedesktop.org/libinput/doc/latest/udev_config.html#hwdb for more info. If you already have something semi-working, you could push it somewhere (or just send me the patch) and I can see if I can clean it up quickly. Not sure if I  get to it before you do though :)
Comment 15 Peter Hutterer 2017-12-18 05:34:29 UTC
Created attachment 136243 [details] [review]
0001-evdev-don-t-disable-the-keyboard-on-the-Yoga-1-in-ta.patch

Untested, please give it a try, thanks.
Comment 16 Peter Hutterer 2018-01-08 00:06:15 UTC
ping?
Comment 17 Phoebe 2018-01-15 00:30:52 UTC
I'm here! Sorry! Back from holidays.

I shall try building and get back to you.
Comment 18 Peter Hutterer 2018-02-01 04:59:10 UTC
sorry, closing after another 2+ weeks in needinfo. I can't merge these things without testing the hwdb is not something I can test locally unless I have the exact same device.
Comment 19 Michał Kopeć 2018-03-14 15:56:56 UTC
I have the same problem with my ThinkPad X60 hardware tablet buttons. Switching the machine to laptop mode (rotating the screen) disables the buttons located on the screen bezel. 

Components:
- libinput 1.10.2
- Arch Linux (kernel 4.15.8)
- GNOME Shell 3.26.2
Comment 20 Michał Kopeć 2018-03-14 17:27:41 UTC
I've managed to adapt the patch (comment #15) to my specific machine and latest libinput version, but there's no change in behaviour. Let me know if you want me to run any tests or provide information about my configuration.
Comment 21 Peter Hutterer 2018-03-19 00:27:01 UTC
Does the LIBINPUT_MODEL_TABLET_MODE_NEEDS_KEYBOARD udev property show up on the device when you apply the hwdb (and you'll need to restart too)? Please attach the output of libinput debug-events --verbose with the patch applied
Comment 22 Davide Depau 2018-03-19 08:43:36 UTC
Created attachment 138192 [details]
libinput debug-events --verbose with patch, ThinkPad X1 Yoga 1st

Coming here from 101008. I applied the patch to Libinput (latest git, I added it to the AUR package: https://aur.archlinux.org/packages/libinput-git/, had a hard time applying it due to changes in evdev.h) and it seems like it's not working.

Side volume buttons and touchscreen Super key don't work. See `debug-events` with patch applied.
Comment 23 Michał Kopeć 2018-03-19 13:52:16 UTC
Created attachment 138195 [details]
libinput debug-events --verbose with patch on ThinkPad X60 Tablet

Yes, the property does show up correctly when running udevadm test /sys/class/input/event4. The patch does not seem to be working for me even after a restart.
Comment 24 Peter Hutterer 2018-03-20 01:18:59 UTC
(In reply to Davide Depau from comment #22)
> Created attachment 138192 [details]

event4  - tablet_mode_switch: activated for AT Translated Set 2 keyboard<->ThinkPad Extra Buttons


(In reply to Michał Kopeć from comment #23)
> Created attachment 138195 [details]

event4  - tablet_mode_switch: activated for AT Translated Set 2 keyboard<->ThinkPad Extra Buttons

same thing here

This indicates that the keyboard is still paired for the tablet mode switch. So either the udev property isn't applied correctly, or the patch isn't working.
Unfortunately I don't currently have time to work on this, sorry.

However, it's easy to debug locally, sudo gdb ./builddir/libinput-debug-events is all you need. The keyboard is only disabled in that libinput context so it won't affect your session and you can fake a tablet mode switch with sudo evemu-event --type EV_SW --code SW_TABLET_MODE --value 1 --sync (and then again with value 0).


> the property does show up correctly when running udevadm test

just fwwiw: udevadm info shows the current properties, udevadm test shows the ones to be applied. So without a restart, udevadm test won't really do anything.
Comment 25 Davide Depau 2018-03-20 16:20:05 UTC
Created attachment 138226 [details] [review]
0001-evdev-don-t-suspend-keyboard-on-the-X1-Yoga-1st-DEPAU.patch

I tried to check the code and came up with this patch, which fixes this issue.
For some reason, the touchpad not disabled issue seems fixed too. I really hope somebody fixed it upstream while I was working on this, otherwise I would have no idea on why it works.

I didn't spend too much time figuring out how libinput really works and how the functions I changed are called, so I might have fixed it in the wrong place.

Let me know what you think.
Comment 26 Davide Depau 2018-03-20 16:21:27 UTC
By the way, the patch should be applied on top of master.

See my fork: https://github.com/Depau/libinput/tree/tablet_mode
Comment 27 Peter Hutterer 2018-03-20 23:14:44 UTC
Comment on attachment 138226 [details] [review]
0001-evdev-don-t-suspend-keyboard-on-the-X1-Yoga-1st-DEPAU.patch

Review of attachment 138226 [details] [review]:
-----------------------------------------------------------------

This should work, but it's inefficient. You're still pairing the tablet mode and the keyboard device, installing an event handler, etc. But then you just return early when the event comes in. The original patch from comment 15 just skips the pairing, which means it should have the same effect. So it'd be useful to debug why the other patch didn't take effect in fallback_keyboard_pair_tablet_mode().

::: src/evdev-fallback.c
@@ +1151,4 @@
>  	swev = libinput_event_get_switch_event(event);
>  	if (libinput_event_switch_get_switch(swev) !=
>  	    LIBINPUT_SWITCH_TABLET_MODE)
> +        return;

detritus, please remove

@@ +1155,3 @@
>  
> +    if (device->model_flags & EVDEV_MODEL_TABLET_MODE_NO_SUSPEND)
> +        return;

please stick to the indentation rules the rest of libinput uses
Comment 28 Davide Depau 2018-03-20 23:19:29 UTC
(In reply to Peter Hutterer from comment #27)
> Comment on attachment 138226 [details] [review] [review]
> 0001-evdev-don-t-suspend-keyboard-on-the-X1-Yoga-1st-DEPAU.patch
> 
> Review of attachment 138226 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> This should work, but it's inefficient. You're still pairing the tablet mode
> and the keyboard device, installing an event handler, etc. But then you just
> return early when the event comes in. The original patch from comment 15
> just skips the pairing, which means it should have the same effect. So it'd
> be useful to debug why the other patch didn't take effect in
> fallback_keyboard_pair_tablet_mode().

I'll try to check again tomorrow or in the next few days and come up with something more efficient.
 
> ::: src/evdev-fallback.c
> @@ +1151,4 @@
> >  	swev = libinput_event_get_switch_event(event);
> >  	if (libinput_event_switch_get_switch(swev) !=
> >  	    LIBINPUT_SWITCH_TABLET_MODE)
> > +        return;
> 
> detritus, please remove
> 
> @@ +1155,3 @@
> >  
> > +    if (device->model_flags & EVDEV_MODEL_TABLET_MODE_NO_SUSPEND)
> > +        return;
> 
> please stick to the indentation rules the rest of libinput uses

IDE's fault ;)
Comment 29 Peter Hutterer 2018-04-03 06:41:38 UTC
ping?
Comment 30 Davide Depau 2018-04-03 15:30:48 UTC
Created attachment 138550 [details]
email garbage

Hello Peter,
sorry I've been quite busy lately and had almost no free time.
I plan on working on this in around two weeks once I get some important
stuff done.

On Tue, Apr 3, 2018, 8:41 AM <bugzilla-daemon@freedesktop.org> wrote:

> *Comment # 29 <https://bugs.freedesktop.org/show_bug.cgi?id=103749#c29> on
> bug 103749 <https://bugs.freedesktop.org/show_bug.cgi?id=103749> from Peter
> Hutterer <peter.hutterer@who-t.net> *
>
> ping?
>
> ------------------------------
> You are receiving this mail because:
>
>    - You are on the CC list for the bug.
>
> --

--

Davide Depau

Full-stack developer

http://www.gruppoyec.com


Tel: +39 327 798 7963 <3277987963>




Young Endeavor Consulting srls - via Tortona 33 Milano 20144.

P.IVA 01835450493 <0183%20545%200493>

Ai sensi del Decreto Legislativo n. 196/2003, si precisa che le
informazioni contenute in questo messaggio e negli eventuali allegati
sono riservate e per uso esclusivo del destinatario. Chiunque riceva
questo messaggio per errore, è pregato di distruggerlo e di informarci
immediatamente.
Comment 31 Peter Hutterer 2018-04-17 01:54:50 UTC
Closing due to needinfo, please re-open when the information was provided.
Comment 32 Davide Depau 2018-04-17 13:25:06 UTC
Created attachment 138882 [details] [review]
0001-evdev-don-t-suspend-keyboard-on-ThinkPad-X1-Yoga-1st.patch

Sorry I forgot about this.
Not tested. I will test it later.
Comment 33 Davide Depau 2018-04-17 13:28:46 UTC
Created attachment 138883 [details] [review]
Same as previous patch, fixed style

Damn I screwed up indentation style again. This should be good.
Comment 34 Peter Hutterer 2018-04-18 04:58:11 UTC
Thanks, fixed up a few more minor things and pushed as:

commit 5feaa5f00ce31a927924a6346706bf93451fa704
Author: Davide Depau <>
Date:   Tue Apr 17 15:22:42 2018 +0200

     evdev: don't suspend keyboard on ThinkPad X1 Yoga 1st in tablet mode
Comment 35 Davide Depau 2018-04-18 15:18:46 UTC
Great. I just tested it again and it works.
Comment 36 Sergey Koziakov 2018-06-03 12:36:38 UTC
Created attachment 139987 [details]
evemu-describe report on keyboard device of ThinkPad X41t

Hello! Sorry for writing in closed bug. I have encountered similar problem with ThinkPad X41t - according to evtest, lid buttons are part of 'main' AT Translated Set 2 keyboard, and when device is in tablet mode, they are disabled together with main keyboard. Is there a possibility to add similar fix for this device? I attached output from evemu-describe, and, if required, I will provide any additional information. Thanks in advance.
Comment 37 Peter Hutterer 2018-06-03 22:21:26 UTC
Sergey: I moved this to Bug 106799


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.