[PATCH libinput 6/6] touchpad: pretend the jumpy semi-mt touchpad is a single-touch touchpad

[Peter Hutterer]

On Thu, Jul 30, 2015 at 04:45:04PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 30-07-15 14:33, Hans de Goede wrote:
> >Hi,
> >
> >On 30-07-15 08:11, Peter Hutterer wrote:
> >>The first finger is accurate, it's just the second finger that is imprecise,
> >>so we can't handle it as a true touch. Instead, revert the device back to
> >>being a single-touch touchpad and use the fake touch bits for second finger
> >>handling.
> >>
> >>Two-finger scrolling thus becomes usable though we will lose out on
> >>other features like thumb detection. Useful scrolling trumps that though.
> >
> >I think we should wait with doing this until the scroll situation is more
> >clear, see my last comment here:
> >
> >https://bugzilla.redhat.com/show_bug.cgi?id=1235175
> 
> Ok so Benjamin responded in the bug report, these touchpads
> report accurate single touch data in the old-style single-touch
> events, and inaccurate data in the mt events when 2 fingers are down.
> 
> So this patch which switches things over to using st coordinates
> is the right thing todo. Review inline.
> 
> 
> >>Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >>---
> >>  src/evdev-mt-touchpad.c | 19 ++++++++++++++-----
> >>  test/touchpad-tap.c     |  3 +++
> >>  test/touchpad.c         |  9 +++------
> >>  3 files changed, 20 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> >>index af1cd47..54ea819 100644
> >>--- a/src/evdev-mt-touchpad.c
> >>+++ b/src/evdev-mt-touchpad.c
> >>@@ -1465,6 +1465,19 @@ tp_init_slots(struct tp_dispatch *tp,
> >>
> >>      tp->semi_mt = libevdev_has_property(device->evdev, INPUT_PROP_SEMI_MT);
> >>
> >>+    /* This device has a terrible 2fg resolution, but only for the
> >>+     * second finger, causing scroll jumps when we use the touch points
> >>+     * properly. The first finger resolution is accurate though, so
> >>+     * we simply pretend it's a single touch touchpad with the BTN_TOOL
> >>+     * bits.
> >>+     */
> 
> This comment is wrong, the old-style st coordinates report accurate
> data where as the mt data reports inaccurate data for both fingers
> when 2 fingers are down. What is happening is that when a single finger
> is down the st data is copied over to the mt data slot 0, so as to
> have something to report to user-space which only listens to mt events,
> and as soon as a second finger comes down the driver starts reporting
> the bounding box limits in the mt data, making *both* slots inaccurate.
> 
> So it is not "only for the second finger" nor is "The first finger
> resolution is accurate" really accurate to say :) The first finger as
> reported in the st events is accurate, the first finger reported in the
> mt events is still no good, otherwise we would not need this commit at
> all.

ok, I've amended the comment to:

+       /* This device has a 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.

I hope that's close enough now, let me know if you want me to amend this
further. Worst case, the bugzilla link has the more detailed comments :)

Cheers,
   Peter

> 
> The rest of the patch looks good and is:
> 
> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
> 
> Regards,
> 
> Hans
> 
> 
> >>+    if (tp->semi_mt &&
> >>+        (device->model_flags & EVDEV_MODEL_JUMPING_SEMI_MT)) {
> >>+        tp->num_slots = 1;
> >>+        tp->slot = 0;
> >>+        tp->has_mt = false;
> >>+    }
> >>+
> >>      ARRAY_FOR_EACH(max_touches, m) {
> >>          if (libevdev_has_event_code(device->evdev,
> >>                          EV_KEY,
> >>@@ -1526,11 +1539,7 @@ tp_scroll_get_methods(struct tp_dispatch *tp)
> >>  {
> >>      uint32_t methods = LIBINPUT_CONFIG_SCROLL_EDGE;
> >>
> >>-    /* some Synaptics semi-mt touchpads have a terrible 2fg resolution,
> >>-     * causing scroll jumps. For all other 2fg touchpads, we enable 2fg
> >>-     * scrolling */
> >>-    if (tp->ntouches >= 2 &&
> >>-        (tp->device->model_flags & EVDEV_MODEL_JUMPING_SEMI_MT) == 0)
> >>+    if (tp->ntouches >= 2)
> >>          methods |= LIBINPUT_CONFIG_SCROLL_2FG;
> >>
> >>      return methods;
> >>diff --git a/test/touchpad-tap.c b/test/touchpad-tap.c
> >>index 00afcdb..62c7a5c 100644
> >>--- a/test/touchpad-tap.c
> >>+++ b/test/touchpad-tap.c
> >>@@ -241,6 +241,9 @@ START_TEST(touchpad_1fg_multitap_n_drag_2fg)
> >>      int range = _i,
> >>          ntaps;
> >>
> >>+    if (litest_is_synaptics_semi_mt(dev))
> >>+        return;
> >>+
> >>      litest_enable_tap(dev->libinput_device);
> >>
> >>      litest_drain_events(li);
> >>diff --git a/test/touchpad.c b/test/touchpad.c
> >>index a6989e7..77c1d2d 100644
> >>--- a/test/touchpad.c
> >>+++ b/test/touchpad.c
> >>@@ -402,14 +402,12 @@ START_TEST(touchpad_scroll_defaults)
> >>
> >>      method = libinput_device_config_scroll_get_methods(device);
> >>      ck_assert(method & LIBINPUT_CONFIG_SCROLL_EDGE);
> >>-    if (libevdev_get_num_slots(evdev) > 1 &&
> >>-        !litest_is_synaptics_semi_mt(dev))
> >>+    if (libevdev_get_num_slots(evdev) > 1)
> >>          ck_assert(method & LIBINPUT_CONFIG_SCROLL_2FG);
> >>      else
> >>          ck_assert((method & LIBINPUT_CONFIG_SCROLL_2FG) == 0);
> >>
> >>-    if (libevdev_get_num_slots(evdev) > 1 &&
> >>-        !litest_is_synaptics_semi_mt(dev))
> >>+    if (libevdev_get_num_slots(evdev) > 1)
> >>          expected = LIBINPUT_CONFIG_SCROLL_2FG;
> >>      else
> >>          expected = LIBINPUT_CONFIG_SCROLL_EDGE;
> >>@@ -425,8 +423,7 @@ START_TEST(touchpad_scroll_defaults)
> >>      status = libinput_device_config_scroll_set_method(device,
> >>                        LIBINPUT_CONFIG_SCROLL_2FG);
> >>
> >>-    if (libevdev_get_num_slots(evdev) > 1 &&
> >>-        !litest_is_synaptics_semi_mt(dev))
> >>+    if (libevdev_get_num_slots(evdev) > 1)
> >>          ck_assert_int_eq(status, LIBINPUT_CONFIG_STATUS_SUCCESS);
> >>      else
> >>          ck_assert_int_eq(status, LIBINPUT_CONFIG_STATUS_UNSUPPORTED);
> >>