Bug 77161 - Synaptics Driver Does Not Report ABS_TOOL_WIDTH When Available
Summary: Synaptics Driver Does Not Report ABS_TOOL_WIDTH When Available
Status: NEW
Alias: None
Product: Drivers
Classification: Unclassified
Component: Input Devices (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: drivers_input-devices
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-31 17:34 UTC by Maxwell Anselm
Modified: 2018-08-18 05:26 UTC (History)
20 users (show)

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


Attachments

Description Maxwell Anselm 2014-05-31 17:34:17 UTC
A lot of users have reported problems with synaptics' palm detection feature. The palm detection code requires Z and width values for each touch in order to detect a palm in most cases. However the kernel is not reporting width values at all.

Expected:

Running evtest on the touchpad device will show ABS_MT_PRESSURE and ABS_TOOL_WIDTH records in the event stream.

Actual:

No ABS_TOOL_WIDTH records are reported even though the device claims to support them in its capability bits.


There is some code in drivers/input/mouse/synaptics.c which looks like it should report widths if the touchpad supports palm detection, but at least for my X1 Carbon (which claims it supports finger widths and palm detection) this code is never reached.

A crude patch allows some widths to be reported:
http://pastebin.com/rjGyM5pC

But I don't think this is a proper solution as finger width is still missing from many reports.

See downstream bug report: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1245328
Comment 1 Gabriele Mazzotta 2014-06-01 10:31:08 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.
Comment 2 rainer+kernelorg 2014-12-17 15:56:41 UTC
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
Comment 3 Gabriele Mazzotta 2014-12-17 16:55:58 UTC
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
Comment 4 Maxwell Anselm 2014-12-17 17:17:27 UTC
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.
Comment 5 Dmitry Torokhov 2014-12-24 07:51:11 UTC
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...
Comment 6 Maxwell Anselm 2014-12-24 15:45:35 UTC
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.
Comment 7 Gabriele Mazzotta 2014-12-24 17:51:52 UTC
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 */
Comment 8 Gabriele Mazzotta 2014-12-24 18:21:57 UTC
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.
Comment 9 Felipe Balbi 2014-12-28 03:43:10 UTC
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.
Comment 10 Felipe Balbi 2014-12-28 03:46:00 UTC
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.
Comment 11 Felipe Balbi 2014-12-28 05:13:12 UTC
Still the same with v3.18. ABS_TOOL_WIDTH is never reported
Comment 12 Gabriele Mazzotta 2014-12-28 15:52:54 UTC
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
Comment 13 Gabriele Mazzotta 2014-12-28 18:10:06 UTC
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);
Comment 14 Gabriele Mazzotta 2015-03-04 21:35:32 UTC
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/
Comment 15 Shunsuke Shimizu 2015-03-07 05:01:44 UTC
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.
Comment 16 Shunsuke Shimizu 2015-03-07 05:04:11 UTC
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)) {
Comment 17 Shunsuke Shimizu 2015-03-07 05:16:47 UTC
(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);
Comment 18 Gabriele Mazzotta 2015-03-07 09:04:51 UTC
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.
Comment 19 Tore Anderson 2015-06-08 14:07:15 UTC
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...
Comment 20 Gabriele Mazzotta 2015-06-08 14:17:25 UTC
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
Comment 21 Sky 2015-06-08 14:30:41 UTC
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.
Comment 22 AnAkkk 2015-06-08 14:31:34 UTC
Have you tried with xf86-input-libinput?
Comment 23 Sky 2015-06-08 15:23:09 UTC
Nope… I didn't knew it existed until now^^
Comment 24 Tore Anderson 2015-06-08 16:26:10 UTC
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
Comment 25 AnAkkk 2015-06-08 16:29:42 UTC
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
Comment 26 Tore Anderson 2015-06-08 17:14:37 UTC
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".)
Comment 27 Sky 2015-06-09 09:10:30 UTC
@AnAkkk, I tried libinput too and it worked well :) Better than synaptics.
Comment 28 najoll 2018-04-04 01:51:38 UTC
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.

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