Multitouch regression in 3.3 on thinkpad X220 clickpad

Multitouch regression in 3.3 on thinkpad X220 clickpad

[Benjamin Herrenschmidt]

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.

Re: Multitouch regression in 3.3 on thinkpad X220 clickpad

[Daniel Drake]

On Fri, Apr 20, 2012 at 1:21 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> 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 :-)

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

Re: Multitouch regression in 3.3 on thinkpad X220 clickpad

[Dmitry Torokhov]

On Fri, Apr 20, 2012 at 08:27:37AM -0600, Daniel Drake wrote:
> On Fri, Apr 20, 2012 at 1:21 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > 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 :-)
> 
> 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?

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.

-- 
Dmitry

Re: Multitouch regression in 3.3 on thinkpad X220 clickpad

[Benjamin Herrenschmidt]

On Fri, 2012-04-20 at 10:01 -0700, Dmitry Torokhov wrote:

> 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?

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 <benh@kernel.crashing.org>
CC: stable@kernel.org [3.3]

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index a3bb49c..37b3792 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -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))

Re: Multitouch regression in 3.3 on thinkpad X220 clickpad

[Dmitry Torokhov]

On Sat, Apr 21, 2012 at 10:16:40AM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2012-04-20 at 10:01 -0700, Dmitry Torokhov wrote:
> 
> > 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?
> 
> 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 <benh@kernel.crashing.org>
> CC: stable@kernel.org [3.3]
> 

Applied, thank you Ben.

-- 
Dmitry

Re: Multitouch regression in 3.3 on thinkpad X220 clickpad

[Daniel Drake]

On Fri, Apr 20, 2012 at 6:16 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>> 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?
>
> Yes, that's it. Please shoot the patch below to Linus.

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

Daniel