The single touch path imposes a minimum z value (with hysterisis) before
registering BTN_TOUCH. Apply the same (hard coded) threshold when
deciding how many fingers to report in the semi-mt path.
This patch improves performance of the Google Cr-48 chromebook's
extremely sensitive Synaptics profile sensor touchpad by filtering out
touch events for hovering fingers.
Note: We continue to use the same hard coded threshold value used in the
single touch case as it appears this works just as well on these
multitouch profile sensor pads as on whatever pads it was originally
discovered.
Signed-off-by: Chung-Yih Wang <[email protected]>
---
drivers/input/mouse/synaptics.c | 15 +++++++++++----
1 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 8081a0a..746dbcc 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -568,17 +568,22 @@ static void synaptics_report_semi_mt_slot(struct input_dev *dev, int slot,
}
}
+static bool finger_touched(const struct synaptics_hw_state *hw)
+{
+ return (hw->z > 30);
+}
+
static void synaptics_report_semi_mt_data(struct input_dev *dev,
const struct synaptics_hw_state *a,
const struct synaptics_hw_state *b,
int num_fingers)
{
- if (num_fingers >= 2) {
+ if ((num_fingers >= 2) && finger_touched(a) && finger_touched(b)) {
synaptics_report_semi_mt_slot(dev, 0, true, min(a->x, b->x),
min(a->y, b->y));
synaptics_report_semi_mt_slot(dev, 1, true, max(a->x, b->x),
max(a->y, b->y));
- } else if (num_fingers == 1) {
+ } else if ((num_fingers == 1) && finger_touched(a)) {
synaptics_report_semi_mt_slot(dev, 0, true, a->x, a->y);
synaptics_report_semi_mt_slot(dev, 1, false, 0, 0);
} else {
@@ -1040,8 +1045,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
* BTN_TOUCH has to be first as mousedev relies on it when doing
* absolute -> relative conversion
*/
- if (hw.z > 30) input_report_key(dev, BTN_TOUCH, 1);
- if (hw.z < 25) input_report_key(dev, BTN_TOUCH, 0);
+ if (finger_touched(&hw))
+ input_report_key(dev, BTN_TOUCH, 1);
+ if (hw.z < 25)
+ input_report_key(dev, BTN_TOUCH, 0);
if (num_fingers > 0) {
input_report_abs(dev, ABS_X, hw.x);
--
1.7.7.3
Hi Chung-yih Wang,
> The single touch path imposes a minimum z value (with hysterisis) before
> registering BTN_TOUCH. Apply the same (hard coded) threshold when
> deciding how many fingers to report in the semi-mt path.
>
> This patch improves performance of the Google Cr-48 chromebook's
> extremely sensitive Synaptics profile sensor touchpad by filtering out
> touch events for hovering fingers.
>
> Note: We continue to use the same hard coded threshold value used in the
> single touch case as it appears this works just as well on these
> multitouch profile sensor pads as on whatever pads it was originally
> discovered.
>
> Signed-off-by: Chung-Yih Wang <[email protected]>
> ---
The idea is sound and has worked well in practise for a long
time. However, please see the comments inline.
> drivers/input/mouse/synaptics.c | 15 +++++++++++----
> 1 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 8081a0a..746dbcc 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -568,17 +568,22 @@ static void synaptics_report_semi_mt_slot(struct input_dev *dev, int slot,
> }
> }
>
> +static bool finger_touched(const struct synaptics_hw_state *hw)
> +{
> + return (hw->z > 30);
> +}
> +
> static void synaptics_report_semi_mt_data(struct input_dev *dev,
> const struct synaptics_hw_state *a,
> const struct synaptics_hw_state *b,
> int num_fingers)
> {
> - if (num_fingers >= 2) {
> + if ((num_fingers >= 2) && finger_touched(a) && finger_touched(b)) {
> synaptics_report_semi_mt_slot(dev, 0, true, min(a->x, b->x),
> min(a->y, b->y));
> synaptics_report_semi_mt_slot(dev, 1, true, max(a->x, b->x),
> max(a->y, b->y));
> - } else if (num_fingers == 1) {
> + } else if ((num_fingers == 1) && finger_touched(a)) {
> synaptics_report_semi_mt_slot(dev, 0, true, a->x, a->y);
> synaptics_report_semi_mt_slot(dev, 1, false, 0, 0);
> } else {
So if num_fingers == 2 and only one of a and b returns
finger_touched() == true, we fall back to zero fingers?
> @@ -1040,8 +1045,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> * BTN_TOUCH has to be first as mousedev relies on it when doing
> * absolute -> relative conversion
> */
> - if (hw.z > 30) input_report_key(dev, BTN_TOUCH, 1);
> - if (hw.z < 25) input_report_key(dev, BTN_TOUCH, 0);
> + if (finger_touched(&hw))
> + input_report_key(dev, BTN_TOUCH, 1);
> + if (hw.z < 25)
> + input_report_key(dev, BTN_TOUCH, 0);
>
> if (num_fingers > 0) {
> input_report_abs(dev, ABS_X, hw.x);
Why not introduce hysteresis for all fingers here? There is an example
implementation in bcm5974.c in the same directory.
Thanks,
Henrik
On Wed, Feb 22, 2012 at 4:38 PM, Henrik Rydberg <[email protected]> wrote:
> Hi Chung-yih Wang,
>
>> The single touch path imposes a minimum z value (with hysterisis) before
>> registering BTN_TOUCH. Apply the same (hard coded) threshold when
>> deciding how many fingers to report in the semi-mt path.
>>
>> This patch improves performance of the Google Cr-48 chromebook's
>> extremely sensitive Synaptics profile sensor touchpad by filtering out
>> touch events for hovering fingers.
>>
>> Note: We continue to use the same hard coded threshold value used in the
>> single touch case as it appears this works just as well on these
>> multitouch profile sensor pads as on whatever pads it was originally
>> discovered.
>>
>> Signed-off-by: Chung-Yih Wang <[email protected]>
>> ---
>
> The idea is sound and has worked well in practise for a long
> time. However, please see the comments inline.
>
>> ?drivers/input/mouse/synaptics.c | ? 15 +++++++++++----
>> ?1 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index 8081a0a..746dbcc 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -568,17 +568,22 @@ static void synaptics_report_semi_mt_slot(struct input_dev *dev, int slot,
>> ? ? ? }
>> ?}
>>
>> +static bool finger_touched(const struct synaptics_hw_state *hw)
>> +{
>> + ? ? return (hw->z > 30);
>> +}
>> +
>> ?static void synaptics_report_semi_mt_data(struct input_dev *dev,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const struct synaptics_hw_state *a,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const struct synaptics_hw_state *b,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int num_fingers)
>> ?{
>> - ? ? if (num_fingers >= 2) {
>> + ? ? if ((num_fingers >= 2) && finger_touched(a) && finger_touched(b)) {
>> ? ? ? ? ? ? ? synaptics_report_semi_mt_slot(dev, 0, true, min(a->x, b->x),
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? min(a->y, b->y));
>> ? ? ? ? ? ? ? synaptics_report_semi_mt_slot(dev, 1, true, max(a->x, b->x),
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? max(a->y, b->y));
>> - ? ? } else if (num_fingers == 1) {
>> + ? ? } else if ((num_fingers == 1) && finger_touched(a)) {
>> ? ? ? ? ? ? ? synaptics_report_semi_mt_slot(dev, 0, true, a->x, a->y);
>> ? ? ? ? ? ? ? synaptics_report_semi_mt_slot(dev, 1, false, 0, 0);
>> ? ? ? } else {
>
> So if num_fingers == 2 and only one of a and b returns
> finger_touched() == true, we fall back to zero fingers?
Actually, yes. In this case, we will have 2 x's and 2 y's, but not
know which belong to a good finger and which belong to a too light
finger.... sigh... synaptics... sigh...
>
>> @@ -1040,8 +1045,10 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>> ? ? ? ?* BTN_TOUCH has to be first as mousedev relies on it when doing
>> ? ? ? ?* absolute -> relative conversion
>> ? ? ? ?*/
>> - ? ? if (hw.z > 30) input_report_key(dev, BTN_TOUCH, 1);
>> - ? ? if (hw.z < 25) input_report_key(dev, BTN_TOUCH, 0);
>> + ? ? if (finger_touched(&hw))
>> + ? ? ? ? ? ? input_report_key(dev, BTN_TOUCH, 1);
>> + ? ? if (hw.z < 25)
>> + ? ? ? ? ? ? input_report_key(dev, BTN_TOUCH, 0);
>>
>> ? ? ? if (num_fingers > 0) {
>> ? ? ? ? ? ? ? input_report_abs(dev, ABS_X, hw.x);
>
> Why not introduce hysteresis for all fingers here? There is an example
> implementation in bcm5974.c in the same directory.
Good idea, can it be in a different, follow-up patch?
>
> Thanks,
> Henrik
Hi Daniel,
> > So if num_fingers == 2 and only one of a and b returns
> > finger_touched() == true, we fall back to zero fingers?
>
> Actually, yes. In this case, we will have 2 x's and 2 y's, but not
> know which belong to a good finger and which belong to a too light
> finger.... sigh... synaptics... sigh...
I see the problem. However, ignoring it will just move the problem
forward to another bug report, will it not? Hysteresis is a slam dunk
here. In addition, since the low-pressure state is bound to be
transitional (soon to be followed by a real num_fingers==1 package),
simply skipping such packages might be a better option.
> > Why not introduce hysteresis for all fingers here? There is an example
> > implementation in bcm5974.c in the same directory.
>
> Good idea, can it be in a different, follow-up patch?
Why should it be?
Thanks,
Henrik
On Wed, Feb 22, 2012 at 7:04 PM, Henrik Rydberg <[email protected]> wrote:
> Hi Daniel,
>
>> > So if num_fingers == 2 and only one of a and b returns
>> > finger_touched() == true, we fall back to zero fingers?
>>
>> Actually, yes. ? In this case, we will have 2 x's and 2 y's, but not
>> know which belong to a good finger and which belong to a too light
>> finger.... ?sigh... synaptics... sigh...
>
> I see the problem. However, ignoring it will just move the problem
> forward to another bug report, will it not? ?Hysteresis is a slam dunk
> here. ?In addition, since the low-pressure state is bound to be
> transitional (soon to be followed by a real num_fingers==1 package),
> simply skipping such packages might be a better option.
In practice, we don't actually see the profile sensor pad sending one
low-z finger, and one high-z finger. In the case where one finger is
solidly on the pad, and another finger is hovering, lifting, or
alighting, the pad sends 2 high-z fingers, with one of them having a
completely wrong x or y coordinate. The two reported z-values are
nearly, but not exactly, identical. I can't think of good fix for
this, other than adding finger tracking and filtering out via
'moved-too-far-too-fast', where possible, and I'd prefer that this be
handled in userspace. The 1-low-z && 1-high-z case that we are
discussing here isn't actually ever triggered; either both fingers are
high-z, or neither are.
The real usefulness of this patch is filtering out the 1-low-z-finger
and 2-low-z fingers cases.
As for the hypothetical 1-finger-hi, 1-finger-low case, which I asked
Chung-yih to add because it seemed like a good idea in theory...
Yes, I think you have a good point. Thanks to evdev's stateful
nature, simply skipping the (1-strong,1-weak) packet might actually
work better than forcing num_fingers == 0.
For cases where a second finger is temporarily reporting low-z because
it is arriving or leaving, evdev would just lock the (1 or 2 initial)
fingers in their current position until the transition is over, and
then start reporting the new number of fingers at their new positions.
For cases where there is one high-z finger, and a hovering thumb or
palm triggers 2-finger reporting temporarily (without ever going above
the threshold), the original finger will get frozen in its current
position until the hovering finger is no longer detected, and then
snap to its new position. This might cause strange sudden jumps, but
that seems unavoidable.
I'm not sure hysteresis is a "slam dunk"... in fact, I don't see how
it would help much. But, it is hard to argue against adding the
functionality, since the hysteresis window can be made arbitrarily
small. Perhaps if you are inclined, you can elaborate on why you
think it is important.
Thanks for you excellent feedback!
-Daniel
>
>> > Why not introduce hysteresis for all fingers here? There is an example
>> > implementation in bcm5974.c in the same directory.
>>
>> Good idea, can it be in a different, follow-up patch?
>
> Why should it be?
>
> Thanks,
> Henrik
> > I see the problem. However, ignoring it will just move the problem
> > forward to another bug report, will it not? ?Hysteresis is a slam dunk
> > here. ?In addition, since the low-pressure state is bound to be
> > transitional (soon to be followed by a real num_fingers==1 package),
> > simply skipping such packages might be a better option.
>
> In practice, we don't actually see the profile sensor pad sending one
> low-z finger, and one high-z finger. In the case where one finger is
> solidly on the pad, and another finger is hovering, lifting, or
> alighting, the pad sends 2 high-z fingers, with one of them having a
> completely wrong x or y coordinate.
Urgh.
> The two reported z-values are
> nearly, but not exactly, identical. I can't think of good fix for
> this, other than adding finger tracking and filtering out via
> 'moved-too-far-too-fast', where possible, and I'd prefer that this be
> handled in userspace.
It sounds like the z value in the second packet carries zero
information. If that were true, the fact that the patch is effective
suggests the semi-mt slot reporting could follow BTN_TOUCH, more or
less. In doing so, you would also obtain hysteresis automatically.
> The 1-low-z && 1-high-z case that we are
> discussing here isn't actually ever triggered; either both fingers are
> high-z, or neither are.
I suspect it depends a bit on the values of low-z and hi-z,
respectively? Otherwise, there really is no information in that extra
packet.
> The real usefulness of this patch is filtering out the 1-low-z-finger
> and 2-low-z fingers cases.
>
> As for the hypothetical 1-finger-hi, 1-finger-low case, which I asked
> Chung-yih to add because it seemed like a good idea in theory...
>
> Yes, I think you have a good point. Thanks to evdev's stateful
> nature, simply skipping the (1-strong,1-weak) packet might actually
> work better than forcing num_fingers == 0.
>
> For cases where a second finger is temporarily reporting low-z because
> it is arriving or leaving, evdev would just lock the (1 or 2 initial)
> fingers in their current position until the transition is over, and
> then start reporting the new number of fingers at their new positions.
>
> For cases where there is one high-z finger, and a hovering thumb or
> palm triggers 2-finger reporting temporarily (without ever going above
> the threshold), the original finger will get frozen in its current
> position until the hovering finger is no longer detected, and then
> snap to its new position. This might cause strange sudden jumps, but
> that seems unavoidable.
A lot of things seem unavoidable with this hardware. :-)
> I'm not sure hysteresis is a "slam dunk"... in fact, I don't see how
> it would help much. But, it is hard to argue against adding the
> functionality, since the hysteresis window can be made arbitrarily
> small. Perhaps if you are inclined, you can elaborate on why you
> think it is important.
The most striking effect is the ability to better retain a
drag. Although the statement was made in light of possible
(1-strong,1-weak) packets, it should help in the 2-weak case too.
Thanks,
Henrik
On Wed, Feb 22, 2012 at 04:24:25PM +0100, Henrik Rydberg wrote:
> > > I see the problem. However, ignoring it will just move the problem
> > > forward to another bug report, will it not? ?Hysteresis is a slam dunk
> > > here. ?In addition, since the low-pressure state is bound to be
> > > transitional (soon to be followed by a real num_fingers==1 package),
> > > simply skipping such packages might be a better option.
> >
> > In practice, we don't actually see the profile sensor pad sending one
> > low-z finger, and one high-z finger. In the case where one finger is
> > solidly on the pad, and another finger is hovering, lifting, or
> > alighting, the pad sends 2 high-z fingers, with one of them having a
> > completely wrong x or y coordinate.
>
> Urgh.
>
> > The two reported z-values are
> > nearly, but not exactly, identical. I can't think of good fix for
> > this, other than adding finger tracking and filtering out via
> > 'moved-too-far-too-fast', where possible, and I'd prefer that this be
> > handled in userspace.
>
> It sounds like the z value in the second packet carries zero
> information. If that were true, the fact that the patch is effective
> suggests the semi-mt slot reporting could follow BTN_TOUCH, more or
> less. In doing so, you would also obtain hysteresis automatically.
>
> > The 1-low-z && 1-high-z case that we are
> > discussing here isn't actually ever triggered; either both fingers are
> > high-z, or neither are.
>
> I suspect it depends a bit on the values of low-z and hi-z,
> respectively? Otherwise, there really is no information in that extra
> packet.
>
> > The real usefulness of this patch is filtering out the 1-low-z-finger
> > and 2-low-z fingers cases.
> >
> > As for the hypothetical 1-finger-hi, 1-finger-low case, which I asked
> > Chung-yih to add because it seemed like a good idea in theory...
> >
> > Yes, I think you have a good point. Thanks to evdev's stateful
> > nature, simply skipping the (1-strong,1-weak) packet might actually
> > work better than forcing num_fingers == 0.
> >
> > For cases where a second finger is temporarily reporting low-z because
> > it is arriving or leaving, evdev would just lock the (1 or 2 initial)
> > fingers in their current position until the transition is over, and
> > then start reporting the new number of fingers at their new positions.
> >
> > For cases where there is one high-z finger, and a hovering thumb or
> > palm triggers 2-finger reporting temporarily (without ever going above
> > the threshold), the original finger will get frozen in its current
> > position until the hovering finger is no longer detected, and then
> > snap to its new position. This might cause strange sudden jumps, but
> > that seems unavoidable.
>
> A lot of things seem unavoidable with this hardware. :-)
>
> > I'm not sure hysteresis is a "slam dunk"... in fact, I don't see how
> > it would help much. But, it is hard to argue against adding the
> > functionality, since the hysteresis window can be made arbitrarily
> > small. Perhaps if you are inclined, you can elaborate on why you
> > think it is important.
>
> The most striking effect is the ability to better retain a
> drag. Although the statement was made in light of possible
> (1-strong,1-weak) packets, it should help in the 2-weak case too.
The bigger question is why is this needed in kernel. The original
hysteresis with BTN_TOUCH was done for sole benefit of mousedev so that
we could allow somewhat better transition from standard PS/2 mode into
native Synaptics mode with absolute coordinates at time when barely
anyone had Synaptics X driver installed. This was, what, 10 years ago?
Thanks.
--
Dmitry
> > The most striking effect is the ability to better retain a
> > drag. Although the statement was made in light of possible
> > (1-strong,1-weak) packets, it should help in the 2-weak case too.
>
> The bigger question is why is this needed in kernel. The original
> hysteresis with BTN_TOUCH was done for sole benefit of mousedev so that
> we could allow somewhat better transition from standard PS/2 mode into
> native Synaptics mode with absolute coordinates at time when barely
> anyone had Synaptics X driver installed. This was, what, 10 years ago?
The semi-mt behavior is obviously a special case, where userspace
relies on the reported number of fingers to transition between one
touch and two touches. There is no pressure information sent to
userspace in this case, so the situation is in fact quite similar to
the ancient mousedev situation.
Henrik
On Fri, Feb 24, 2012 at 10:08:35AM +0100, Henrik Rydberg wrote:
> > > The most striking effect is the ability to better retain a
> > > drag. Although the statement was made in light of possible
> > > (1-strong,1-weak) packets, it should help in the 2-weak case too.
> >
> > The bigger question is why is this needed in kernel. The original
> > hysteresis with BTN_TOUCH was done for sole benefit of mousedev so that
> > we could allow somewhat better transition from standard PS/2 mode into
> > native Synaptics mode with absolute coordinates at time when barely
> > anyone had Synaptics X driver installed. This was, what, 10 years ago?
>
> The semi-mt behavior is obviously a special case, where userspace
> relies on the reported number of fingers to transition between one
> touch and two touches. There is no pressure information sent to
> userspace in this case, so the situation is in fact quite similar to
> the ancient mousedev situation.
We still report ABS_PRESSURE but maybe we should report ABS_MT_PRESSURE
as well?
--
Dmitry
On Fri, Feb 24, 2012 at 01:14:52AM -0800, Dmitry Torokhov wrote:
> On Fri, Feb 24, 2012 at 10:08:35AM +0100, Henrik Rydberg wrote:
> > > > The most striking effect is the ability to better retain a
> > > > drag. Although the statement was made in light of possible
> > > > (1-strong,1-weak) packets, it should help in the 2-weak case too.
> > >
> > > The bigger question is why is this needed in kernel. The original
> > > hysteresis with BTN_TOUCH was done for sole benefit of mousedev so that
> > > we could allow somewhat better transition from standard PS/2 mode into
> > > native Synaptics mode with absolute coordinates at time when barely
> > > anyone had Synaptics X driver installed. This was, what, 10 years ago?
> >
> > The semi-mt behavior is obviously a special case, where userspace
> > relies on the reported number of fingers to transition between one
> > touch and two touches. There is no pressure information sent to
> > userspace in this case, so the situation is in fact quite similar to
> > the ancient mousedev situation.
>
> We still report ABS_PRESSURE but maybe we should report ABS_MT_PRESSURE
> as well?
BTW, the reason I do not like this in kernel is because it dos not allow
users control touchpad sensitivity.
--
Dmitry
On Fri, Feb 24, 2012 at 01:16:02AM -0800, Dmitry Torokhov wrote:
> On Fri, Feb 24, 2012 at 01:14:52AM -0800, Dmitry Torokhov wrote:
> > On Fri, Feb 24, 2012 at 10:08:35AM +0100, Henrik Rydberg wrote:
> > > > > The most striking effect is the ability to better retain a
> > > > > drag. Although the statement was made in light of possible
> > > > > (1-strong,1-weak) packets, it should help in the 2-weak case too.
> > > >
> > > > The bigger question is why is this needed in kernel. The original
> > > > hysteresis with BTN_TOUCH was done for sole benefit of mousedev so that
> > > > we could allow somewhat better transition from standard PS/2 mode into
> > > > native Synaptics mode with absolute coordinates at time when barely
> > > > anyone had Synaptics X driver installed. This was, what, 10 years ago?
> > >
> > > The semi-mt behavior is obviously a special case, where userspace
> > > relies on the reported number of fingers to transition between one
> > > touch and two touches. There is no pressure information sent to
> > > userspace in this case, so the situation is in fact quite similar to
> > > the ancient mousedev situation.
> >
> > We still report ABS_PRESSURE but maybe we should report ABS_MT_PRESSURE
> > as well?
>
> BTW, the reason I do not like this in kernel is because it dos not allow
> users control touchpad sensitivity.
...which is perfectly reasonable, I agree. OTOH, the semi-mt devices
tend to report close-to-bogus information for anything related to
individual touches. Pressure is sadly no different, as Daniel pointed
out. I don't know what is the lesser evil here.
Henrik