[7/9,v2] Input: synaptics - improved 2->3 finger transition handling

Commit Message

Daniel Kurtz July 20, 2011, 1:39 p.m. UTC

From: Daniel Kurtz <[e-mail]>

As long as we know which slots are currently contained in SGM and AGM
packets, it is possible to track the slot 0 finger when transitioning from
2->3 fingers.  This is the case when fingers are being added one at a
time, 1->2->3.

However, when fingers are removed, we sometimes lose track of which slots
are contained in SGM and AGM. In particular, when transitioning from 3->2
and sometimes 3->1.  In both of these cases, we can no longer track slot 0
during 2->3 transitions.

Signed-off-by: Daniel Kurtz <[e-mail]>
---
 drivers/input/mouse/synaptics.c |   33 +++++++++++++++++++++++++++------
 drivers/input/mouse/synaptics.h |    1 +
 2 files changed, 28 insertions(+), 6 deletions(-)

Comments

Chase Douglas July 23, 2011, 1:07 a.m. UTC | #1

On 07/20/2011 06:39 AM, [e-mail] wrote:
> From: Daniel Kurtz <[e-mail]>
> 
> As long as we know which slots are currently contained in SGM and AGM
> packets, it is possible to track the slot 0 finger when transitioning from
> 2->3 fingers.  This is the case when fingers are being added one at a
> time, 1->2->3.
> 
> However, when fingers are removed, we sometimes lose track of which slots
> are contained in SGM and AGM. In particular, when transitioning from 3->2
> and sometimes 3->1.  In both of these cases, we can no longer track slot 0
> during 2->3 transitions.
> 
> Signed-off-by: Daniel Kurtz <[e-mail]>
> ---
>  drivers/input/mouse/synaptics.c |   33 +++++++++++++++++++++++++++------
>  drivers/input/mouse/synaptics.h |    1 +
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index b626b98..893e567 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -659,6 +659,7 @@ static void synaptics_image_sensor_0f(struct synaptics_data *priv,
>  				      struct synaptics_mt_state *mt_state)
>  {
>  	synaptics_mt_state_set(mt_state, 0, -1, -1);
> +	priv->mt_state_lost = false;
>  }
>  
>  /* Handle case where mt_state->count = 1 */
> @@ -726,6 +727,7 @@ static void synaptics_image_sensor_1f(struct synaptics_data *priv,
>  		 * So, empty all slots. We will guess slot 0 on subsequent 1->1
>  		 */
>  		synaptics_mt_state_set(mt_state, 0, -1, -1);
> +		priv->mt_state_lost = true;
>  		break;
>  	}
>  }
> @@ -771,6 +773,7 @@ static void synaptics_image_sensor_2f(struct synaptics_data *priv,
>  		 * subsequent 2->2
>  		 */
>  		synaptics_mt_state_set(mt_state, 0, -1, -1);
> +		priv->mt_state_lost = true;
>  		break;
>  	}
>  }
> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>  		break;
>  	case 2:
>  		/*
> -		 * On 2->3 transitions, we are given no indication which finger
> -		 * was added.
> -		 * We don't even know what finger the current AGM packet
> -		 * contained.
> +		 * After some 3->1 and all 3->2 transitions, we lose track
> +		 * of which slot is reported by sgm and agm.
>  		 *
> -		 * So, empty all slots. They get filled on a subsequent 3->3
> +		 * For 2->3 in this state, empty all slots, and we will guess
> +		 * (0,1) on a subsequent 0->3.
> +		 *
> +		 * To userspace, the resulting transition will look like:
> +		 *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]

I don't think this should be allowed. We shouldn't be giving userspace
wrong information. One could argue that userspace could watch for these
transitions, but then I would argue that the driver should be handling
this :).

I don't know what the best resolution to this issue is, but any
transition in the number of fingers must be accurate. In uTouch, we
watch for touch count transitions for "continuation" gestures.

>  		 */
> -		synaptics_mt_state_set(mt_state, 0, -1, -1);
> +		if (priv->mt_state_lost) {
> +			synaptics_mt_state_set(mt_state, 0, -1, -1);
> +			break;
> +		}
> +
> +		/*
> +		 * If the (SGM,AGM) really previously contained slots (0, 1),
> +		 * then we cannot know what slot was just reported by the AGM,
> +		 * because the 2->3 transition can occur either before or after
> +		 * the AGM packet. Thus, this most recent AGM could contain
> +		 * either the same old slot 1 or the new slot 2.
> +		 * Subsequent AGMs will be reporting slot 2.
> +		 *
> +		 * To userspace, the resulting transition will look like:
> +		 *    2:[0,1] -> 1:[0,-1] -> 3:[0,2]
> +		 */
> +		synaptics_mt_state_set(mt_state, 1, 0, -1);
>  		break;
>  	case 3:
>  		/*
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index 87be1fe..e3edfea 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -162,6 +162,7 @@ struct synaptics_data {
>  	struct serio *pt_port;			/* Pass-through serio port */
>  
>  	struct synaptics_mt_state mt_state;	/* Current mt finger state */
> +	bool mt_state_lost;			/* mt_state may be incorrect */
>  
>  	/*
>  	 * Last received Advanced Gesture Mode (AGM) packet. An AGM packet

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Daniel Kurtz July 23, 2011, 4:36 a.m. UTC | #2

Hi Chase,

On Sat, Jul 23, 2011 at 9:07 AM, Chase Douglas
<[e-mail]> wrote:
> On 07/20/2011 06:39 AM, [e-mail] wrote:
>> From: Daniel Kurtz <[e-mail]>
>>
>> As long as we know which slots are currently contained in SGM and AGM
>> packets, it is possible to track the slot 0 finger when transitioning from
>> 2->3 fingers.  This is the case when fingers are being added one at a
>> time, 1->2->3.
>>
>> However, when fingers are removed, we sometimes lose track of which slots
>> are contained in SGM and AGM. In particular, when transitioning from 3->2
>> and sometimes 3->1.  In both of these cases, we can no longer track slot 0
>> during 2->3 transitions.
>>
>> Signed-off-by: Daniel Kurtz <[e-mail]>
>> ---
>>  drivers/input/mouse/synaptics.c |   33 +++++++++++++++++++++++++++------
>>  drivers/input/mouse/synaptics.h |    1 +
>>  2 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index b626b98..893e567 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -659,6 +659,7 @@ static void synaptics_image_sensor_0f(struct synaptics_data *priv,
>>                                     struct synaptics_mt_state *mt_state)
>>  {
>>       synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +     priv->mt_state_lost = false;
>>  }
>>
>>  /* Handle case where mt_state->count = 1 */
>> @@ -726,6 +727,7 @@ static void synaptics_image_sensor_1f(struct synaptics_data *priv,
>>                * So, empty all slots. We will guess slot 0 on subsequent 1->1
>>                */
>>               synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +             priv->mt_state_lost = true;
>>               break;
>>       }
>>  }
>> @@ -771,6 +773,7 @@ static void synaptics_image_sensor_2f(struct synaptics_data *priv,
>>                * subsequent 2->2
>>                */
>>               synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +             priv->mt_state_lost = true;
>>               break;
>>       }
>>  }
>> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>>               break;
>>       case 2:
>>               /*
>> -              * On 2->3 transitions, we are given no indication which finger
>> -              * was added.
>> -              * We don't even know what finger the current AGM packet
>> -              * contained.
>> +              * After some 3->1 and all 3->2 transitions, we lose track
>> +              * of which slot is reported by sgm and agm.
>>                *
>> -              * So, empty all slots. They get filled on a subsequent 3->3
>> +              * For 2->3 in this state, empty all slots, and we will guess
>> +              * (0,1) on a subsequent 0->3.
>> +              *
>> +              * To userspace, the resulting transition will look like:
>> +              *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
>
> I don't think this should be allowed. We shouldn't be giving userspace
> wrong information. One could argue that userspace could watch for these
> transitions, but then I would argue that the driver should be handling
> this :).
>
> I don't know what the best resolution to this issue is, but any
> transition in the number of fingers must be accurate. In uTouch, we
> watch for touch count transitions for "continuation" gestures.

So, you want the count to be accurate.
But, during these transitions, there is not enough information to
guarantee all of the following at the same time:
 (1) finger count
 (2) track_id
 (3) finger position

Would you prefer an implementation that continued to report count (via
BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
these cases where it could not determine the correct position or track_id
to report?

It seems like it would be more work for userspace to handle this new way
than the simulated number-of-touch transitions, where the transient
states are all normal valid states.

-Daniel

>
>>                */
>> -             synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +             if (priv->mt_state_lost) {
>> +                     synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +                     break;
>> +             }
>> +
>> +             /*
>> +              * If the (SGM,AGM) really previously contained slots (0, 1),
>> +              * then we cannot know what slot was just reported by the AGM,
>> +              * because the 2->3 transition can occur either before or after
>> +              * the AGM packet. Thus, this most recent AGM could contain
>> +              * either the same old slot 1 or the new slot 2.
>> +              * Subsequent AGMs will be reporting slot 2.
>> +              *
>> +              * To userspace, the resulting transition will look like:
>> +              *    2:[0,1] -> 1:[0,-1] -> 3:[0,2]
>> +              */
>> +             synaptics_mt_state_set(mt_state, 1, 0, -1);
>>               break;
>>       case 3:
>>               /*
>> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
>> index 87be1fe..e3edfea 100644
>> --- a/drivers/input/mouse/synaptics.h
>> +++ b/drivers/input/mouse/synaptics.h
>> @@ -162,6 +162,7 @@ struct synaptics_data {
>>       struct serio *pt_port;                  /* Pass-through serio port */
>>
>>       struct synaptics_mt_state mt_state;     /* Current mt finger state */
>> +     bool mt_state_lost;                     /* mt_state may be incorrect */
>>
>>       /*
>>        * Last received Advanced Gesture Mode (AGM) packet. An AGM packet
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chase Douglas July 26, 2011, 11:14 p.m. UTC | #3

On 07/22/2011 09:36 PM, Daniel Kurtz wrote:
> On Sat, Jul 23, 2011 at 9:07 AM, Chase Douglas
> <[e-mail]> wrote:
>> On 07/20/2011 06:39 AM, [e-mail] wrote:
>>> From: Daniel Kurtz <[e-mail]>
>>> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>>>               break;
>>>       case 2:
>>>               /*
>>> -              * On 2->3 transitions, we are given no indication which finger
>>> -              * was added.
>>> -              * We don't even know what finger the current AGM packet
>>> -              * contained.
>>> +              * After some 3->1 and all 3->2 transitions, we lose track
>>> +              * of which slot is reported by sgm and agm.
>>>                *
>>> -              * So, empty all slots. They get filled on a subsequent 3->3
>>> +              * For 2->3 in this state, empty all slots, and we will guess
>>> +              * (0,1) on a subsequent 0->3.
>>> +              *
>>> +              * To userspace, the resulting transition will look like:
>>> +              *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
>>
>> I don't think this should be allowed. We shouldn't be giving userspace
>> wrong information. One could argue that userspace could watch for these
>> transitions, but then I would argue that the driver should be handling
>> this :).
>>
>> I don't know what the best resolution to this issue is, but any
>> transition in the number of fingers must be accurate. In uTouch, we
>> watch for touch count transitions for "continuation" gestures.
> 
> So, you want the count to be accurate.
> But, during these transitions, there is not enough information to
> guarantee all of the following at the same time:
>  (1) finger count
>  (2) track_id
>  (3) finger position

If I had to pick one to give up, it would be the tracking id. It carries
the least useful information for a device that you may not get the whole
touch stream. Semi-mt devices already forsake the tracking id value,
IIRC. The id is always incremented when you create a new slot, but when
you go from 2->3 fingers the ids in the slots stay the same even if the
third finger expands the bounding box.

> Would you prefer an implementation that continued to report count (via
> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
> these cases where it could not determine the correct position or track_id
> to report?

That may be doable, but I would prefer to just assume that tracking ids
are not valid when (tracked touches > reported touches).

> It seems like it would be more work for userspace to handle this new way
> than the simulated number-of-touch transitions, where the transient
> states are all normal valid states.

This harkens back to my earlier statements where I said this new
Synaptics protocol is worse than the previous one :).

I agree that the implementation you gave here might be trickier for
userspace, so I'd rather table it unless you feel that the "tracking ids
are meaningless!" solution won't work. If you think there are problems
with that approach, we can re-evaluate.

Thanks!

-- Chase
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Daniel Kurtz July 27, 2011, 4:48 a.m. UTC | #4

On Wed, Jul 27, 2011 at 7:14 AM, Chase Douglas
<[e-mail]> wrote:
> On 07/22/2011 09:36 PM, Daniel Kurtz wrote:
>> On Sat, Jul 23, 2011 at 9:07 AM, Chase Douglas
>> <[e-mail]> wrote:
>>> On 07/20/2011 06:39 AM, [e-mail] wrote:
>>>> From: Daniel Kurtz <[e-mail]>
>>>> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>>>>               break;
>>>>       case 2:
>>>>               /*
>>>> -              * On 2->3 transitions, we are given no indication which finger
>>>> -              * was added.
>>>> -              * We don't even know what finger the current AGM packet
>>>> -              * contained.
>>>> +              * After some 3->1 and all 3->2 transitions, we lose track
>>>> +              * of which slot is reported by sgm and agm.
>>>>                *
>>>> -              * So, empty all slots. They get filled on a subsequent 3->3
>>>> +              * For 2->3 in this state, empty all slots, and we will guess
>>>> +              * (0,1) on a subsequent 0->3.
>>>> +              *
>>>> +              * To userspace, the resulting transition will look like:
>>>> +              *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
>>>
>>> I don't think this should be allowed. We shouldn't be giving userspace
>>> wrong information. One could argue that userspace could watch for these
>>> transitions, but then I would argue that the driver should be handling
>>> this :).
>>>
>>> I don't know what the best resolution to this issue is, but any
>>> transition in the number of fingers must be accurate. In uTouch, we
>>> watch for touch count transitions for "continuation" gestures.
>>
>> So, you want the count to be accurate.
>> But, during these transitions, there is not enough information to
>> guarantee all of the following at the same time:
>>  (1) finger count
>>  (2) track_id
>>  (3) finger position
>
> If I had to pick one to give up, it would be the tracking id. It carries
> the least useful information for a device that you may not get the whole
> touch stream. Semi-mt devices already forsake the tracking id value,
> IIRC. The id is always incremented when you create a new slot, but when
> you go from 2->3 fingers the ids in the slots stay the same even if the
> third finger expands the bounding box.

For synaptics profile sensors, adding additional fingers does not
change which fingers are reported.
You always get the first two fingers.
AFAICT, you cannot "expand the bounding box" by adding new fingers.
Thus, throwing away the track_id is irrelevant, since once the semi-mt
driver starts reporting 2+ fingers, the track_ids are fixed.

The image sensor does not work that way.
As you add new fingers, the trackpad will, under certain conditions,
switch to reporting the locations of these new fingers.
Changing track_id is how we report this to userspace.

>
>> Would you prefer an implementation that continued to report count (via
>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>> these cases where it could not determine the correct position or track_id
>> to report?
>
> That may be doable, but I would prefer to just assume that tracking ids
> are not valid when (tracked touches > reported touches).

Userspace is free to make this assumption, of course, but, in fact,
the image sensor trackpad actually does a pretty good job of tracking
the fingers - it just has serious issues reporting them!
Since a track_id change is how userspace is told that the identity of
the reported finger is changing, if the track_id of a finger position
datum is unknowable, I'd rather just discard it in the kernel than
report it to userspace with the wrong track_id.
Otherwise, the heuristics used in the userspace finger tracking
algorithms would need to be overly aggressively tuned to handle this
known error cases:
  2->3 and 3->2 finger transitions look like 2nd finger motion,
instead of reported finger changes.

>
>> It seems like it would be more work for userspace to handle this new way
>> than the simulated number-of-touch transitions, where the transient
>> states are all normal valid states.
>
> This harkens back to my earlier statements where I said this new
> Synaptics protocol is worse than the previous one :).
>
> I agree that the implementation you gave here might be trickier for
> userspace, so I'd rather table it unless you feel that the "tracking ids
> are meaningless!" solution won't work. If you think there are problems
> with that approach, we can re-evaluate.
>
> Thanks!
>
> -- Chase

Yes, I feel there are problems with this approach, as I tried to explain above.
Can you explain why you 'continuation gestures' can't handle 1->2->3
finger transitions looking like 1->2->1->3, and 3->2->3 looking like
3->2->0->3?

I think the only real point left to decide is what BTN_* events should
signify during these rare transition states:
  (a) the actually number of fingers on the pad,
  (b) the number of fingers being reported via the slots

The current patchset does (a).
We could do (b), if that would get these patches accepted sooner :)

Thanks!
-Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chase Douglas July 27, 2011, 9:13 p.m. UTC | #5

On 07/26/2011 09:48 PM, Daniel Kurtz wrote:
> On Wed, Jul 27, 2011 at 7:14 AM, Chase Douglas
> <[e-mail]> wrote:
>> On 07/22/2011 09:36 PM, Daniel Kurtz wrote:
>>> On Sat, Jul 23, 2011 at 9:07 AM, Chase Douglas
>>> <[e-mail]> wrote:
>>>> On 07/20/2011 06:39 AM, [e-mail] wrote:
>>>>> From: Daniel Kurtz <[e-mail]>
>>>>> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>>>>>               break;
>>>>>       case 2:
>>>>>               /*
>>>>> -              * On 2->3 transitions, we are given no indication which finger
>>>>> -              * was added.
>>>>> -              * We don't even know what finger the current AGM packet
>>>>> -              * contained.
>>>>> +              * After some 3->1 and all 3->2 transitions, we lose track
>>>>> +              * of which slot is reported by sgm and agm.
>>>>>                *
>>>>> -              * So, empty all slots. They get filled on a subsequent 3->3
>>>>> +              * For 2->3 in this state, empty all slots, and we will guess
>>>>> +              * (0,1) on a subsequent 0->3.
>>>>> +              *
>>>>> +              * To userspace, the resulting transition will look like:
>>>>> +              *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
>>>>
>>>> I don't think this should be allowed. We shouldn't be giving userspace
>>>> wrong information. One could argue that userspace could watch for these
>>>> transitions, but then I would argue that the driver should be handling
>>>> this :).
>>>>
>>>> I don't know what the best resolution to this issue is, but any
>>>> transition in the number of fingers must be accurate. In uTouch, we
>>>> watch for touch count transitions for "continuation" gestures.
>>>
>>> So, you want the count to be accurate.
>>> But, during these transitions, there is not enough information to
>>> guarantee all of the following at the same time:
>>>  (1) finger count
>>>  (2) track_id
>>>  (3) finger position
>>
>> If I had to pick one to give up, it would be the tracking id. It carries
>> the least useful information for a device that you may not get the whole
>> touch stream. Semi-mt devices already forsake the tracking id value,
>> IIRC. The id is always incremented when you create a new slot, but when
>> you go from 2->3 fingers the ids in the slots stay the same even if the
>> third finger expands the bounding box.
> 
> For synaptics profile sensors, adding additional fingers does not
> change which fingers are reported.
> You always get the first two fingers.
> AFAICT, you cannot "expand the bounding box" by adding new fingers.
> Thus, throwing away the track_id is irrelevant, since once the semi-mt
> driver starts reporting 2+ fingers, the track_ids are fixed.

Hmmm... you're right. I was completely mistaken here. This changes some
of my thinking about how to handle these devices.

> The image sensor does not work that way.
> As you add new fingers, the trackpad will, under certain conditions,
> switch to reporting the locations of these new fingers.
> Changing track_id is how we report this to userspace.
> 
>>
>>> Would you prefer an implementation that continued to report count (via
>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>> these cases where it could not determine the correct position or track_id
>>> to report?
>>
>> That may be doable, but I would prefer to just assume that tracking ids
>> are not valid when (tracked touches > reported touches).
> 
> Userspace is free to make this assumption, of course, but, in fact,
> the image sensor trackpad actually does a pretty good job of tracking
> the fingers - it just has serious issues reporting them!
> Since a track_id change is how userspace is told that the identity of
> the reported finger is changing, if the track_id of a finger position
> datum is unknowable, I'd rather just discard it in the kernel than
> report it to userspace with the wrong track_id.
> Otherwise, the heuristics used in the userspace finger tracking
> algorithms would need to be overly aggressively tuned to handle this
> known error cases:
>   2->3 and 3->2 finger transitions look like 2nd finger motion,
> instead of reported finger changes.
> 
>>
>>> It seems like it would be more work for userspace to handle this new way
>>> than the simulated number-of-touch transitions, where the transient
>>> states are all normal valid states.
>>
>> This harkens back to my earlier statements where I said this new
>> Synaptics protocol is worse than the previous one :).
>>
>> I agree that the implementation you gave here might be trickier for
>> userspace, so I'd rather table it unless you feel that the "tracking ids
>> are meaningless!" solution won't work. If you think there are problems
>> with that approach, we can re-evaluate.
>>
>> Thanks!
>>
>> -- Chase
> 
> Yes, I feel there are problems with this approach, as I tried to explain above.
> Can you explain why you 'continuation gestures' can't handle 1->2->3
> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
> 3->2->0->3?
> 
> I think the only real point left to decide is what BTN_* events should
> signify during these rare transition states:
>   (a) the actually number of fingers on the pad,
>   (b) the number of fingers being reported via the slots
> 
> The current patchset does (a).
> We could do (b), if that would get these patches accepted sooner :)

I was thinking that the current patchset does (b). I think (a) is
better, and if it really works that way then I'm fine with it. It's hard
for me to keep track of the flow of the logic across the patches :).

That said, merging this patchset as is effectively means that the number
of slots is completely decoupled from the number of touches on the
device. Previously, one could say that the number of touches on the
device was equal to the number of open slots or more if all slots were
open. With this approach, we could have 0 slots open during a transition
when there are still touches down.

While the distinction makes sense for these synaptics devices, I don't
want the semantics to hold for full multitouch devices. Otherwise, we
would have to add many more BTN_*TAPs. If we go this route, we must have
a way to tell userspace that this is a special device where the number
of active touches can only be determined from BTN_*TAP. Thus, we would
need a property for this exception to normal behavior.

(PS: As I've thought more about it, I don't think we need the property I
was advocating for before. That property was for denoting that the
device tracks more than it reports. If we're going to get this complex
in the protocol, there's not much you can do with bitmask properties to
denote every specific special case.)

-- Chase
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Daniel Kurtz July 28, 2011, 1 a.m. UTC | #6

On Thu, Jul 28, 2011 at 5:13 AM, Chase Douglas
<[e-mail]> wrote:
[...]
>>>
>>>> Would you prefer an implementation that continued to report count (via
>>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>>> these cases where it could not determine the correct position or track_id
>>>> to report?
>>>
>>> That may be doable, but I would prefer to just assume that tracking ids
>>> are not valid when (tracked touches > reported touches).
>>
>> Userspace is free to make this assumption, of course, but, in fact,
>> the image sensor trackpad actually does a pretty good job of tracking
>> the fingers - it just has serious issues reporting them!
>> Since a track_id change is how userspace is told that the identity of
>> the reported finger is changing, if the track_id of a finger position
>> datum is unknowable, I'd rather just discard it in the kernel than
>> report it to userspace with the wrong track_id.
>> Otherwise, the heuristics used in the userspace finger tracking
>> algorithms would need to be overly aggressively tuned to handle this
>> known error cases:
>>   2->3 and 3->2 finger transitions look like 2nd finger motion,
>> instead of reported finger changes.
>>
>>>
>>>> It seems like it would be more work for userspace to handle this new way
>>>> than the simulated number-of-touch transitions, where the transient
>>>> states are all normal valid states.
>>>
>>> This harkens back to my earlier statements where I said this new
>>> Synaptics protocol is worse than the previous one :).
>>>
>>> I agree that the implementation you gave here might be trickier for
>>> userspace, so I'd rather table it unless you feel that the "tracking ids
>>> are meaningless!" solution won't work. If you think there are problems
>>> with that approach, we can re-evaluate.
>>>
>>> Thanks!
>>>
>>> -- Chase
>>
>> Yes, I feel there are problems with this approach, as I tried to explain above.
>> Can you explain why you 'continuation gestures' can't handle 1->2->3
>> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
>> 3->2->0->3?
>>
>> I think the only real point left to decide is what BTN_* events should
>> signify during these rare transition states:
>>   (a) the actually number of fingers on the pad,
>>   (b) the number of fingers being reported via the slots
>>
>> The current patchset does (a).
>> We could do (b), if that would get these patches accepted sooner :)
>
> I was thinking that the current patchset does (b). I think (a) is
> better, and if it really works that way then I'm fine with it. It's hard
> for me to keep track of the flow of the logic across the patches :).

Argh, my bad.  You are correct.  Current patchset does (b)!
It reports the number of active slots, not the number of touches.

In any case, I really don't know why you need (a).  We are talking
about some degenerate transitions here.  Your userspace driver should
work just fine with the (b) driver, it just loses some really
complicated continued gestures for hardware that can't support them.

>
> That said, merging this patchset as is effectively means that the number
> of slots is completely decoupled from the number of touches on the
> device. Previously, one could say that the number of touches on the
> device was equal to the number of open slots or more if all slots were
> open. With this approach, we could have 0 slots open during a transition
> when there are still touches down.
>
> While the distinction makes sense for these synaptics devices, I don't
> want the semantics to hold for full multitouch devices. Otherwise, we
> would have to add many more BTN_*TAPs. If we go this route, we must have
> a way to tell userspace that this is a special device where the number
> of active touches can only be determined from BTN_*TAP. Thus, we would
> need a property for this exception to normal behavior.

Henrik & Dmitry roughly suggested "do not define a new property; let
userspace detect max BTN_*TAP > ABS_MT_SLOT.max to indicate that
BTN_*TAP carries the total number of touches".  I wish they would
chime in on this patchset...

>
> (PS: As I've thought more about it, I don't think we need the property I
> was advocating for before. That property was for denoting that the
> device tracks more than it reports. If we're going to get this complex
> in the protocol, there's not much you can do with bitmask properties to
> denote every specific special case.)
>
> -- Chase
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chase Douglas July 28, 2011, 2:07 a.m. UTC | #7

On 07/27/2011 06:00 PM, Daniel Kurtz wrote:
> On Thu, Jul 28, 2011 at 5:13 AM, Chase Douglas
> <[e-mail]> wrote:
> [...]
>>>>
>>>>> Would you prefer an implementation that continued to report count (via
>>>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>>>> these cases where it could not determine the correct position or track_id
>>>>> to report?
>>>>
>>>> That may be doable, but I would prefer to just assume that tracking ids
>>>> are not valid when (tracked touches > reported touches).
>>>
>>> Userspace is free to make this assumption, of course, but, in fact,
>>> the image sensor trackpad actually does a pretty good job of tracking
>>> the fingers - it just has serious issues reporting them!
>>> Since a track_id change is how userspace is told that the identity of
>>> the reported finger is changing, if the track_id of a finger position
>>> datum is unknowable, I'd rather just discard it in the kernel than
>>> report it to userspace with the wrong track_id.
>>> Otherwise, the heuristics used in the userspace finger tracking
>>> algorithms would need to be overly aggressively tuned to handle this
>>> known error cases:
>>>   2->3 and 3->2 finger transitions look like 2nd finger motion,
>>> instead of reported finger changes.
>>>
>>>>
>>>>> It seems like it would be more work for userspace to handle this new way
>>>>> than the simulated number-of-touch transitions, where the transient
>>>>> states are all normal valid states.
>>>>
>>>> This harkens back to my earlier statements where I said this new
>>>> Synaptics protocol is worse than the previous one :).
>>>>
>>>> I agree that the implementation you gave here might be trickier for
>>>> userspace, so I'd rather table it unless you feel that the "tracking ids
>>>> are meaningless!" solution won't work. If you think there are problems
>>>> with that approach, we can re-evaluate.
>>>>
>>>> Thanks!
>>>>
>>>> -- Chase
>>>
>>> Yes, I feel there are problems with this approach, as I tried to explain above.
>>> Can you explain why you 'continuation gestures' can't handle 1->2->3
>>> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
>>> 3->2->0->3?
>>>
>>> I think the only real point left to decide is what BTN_* events should
>>> signify during these rare transition states:
>>>   (a) the actually number of fingers on the pad,
>>>   (b) the number of fingers being reported via the slots
>>>
>>> The current patchset does (a).
>>> We could do (b), if that would get these patches accepted sooner :)
>>
>> I was thinking that the current patchset does (b). I think (a) is
>> better, and if it really works that way then I'm fine with it. It's hard
>> for me to keep track of the flow of the logic across the patches :).
> 
> Argh, my bad.  You are correct.  Current patchset does (b)!
> It reports the number of active slots, not the number of touches.
> 
> In any case, I really don't know why you need (a).  We are talking
> about some degenerate transitions here.  Your userspace driver should
> work just fine with the (b) driver, it just loses some really
> complicated continued gestures for hardware that can't support them.

To give userspace incorrect information about the number of touches on
the device is always bad. Lets say the degenerate case is when you go
from two touches to three (maybe Synaptics doesn't do this, but someone
else might). When the user performs a three finger tap, we'd see two
touches down, two lifted up, three touches down, three lifted up in
short succession. Userspace is gonna get pretty confused about that :).

(Please don't make me try to come up with a use case we already have in
Unity that would be broken for Synaptics due to this. I could probably
find one, but it would take quite a bit of thinking. :)

>> That said, merging this patchset as is effectively means that the number
>> of slots is completely decoupled from the number of touches on the
>> device. Previously, one could say that the number of touches on the
>> device was equal to the number of open slots or more if all slots were
>> open. With this approach, we could have 0 slots open during a transition
>> when there are still touches down.
>>
>> While the distinction makes sense for these synaptics devices, I don't
>> want the semantics to hold for full multitouch devices. Otherwise, we
>> would have to add many more BTN_*TAPs. If we go this route, we must have
>> a way to tell userspace that this is a special device where the number
>> of active touches can only be determined from BTN_*TAP. Thus, we would
>> need a property for this exception to normal behavior.
> 
> Henrik & Dmitry roughly suggested "do not define a new property; let
> userspace detect max BTN_*TAP > ABS_MT_SLOT.max to indicate that
> BTN_*TAP carries the total number of touches".  I wish they would
> chime in on this patchset...

I think it sets a really bad precedent to obey the stated protocol in
most cases, but disregard it in others if specific conditions are met,
which are not documented and are not presented in a clear manner to
userspace. At the *very least*, this change would need documentation to
go in at the same time, but I strongly believe a property is merited here.

-- Chase
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Daniel Kurtz July 28, 2011, 1:56 p.m. UTC | #8

On Thu, Jul 28, 2011 at 10:07 AM, Chase Douglas
<[e-mail]> wrote:
> On 07/27/2011 06:00 PM, Daniel Kurtz wrote:
>> On Thu, Jul 28, 2011 at 5:13 AM, Chase Douglas
>> <[e-mail]> wrote:
>> [...]
>>>>>
>>>>>> Would you prefer an implementation that continued to report count (via
>>>>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>>>>> these cases where it could not determine the correct position or track_id
>>>>>> to report?
>>>>>
>>>>> That may be doable, but I would prefer to just assume that tracking ids
>>>>> are not valid when (tracked touches > reported touches).
>>>>
>>>> Userspace is free to make this assumption, of course, but, in fact,
>>>> the image sensor trackpad actually does a pretty good job of tracking
>>>> the fingers - it just has serious issues reporting them!
>>>> Since a track_id change is how userspace is told that the identity of
>>>> the reported finger is changing, if the track_id of a finger position
>>>> datum is unknowable, I'd rather just discard it in the kernel than
>>>> report it to userspace with the wrong track_id.
>>>> Otherwise, the heuristics used in the userspace finger tracking
>>>> algorithms would need to be overly aggressively tuned to handle this
>>>> known error cases:
>>>>   2->3 and 3->2 finger transitions look like 2nd finger motion,
>>>> instead of reported finger changes.
>>>>
>>>>>
>>>>>> It seems like it would be more work for userspace to handle this new way
>>>>>> than the simulated number-of-touch transitions, where the transient
>>>>>> states are all normal valid states.
>>>>>
>>>>> This harkens back to my earlier statements where I said this new
>>>>> Synaptics protocol is worse than the previous one :).
>>>>>
>>>>> I agree that the implementation you gave here might be trickier for
>>>>> userspace, so I'd rather table it unless you feel that the "tracking ids
>>>>> are meaningless!" solution won't work. If you think there are problems
>>>>> with that approach, we can re-evaluate.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> -- Chase
>>>>
>>>> Yes, I feel there are problems with this approach, as I tried to explain above.
>>>> Can you explain why you 'continuation gestures' can't handle 1->2->3
>>>> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
>>>> 3->2->0->3?
>>>>
>>>> I think the only real point left to decide is what BTN_* events should
>>>> signify during these rare transition states:
>>>>   (a) the actually number of fingers on the pad,
>>>>   (b) the number of fingers being reported via the slots
>>>>
>>>> The current patchset does (a).
>>>> We could do (b), if that would get these patches accepted sooner :)
>>>
>>> I was thinking that the current patchset does (b). I think (a) is
>>> better, and if it really works that way then I'm fine with it. It's hard
>>> for me to keep track of the flow of the logic across the patches :).
>>
>> Argh, my bad.  You are correct.  Current patchset does (b)!
>> It reports the number of active slots, not the number of touches.
>>
>> In any case, I really don't know why you need (a).  We are talking
>> about some degenerate transitions here.  Your userspace driver should
>> work just fine with the (b) driver, it just loses some really
>> complicated continued gestures for hardware that can't support them.
>
> To give userspace incorrect information about the number of touches on
> the device is always bad. Lets say the degenerate case is when you go
> from two touches to three (maybe Synaptics doesn't do this, but someone
> else might). When the user performs a three finger tap, we'd see two
> touches down, two lifted up, three touches down, three lifted up in
> short succession. Userspace is gonna get pretty confused about that :).
>
> (Please don't make me try to come up with a use case we already have in
> Unity that would be broken for Synaptics due to this. I could probably
> find one, but it would take quite a bit of thinking. :)

Just to be clear:

Debouncing num-fingers at the start of a tap works just fine.
For a 3f-tap, you would see one of:
  0->1->2->3
  0->2->3
  0->3

Not:
  0->1->2->0->3
  0->2->0->3

You only see 2->0->3 if there had previously been 3 fingers down, and
some of those fingers are removed:
  0->3->0*->2*->0*->3*
  0->3->0*->1*

Where the "*" states are when mt_state_lost = true.
So, with method (b), you might see 3->0->2->0 if the release of the
other two fingers was really slow (longer than 37.5 ms), or, more
likely on a tap, just 3->0.

In any case, I don't think we are making progress arguing (a) vs. (b).
Let me implement method (a), and upload for review.
I agree that it makes some sense to always accurately report number of
touches with the BTN_*, independent of number of slots.

A true MT-B userspace app would always do the right thing with the
slots, legacy apps can always do the right thing in legacy mode, and a
hybrid app get do 2-touch multitouch & use BTN_* to determine total
number of touches.

Was there anything else I should add/change while I'm at it?

You mention documentation below, was there something in particular
that you'd like to see documented better?  Where?

-Dan

>
>>> That said, merging this patchset as is effectively means that the number
>>> of slots is completely decoupled from the number of touches on the
>>> device. Previously, one could say that the number of touches on the
>>> device was equal to the number of open slots or more if all slots were
>>> open. With this approach, we could have 0 slots open during a transition
>>> when there are still touches down.
>>>
>>> While the distinction makes sense for these synaptics devices, I don't
>>> want the semantics to hold for full multitouch devices. Otherwise, we
>>> would have to add many more BTN_*TAPs. If we go this route, we must have
>>> a way to tell userspace that this is a special device where the number
>>> of active touches can only be determined from BTN_*TAP. Thus, we would
>>> need a property for this exception to normal behavior.
>>
>> Henrik & Dmitry roughly suggested "do not define a new property; let
>> userspace detect max BTN_*TAP > ABS_MT_SLOT.max to indicate that
>> BTN_*TAP carries the total number of touches".  I wish they would
>> chime in on this patchset...
>
> I think it sets a really bad precedent to obey the stated protocol in
> most cases, but disregard it in others if specific conditions are met,
> which are not documented and are not presented in a clear manner to
> userspace. At the *very least*, this change would need documentation to
> go in at the same time, but I strongly believe a property is merited here.
>
> -- Chase
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chase Douglas July 28, 2011, 5:31 p.m. UTC | #9

On 07/28/2011 06:56 AM, Daniel Kurtz wrote:
> On Thu, Jul 28, 2011 at 10:07 AM, Chase Douglas
> <[e-mail]> wrote:
>> On 07/27/2011 06:00 PM, Daniel Kurtz wrote:
>>> On Thu, Jul 28, 2011 at 5:13 AM, Chase Douglas
>>> <[e-mail]> wrote:
>>> [...]
>>>>>>
>>>>>>> Would you prefer an implementation that continued to report count (via
>>>>>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>>>>>> these cases where it could not determine the correct position or track_id
>>>>>>> to report?
>>>>>>
>>>>>> That may be doable, but I would prefer to just assume that tracking ids
>>>>>> are not valid when (tracked touches > reported touches).
>>>>>
>>>>> Userspace is free to make this assumption, of course, but, in fact,
>>>>> the image sensor trackpad actually does a pretty good job of tracking
>>>>> the fingers - it just has serious issues reporting them!
>>>>> Since a track_id change is how userspace is told that the identity of
>>>>> the reported finger is changing, if the track_id of a finger position
>>>>> datum is unknowable, I'd rather just discard it in the kernel than
>>>>> report it to userspace with the wrong track_id.
>>>>> Otherwise, the heuristics used in the userspace finger tracking
>>>>> algorithms would need to be overly aggressively tuned to handle this
>>>>> known error cases:
>>>>>   2->3 and 3->2 finger transitions look like 2nd finger motion,
>>>>> instead of reported finger changes.
>>>>>
>>>>>>
>>>>>>> It seems like it would be more work for userspace to handle this new way
>>>>>>> than the simulated number-of-touch transitions, where the transient
>>>>>>> states are all normal valid states.
>>>>>>
>>>>>> This harkens back to my earlier statements where I said this new
>>>>>> Synaptics protocol is worse than the previous one :).
>>>>>>
>>>>>> I agree that the implementation you gave here might be trickier for
>>>>>> userspace, so I'd rather table it unless you feel that the "tracking ids
>>>>>> are meaningless!" solution won't work. If you think there are problems
>>>>>> with that approach, we can re-evaluate.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> -- Chase
>>>>>
>>>>> Yes, I feel there are problems with this approach, as I tried to explain above.
>>>>> Can you explain why you 'continuation gestures' can't handle 1->2->3
>>>>> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
>>>>> 3->2->0->3?
>>>>>
>>>>> I think the only real point left to decide is what BTN_* events should
>>>>> signify during these rare transition states:
>>>>>   (a) the actually number of fingers on the pad,
>>>>>   (b) the number of fingers being reported via the slots
>>>>>
>>>>> The current patchset does (a).
>>>>> We could do (b), if that would get these patches accepted sooner :)
>>>>
>>>> I was thinking that the current patchset does (b). I think (a) is
>>>> better, and if it really works that way then I'm fine with it. It's hard
>>>> for me to keep track of the flow of the logic across the patches :).
>>>
>>> Argh, my bad.  You are correct.  Current patchset does (b)!
>>> It reports the number of active slots, not the number of touches.
>>>
>>> In any case, I really don't know why you need (a).  We are talking
>>> about some degenerate transitions here.  Your userspace driver should
>>> work just fine with the (b) driver, it just loses some really
>>> complicated continued gestures for hardware that can't support them.
>>
>> To give userspace incorrect information about the number of touches on
>> the device is always bad. Lets say the degenerate case is when you go
>> from two touches to three (maybe Synaptics doesn't do this, but someone
>> else might). When the user performs a three finger tap, we'd see two
>> touches down, two lifted up, three touches down, three lifted up in
>> short succession. Userspace is gonna get pretty confused about that :).
>>
>> (Please don't make me try to come up with a use case we already have in
>> Unity that would be broken for Synaptics due to this. I could probably
>> find one, but it would take quite a bit of thinking. :)
> 
> Just to be clear:
> 
> Debouncing num-fingers at the start of a tap works just fine.
> For a 3f-tap, you would see one of:
>   0->1->2->3
>   0->2->3
>   0->3
> 
> Not:
>   0->1->2->0->3
>   0->2->0->3
> 
> You only see 2->0->3 if there had previously been 3 fingers down, and
> some of those fingers are removed:
>   0->3->0*->2*->0*->3*
>   0->3->0*->1*
> 
> Where the "*" states are when mt_state_lost = true.
> So, with method (b), you might see 3->0->2->0 if the release of the
> other two fingers was really slow (longer than 37.5 ms), or, more
> likely on a tap, just 3->0.
> 
> In any case, I don't think we are making progress arguing (a) vs. (b).
> Let me implement method (a), and upload for review.
> I agree that it makes some sense to always accurately report number of
> touches with the BTN_*, independent of number of slots.
> 
> A true MT-B userspace app would always do the right thing with the
> slots, legacy apps can always do the right thing in legacy mode, and a
> hybrid app get do 2-touch multitouch & use BTN_* to determine total
> number of touches.
> 
> Was there anything else I should add/change while I'm at it?

No, I think functionally I would be happy with the new version. I still
want the property bit :). Dmitry, can you weigh in here? What I remember
was you were disinclined towards a new property bit, but I'd like to see
a firm decision taken and the reasoning behind it.

> You mention documentation below, was there something in particular
> that you'd like to see documented better?  Where?

Documentation of anything that goes against the normal protocol should
be in Documentation/input/event-codes.txt and/or
Documentation/input/multi-touch-protocol.txt. The latter seems the most
appropriate place for this. If you want extra credit you could document
SEMI_MT as well, I think that slipped through the cracks; this isn't a
requirement for me, though :).

Thanks!

>>>> That said, merging this patchset as is effectively means that the number
>>>> of slots is completely decoupled from the number of touches on the
>>>> device. Previously, one could say that the number of touches on the
>>>> device was equal to the number of open slots or more if all slots were
>>>> open. With this approach, we could have 0 slots open during a transition
>>>> when there are still touches down.
>>>>
>>>> While the distinction makes sense for these synaptics devices, I don't
>>>> want the semantics to hold for full multitouch devices. Otherwise, we
>>>> would have to add many more BTN_*TAPs. If we go this route, we must have
>>>> a way to tell userspace that this is a special device where the number
>>>> of active touches can only be determined from BTN_*TAP. Thus, we would
>>>> need a property for this exception to normal behavior.
>>>
>>> Henrik & Dmitry roughly suggested "do not define a new property; let
>>> userspace detect max BTN_*TAP > ABS_MT_SLOT.max to indicate that
>>> BTN_*TAP carries the total number of touches".  I wish they would
>>> chime in on this patchset...
>>
>> I think it sets a really bad precedent to obey the stated protocol in
>> most cases, but disregard it in others if specific conditions are met,
>> which are not documented and are not presented in a clear manner to
>> userspace. At the *very least*, this change would need documentation to
>> go in at the same time, but I strongly believe a property is merited here.
>>
>> -- Chase
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index b626b98..893e567 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -659,6 +659,7 @@  static void synaptics_image_sensor_0f(struct synaptics_data *priv,
 				      struct synaptics_mt_state *mt_state)
 {
 	synaptics_mt_state_set(mt_state, 0, -1, -1);
+	priv->mt_state_lost = false;
 }
 
 /* Handle case where mt_state->count = 1 */
@@ -726,6 +727,7 @@  static void synaptics_image_sensor_1f(struct synaptics_data *priv,
 		 * So, empty all slots. We will guess slot 0 on subsequent 1->1
 		 */
 		synaptics_mt_state_set(mt_state, 0, -1, -1);
+		priv->mt_state_lost = true;
 		break;
 	}
 }
@@ -771,6 +773,7 @@  static void synaptics_image_sensor_2f(struct synaptics_data *priv,
 		 * subsequent 2->2
 		 */
 		synaptics_mt_state_set(mt_state, 0, -1, -1);
+		priv->mt_state_lost = true;
 		break;
 	}
 }
@@ -800,14 +803,32 @@  static void synaptics_image_sensor_3f(struct synaptics_data *priv,
 		break;
 	case 2:
 		/*
-		 * On 2->3 transitions, we are given no indication which finger
-		 * was added.
-		 * We don't even know what finger the current AGM packet
-		 * contained.
+		 * After some 3->1 and all 3->2 transitions, we lose track
+		 * of which slot is reported by sgm and agm.
 		 *
-		 * So, empty all slots. They get filled on a subsequent 3->3
+		 * For 2->3 in this state, empty all slots, and we will guess
+		 * (0,1) on a subsequent 0->3.
+		 *
+		 * To userspace, the resulting transition will look like:
+		 *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
 		 */
-		synaptics_mt_state_set(mt_state, 0, -1, -1);
+		if (priv->mt_state_lost) {
+			synaptics_mt_state_set(mt_state, 0, -1, -1);
+			break;
+		}
+
+		/*
+		 * If the (SGM,AGM) really previously contained slots (0, 1),
+		 * then we cannot know what slot was just reported by the AGM,
+		 * because the 2->3 transition can occur either before or after
+		 * the AGM packet. Thus, this most recent AGM could contain
+		 * either the same old slot 1 or the new slot 2.
+		 * Subsequent AGMs will be reporting slot 2.
+		 *
+		 * To userspace, the resulting transition will look like:
+		 *    2:[0,1] -> 1:[0,-1] -> 3:[0,2]
+		 */
+		synaptics_mt_state_set(mt_state, 1, 0, -1);
 		break;
 	case 3:
 		/*
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 87be1fe..e3edfea 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -162,6 +162,7 @@  struct synaptics_data {
 	struct serio *pt_port;			/* Pass-through serio port */
 
 	struct synaptics_mt_state mt_state;	/* Current mt finger state */
+	bool mt_state_lost;			/* mt_state may be incorrect */
 
 	/*
 	 * Last received Advanced Gesture Mode (AGM) packet. An AGM packet