[PATCH libinput] touchpad: disable MT for all semi-mt devices

[Hans de Goede]

Hi,

On 19-01-16 01:01, Peter Hutterer wrote:
> Synaptics, Elantech and Alps semi-mt devices all have issues with reporting
> correct MT data, even the bounding box which semi-mt devices are supposed to
> report is wrong.
>
> Synaptics devices have massive jumps with two fingers down. Elantech devices
> may open slots without coordinate data. Alps devices may send 0/0 coordinates
> as initial slot position.
>
> All these may be addressable with specific quirks, but the actual benefit is
> largely restricted to better palm detection (though even with quirks this is
> unlikely to work) and support for pinch gestures (again, lack of coordinates
> makes supporting those hard anyway).
>
> Elantech: https://bugs.freedesktop.org/show_bug.cgi?id=93583
> Alps: https://bugzilla.redhat.com/show_bug.cgi?id=1295073
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>

I've spend a bit thinking about this, and ultimately I think simply completely
disabling gestures on these may indeed be best. If we do this, then
the src/evdev-mt-touchpad-gestures.c can be simplified further though, the
following bits can be removed / simplified then :

tp_gesture_get_direction() :

         /*
          * Semi-mt touchpads have somewhat inaccurate coordinates when
          * 2 fingers are down, so use a slightly larger threshold.
          * Elantech semi-mt touchpads are accurate enough though.
          */
         if (tp->semi_mt &&
             (tp->device->model_flags & EVDEV_MODEL_ELANTECH_TOUCHPAD) == 0)
                 move_threshold = TP_MM_TO_DPI_NORMALIZED(4);
         else
                 move_threshold = TP_MM_TO_DPI_NORMALIZED(1);

I think the entire move_threshold variable can be dropped here and instead
we can simply use TP_MM_TO_DPI_NORMALIZED(1) directly.

tp_gesture_get_pinch_info():

         if (!tp->semi_mt)
                 *angle = atan2(normalized.y, normalized.x) * 180.0 / M_PI;
         else
                 *angle = 0.0;

tp_gesture_twofinger_handle_state_scroll():

         /* On some semi-mt models slot 0 is more accurate, so for semi-mt
          * we only use slot 0. */
         if (tp->semi_mt) {
                 if (!tp->touches[0].dirty)
                         return GESTURE_2FG_STATE_SCROLL;

                 delta = tp_get_delta(&tp->touches[0]);
         } else {
                 delta = tp_get_average_touches_delta(tp);
         }

Still seems a petty to throw away all the work we've put into getting
pinch to work semi-mt devices, otoh I guess that the amount of workarounds
already in the code shows that it really is best to just drop gesture
support on these.



> ---
>   src/evdev-mt-touchpad-gestures.c |  6 ++----
>   src/evdev-mt-touchpad.c          | 21 +++++++++++++--------
>   test/gestures.c                  |  2 +-
>   test/litest.h                    | 10 ----------
>   test/touchpad-tap.c              |  2 +-
>   5 files changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/src/evdev-mt-touchpad-gestures.c b/src/evdev-mt-touchpad-gestures.c
> index 80aa89f..24f6a6d 100644
> --- a/src/evdev-mt-touchpad-gestures.c
> +++ b/src/evdev-mt-touchpad-gestures.c
> @@ -568,10 +568,8 @@ tp_gesture_handle_state(struct tp_dispatch *tp, uint64_t time)
>   int
>   tp_init_gesture(struct tp_dispatch *tp)
>   {
> -	if (tp->device->model_flags & EVDEV_MODEL_JUMPING_SEMI_MT)
> -		tp->gesture.enabled = false;
> -	else
> -		tp->gesture.enabled = true;
> +	/* semi-mt devices are too unreliable to do pinch gestures */
> +	tp->gesture.enabled = !tp->semi_mt;

This does not seem like the right fix, this only disables reporting
of pinch gestures, but we still end up checking if a 2fg gesture
is a scroll or a pinch, always ending up in a scroll anyways since
we only have 1 real touch, so just needlessly delaying the scroll.

Where as if we never want to allow pinch on semi-mt we should simply
jump straight to GESTURE_2FG_STATE_SCROLL if (!tp->has_mt) in
tp_gesture_twofinger_handle_state_none()

Then we can also revert:

http://cgit.freedesktop.org/wayland/libinput/commit/?id=0d40aefe

Making tp_gesture_get_active_touches() look only at real touches
again as intended.

Regards,

Hans


>
>   	tp->gesture.twofinger_state = GESTURE_2FG_STATE_NONE;
>
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index 62087fb..7f5bbf5 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -1490,17 +1490,22 @@ tp_init_slots(struct tp_dispatch *tp,
>
>   	tp->semi_mt = libevdev_has_property(device->evdev, INPUT_PROP_SEMI_MT);
>
> -	/* This device has a terrible resolution when two fingers are down,
> +	/* Semi-mt devices are not reliable for true multitouch data, so we
> +	 * simply pretend they're single touch touchpads with BTN_TOOL bits.
> +	 * Synaptics:
> +	 * Terrible resolution when two fingers are down,
>   	 * causing scroll jumps. The single-touch emulation ABS_X/Y is
>   	 * accurate but the ABS_MT_POSITION touchpoints report the bounding
> -	 * box and that causes jumps.  So we simply pretend it's a single
> -	 * touch touchpad with the BTN_TOOL bits.
> -	 * See https://bugzilla.redhat.com/show_bug.cgi?id=1235175 for an
> -	 * explanation.
> +	 * box and that causes jumps. See https://bugzilla.redhat.com/1235175
> +	 * Elantech:
> +	 * On three-finger taps/clicks, one slot doesn't get a coordinate
> +	 * assigned. See https://bugs.freedesktop.org/show_bug.cgi?id=93583
> +	 * Alps:
> +	 * If three fingers are set down in the same frame, one slot has the
> +	 * coordinates 0/0 and may not get updated for several frames.
> +	 * See https://bugzilla.redhat.com/show_bug.cgi?id=1295073
>   	 */
> -	if (tp->semi_mt &&
> -	    (device->model_flags &
> -	     (EVDEV_MODEL_JUMPING_SEMI_MT|EVDEV_MODEL_ELANTECH_TOUCHPAD))) {
> +	if (tp->semi_mt) {
>   		tp->num_slots = 1;
>   		tp->slot = 0;
>   		tp->has_mt = false;
> diff --git a/test/gestures.c b/test/gestures.c
> index 9fc73b9..0fc3964 100644
> --- a/test/gestures.c
> +++ b/test/gestures.c
> @@ -34,7 +34,7 @@ START_TEST(gestures_cap)
>   	struct litest_device *dev = litest_current_device();
>   	struct libinput_device *device = dev->libinput_device;
>
> -	if (litest_is_synaptics_semi_mt(dev))
> +	if (libevdev_has_property(dev->evdev, INPUT_PROP_SEMI_MT))
>   		ck_assert(!libinput_device_has_capability(device,
>   					  LIBINPUT_DEVICE_CAP_GESTURE));
>   	else
> diff --git a/test/litest.h b/test/litest.h
> index e74e923..61b1b01 100644
> --- a/test/litest.h
> +++ b/test/litest.h
> @@ -552,16 +552,6 @@ litest_enable_buttonareas(struct litest_device *dev)
>   	litest_assert_int_eq(status, expected);
>   }
>
> -static inline int
> -litest_is_synaptics_semi_mt(struct litest_device *dev)
> -{
> -	struct libevdev *evdev = dev->evdev;
> -
> -	return libevdev_has_property(evdev, INPUT_PROP_SEMI_MT) &&
> -		libevdev_get_id_vendor(evdev) == 0x2 &&
> -		libevdev_get_id_product(evdev) == 0x7;
> -}
> -
>   static inline void
>   litest_enable_drag_lock(struct libinput_device *device)
>   {
> diff --git a/test/touchpad-tap.c b/test/touchpad-tap.c
> index 4450ec3..7a7e64c 100644
> --- a/test/touchpad-tap.c
> +++ b/test/touchpad-tap.c
> @@ -241,7 +241,7 @@ START_TEST(touchpad_1fg_multitap_n_drag_2fg)
>   	int range = _i,
>   	    ntaps;
>
> -	if (litest_is_synaptics_semi_mt(dev))
> +	if (libevdev_has_property(dev->evdev, INPUT_PROP_SEMI_MT))
>   		return;
>
>   	litest_enable_tap(dev->libinput_device);
>