jakeday / linux-surface

Linux Kernel for Surface Devices
2.6k stars 243 forks source link

Setup.sh doesn't work for pen/touch on Surface Pro 2017 LTE (info for fix) #549

Open qorsair opened 5 years ago

qorsair commented 5 years ago

The setup.sh file uses incorrect settings for the Surface Pro 2017 LTE

I have been troubleshooting my pen/touchscreen in portrait mode (incorrect x,y location relative to touch) and figured out the 2017-LTE model doesn't have the Surface Pro 2017 touchscreen, it has the same model as the Surface Pro 6.

The SKU output from dmidecode | grep sku:

SKU Number: Surface_Pro_1807 SKU Number: Not Specified

Product name from dmidecode:

Product Name: Surface Pro

Output from sudo evtest:

Available devices: /dev/input/event0: Lid Switch /dev/input/event1: Video Bus /dev/input/event2: Microsoft Surface Type Cover Keyboard /dev/input/event3: Microsoft Surface Type Cover Mouse /dev/input/event4: Microsoft Surface Type Cover Consumer Control /dev/input/event5: Microsoft Surface Type Cover Touchpad /dev/input/event6: ipts 045E:001F UNKNOWN /dev/input/event7: ipts 045E:001F /dev/input/event8: ipts 045E:001F Touchscreen /dev/input/event9: ipts 045E:001F Mouse /dev/input/event10: ipts 045E:001F UNKNOWN /dev/input/event11: HDA Intel PCH Mic /dev/input/event12: HDA Intel PCH Headphone /dev/input/event13: gpio-keys /dev/input/event14: gpio-keys

I set SUR_MODEL="Surface Pro 6" in the setup.sh, and ran it again, now my touchscreen works as expected in portrait mode.

Rakksor commented 5 years ago

I have the same problem, the touchscreen input is not rotated with the screen on a Surface Pro 2017 LTE.

Choosing the Surface Pro 6 instead in the setup script didn't help though, and when looking at the setup script code it copies the same firmware files for both models. Are you sure this is the only thing you changed?

I'm using Gnome with Wayland on Arch Linux (with dmhackers version of the kernel). Using older versions of the kernel did not help either, but I remember it working a couple of months ago. Could this be an Arch specific problem due to a package update?

StollD commented 5 years ago

I have the same problem, the touchscreen input is not rotated with the screen on a Surface Pro 2017 LTE.

[...]

I'm using Gnome with Wayland on Arch Linux (with dmhackers version of the kernel). Using older versions of the kernel did not help either, but I remember it working a couple of months ago. Could this be an Arch specific problem due to a package update?

This is not caused by a change in the kernel, but by a change in how GNOME/mutter handle touchscreen input mapping. I don't really understand the reasons for it, but I faced the same bug on Fedora, and tracked it down to one commit. [1]

Interestingly, the stylus still rotates correctly, even without a patched libwacom in the latest GNOME/mutter version.

The fix is either to compile mutter with that commit reverted, or to downgrade mutter to 3.32.1 which doesn't contain the change (it got added in 3.32.2).

However, downgrading to an older version will likely break when GNOME 3.34 is released, as mutter and gnome shell are integrated very tightly.

Rakksor commented 5 years ago

That was it, thank you! I compiled mutter 3.32.2 with the commit you mentioned reverted and it works again.

I don't really understand what is happening as the touchscreen size is reported:

sudo libinput list-devices
...

Device:           ipts 1B96:001F Touchscreen
Kernel:           /dev/input/event13
Group:            4
Seat:             seat0, default
Size:             259x171mm
Capabilities:     touch 
Tap-to-click:     n/a
Tap-and-drag:     n/a
Tap drag lock:    n/a
Left-handed:      n/a
Nat.scrolling:    n/a
Middle emulation: n/a
Calibration:      identity matrix
Scroll methods:   none
Click methods:    none
Disable-w-typing: n/a
Accel profiles:   n/a
Rotation:         n/a

...

But if I understand the code correctly the reported touchscreen size doesn't match any monitor entry, so with the change in the commit it then maps the touchscreen to the builtin monitor which would be correct but somehow breaks rotation. With the commit reverted it doesn't map the touchscreen at all but it works for some reason.

Below is the PKGBUILD and package for the modified version of mutter in case there are any other Arch users having this issue.

PKGBUILD: mutter.zip package: mutter-3.32.2+43+gb7f158811-1-x86_64.pkg.zip

To install the package:

unzip mutter-3.32.2+43+gb7f158811-1-x86_64.pkg.zip
sudo pacman -U mutter-3.32.2+43+gb7f158811-1-x86_64.pkg.tar.xz
kitakar5525 commented 5 years ago

On Surface 3 (non-pro), I don't have this issue. The device does not use IPTS. Currently, 0005-ipts.patch file patches hid-multitouch.c. So, I looked into the patch.

Current patch to hid-multitouch.c that 0005-ipts.patch file contains

```diff diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index b603c14d043b..03448d3a29f2 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -169,6 +169,7 @@ struct mt_device { static void mt_post_parse_default_settings(struct mt_device *td,                     struct mt_application *app); static void mt_post_parse(struct mt_device *td, struct mt_application *app); +static int cc_seen = 0; /* classes of device behavior */ #define MT_CLS_DEFAULT             0x0001 @@ -795,8 +796,11 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,            app->scantime_logical_max = field->logical_maximum;            return 1;        case HID_DG_CONTACTCOUNT: -           app->have_contact_count = true; -           app->raw_cc = &field->value[usage->usage_index]; +           if(cc_seen != 1) { +               app->have_contact_count = true; +               app->raw_cc = &field->value[usage->usage_index]; +               cc_seen++; +           }            return 1;        case HID_DG_AZIMUTH:            /* @@ -1286,9 +1290,11 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,     field->application != HID_DG_TOUCHSCREEN &&     field->application != HID_DG_PEN &&     field->application != HID_DG_TOUCHPAD && +    field->application != HID_GD_MOUSE &&     field->application != HID_GD_KEYBOARD &&     field->application != HID_GD_SYSTEM_CONTROL &&     field->application != HID_CP_CONSUMER_CONTROL && +    field->logical != HID_DG_TOUCHSCREEN &&     field->application != HID_GD_WIRELESS_RADIO_CTLS &&     field->application != HID_GD_SYSTEM_MULTIAXIS &&     !(field->application == HID_VD_ASUS_CUSTOM_MEDIA_KEYS && @@ -1340,6 +1346,14 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,    struct mt_device *td = hid_get_drvdata(hdev);    struct mt_report_data *rdata; +   if (field->application == HID_DG_TOUCHSCREEN || +           field->application == HID_DG_TOUCHPAD) { +       if (usage->type == EV_KEY || usage->type == EV_ABS) +           set_bit(usage->type, hi->input->evbit); + +       return -1; +   } +    rdata = mt_find_report_data(td, field->report);    if (rdata && rdata->is_mt_collection) {        /* We own these mappings, tell hid-input to ignore them */ @@ -1551,12 +1565,13 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)        /* already handled by hid core */        break;    case HID_DG_TOUCHSCREEN: -       /* we do not set suffix = "Touchscreen" */ +       suffix = "Touchscreen";        hi->input->name = hdev->name;        break;    case HID_DG_STYLUS:        /* force BTN_STYLUS to allow tablet matching in udev */        __set_bit(BTN_STYLUS, hi->input->keybit); +       __set_bit(INPUT_PROP_DIRECT, hi->input->propbit);        break;    case HID_VD_ASUS_CUSTOM_MEDIA_KEYS:        suffix = "Custom Media Keys"; @@ -1672,6 +1687,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)    td->hdev = hdev;    td->mtclass = *mtclass;    td->inputmode_value = MT_INPUTMODE_TOUCHSCREEN; +   cc_seen = 0;    hid_set_drvdata(hdev, td);    INIT_LIST_HEAD(&td->applications); ```

and I found that

+       field->logical != HID_DG_TOUCHSCREEN &&

is the cause.

I changed logical to application and this issue has been fixed.

ipts-hid-multitouch-change-logical-to-application.patch

```diff From cabafc92056f4f9e420602530eda3d2cb89de69c Mon Sep 17 00:00:00 2001 From: kitakar5525 <34676735+kitakar5525@users.noreply.github.com> Date: Wed, 28 Aug 2019 17:54:10 +0900 Subject: [PATCH] ipts: hid-multitouch: change logical to application --- drivers/hid/hid-multitouch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index c3820755d..5ad415002 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -1294,7 +1294,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,     field->application != HID_GD_KEYBOARD &&     field->application != HID_GD_SYSTEM_CONTROL &&     field->application != HID_CP_CONSUMER_CONTROL && -    field->logical != HID_DG_TOUCHSCREEN && +    field->application != HID_DG_TOUCHSCREEN &&     field->application != HID_GD_WIRELESS_RADIO_CTLS &&     field->application != HID_GD_SYSTEM_MULTIAXIS &&     !(field->application == HID_VD_ASUS_CUSTOM_MEDIA_KEYS && -- 2.23.0 ```

This change also removed UNKNOWN device:

# before
$ libinput list-devices | grep ipts
Device:           ipts 1B96:005E UNKNOWN
Device:           ipts 1B96:005E
Device:           ipts 1B96:005E Touchscreen
Device:           ipts 1B96:005E Mouse

# after
$ libinput list-devices | grep ipts
Device:           ipts 1B96:005E
Device:           ipts 1B96:005E Touchscreen
Device:           ipts 1B96:005E Mouse

I'm not sure if the logical has another purpose or not, though.

StollD commented 5 years ago

This indeed fixes the input mapping with the latest mutter version, awesome!

You can actually make the patch even simpler, because field->application != HID_DG_TOUCHSCREEN is a condition earlier already, so adding it a second time makes it a effectively a NOP.

I'm not sure if the logical has another purpose or not, though.

According to the original Intel commit [1] it makes the IPTS single touch mode work. Interestingly, when I enabled single touch mode on an unpached kernel, the input mapping worked again too. When enabling it with the above patch, no touch inputs were processed anymore.

# Enable single touch mode
$ echo 0 | sudo tee /sys/kernel/debug/ipts/mode

# Disable it again
$ echo 1 | sudo tee /sys/kernel/debug/ipts/mode
kitakar5525 commented 5 years ago

You can actually make the patch even simpler, because field->application != HID_DG_TOUCHSCREEN is a condition earlier already

Ah, I didn't see that.

When enabling it with the above patch, no touch inputs were processed anymore

Indeed, that patch broke the single touch mode for me, too.

It seems that the UNKNOWN device ipts 1B96:005E UNKNOWN is used when IPTS is in single touch mode.

kitakar5525 commented 5 years ago

I tried giving a name to single and multi touch devices

ipts-hid-multitouch-give-a-name-to-single-and-multi-.patch

```diff From 7f662a5efe25608cfc8e009cb02e63478a71e384 Mon Sep 17 00:00:00 2001 From: kitakar5525 <34676735+kitakar5525@users.noreply.github.com> Date: Wed, 28 Aug 2019 22:39:16 +0900 Subject: [PATCH] ipts: hid-multitouch: give a name to single and multi touch devices --- drivers/hid/hid-multitouch.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index c3820755d..a914c119c 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -1551,6 +1551,12 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)            if (ret)                return ret;        } + +       if (report->field[0]->logical == HID_DG_TOUCHSCREEN) { +           suffix = "Touchscreen single touch"; +           /* force BTN_STYLUS to allow tablet matching in udev */ +           __set_bit(BTN_STYLUS, hi->input->keybit); +       }    }    switch (hi->application) { @@ -1565,7 +1571,7 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)        /* already handled by hid core */        break;    case HID_DG_TOUCHSCREEN: -       suffix = "Touchscreen"; +       suffix = "Touchscreen multi touch";        hi->input->name = hdev->name;        break;    case HID_DG_STYLUS: @@ -1577,7 +1583,8 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)        suffix = "Custom Media Keys";        break;    default: -       suffix = "UNKNOWN"; +       if (!suffix) +           suffix = "UNKNOWN";        break;    } -- 2.23.0 ```

$ libinput list-devices | grep ipts
Device:           ipts 1B96:005E Touchscreen single touch
Device:           ipts 1B96:005E
Device:           ipts 1B96:005E Touchscreen multi touch
Device:           ipts 1B96:005E Mouse

and rotation and single touch are working. However, in single touch mode, it started showing mouse cursor with finger touch. Strange.


EDIT: It seems that __set_bit(BTN_STYLUS, hi->input->keybit); is doing something. When I commented it out, rotation broke again. Why they added BTN_STYLUS to single touch?

StollD commented 5 years ago

However, in single touch mode, it started showing mouse cursor with finger touch. Strange.

Another side effect is that the actual stylus doesn't work anymore. There exists a BTN_STYLUS2, so I am not sure how much of an effect using that for single touch would have.

EDIT: It seems that __set_bit(BTN_STYLUS, hi->input->keybit); is doing something. When I commented it out, rotation broke again. Why they added BTN_STYLUS to single touch?

Because userspace applications have no way to check which one of the two touchscreens is active, and then rely on a touchscreen that isn't even in use (see: mutter).

And since the IPTS implementation from Intel feels more like a tech demo that something that would go upstream, it makes sense to expose it as a Stylus, just to get the features to work.

That said, does anyone use the single touch mode? Not that it should be removed, but it feels very obscure to me, so I am curious.

kitakar5525 commented 5 years ago

Because userspace applications have no way to check which one of the two touchscreens is active, and then rely on a touchscreen that isn't even in use (see: mutter).

I figured it out! I think you mean mutter rotated only one touchscreen device which has lower event number (/dev/input/event??).

Regarding Pen input not working on single touch mode, I think it is expected. On single touch with non-patched IPTS patch, the pen input is not working anyway.

I tried changing it to BTN_STYLUS2, the result is

StollD commented 5 years ago

I figured it out! I think you mean mutter rotated only one touchscreen device which has lower event number (/dev/input/event??).

Yeah. Mutter just uses the first one it can find, because it doesn't have logic to handle two touchscreen devices on one physical device.

Regarding Pen input not working on single touch mode, I think it is expected. On single touch with non-patched IPTS patch, the pen input is not working anyway.

Ah ok.

I tried changing it to BTN_STYLUS2, the result is

  • It broked rotation on multi touch mode again

  • Pen input is still not working on single touch mode

  • The mouse cursor stopped showing with finger touch on single touch mode

I guess that means that the stylus gets disabled in hardware in single touch mode. I don't think the kernel cannot handle more than one stylus at the same time to be honest.

kitakar5525 commented 5 years ago

Ah… I noticed that if I set BTN_STYLUS to single touch, it breaks pen rotation now.

StollD commented 5 years ago

Try removing the patched libwacom. It works fine for me with the stock one, but when I install the patched one pen rotation breaks.

I think this is a GNOME specific quirk, other window managers will probably still need the patched libwacom.

kitakar5525 commented 5 years ago

Try removing the patched libwacom.

Yeah, it worked but "Wacom Tablet" section in GNOME settings will not be available anymore (on Wayland).

We may want to consider moving the single touch device below multi touch device (How?) or even disabling the device in order to achieve these things at the same time:

I'm personally happy with just disabling the single touch device for now:

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index a914c119c..d5f3c4591 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1554,8 +1554,11 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)

                if (report->field[0]->logical == HID_DG_TOUCHSCREEN) {
                        suffix = "Touchscreen single touch";
-                       /* force BTN_STYLUS to allow tablet matching in udev */
-                       __set_bit(BTN_STYLUS, hi->input->keybit);
+                       /*
+                        * Disable single touch device for now.
+                        * Clearing the `BTN_TOUCH` will disable the device.
+                        */
+                       __clear_bit(BTN_TOUCH, hi->input->keybit);
                }
        }

The proper way is we make mutter handle the two touchscreen device at the same time. It seems not easy for me.

kitakar5525 commented 5 years ago

I just reverted all the changes to hid-multitouch.c made by 0005-ipts.patch on 5.2 and it seems to be working without any problem (except single touch device is not available anymore).

ipts-hid-multitouch-revert-all-the-changes.patch

```diff From 2a74bf487ec419a99fa2c11c755d715f40efdc8d Mon Sep 17 00:00:00 2001 From: kitakar5525 <34676735+kitakar5525@users.noreply.github.com> Date: Wed, 28 Aug 2019 15:19:47 +0900 Subject: [PATCH] ipts: hid-multitouch: revert all the changes This commit will revert all the changes made by 0005-ipts.patch on 5.2. --- drivers/hid/hid-multitouch.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index c3820755d..cc22f5dff 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -169,7 +169,6 @@ struct mt_device { static void mt_post_parse_default_settings(struct mt_device *td, struct mt_application *app); static void mt_post_parse(struct mt_device *td, struct mt_application *app); -static int cc_seen = 0; /* classes of device behavior */ #define MT_CLS_DEFAULT 0x0001 @@ -796,11 +795,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, app->scantime_logical_max = field->logical_maximum; return 1; case HID_DG_CONTACTCOUNT: - if(cc_seen != 1) { - app->have_contact_count = true; - app->raw_cc = &field->value[usage->usage_index]; - cc_seen++; - } + app->have_contact_count = true; + app->raw_cc = &field->value[usage->usage_index]; return 1; case HID_DG_AZIMUTH: /* @@ -1290,11 +1286,9 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, field->application != HID_DG_TOUCHSCREEN && field->application != HID_DG_PEN && field->application != HID_DG_TOUCHPAD && - field->application != HID_GD_MOUSE && field->application != HID_GD_KEYBOARD && field->application != HID_GD_SYSTEM_CONTROL && field->application != HID_CP_CONSUMER_CONTROL && - field->logical != HID_DG_TOUCHSCREEN && field->application != HID_GD_WIRELESS_RADIO_CTLS && field->application != HID_GD_SYSTEM_MULTIAXIS && !(field->application == HID_VD_ASUS_CUSTOM_MEDIA_KEYS && @@ -1346,14 +1340,6 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi, struct mt_device *td = hid_get_drvdata(hdev); struct mt_report_data *rdata; - if (field->application == HID_DG_TOUCHSCREEN || - field->application == HID_DG_TOUCHPAD) { - if (usage->type == EV_KEY || usage->type == EV_ABS) - set_bit(usage->type, hi->input->evbit); - - return -1; - } - rdata = mt_find_report_data(td, field->report); if (rdata && rdata->is_mt_collection) { /* We own these mappings, tell hid-input to ignore them */ @@ -1565,13 +1551,12 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi) /* already handled by hid core */ break; case HID_DG_TOUCHSCREEN: - suffix = "Touchscreen"; + /* we do not set suffix = "Touchscreen" */ hi->input->name = hdev->name; break; case HID_DG_STYLUS: /* force BTN_STYLUS to allow tablet matching in udev */ __set_bit(BTN_STYLUS, hi->input->keybit); - __set_bit(INPUT_PROP_DIRECT, hi->input->propbit); break; case HID_VD_ASUS_CUSTOM_MEDIA_KEYS: suffix = "Custom Media Keys"; @@ -1687,7 +1672,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) td->hdev = hdev; td->mtclass = *mtclass; td->inputmode_value = MT_INPUTMODE_TOUCHSCREEN; - cc_seen = 0; hid_set_drvdata(hdev, td); INIT_LIST_HEAD(&td->applications); -- 2.23.0 ```

# no suffix without patch to hid-multitouch.c
$ libinput list-devices | grep ipts
Device:           ipts 1B96:005E
Device:           ipts 1B96:005E
StollD commented 5 years ago

I just reverted all the changes to hid-multitouch.c made by 0005-ipts.patch on 5.2 and it seems to be working without any problem (except single touch device is not available anymore).

I hope we can add suffix by IPTS driver side rather than patching to hid-multitouch.c.

The suffix is not actually needed anywhere as far as I can see. Only the udev rule configs from this repository use it, and those can be removed safely on my end, without breaking touch or stylus functionality. Since the kernel doesn't add a suffix without a patch, it's impossible that userspace tools rely on this either (at least unless they were written explicitly for surface / IPTS).

  • Does anyone know what is the purpose of the device ipts 1B96:005E Mouse? It seems that it's not used by any input for me.

If it isn't used we might consider editing it out of the HID descriptor for IPTS (intel_desc.bin and vendor_desc.bin concatinated together). We might also edit out the single touch device, if thats not going to be supported by the kernel (ofc with proper documentation on how to revert the change). The kernel would just forget about the existence of those devices.

I tried this out on my surface and multi-touch + stylus are still working normaly.

If anyone is interested in the decompiled HID descriptor (for SB2 or any other model), I can upload it.