On Wed, Jan 20, 2016 at 11:31:42AM +0100, Hans de Goede wrote:
> 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.hutte...@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);
> }
>
good point, I've fixed all these in the v2.
> 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.
>
yeah, unfortunately I don't have enough data to really know if there are
multiple semi-mt touchpads that behave differently and need hwdb exceptions
or if it the issues are the same across all. i suspect the latter, though,
hence the heavy-handed approach.
>
>
> >---
> > 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()
true, did so in v2.
> 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.
I don't think this is correct, we still need to count fake fingers on an ST
touchpad for two-finger scrolling to trigger correctly.
Cheers,
Peter
>
> >
> > 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);
> >
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
