Bug 94379 - Hard to position a cursor on Thinkad X1 Carbon 3rd gen after f6c2d4b8b5e1968411568d81b47488a655ba47a1.
Summary: Hard to position a cursor on Thinkad X1 Carbon 3rd gen after f6c2d4b8b5e19684...
Status: RESOLVED FIXED
Alias: None
Product: Wayland
Classification: Unclassified
Component: libinput (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Peter Hutterer
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 94601
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-03 01:03 UTC by Vasily Khoruzhick
Modified: 2016-06-24 04:02 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
evemu-record capture (290.53 KB, text/plain)
2016-03-03 01:03 UTC, Vasily Khoruzhick
Details
record-jumps.txt (220.23 KB, text/plain)
2016-03-03 02:15 UTC, Vasily Khoruzhick
Details
reproducer on the t450s (127.75 KB, text/plain)
2016-03-03 10:41 UTC, Benjamin Tissoires
Details
0001-touchpad-add-quirk-for-the-T450-generation-hardware.patch (2.77 KB, patch)
2016-03-07 06:07 UTC, Peter Hutterer
Details | Splinter Review
0001-touchpad-add-quirk-for-the-T450-generation-hardware.patch (3.55 KB, patch)
2016-03-07 21:38 UTC, Peter Hutterer
Details | Splinter Review
0001-touchpad-add-quirk-for-the-T450-generation-hardware.patch (3.38 KB, patch)
2016-03-10 07:36 UTC, Peter Hutterer
Details | Splinter Review
0001-touchpad-only-post-motion-events-if-we-have-motion.patch (1.76 KB, patch)
2016-03-29 05:59 UTC, Peter Hutterer
Details | Splinter Review
evemu-record-jumps-of-finger-removal.txt (217.11 KB, text/plain)
2016-03-29 23:50 UTC, Vasily Khoruzhick
Details
0001-touchpad-reset-the-motion-history-on-significant-neg.patch (4.83 KB, patch)
2016-03-30 05:50 UTC, Peter Hutterer
Details | Splinter Review
another_evemu_record.txt (160.19 KB, text/plain)
2016-03-30 06:51 UTC, Vasily Khoruzhick
Details
evemu-record-for-video.txt (306.91 KB, text/plain)
2016-04-14 18:21 UTC, Vasily Khoruzhick
Details

Description Vasily Khoruzhick 2016-03-03 01:03:58 UTC
Created attachment 122089 [details]
evemu-record capture

It's quite hard to position a cursor on small elements (menu items, close button, etc) after f6c2d4b8b5e1968411568d81b47488a655ba47a1. Small movements are too jerky in vertical direction.

Reverting this commit helps.
Comment 1 Vasily Khoruzhick 2016-03-03 02:15:16 UTC
Created attachment 122090 [details]
record-jumps.txt
Comment 2 Peter Hutterer 2016-03-03 04:37:00 UTC
recap from the IRC conversation we had about this: the main cause for the finger motion seems to be the pressure on the touchpad, lifting the finger causes a jump as well. light touches don't see the jerky movements.
Comment 3 Benjamin Tissoires 2016-03-03 10:41:40 UTC
Created attachment 122095 [details]
reproducer on the t450s

I think I can reproduce it here as well.
This is a reproducer for Peter to try a patch
Comment 4 Peter Hutterer 2016-03-07 05:16:27 UTC
the common pattern in attachment 122095 [details] is: a series of events with x and/or y updates, then a set of updates for pressure and major only, followed by at least one x/y event. That event usually has a jump of ~20 device units and afaict from this event is then immediately followed by other small motion events.

see the events at 9.710062. X is at 850, pressure only until 10.797246 where X jumps to 870.
Comment 5 Peter Hutterer 2016-03-07 06:07:28 UTC
Created attachment 122133 [details] [review]
0001-touchpad-add-quirk-for-the-T450-generation-hardware.patch

Possible fix. This would need some extra work to only apply to this touchpad but for you guys this doesn't matter.

It avoids the jumps in benjamin's recording but I have no ideal if it feels right, the recording obviously won't show me that. Please test this and let me know how it works.
Comment 6 Benjamin Tissoires 2016-03-07 09:30:42 UTC
(In reply to Peter Hutterer from comment #5)
> It avoids the jumps in benjamin's recording but I have no ideal if it feels
> right, the recording obviously won't show me that. Please test this and let
> me know how it works.

It doesn't feel right unfortunately:

At low speed (when trying to reach pixels), the feeling is good IMO. It's not perfect but at least there are no more jumps. The pointer seams to be stuck from time to time, but that's pretty much the best we can do given the incoming data.

However, at regular speed of use, this patch is going to annoy users. The events are now jumpy and and the cursor is definitively not as smooth as it was.

We should try to apply the heuristic only when the speed is low and disable it most of the time...
Comment 7 Peter Hutterer 2016-03-07 21:38:45 UTC
Created attachment 122153 [details] [review]
0001-touchpad-add-quirk-for-the-T450-generation-hardware.patch

v2 of the patch, this time with a timer but more importantly, the event counting/resetting was fixed. before it would accidentally reset every so often during normal processing.

If this patch works better please also try taking out the time check (third hunk, remove the line:

+                   time - tp->quirks.nonmotion_first_time > ms2us(500)) {
Comment 8 Peter Hutterer 2016-03-10 07:36:24 UTC
Created attachment 122196 [details] [review]
0001-touchpad-add-quirk-for-the-T450-generation-hardware.patch

Ok, version 3, I just took the timeout out again. I tested this on a T450 here with the 4.4.3-300.fc23 kernel and it works pretty much as I'd expect. I still get the occasional jump on slow motion, fast motion and medium motion is fine.
Comment 9 Benjamin Tissoires 2016-03-10 14:15:09 UTC
OK, this one works for me. Thanks for doing the fine tuning.

You can add my tested by. We still need to make this t450s specific, and probably also test the X1 carbon3, W541 and the other laptops with similar touchpad.
Comment 11 Marco Neumann 2016-03-20 16:23:41 UTC
Seems that the change leads to a new bug: https://bugs.freedesktop.org/show_bug.cgi?id=94601
Comment 12 Vasily Khoruzhick 2016-03-23 19:27:11 UTC
Patch doesn't fix the issue for me. Cursor still jumps when I remove a finger.
Comment 13 Peter Hutterer 2016-03-28 23:41:27 UTC
Fix for the issue identified in comment 3 was committed, but that's not the original issue reported (which has the jump on removing the finger)

commit a608d9dc2c70c7915fc94466ed27c1684f65409e
Author: Peter Hutterer <peter.hutterer@who-t.net>
Date:   Mon Mar 7 16:05:25 2016 +1000

    touchpad: add quirk for the T450 and T460 generation hardware
Comment 14 Peter Hutterer 2016-03-29 05:59:16 UTC
Created attachment 122601 [details] [review]
0001-touchpad-only-post-motion-events-if-we-have-motion.patch

Follow-up to attachment 122196 [details] [review], this should correct bug 94601 (I think). Right now I'm mostly looking for feedback if that changes the motion in some unexpected way that I haven't noticed yet, it's likely not complete yet (I don't have tests yet). If you can give it a try, that'd be much appreciated.

Probably won't fix the jump on release either, just the jerky motion when moving slowly.
Comment 15 Marco Neumann 2016-03-29 20:10:54 UTC
Good news: I've tested attachment 122601 [details] [review] (patched version 1.2.2 with it) for about 10 hours. Bug 94601 does not occur anymore (neither the visible effect that bugs the user nor the gdm log entry).

On the other hand: I've never experienced the original bug 94379 (or didn't pay close attention).
Comment 16 Vasily Khoruzhick 2016-03-29 23:50:54 UTC
Created attachment 122628 [details]
evemu-record-jumps-of-finger-removal.txt
Comment 17 Peter Hutterer 2016-03-30 05:50:14 UTC
Created attachment 122632 [details] [review]
0001-touchpad-reset-the-motion-history-on-significant-neg.patch

This patch should fix the cursor jump on release, at least for most cases. It goes on top of attachment 122601 [details] [review] which goes on top of today's master (455498e9).

It uses negative pressure changes to detect releases and swallows the motion. Please give it a test.
Comment 18 Vasily Khoruzhick 2016-03-30 06:51:47 UTC
Created attachment 122633 [details]
another_evemu_record.txt
Comment 19 Peter Hutterer 2016-03-31 02:57:11 UTC
I've reduced the threshold to <= -5 in the patch above, but the recording still shows some downwards movement at the end of the touch. The issue is largely that we don't see the pressure change until the downwards finger movement has already started. 

the events starting 1.333662 show this nicely. the y movement goes from 2463 to 2473 before the final event with a pressure change which we now skip. That movement though is undetectable and indistinguishable from a normal finger movement. I don't know how to fix this short of obviously reverting the motion history patch, but doing so makes the pointer terribly unresponsive to slow motions.
Comment 20 Peter Hutterer 2016-04-04 04:00:26 UTC
just for the archives (conversation on IRC) if you can record a video of your finger position and interaction with the touchpad that'd be appreciated. it may show why you trigger this, even though I fear that there may a "you're holding it wrong" outcome...
Comment 21 Vasily Khoruzhick 2016-04-14 18:20:06 UTC
(In reply to Peter Hutterer from comment #20)
> just for the archives (conversation on IRC) if you can record a video of
> your finger position and interaction with the touchpad that'd be
> appreciated. it may show why you trigger this, even though I fear that there
> may a "you're holding it wrong" outcome...

Here's link to a video:

https://www.dropbox.com/s/c08mgg81rvdnl1p/VID_20160414_111037094.mp4?dl=0
Comment 22 Vasily Khoruzhick 2016-04-14 18:21:17 UTC
Created attachment 122947 [details]
evemu-record-for-video.txt
Comment 23 Peter Hutterer 2016-04-18 06:03:00 UTC
hmm, when I replay this on the current libinput master I get almost no pointer motion at the end of a touch. there may be still a pixel or two, but not on all those touches. In fact, I think it's only the first touch sequence that may have some extra movement, can you confirm this?
Comment 24 Vasily Khoruzhick 2016-04-18 16:53:41 UTC
(In reply to Peter Hutterer from comment #23)
> hmm, when I replay this on the current libinput master I get almost no
> pointer motion at the end of a touch. there may be still a pixel or two, but
> not on all those touches. In fact, I think it's only the first touch
> sequence that may have some extra movement, can you confirm 

First 3 motions and last 3 definitely have motion at the end of touch with libinput-1.2.3. Do you want me to retry with current master?
Comment 25 Peter Hutterer 2016-04-18 21:20:28 UTC
yes please
Comment 26 Vasily Khoruzhick 2016-04-18 21:39:19 UTC
(In reply to Peter Hutterer from comment #25)
> yes please

There's a jump in 1st, 2nd, 4th, 8th, 12th movements. It's enough to jump out of edges of some button, or change chosen menu item to another one.
Comment 27 Peter Hutterer 2016-04-19 07:28:31 UTC
ok, after a fair bit of trying I think I can reproduce this now, ca once in five attempts. I also pushed a patch to display a grid in the event gui, that makes the movement a bit easier to spot.

what I can see is a 1-2 pixel movement on finger release, when I hold my finger in a specific position and exert some pressure (more than I would usually, but not excessive). Unfortunately, that movement is impossible to spot from the event stream, pressure doesn't change significantly and it still looks the same as normal movements. So yeah, I'm almost resigned to having to re-introduce the hysteresis, the main question is now just whether it's needed for other touchpads too, haven't tested that yet.
Comment 28 Vasily Khoruzhick 2016-04-20 00:45:35 UTC
(In reply to Peter Hutterer from comment #27)
> ok, after a fair bit of trying I think I can reproduce this now, ca once in
> five attempts. I also pushed a patch to display a grid in the event gui,
> that makes the movement a bit easier to spot.
> 
> what I can see is a 1-2 pixel movement on finger release, when I hold my
> finger in a specific position and exert some pressure (more than I would
> usually, but not excessive). Unfortunately, that movement is impossible to
> spot from the event stream, pressure doesn't change significantly and it
> still looks the same as normal movements. So yeah, I'm almost resigned to
> having to re-introduce the hysteresis, the main question is now just whether
> it's needed for other touchpads too, haven't tested that yet.

Is it possible to introduce a knob for hysteresis? Some people may want to keep it disabled.
Comment 29 Peter Hutterer 2016-04-26 05:26:24 UTC
Not for hysteresis directly, but *maybe* a more generic knob for accuracy vs. steadiness. I'm loath to expose internal implementation as a config knob.
Comment 30 Vasily Khoruzhick 2016-04-26 05:27:13 UTC
(In reply to Peter Hutterer from comment #29)
> Not for hysteresis directly, but *maybe* a more generic knob for accuracy
> vs. steadiness. I'm loath to expose internal implementation as a config knob.

Sounds good.
Comment 31 Peter Hutterer 2016-06-24 04:02:52 UTC
long story short, hysteresis is re-enabled for all devices now. all other attempts failed.

commit 48473994c8e60189356feae7b7eae25288e5ac28
Author: Peter Hutterer <peter.hutterer@who-t.net>
Date:   Thu Jun 16 16:11:56 2016 +1000

    touchpad: re-enable hysteresis by default for all devices


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.