Multitouch regression in 3.3 on thinkpad X220 clickpad

DISCUSSION Applied Benjamin Herrenschmidt 3 participants 6 messages Apr 20, 2012 – Apr 21, 2012 Input drivers

Share & tools

Benjamin Herrenschmidt

author

2012-04-20 07:21 UTC #0 lore raw

Hi folks !

So Peter and I have been discussing a problem I observed on my brand new ThinkPad X220 and it's multitouch "clickpad". I've been digging a bit more today and bisected the regression to:

commit 7968a5dd492ccc38345013e534ad4c8d6eb60ed1
Input: synaptics - add support for Relative mode

Without that commit, I can draw two "traces" when using two fingers in mtview (after removing the device from X), each follow one finger.

With that commit applied, this doesn't work anymore: it appears to be unable to track more than one finger. IE. If i put a finger on the touchpad, I can draw, but a second finger is then ignored (or causes one or two spots in a random place to appear but that's about it).

Now other multitouch operations such as two finger scrolling seem to work in X. I'm not familiar with the inner workings of the input layer or the synaptic touchpads. I'll dig a bit more this week-end if I have time, but it would save me plenty of that precious time if you guys could hint me at things to look at :-)

I have a couple more bits of interesting info.

If I boot with a "bad" driver. rmmod it. insmod a "good" one (one build from the same kernel tree set to the commit right before the one above) then things work fine. The interesting bit is that if I then rmmod it and insmod the "bad" one again ... it still works. So it looks like it might have something to do with how the touchpad is initialized.

Some more info about the touchpad itself from dmesg:

psmouse serio1: synaptics: Touchpad model: 1, fw: 8.0, id: 0x1e2b1, caps: 0xd001a3/0x940300/0x120c00 psmouse serio1: synaptics: serio: Synaptics pass-through port at isa0060/serio1/input0
input: SynPS/2 Synaptics TouchPad as /devices/platform/i8042/serio1/input/input13

Let me know if there's any other info of value I can collect.

Cheers,
Ben.

Daniel Drake

2012-04-20 14:27 UTC #1 lore raw

Quoted 15 lines · Benjamin Herrenschmidt

So Peter and I have been discussing a problem I observed on my brand new ThinkPad X220 and it's multitouch "clickpad". I've been digging a bit more today and bisected the regression to:
commit 7968a5dd492ccc38345013e534ad4c8d6eb60ed1
Input: synaptics - add support for Relative mode
Without that commit, I can draw two "traces" when using two fingers in mtview (after removing the device from X), each follow one finger.
With that commit applied, this doesn't work anymore: it appears to be unable to track more than one finger. IE. If i put a finger on the touchpad, I can draw, but a second finger is then ignored (or causes one or two spots in a random place to appear but that's about it).

Quoted 5 lines

Now other multitouch operations such as two finger scrolling seem to work in X. I'm not familiar with the inner workings of the input layer or the synaptic touchpads. I'll dig a bit more this week-end if I have time, but it would save me plenty of that precious time if you guys could hint me at things to look at :-)

Sorry to cause you this trouble.
It's the first I've heard of issues with this patch, and unfortunately I don't have any immediate diagnosis for you.
The patch isn't supposed to change behaviour of the existing default mode which is called "absolute mode".

What I would suggest looking at is synaptics_set_advanced_gesture_mode(). Is this being called, does it run to completion?
You could also look at the the values passed to synaptics_mode_cmd() - how do they compare from good and bad drivers?

Does this line change over good/bad drivers?
psmouse serio1: synaptics: Touchpad model: 1, fw: 8.0, id: 0x1e2b1,
caps: 0xd001a3/0x940300/0x120c00

Another approach may be to compare the communication with the device between good and bad drivers. This can be done with
echo 1 > /sys/module/i8042/parameters/debug

The device specs are public and remarkably good:
http://www.synaptics.com/sites/default/files/511-000275-01_RevB.pdf

Daniel

Dmitry Torokhov

maintainer

2012-04-20 17:01 UTC #2 lore raw

Quoted 29 lines · Daniel Drake

Quoted 15 lines · Benjamin Herrenschmidt
So Peter and I have been discussing a problem I observed on my brand new ThinkPad X220 and it's multitouch "clickpad". I've been digging a bit more today and bisected the regression to:
commit 7968a5dd492ccc38345013e534ad4c8d6eb60ed1
Input: synaptics - add support for Relative mode
Without that commit, I can draw two "traces" when using two fingers in mtview (after removing the device from X), each follow one finger.
With that commit applied, this doesn't work anymore: it appears to be unable to track more than one finger. IE. If i put a finger on the touchpad, I can draw, but a second finger is then ignored (or causes one or two spots in a random place to appear but that's about it).
Quoted 5 lines
Now other multitouch operations such as two finger scrolling seem to work in X. I'm not familiar with the inner workings of the input layer or the synaptic touchpads. I'll dig a bit more this week-end if I have time, but it would save me plenty of that precious time if you guys could hint me at things to look at :-)
Sorry to cause you this trouble.
It's the first I've heard of issues with this patch, and unfortunately I don't have any immediate diagnosis for you.
The patch isn't supposed to change behaviour of the existing default mode which is called "absolute mode".

Quoted 3 lines

What I would suggest looking at is synaptics_set_advanced_gesture_mode(). Is this being called, does it run to completion?

It looks we lost a condition in synaptics_set_advanced_gesture_mode(). It used to be:

	if (!(SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) ||
			SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)))
		return 0;

and now simply is:

	if (!SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
		return 0;

Could you try restoring the condition and see if it fixes the regression?

Thanks.

Signature · 1 line

Dmitry

Benjamin Herrenschmidt

author

2012-04-21 00:17 UTC #3 lore raw

Quoted 8 lines · Dmitry Torokhov

It looks we lost a condition in synaptics_set_advanced_gesture_mode(). It used to be:
	if (!(SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) ||
			SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)))
		return 0;

Quoted 7 lines

and now simply is:
	if (!SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
		return 0;
Could you try restoring the condition and see if it fixes the regression?

Yes, that's it. Please shoot the patch below to Linus.

Thanks,
Ben.

input/synaptics: Fix regression with "image sensor" trackpads

commit 7968a5dd492ccc38345013e534ad4c8d6eb60ed1
Input: synaptics - add support for Relative mode

Accidentally broke support for advanced gestures (multitouch) on some trackpads such as the one in my ThinkPad X220 by incorretly changing the condition for enabling them. This restores it.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

CC [email protected] [3.3]

drivers/input/mouse/synaptics.c +2 -1

index a3bb49c..37b3792 100644
@@ -273,7 +273,8 @@ static int synaptics_set_advanced_gesture_mode(struct psmouse *psmouse)
 	static unsigned char param = 0xc8;
 	struct synaptics_data *priv = psmouse->private;
 
-	if (!SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
+	if (!(SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) ||
+	      SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)))
 		return 0;
 
 	if (psmouse_sliced_command(psmouse, SYN_QUE_MODEL))

Dmitry Torokhov

maintainer Applied

2012-04-21 06:00 UTC #4 lore raw

Quoted 28 lines · Benjamin Herrenschmidt

Quoted 8 lines · Dmitry Torokhov
It looks we lost a condition in synaptics_set_advanced_gesture_mode(). It used to be:
	if (!(SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) ||
			SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)))
		return 0;
Quoted 7 lines
and now simply is:
	if (!SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
		return 0;
Could you try restoring the condition and see if it fixes the regression?
Yes, that's it. Please shoot the patch below to Linus.
Thanks,
Ben.
input/synaptics: Fix regression with "image sensor" trackpads
commit 7968a5dd492ccc38345013e534ad4c8d6eb60ed1
Input: synaptics - add support for Relative mode

Quoted 8 lines

Accidentally broke support for advanced gestures (multitouch) on some trackpads such as the one in my ThinkPad X220 by incorretly changing the condition for enabling them. This restores it.
Signed-off-by Benjamin Herrenschmidt <[email protected]>
CC [email protected] [3.3]

Applied, thank you Ben.

Signature · 1 line

Dmitry

Daniel Drake

2012-04-21 15:03 UTC #5 lore raw

In reply to Benjamin Herrenschmidt

Quoted 10 lines · Benjamin Herrenschmidt

Quoted 10 lines
It looks we lost a condition in synaptics_set_advanced_gesture_mode(). It used to be:
if (!(SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) || SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))) return 0;
and now simply is:

Quoted 7 lines

Quoted 5 lines
if (!SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
return 0;
Could you try restoring the condition and see if it fixes the regression?
Yes, that's it. Please shoot the patch below to Linus.

Oops. Thanks Ben and Dmitry, and apologies for causing this bug.

Daniel