Bug 77161
Summary: | Synaptics Driver Does Not Report ABS_TOOL_WIDTH When Available | ||
---|---|---|---|
Product: | Drivers | Reporter: | Maxwell Anselm (silverhammermba) |
Component: | Input Devices | Assignee: | drivers_input-devices |
Status: | NEW --- | ||
Severity: | normal | CC: | anakin.cs, angizia89, balbi, dion, dmitry.torokhov, gabriele.mzt, grafi, ingo+kernel, jckeerthan, jed-kernel.org, lis.kernel, lukebenes, michael, mike.cloaked, najoll, nospam, rainer+kernelorg, sky, tore, unbrice |
Priority: | P1 | ||
Hardware: | All | ||
OS: | Linux | ||
Kernel Version: | 3.14.4 | Subsystem: | |
Regression: | No | Bisected commit-id: |
Description
Maxwell Anselm
2014-05-31 17:34:17 UTC
Most likely the reason why you don't always see ABS_TOOL_WIDTH reported is that events are emitted only when the value of an event code changes. Since there are few values possible for the width, the event is not sent as frequently as the others. I don't think that any ABS_TOOL_WIDTH events are generated at all. I've just tried evtool while attempting to generate events varing in width as far as I could, and in the 6401 events that I have recorded, ABS_PRESSURE has values between 0 and 255, and ABS_MT_PRESSURE between 33 and 255, but there's not a single ABS_TOOL_WIDTH event. evtool alleges that ABS_TOOL_WIDTH is supported, but I'm a bit surprised by the small Max value: Event code 28 (ABS_TOOL_WIDTH) Value 0 Min 0 Max 15 You are right, the events are not generated. You need to apply a patch like the one linked by Maxwell. @Maxwell I forgot to say something. The lack of some reports is due to the fact that the width is not reported if you are using the touchpad with more than one finger and less than four fingers. Also, the width is not reported if it's smaller than 4, as you can cleary see from the code of the patch. The reason is here explained: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/input/mouse/synaptics.c?id=a6ca40c11eb5d98e53176adf527e430f7037a8c9 I don't think we always need widths reported. For palm detection (which is how the bug started out), synaptics only considers single finger touches. Discarding small widths seems a little odd, since I believe that the driver lets you set the PalmMinWidth option as small as you like. Which driver? Not the kernel one since hardware always used low W values to mean things other than finger width. Hmm, maybe we need to set the limits to be 4..15 instead of 0..15 for ABS_TOOL_WIDTH. For multi-touch devices the size of the contact shoudl be reported by ABS_MT_TOUCH_MAJOR/ABS_MT_TOUCH_MINOR, not ABS_TOOL_WIDTH. I guess we still need ABS_TOOL_WIDTH for single-touch emulation events... Sorry, I was referring to the xf86-input-synaptics driver, which relies on the data from the kernel driver to do palm detection. It could be that Xorg needs to change their palm detection code as well, but as far as the kernel is concerned there is the fundamental problem that devices which claim to support ABS_TOOL_WIDTH never report it. I improved the patch linked here above. I tested it on my XPS13 9333 and seems to work fine. The width of all the fingers should now be reported. Note that when one than more one finger is present, the minimum width reported is 8. My touchpad does not report all the widths in the range 4-15, but only the followings: 4, 6, 8, 12, 15. diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c index f947292..80c8062 100644 --- a/drivers/input/mouse/synaptics.c +++ b/drivers/input/mouse/synaptics.c @@ -593,7 +593,9 @@ static void synaptics_parse_agm(const unsigned char buf[], switch (agm_packet_type) { case 1: /* Gesture packet: (x, y, z) half resolution */ - agm->w = hw->w; + agm->w = ((buf[1] & 0x01) | + (buf[2] & 0x01) << 1 | + (buf[5] & 0x01) << 2) + 8; agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1; agm->y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1; agm->z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1; @@ -814,6 +816,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot, input_report_abs(dev, ABS_MT_POSITION_X, hw->x); input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y)); input_report_abs(dev, ABS_MT_PRESSURE, hw->z); + if (hw->w >= 4) + input_report_abs(dev, ABS_MT_TOUCH_MAJOR, hw->w); } static void synaptics_report_mt_data(struct psmouse *psmouse, @@ -1444,6 +1448,7 @@ static void set_input_params(struct psmouse *psmouse, ABS_MT_POSITION_Y); /* Image sensors can report per-contact pressure */ input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0); + input_set_abs_params(dev, ABS_MT_TOUCH_MAJOR, 0, 15, 0, 0); input_mt_init_slots(dev, 2, INPUT_MT_POINTER); /* Image sensors can signal 4 and 5 finger clicks */ I noticed that the patch above sets the wrong range for ABS_MT_TOUCH_MAJOR. I also guess I should skip ABS_TOOL_WIDTH for image sensors, since ABS_MT_TOUCH_MAJOR is instead reported. alright, I still have the same problem with a Thinkpad X1 Carbon 2nd (from 2014) the problem is that none of the synaptics drivers are binding to the synaptics device, rather psmouse.ko is getting bound. That driver, however, doesn't report ABS_TOOL_WIDTH, so that's why palm rejection doesn't work. I'll see if I get some time to build v3.18 and see if the same behavior is still there, but it seems like we would need a synaptics_ps2.ko, perhaps ? Or, at least, add ABS_TOOL_WIDTH to psmouse.ko. Oh, now I see that psmouse.ko is built with synaptics.c, so for whatever reason, we're retuning before ABS_TOOL_WIDTH can be reported. Still the same with v3.18. ABS_TOOL_WIDTH is never reported Hi Felipe, I recently submitted two patches that should make the kernel emit ABS_MT_TOUCH_MAJOR to report widths, but they haven't been reviewed yet. Try them and see if they work. You can find the patches at the following addresses: https://patchwork.kernel.org/patch/5544561/ https://patchwork.kernel.org/patch/5544591/ Note that the current stable versions of xf86-input-synaptics ignores the values of ABS_MT_TOUCH_MAJOR, but the next release will take them into account: http://cgit.freedesktop.org/xorg/driver/xf86-input-synaptics/commit/?id=a897147be04d74ed452cda166fd4e01f9615ff72 And in case you want ABS_TOOL_WIDTH reported, I'd do some like the following. It has to be applied on top of the patches I linked here above. diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c index fbe29fc..b396886 100644 --- a/drivers/input/input-mt.c +++ b/drivers/input/input-mt.c @@ -65,6 +65,7 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots, copy_abs(dev, ABS_X, ABS_MT_POSITION_X); copy_abs(dev, ABS_Y, ABS_MT_POSITION_Y); copy_abs(dev, ABS_PRESSURE, ABS_MT_PRESSURE); + copy_abs(dev, ABS_TOOL_WIDTH, ABS_MT_TOUCH_MAJOR); } if (flags & INPUT_MT_POINTER) { __set_bit(BTN_TOOL_FINGER, dev->keybit); @@ -229,9 +230,17 @@ void input_mt_report_pointer_emulation(struct input_dev *dev, bool use_count) int p = input_mt_get_value(oldest, ABS_MT_PRESSURE); input_event(dev, EV_ABS, ABS_PRESSURE, p); } + + if (test_bit(ABS_MT_TOUCH_MAJOR, dev->absbit)) { + int w = input_mt_get_value(oldest, ABS_MT_TOUCH_MAJOR); + input_event(dev, EV_ABS, ABS_TOOL_WIDTH, w); + } } else { if (test_bit(ABS_MT_PRESSURE, dev->absbit)) input_event(dev, EV_ABS, ABS_PRESSURE, 0); + + if (test_bit(ABS_MT_TOUCH_MAJOR, dev->absbit)) + input_event(dev, EV_ABS, ABS_TOOL_WIDTH, 0); } } EXPORT_SYMBOL(input_mt_report_pointer_emulation); diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c index 5ff4c5b..3858493 100644 --- a/drivers/input/mouse/synaptics.c +++ b/drivers/input/mouse/synaptics.c @@ -1467,11 +1467,10 @@ static void set_input_params(struct psmouse *psmouse, } if (SYN_CAP_PALMDETECT(priv->capabilities)) { + input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0); if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) input_set_abs_params(dev, ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0); - else - input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0); } __set_bit(BTN_TOUCH, dev->keybit); I add here the links to the second patch series since the previous one can no longer be applied and some of you might be interested: https://patchwork.kernel.org/patch/5570751/ https://patchwork.kernel.org/patch/5570681/ https://patchwork.kernel.org/patch/5570741/ https://patchwork.kernel.org/patch/5570731/ I've got Thinkpad Carbon X1 Gen3 and had interest about this patch. However, when I run Gentoo Linux with kernel 4.0-rc2 with this patch applied, PalmDetect does not work. I tried evtest and found that not ABS_MT_TOUCH_MAJOR but ABS_TOOL_WIDTH is still reported to be supported by the device. Patch 4 seems to be wrong and I modified this patch and applied new patch. Then everything works fine. The modified patch is attached. Sorry for the submission without the patch. It was just a mistake. --- synaptics.c.orig 2015-03-07 13:50:45.652997759 +0900 +++ synaptics.c 2015-03-07 13:50:50.669997623 +0900 @@ -821,6 +821,7 @@ input_report_abs(dev, ABS_MT_POSITION_X, pos[i].x); input_report_abs(dev, ABS_MT_POSITION_Y, pos[i].y); input_report_abs(dev, ABS_MT_PRESSURE, hw[i]->z); + input_report_abs(dev, ABS_MT_TOUCH_MAJOR, hw[i]->w); } input_mt_drop_unused(dev); @@ -945,8 +946,14 @@ } input_report_abs(dev, ABS_PRESSURE, hw.z); - if (SYN_CAP_PALMDETECT(priv->capabilities)) - input_report_abs(dev, ABS_TOOL_WIDTH, finger_width); + if (SYN_CAP_PALMDETECT(priv->capabilities)) { + if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c) || + cr48_profile_sensor) + input_set_abs_params(dev, + ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0); + else + input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0); + } input_report_key(dev, BTN_TOOL_FINGER, num_fingers == 1); if (SYN_CAP_MULTIFINGER(priv->capabilities)) { (In reply to Shunsuke Shimizu from comment #16) > Sorry for the submission without the patch. It was just a mistake. Again this patch is wrong and same as the original patch, sorry. Following is correct. --- synaptics.c.orig 2015-03-07 13:50:45.652997759 +0900 +++ synaptics.c 2015-03-07 14:10:10.899966179 +0900 @@ -821,6 +821,7 @@ input_report_abs(dev, ABS_MT_POSITION_X, pos[i].x); input_report_abs(dev, ABS_MT_POSITION_Y, pos[i].y); input_report_abs(dev, ABS_MT_PRESSURE, hw[i]->z); + input_report_abs(dev, ABS_MT_TOUCH_MAJOR, hw[i]->w); } input_mt_drop_unused(dev); @@ -1099,8 +1100,14 @@ INPUT_MT_TRACK : INPUT_MT_SEMI_MT)); } - if (SYN_CAP_PALMDETECT(priv->capabilities)) - input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0); + if (SYN_CAP_PALMDETECT(priv->capabilities)) { + if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c) || + cr48_profile_sensor) + input_set_abs_params(dev, + ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0); + else + input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0); + } __set_bit(BTN_TOUCH, dev->keybit); __set_bit(BTN_TOOL_FINGER, dev->keybit); You are right. That's exactly what I meant to do (see 2/2 of the first series linked here above). I must have done something wrong when I updated it. Thanks. Is there any progress towards getting this bug fixed? My HP EliteBook 9470m's "SynPS/2 Synaptics TouchPad" is quite large and it is not easy to use the keyboard without touching it with my palms. Combine that with a window manager using focus-under-mouse, and it's pretty much impossible to use the machine without disabling the touchpad completely... I submitted the patches long ago, but they haven't been reviewed by the input subsystem maintainer yet. I'll try again to get them reviewed. Here the link to the thread for those of you who are interested: http://www.spinics.net/lists/linux-input/msg37593.html Hi Gabriele, I've tested your patches (v3) applied on kernel 4.0.5 with the master branch of xf86-input-synaptics and it works pretty well on my Thinkpad x240 :) I hope your patches will reach the kernel soon… Anyway thanks for your work. Have you tried with xf86-input-libinput? Nope… I didn't knew it existed until now^^ I wasn't aware of xf86-input-libinput either. Tried it with the standard Fedora kernel, with it there are no xinput property for palm detection (and it certainly doesn't do it by default either): $ xinput list-props "SynPS/2 Synaptics TouchPad" Device 'SynPS/2 Synaptics TouchPad': Device Enabled (136): 1 Coordinate Transformation Matrix (138): 1.000000, 0.000000, 0.000000, 0.000000, 1.000000, 0.000000, 0.000000, 0.000000, 1.000000 libinput Tapping Enabled (284): 1 libinput Tapping Enabled Default (285): 0 libinput Accel Speed (271): 0.000000 libinput Accel Speed Default (272): 0.000000 libinput Natural Scrolling Enabled (273): 0 libinput Natural Scrolling Enabled Default (274): 0 libinput Send Events Modes Available (256): 1, 1 libinput Send Events Mode Enabled (257): 0, 0 libinput Send Events Mode Enabled Default (258): 0, 0 libinput Left Handed Enabled (275): 0 libinput Left Handed Enabled Default (276): 0 libinput Scroll Methods Available (277): 1, 1, 0 libinput Scroll Method Enabled (278): 1, 0, 0 libinput Scroll Method Enabled Default (279): 1, 0, 0 Device Node (259): "/dev/input/event5" Device Product ID (260): 2, 7 There is no property because it's enabled by default and should work, I guess. http://wayland.freedesktop.org/libinput/doc/latest/palm_detection.html Thanks for the pointer, after reading this page I realise it is probably working after all! (My initial testing procedure after changing the drivers was a bit too dumb - I just put my palm in the centre of the touchpad and moved it around a bit - that wasn't ignored as being a "palm movement".) @AnAkkk, I tried libinput too and it worked well :) Better than synaptics. On a 6th gen X1 Carbon, I have tried both Synaptics and Libinput and the palm detection does not seem to work well - or possibly at all - on either. So, the proposed patch, or something like it, would be welcome. |