Bug 105963 - Lenovo P50 - Slow fine touchpad movement makes it jump (bug 2)
Summary: Lenovo P50 - Slow fine touchpad movement makes it jump (bug 2)
Status: RESOLVED FIXED
Alias: None
Product: Wayland
Classification: Unclassified
Component: libinput (show other bugs)
Version: unspecified
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Wayland bug list
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-10 04:06 UTC by Tim Richardson
Modified: 2018-04-18 06:18 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
email garbage (2.05 KB, text/html)
2018-04-10 08:12 UTC, Tim Richardson
Details
patch for P50 (8.39 KB, patch)
2018-04-10 23:44 UTC, Tim Richardson
Details | Splinter Review
email garbage (6.82 KB, text/html)
2018-04-11 01:20 UTC, Tim Richardson
Details
fix duplicate flag value is evdev.h (708 bytes, patch)
2018-04-11 09:43 UTC, Tim Richardson
Details | Splinter Review
Patch to reapply the p50 rule & fix syntax error in existing rules (1.38 KB, patch)
2018-04-11 11:35 UTC, Tim Richardson
Details | Splinter Review
experimental changes to touchpad_accel_profile (2.15 KB, patch)
2018-04-11 13:00 UTC, Tim Richardson
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Richardson 2018-04-10 04:06:42 UTC
as requested https://bugs.freedesktop.org/show_bug.cgi?id=105022#c40 
making a new bug report, following on from that comment:

Tim:
can we move this to a separate bug please? This one is too noisy to still figure out what's going on. As a general tip - when you're working on libinput, put printfs in everywhere and run either sudo ./build/libinput-debug-events (commandline) or sudo ./build/libinput-debug-gui (a GUI). No need to install and restart X just for basic testing.

If even the above makes no difference, it's likely that the model flag is never applied, put a printf in for that in evdev.c. 

> I plan to understand the acceleration logic and change it somehow, so that 
> there must be a certain number of events before acceleration is applied, 
> similar to the idea in the code above.


That's an interesting idea. We previously had code that we required N events for the pointer to start moving, but that didn't work well, too laggy. But not having acceleration for the first N events could work (N <=2, I suspect...)
Comment 1 Tim Richardson 2018-04-10 04:26:01 UTC
in 

tp_need_motion_history_reset(struct tp_dispatch *tp)

I added this 



static inline bool
tp_need_motion_history_reset(struct tp_dispatch *tp)
{
	bool rc = false;
	evdev_log_debug(tp->device, "Checking for consecutive non-motion events\n");
			}





but I am not getting any such log messages at all.
Comment 2 Tim Richardson 2018-04-10 04:52:27 UTC
ok, so I changed to printf and now I get something

the quirk is not active, the AND of the flag is returning 0

printf("Flag check: %d %d and then AND: %d\n",tp->device->model_flags,EVDEV_MODEL_LENOVO_T450_TOUCHPAD,tp->device->model_flags & EVDEV_MODEL_LENOVO_T450_TOUCHPAD);

gives


Flag check: 512 131072 and then AND: 0
Comment 3 Tim Richardson 2018-04-10 05:04:57 UTC
I have a system file in /lib/udev/hwdb.d which is out of date. It has not P50 rule

the ninja install is updating /usr/lib/udev/hwdb.d
this has the P50 rule but it looks like it is not being used

is it in the wrong place?

http://manpages.ubuntu.com/manpages/xenial/man7/udev.7.html
Comment 4 Tim Richardson 2018-04-10 05:13:15 UTC
OK, it was in the wrong place. After copying the correct quirk rules file to the Ubuntu location, and rebooting the quirk is active. It definitely has an effect, I'm not sure it is a good effect but I can play now.
Comment 5 Peter Hutterer 2018-04-10 07:57:32 UTC
/lib isn't a symlink to /usr/lib? interesting... Anyway, you can pass -Dudev-dir=/lib/udev/ at configure time to install to the right directory.
Comment 6 Tim Richardson 2018-04-10 08:12:39 UTC
Created attachment 138720 [details]
email garbage

thanks. I'm having fun working it all out. There is no problem with
scrolling and the sudden movements are better. The acceleration factor
needs to be much slower at slow movements, and the pressure sensitivity may
need tweaking. I'll keep fiddling until it works like Windows. Then we can
see how the configuration can work.

On 10 April 2018 at 17:57, <bugzilla-daemon@freedesktop.org> wrote:

> *Comment # 5 <https://bugs.freedesktop.org/show_bug.cgi?id=105963#c5> on
> bug 105963 <https://bugs.freedesktop.org/show_bug.cgi?id=105963> from Peter
> Hutterer <peter.hutterer@who-t.net> *
>
> /lib isn't a symlink to /usr/lib? interesting... Anyway, you can pass
> -Dudev-dir=/lib/udev/ at configure time to install to the right directory.
>
> ------------------------------
> You are receiving this mail because:
>
>    - You reported the bug.
>
>
Comment 7 Tim Richardson 2018-04-10 09:57:01 UTC
Peter, don't deploy the P50 changes. After a reboot, the touchpad was unusable.
I think sharing a flag with other quirks causes problems. In my branch I have created a separate P50 flag.
Comment 8 Tim Richardson 2018-04-10 21:18:12 UTC
the best of fixing this will be different acceleration behaviour, to mimic windows. I can't get smooth movement any other way, I think. 
when I do 

 xinput --set-prop "$DEVICE" "libinput Accel Speed"  -.9

I get very slow pointer movement, as expected .But I can't see where in libinput this setting influences behaviour. 
printf diagnostics in 
touchpad_accel_profile_linear
don't seem to show any change between a setting of -0.9 and 0 although the pointer behaviour is really different.
Comment 9 Tim Richardson 2018-04-10 23:44:19 UTC
Created attachment 138744 [details] [review]
patch for P50

With a setting such as 
 xinput --set-prop "$DEVICE" "libinput Accel Speed"  1

this patch works pretty well for the P50
Comment 10 Peter Hutterer 2018-04-11 00:42:11 UTC
Comment on attachment 138744 [details] [review]
patch for P50

Review of attachment 138744 [details] [review]:
-----------------------------------------------------------------

Thanks, overall - I don't know why this patch works but the previous one didn't. Functionally they are (or should be) exactly the same on your touchpad.

The (1 << 28) is a definite bug that needs to be fixed, please submit a separate patch for that.

::: src/evdev-mt-touchpad.c
@@ +106,4 @@
>  	 * is reset whenever a new finger is down, so we'd be resetting the
>  	 * speed and failing.
>  	 */
> +	if (t->history.count < HISTORY_COUNT_THRESHOLD) {

fwiw, I haven't used a const here because we only use it in one place. So having the number right here makes it more obvious what the value is. There's no strict rule for this, there is some gut feeling involved :)

Same goes for the NON_MOTION_EVENT_COUNT

@@ +435,4 @@
>  
>  	if (t->history.count <= 1)
>  		return zero;
> +		

this and a whole bunch of others look like detritus and erroneous whitespace being introduced. I recommend looking into git add -p, this saves you from adding spurious hunks.

@@ +1411,4 @@
>  	   reset that touch to non-dirty effectively swallowing that event
>  	   and restarting with the next event again.
>  	 */
> +	if (tp->device->model_flags & EVDEV_MODEL_LENOVO_P50_TOUCHPAD) {

this would break all the touchpads that currently rely on this setting. But I don't quite get why this is even needed, the flag name itself just doesn't matter.

::: src/evdev.h
@@ -126,4 @@
>  	EVDEV_MODEL_LOGITECH_MARBLE_MOUSE = (1 << 26),
>  	EVDEV_MODEL_TABLET_NO_PROXIMITY_OUT = (1 << 27),
>  	EVDEV_MODEL_MS_NANO_TRANSCEIVER = (1 << 28),
> -	EVDEV_MODEL_TABLET_NO_TILT = (1 << 28),

oh crap, that's a real bug (both flags having 1<<28). Can you split this into a separate commit please and I'll get it merged.

::: udev/90-libinput-model-quirks.hwdb
@@ +220,5 @@
> + 
> +# Lenovo P50
> +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:pvrThinkPadP50*
> + LIBINPUT_MODEL_LENOVO_P50_TOUCHPAD=1
> + LIBINPUT_ATTR_PALM_PRESSURE_THRESHOLD=150

I really don't understand why this could be needed. The udev properties you set here are the same as the ones with the previous patch (compare udevadm info /sys/class/input/eventX output to be sure). Even the palm threshold is the same, so all that happens here is a renaming of flags but the functional effect should be the same before and after and this patch is effectively a noop (for your touchpad, it breaks all the ones below :)

::: .gitignore
@@ +16,5 @@
> +/help/
> +/.cproject
> +/.project
> +/tim_notes.txt
> +.externalToolBuilders

can't merge those, too specific. Please remove from the patch, thanks.
Comment 11 Tim Richardson 2018-04-11 01:20:48 UTC
Created attachment 138745 [details]
email garbage

Thanks, I will take those actions, including learning a bit more about git.
I will also see if I over-reacted by making a separate udev rule for the
P50. Before I split it, the touchpad became unusable after a reboot; I
assumed that other quirks somewhere were being applied which were causing
the trouble (so my hypothesis is not that the udev properties need to be
different, but that somewhere other quirks were being activated because a
bunch of different ThinkPads were grouped together). However, these
problems may have been due to some of my other exploratory changes (since
backed out since none of them helped). You're right: the only active change
is the rejection of non-touch events which you already coded, plus the use
of a dedicated P50 rule (I thought you created that for the P50 bug report
which is why I wasn't concerned about a specific P50 rule excluding other
hardware from the non-touch event rejection).

The touchpad is still a poor experience at slow acceleration settings
(<-0.7) but it is considerably improved at xinput acceleration settings
>0.5 (previously close to unusable, but not any more, it's actually
pleasant to use at last).


On 11 April 2018 at 10:42, <bugzilla-daemon@freedesktop.org> wrote:

> *Comment # 10 <https://bugs.freedesktop.org/show_bug.cgi?id=105963#c10> on
> bug 105963 <https://bugs.freedesktop.org/show_bug.cgi?id=105963> from Peter
> Hutterer <peter.hutterer@who-t.net> *
>
> Comment on attachment 138744 [details] [review] <https://bugs.freedesktop.org/attachment.cgi?id=138744> [details] <https://bugs.freedesktop.org/attachment.cgi?id=138744&action=edit> [review] <https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=105963&attachment=138744>
> patch for P50
>
> Review of attachment 138744 [details] [review] <https://bugs.freedesktop.org/attachment.cgi?id=138744> [details] <https://bugs.freedesktop.org/attachment.cgi?id=138744&action=edit> [review] <https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=105963&attachment=138744>:
> -----------------------------------------------------------------
>
> Thanks, overall - I don't know why this patch works but the previous one
> didn't. Functionally they are (or should be) exactly the same on your touchpad.
>
> The (1 << 28) is a definite bug that needs to be fixed, please submit a
> separate patch for that.
>
> ::: src/evdev-mt-touchpad.c
> @@ +106,4 @@>  	 * is reset whenever a new finger is down, so we'd be resetting the
> >  	 * speed and failing.
> >  	 */
> > +	if (t->history.count < HISTORY_COUNT_THRESHOLD) {
>
> fwiw, I haven't used a const here because we only use it in one place. So
> having the number right here makes it more obvious what the value is. There's
> no strict rule for this, there is some gut feeling involved :)
>
> Same goes for the NON_MOTION_EVENT_COUNT
>
> @@ +435,4 @@>
> >  	if (t->history.count <= 1)
> >  		return zero;
> > +		
>
> this and a whole bunch of others look like detritus and erroneous whitespace
> being introduced. I recommend looking into git add -p, this saves you from
> adding spurious hunks.
>
> @@ +1411,4 @@>  	   reset that touch to non-dirty effectively swallowing that event
> >  	   and restarting with the next event again.
> >  	 */
> > +	if (tp->device->model_flags & EVDEV_MODEL_LENOVO_P50_TOUCHPAD) {
>
> this would break all the touchpads that currently rely on this setting. But I
> don't quite get why this is even needed, the flag name itself just doesn't
> matter.
>
> ::: src/evdev.h
> @@ -126,4 @@>  	EVDEV_MODEL_LOGITECH_MARBLE_MOUSE = (1 << 26),
> >  	EVDEV_MODEL_TABLET_NO_PROXIMITY_OUT = (1 << 27),
> >  	EVDEV_MODEL_MS_NANO_TRANSCEIVER = (1 << 28),
> > -	EVDEV_MODEL_TABLET_NO_TILT = (1 << 28),
>
> oh crap, that's a real bug (both flags having 1<<28). Can you split this into a
> separate commit please and I'll get it merged.
>
> ::: udev/90-libinput-model-quirks.hwdb
> @@ +220,5 @@> +
> > +# Lenovo P50
> > +libinput:name:SynPS/2 Synaptics TouchPad:dmi:*svnLENOVO:*:pvrThinkPadP50*
> > + LIBINPUT_MODEL_LENOVO_P50_TOUCHPAD=1
> > + LIBINPUT_ATTR_PALM_PRESSURE_THRESHOLD=150
>
> I really don't understand why this could be needed. The udev properties you set
> here are the same as the ones with the previous patch (compare udevadm info
> /sys/class/input/eventX output to be sure). Even the palm threshold is the
> same, so all that happens here is a renaming of flags but the functional effect
> should be the same before and after and this patch is effectively a noop (for
> your touchpad, it breaks all the ones below :)
>
> ::: .gitignore
> @@ +16,5 @@> +/help/
> > +/.cproject
> > +/.project
> > +/tim_notes.txt
> > +.externalToolBuilders
>
> can't merge those, too specific. Please remove from the patch, thanks.
>
> ------------------------------
> You are receiving this mail because:
>
>    - You reported the bug.
>
>
Comment 12 Peter Hutterer 2018-04-11 01:57:44 UTC
*please* trim your emails. Have a look at the URL of this bug report to see what the bug has become...
Comment 13 Peter Hutterer 2018-04-11 02:14:26 UTC
[This was typed in response to comment #8, but bugzilla didn't take it first time round]

Also look into bug 101139, that is the current bug for the "way too fast" behaviour.

As for debugging this in general: touchpad_accel_profile_linear() is the acceleration profile and the returned factor is applied to the deltas. The threshold and incline are affected by the configured speed, see touchpad_accelerator_set_speed() which is called when you change the property.

Note that if you're using libinput debug-events or any other tools, the context is *not* the one your X server has, it's always a new context and changes don't go across. Think of it as two different processes reading the same byte stream, you can change how you process data in one process but the other one still receives the unmodified byte stream and is thus unaffected. So any changes in X don't affect libinput debug-events and vice versa.

libinput debug-gui has a hidden feature, cursor keys up/down change the pointer acceleration by ±0.1 with each press. That's going to be helpful for testing because you see the immediate effects on the cursor.

Also note that in calculate_acceleration() we're using simpson's rule to smooth things a bit. So that may affect things a bit as well.

> I think sharing a flag with other quirks causes problems

It shouldn't, the flags are just that and libinput doesn't care about them beyond the usual handling. That particular T450 flag is only used in one place, so re-using it is safe.
Comment 14 Tim Richardson 2018-04-11 09:43:55 UTC
Created attachment 138751 [details] [review]
fix duplicate flag value is evdev.h

fix duplicate flag value is evdev.h
Comment 15 Tim Richardson 2018-04-11 11:35:56 UTC
Created attachment 138755 [details] [review]
Patch to reapply the p50 rule & fix syntax error in existing rules
Comment 16 Tim Richardson 2018-04-11 11:49:43 UTC
"Note that if you're using libinput debug-events or any other tools, the context is *not* the one your X server has, it's always a new context and changes don't go across. "

ah, that explains a lot, I was ready to sign up to a flat earth.
Comment 17 Tim Richardson 2018-04-11 12:52:31 UTC
in touchpad_accel_profile_linear the value of
const double threshold = accel_filter->threshold
is crucial.

For me, it is defaulting to 254
This speed is hardly reached; it represents very rapid motion on the P50. 

the threshold code is basically not of any practical use because the condition 

 if (speed_in < threshold) {
		factor = 1;

rarely happens. 

I've attached an experimental idea which is working quite well on my P50.
the threshold now depends on filter->speed_adjustment

and the accel function is simplified
Comment 18 Tim Richardson 2018-04-11 13:00:39 UTC
Created attachment 138757 [details] [review]
experimental changes to touchpad_accel_profile

this is full of constants which probably only make sense on the P50, but it makes a nice difference, plus it seems a bit simpler.
Comment 19 Tim Richardson 2018-04-11 13:04:59 UTC
(In reply to Tim Richardson from comment #17)

> 
>  if (speed_in < threshold) {
> 		factor = 1;
> 
> rarely happens. 
> 
Sorry, tired. I mean the condition is nearly always true.
Comment 20 Peter Hutterer 2018-04-11 22:56:49 UTC
Comment on attachment 138755 [details] [review]
Patch to reapply the p50 rule & fix syntax error in existing rules

Review of attachment 138755 [details] [review]:
-----------------------------------------------------------------

As a general rule, please always separate logical steps within patches. Adding a new device should not be combined with fixing another device - makes it hard to separate the patches out for e.g. stable branches or regression testing. So this patch would have to be two patches, one for the P50, one for the alleged syntax error.

Alas, that 'syntax error' is intentional, these Lenovo series have an optional suffix as well (T450, T450s, T450p, ...). They all have the same hardware, so this rule catches all. It should arguably be a ? instead as there is only one letter but the current rule works. The trailing : doesn't matter because all modalias have a trailing ':'. So it doesn't quite match as you'd expect but it does match :)

As for the P50 patch - this is the same as in bug 105022 now, right? That one apparently made scrolling unworkable so I had to revert it upstream. Does this patch have any effect on scrolling for you?
Comment 21 Tim Richardson 2018-04-12 00:01:23 UTC
Hi Peter, I confirm that I have no adverse side effects from the P50 rule triggering the ignore non-touch events logic with the code I am using. Locally I have gone back to exactly your udev rules including your p50 rule.

You will probably conclude to ignore my patches and re-apply the one you reverted, so assuming that, I won't resubmit patches for this. That is, I assume this is less work for you (and me). Thanks for your patience, I feel that my future contributions will be much better thanks to your help. This is the first C I have looked at in years. The barrier to entry is really low, you have done awesome job making someone like me a contributor, even if not a very good one just yet.


I have continued to do work on better P50 touchpad, see experimental patch. It is dramatically better, building on the non-touch event rejection. 
One reason is the threshold for the application of part 3 of the acceleration profile is about 250 mm/s movement, which is way too fast ... this is probably a nominal figure, and it presumably makes sense of a lot of hardware I will never see, yet is it terribly wrong for the P50. My changes go beyond this, I have simplified the algorithm.

I'm really happy to have a much better laptop thanks to the past few days.
However, the big question for me is how do I submit hardware-specific changes? There is a a separate acceleration profile and default values for the ThinkPad X230 ... should I replicate this? It seems to me based on this experience that we should actually expect quirks to include idiosyncratic magic values and acceleration algorithms, but maybe the P50 is just an outlier.
Comment 22 Peter Hutterer 2018-04-12 04:28:48 UTC
Comment on attachment 138751 [details] [review]
fix duplicate flag value is evdev.h

Review of attachment 138751 [details] [review]:
-----------------------------------------------------------------

Pushed as dd6059aefc80cd540a133390a9068b4b890494bf, thanks.
Comment 23 Peter Hutterer 2018-04-18 06:18:48 UTC
> You will probably conclude to ignore my patches and re-apply the one you reverted

yep, sorry. Much easier this way :)

for reference: commit 0dccaa8a424022b7a4ed6b7022dd849e8d813225


For the touchpad profile, I think moving to bug 101139 is the best idea here. Which means that this bug can be closed, I think.


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.