* [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT @ 2022-03-31 14:45 Angela Czubak 2022-04-05 11:24 ` Angela Czubak 0 siblings, 1 reply; 9+ messages in thread From: Angela Czubak @ 2022-03-31 14:45 UTC (permalink / raw) To: linux-input; +Cc: upstream, benjamin.tissoires, jikos, Angela Czubak Ilitek touchscreens 016E and 016F repeat last finger position after it has left the surface of the device. We send BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1 with the first repeated slot as this may mean that the finger has been lifted. The touchscreen is not actually too sensitive and this would modify the behaviour of the on-screen keyboard, for instance holding the backspace key might be sometimes interpreted as removing a single character only. Add counter of repeated frames and if it is significantly big then assume the finger actually remains stationary. Signed-off-by: Angela Czubak <acz@semihalf.com> --- Hi guys, Please let me know if you think there is some better solution. It felt to me that it would be more correct to solve it on kernel driver level since this issue is device specific, but perhaps there are some userspace input stacks that figured this problem out. With Chromium OS Input Stack the final BTN_TOUCH=0 event comes significantly late which causes hiccups or simply a scrolling fling not to happen, since the input stack thinks the finger is still in the same position and on the screen. drivers/hid/hid-ids.h | 2 + drivers/hid/hid-multitouch.c | 150 ++++++++++++++++++++++++++++++++++- 2 files changed, 149 insertions(+), 3 deletions(-) diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 053853a891c5..cc85f547603c 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -630,6 +630,8 @@ #define USB_VENDOR_ID_ILITEK 0x222a #define USB_DEVICE_ID_ILITEK_MULTITOUCH 0x0001 +#define USB_DEVICE_ID_ILITEK_016E 0x016e +#define USB_DEVICE_ID_ILITEK_016F 0x016f #define USB_VENDOR_ID_INTEL_0 0x8086 #define USB_VENDOR_ID_INTEL_1 0x8087 diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 99eabfb4145b..33cb5e5179c2 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) #define MT_QUIRK_DISABLE_WAKEUP BIT(21) +#define MT_QUIRK_DROP_REPEATED_SLOT BIT(22) #define MT_INPUTMODE_TOUCHSCREEN 0x02 #define MT_INPUTMODE_TOUCHPAD 0x03 @@ -103,12 +104,22 @@ struct mt_usages { bool *confidence_state; /* is the touch made by a finger? */ }; +struct mt_slot_state { + __s32 x, y, cx, cy, p, w, h, a; + __s32 contactid; /* the device ContactID assigned to this slot */ + bool tip_state; /* is the touch valid? */ + bool inrange_state; /* is the finger in proximity of the sensor? */ + bool confidence_state; /* is the touch made by a finger? */ +}; + struct mt_application { struct list_head list; unsigned int application; unsigned int report_id; struct list_head mt_usages; /* mt usages list */ + struct mt_slot_state *mt_slot_state; + __s32 quirks; __s32 *scantime; /* scantime reported */ @@ -116,6 +127,11 @@ struct mt_application { __s32 *raw_cc; /* contact count in the report */ int left_button_state; /* left button state */ + bool touch_change; /* if touch change detected */ + bool touched; /* if touch was present in the last + * message + */ + int repeated_counter; /* how many times last frame repeated */ unsigned int mt_flags; /* flags to pass to input-mt */ unsigned long *pending_palm_slots; /* slots where we reported palm @@ -211,6 +227,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); #define MT_CLS_GOOGLE 0x0111 #define MT_CLS_RAZER_BLADE_STEALTH 0x0112 #define MT_CLS_SMART_TECH 0x0113 +#define MT_CLS_ILITEK_016x 0x0114 #define MT_DEFAULT_MAXCONTACT 10 #define MT_MAX_MAXCONTACT 250 @@ -386,6 +403,15 @@ static const struct mt_class mt_classes[] = { MT_QUIRK_CONTACT_CNT_ACCURATE | MT_QUIRK_SEPARATE_APP_REPORT, }, + { .name = MT_CLS_ILITEK_016x, + .quirks = MT_QUIRK_ALWAYS_VALID | + MT_QUIRK_IGNORE_DUPLICATES | + MT_QUIRK_HOVERING | + MT_QUIRK_CONTACT_CNT_ACCURATE | + MT_QUIRK_STICKY_FINGERS | + MT_QUIRK_WIN8_PTP_BUTTONS | + MT_QUIRK_DROP_REPEATED_SLOT, + .export_all_inputs = true }, { } }; @@ -783,7 +809,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, case HID_DG_CONFIDENCE: if ((cls->name == MT_CLS_WIN_8 || cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT || - cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP) && + cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP || + cls->name == MT_CLS_ILITEK_016x) && (field->application == HID_DG_TOUCHPAD || field->application == HID_DG_TOUCHSCREEN)) app->quirks |= MT_QUIRK_CONFIDENCE; @@ -948,7 +975,13 @@ static void mt_sync_frame(struct mt_device *td, struct mt_application *app, input_event(input, EV_KEY, BTN_LEFT, app->left_button_state); input_mt_sync_frame(input); - input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { + if (app->touch_change) + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); + app->touch_change = false; + } else { + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); + } input_sync(input); mt_release_pending_palms(td, app, input); @@ -993,6 +1026,60 @@ static int mt_touch_event(struct hid_device *hid, struct hid_field *field, return 1; } +static void mt_clear_slot_states(struct mt_slot_state *mt_slot_state, + int size) +{ + int i; + + for (i = 0; i < size; i++) + mt_slot_state[i].tip_state = 0; +} + +static void mt_fill_slot_state(struct mt_slot_state *mt_slot_state, + struct mt_usages *slot) +{ + mt_slot_state->x = *slot->x; + mt_slot_state->y = *slot->y; + mt_slot_state->cx = *slot->cx; + mt_slot_state->cy = *slot->cy; + mt_slot_state->p = *slot->p; + mt_slot_state->w = *slot->w; + mt_slot_state->h = *slot->h; + mt_slot_state->a = *slot->a; + mt_slot_state->contactid = *slot->contactid; + mt_slot_state->tip_state = *slot->tip_state; + mt_slot_state->inrange_state = *slot->inrange_state; + mt_slot_state->confidence_state = *slot->confidence_state; +} + +static bool mt_is_slot_state_equal(struct mt_slot_state *state1, + struct mt_slot_state *state2) +{ + if (state1->x != state2->x) + return false; + if (state1->y != state2->y) + return false; + if (state1->cx != state2->cx) + return false; + if (state1->cy != state2->cy) + return false; + if (state1->p != state2->p) + return false; + if (state1->w != state2->w) + return false; + if (state1->a != state2->a) + return false; + if (state1->contactid != state2->contactid) + return false; + if (state1->tip_state != state2->tip_state) + return false; + if (state1->inrange_state != state2->inrange_state) + return false; + if (state1->confidence_state != state2->confidence_state) + return false; + return true; +} + static int mt_process_slot(struct mt_device *td, struct input_dev *input, struct mt_application *app, struct mt_usages *slot) @@ -1005,6 +1092,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, int active; int slotnum; int tool = MT_TOOL_FINGER; + struct mt_slot_state state = {0}; if (!slot) return -EINVAL; @@ -1058,13 +1146,42 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, * lift-off as userspace will not be aware * of non-confidence, so we need to split * it into 2 events: active MT_TOOL_PALM - * and a separate liftoff. + * and a separate lift off. */ active = true; set_bit(slotnum, app->pending_palm_slots); } } + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { + mt_fill_slot_state(&state, slot); + /* Check if every field in the slot is the same as before. + * Some touchscreens report the same position for several + * frames even though the finger is no longer on the surface. + * This is actually being recognised as the finger remaining in + * one position, which causes scrolling to be stopped. + * Ignore such packets so that the scrolling continues + * and the touchscreen reports the finger liftoff. + */ + if (!mt_is_slot_state_equal(&state, + &app->mt_slot_state[slotnum])) { + app->touch_change |= (active || + app->mt_slot_state[slotnum].tip_state); + app->repeated_counter = 0; + } else { +#define MT_MAX_REPEATED_DROPPED 5 + if (app->repeated_counter < MT_MAX_REPEATED_DROPPED) { + active = false; + if (app->touched) + app->touch_change = true; + } else { + app->touch_change = true; + } + app->repeated_counter++; + } + app->touched = active; + app->mt_slot_state[slotnum] = state; + } input_mt_slot(input, slotnum); input_mt_report_slot_state(input, tool, active); if (active) { @@ -1296,6 +1413,18 @@ static int mt_touch_input_configured(struct hid_device *hdev, if (!app->pending_palm_slots) return -ENOMEM; + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { + app->mt_slot_state = devm_kcalloc(&hi->input->dev, + td->maxcontacts, + sizeof(*app->mt_slot_state), + GFP_KERNEL); + if (!app->mt_slot_state) + return -ENOMEM; + mt_clear_slot_states(app->mt_slot_state, td->maxcontacts); + app->repeated_counter = 0; + } + + ret = input_mt_init_slots(input, td->maxcontacts, app->mt_flags); if (ret) return ret; @@ -1676,6 +1805,12 @@ static void mt_release_contacts(struct hid_device *hid) list_for_each_entry(application, &td->applications, list) { application->num_received = 0; + if (application->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { + application->touch_change = false; + mt_clear_slot_states(application->mt_slot_state, + td->maxcontacts); + application->repeated_counter = 0; + } } } @@ -2014,6 +2149,15 @@ static const struct hid_device_id mt_devices[] = { MT_USB_DEVICE(USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_MULTITOUCH) }, + /* Ilitek 106E/F touchscreen */ + { .driver_data = MT_CLS_ILITEK_016x, + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016E) }, + + { .driver_data = MT_CLS_ILITEK_016x, + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016F) }, + /* LG Melfas panel */ { .driver_data = MT_CLS_LG, HID_USB_DEVICE(USB_VENDOR_ID_LG, -- 2.35.1.1021.g381101b075-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT 2022-03-31 14:45 [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT Angela Czubak @ 2022-04-05 11:24 ` Angela Czubak 2022-04-05 12:57 ` Benjamin Tissoires 0 siblings, 1 reply; 9+ messages in thread From: Angela Czubak @ 2022-04-05 11:24 UTC (permalink / raw) To: open list:HID CORE LAYER, Benjamin Tissoires, Jiri Kosina; +Cc: upstream Hi Benjamin, Hi Jiri, any thoughts on this patch? If you believe that the userspace input stack should be a better place to solve this kind of issue, please let me know. Regards, Angela On Thu, Mar 31, 2022 at 4:45 PM Angela Czubak <acz@semihalf.com> wrote: > > Ilitek touchscreens 016E and 016F repeat last finger position after it has > left the surface of the device. > We send BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1 with the first repeated > slot as this may mean that the finger has been lifted. > The touchscreen is not actually too sensitive and this would > modify the behaviour of the on-screen keyboard, for instance holding the > backspace key might be sometimes interpreted as removing a single character > only. Add counter of repeated frames and if it is significantly big then > assume the finger actually remains stationary. > > Signed-off-by: Angela Czubak <acz@semihalf.com> > --- > > Hi guys, > > Please let me know if you think there is some better solution. > It felt to me that it would be more correct to solve it on kernel > driver level since this issue is device specific, but perhaps > there are some userspace input stacks that figured this problem out. > With Chromium OS Input Stack the final BTN_TOUCH=0 event comes > significantly late which causes hiccups or simply a scrolling fling > not to happen, since the input stack thinks the finger is still > in the same position and on the screen. > > drivers/hid/hid-ids.h | 2 + > drivers/hid/hid-multitouch.c | 150 ++++++++++++++++++++++++++++++++++- > 2 files changed, 149 insertions(+), 3 deletions(-) > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > index 053853a891c5..cc85f547603c 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -630,6 +630,8 @@ > > #define USB_VENDOR_ID_ILITEK 0x222a > #define USB_DEVICE_ID_ILITEK_MULTITOUCH 0x0001 > +#define USB_DEVICE_ID_ILITEK_016E 0x016e > +#define USB_DEVICE_ID_ILITEK_016F 0x016f > > #define USB_VENDOR_ID_INTEL_0 0x8086 > #define USB_VENDOR_ID_INTEL_1 0x8087 > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > index 99eabfb4145b..33cb5e5179c2 100644 > --- a/drivers/hid/hid-multitouch.c > +++ b/drivers/hid/hid-multitouch.c > @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) > #define MT_QUIRK_DISABLE_WAKEUP BIT(21) > +#define MT_QUIRK_DROP_REPEATED_SLOT BIT(22) > > #define MT_INPUTMODE_TOUCHSCREEN 0x02 > #define MT_INPUTMODE_TOUCHPAD 0x03 > @@ -103,12 +104,22 @@ struct mt_usages { > bool *confidence_state; /* is the touch made by a finger? */ > }; > > +struct mt_slot_state { > + __s32 x, y, cx, cy, p, w, h, a; > + __s32 contactid; /* the device ContactID assigned to this slot */ > + bool tip_state; /* is the touch valid? */ > + bool inrange_state; /* is the finger in proximity of the sensor? */ > + bool confidence_state; /* is the touch made by a finger? */ > +}; > + > struct mt_application { > struct list_head list; > unsigned int application; > unsigned int report_id; > struct list_head mt_usages; /* mt usages list */ > > + struct mt_slot_state *mt_slot_state; > + > __s32 quirks; > > __s32 *scantime; /* scantime reported */ > @@ -116,6 +127,11 @@ struct mt_application { > > __s32 *raw_cc; /* contact count in the report */ > int left_button_state; /* left button state */ > + bool touch_change; /* if touch change detected */ > + bool touched; /* if touch was present in the last > + * message > + */ > + int repeated_counter; /* how many times last frame repeated */ > unsigned int mt_flags; /* flags to pass to input-mt */ > > unsigned long *pending_palm_slots; /* slots where we reported palm > @@ -211,6 +227,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); > #define MT_CLS_GOOGLE 0x0111 > #define MT_CLS_RAZER_BLADE_STEALTH 0x0112 > #define MT_CLS_SMART_TECH 0x0113 > +#define MT_CLS_ILITEK_016x 0x0114 > > #define MT_DEFAULT_MAXCONTACT 10 > #define MT_MAX_MAXCONTACT 250 > @@ -386,6 +403,15 @@ static const struct mt_class mt_classes[] = { > MT_QUIRK_CONTACT_CNT_ACCURATE | > MT_QUIRK_SEPARATE_APP_REPORT, > }, > + { .name = MT_CLS_ILITEK_016x, > + .quirks = MT_QUIRK_ALWAYS_VALID | > + MT_QUIRK_IGNORE_DUPLICATES | > + MT_QUIRK_HOVERING | > + MT_QUIRK_CONTACT_CNT_ACCURATE | > + MT_QUIRK_STICKY_FINGERS | > + MT_QUIRK_WIN8_PTP_BUTTONS | > + MT_QUIRK_DROP_REPEATED_SLOT, > + .export_all_inputs = true }, > { } > }; > > @@ -783,7 +809,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, > case HID_DG_CONFIDENCE: > if ((cls->name == MT_CLS_WIN_8 || > cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT || > - cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP) && > + cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP || > + cls->name == MT_CLS_ILITEK_016x) && > (field->application == HID_DG_TOUCHPAD || > field->application == HID_DG_TOUCHSCREEN)) > app->quirks |= MT_QUIRK_CONFIDENCE; > @@ -948,7 +975,13 @@ static void mt_sync_frame(struct mt_device *td, struct mt_application *app, > input_event(input, EV_KEY, BTN_LEFT, app->left_button_state); > > input_mt_sync_frame(input); > - input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > + if (app->touch_change) > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > + app->touch_change = false; > + } else { > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > + } > input_sync(input); > > mt_release_pending_palms(td, app, input); > @@ -993,6 +1026,60 @@ static int mt_touch_event(struct hid_device *hid, struct hid_field *field, > return 1; > } > > +static void mt_clear_slot_states(struct mt_slot_state *mt_slot_state, > + int size) > +{ > + int i; > + > + for (i = 0; i < size; i++) > + mt_slot_state[i].tip_state = 0; > +} > + > +static void mt_fill_slot_state(struct mt_slot_state *mt_slot_state, > + struct mt_usages *slot) > +{ > + mt_slot_state->x = *slot->x; > + mt_slot_state->y = *slot->y; > + mt_slot_state->cx = *slot->cx; > + mt_slot_state->cy = *slot->cy; > + mt_slot_state->p = *slot->p; > + mt_slot_state->w = *slot->w; > + mt_slot_state->h = *slot->h; > + mt_slot_state->a = *slot->a; > + mt_slot_state->contactid = *slot->contactid; > + mt_slot_state->tip_state = *slot->tip_state; > + mt_slot_state->inrange_state = *slot->inrange_state; > + mt_slot_state->confidence_state = *slot->confidence_state; > +} > + > +static bool mt_is_slot_state_equal(struct mt_slot_state *state1, > + struct mt_slot_state *state2) > +{ > + if (state1->x != state2->x) > + return false; > + if (state1->y != state2->y) > + return false; > + if (state1->cx != state2->cx) > + return false; > + if (state1->cy != state2->cy) > + return false; > + if (state1->p != state2->p) > + return false; > + if (state1->w != state2->w) > + return false; > + if (state1->a != state2->a) > + return false; > + if (state1->contactid != state2->contactid) > + return false; > + if (state1->tip_state != state2->tip_state) > + return false; > + if (state1->inrange_state != state2->inrange_state) > + return false; > + if (state1->confidence_state != state2->confidence_state) > + return false; > + return true; > +} > + > static int mt_process_slot(struct mt_device *td, struct input_dev *input, > struct mt_application *app, > struct mt_usages *slot) > @@ -1005,6 +1092,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > int active; > int slotnum; > int tool = MT_TOOL_FINGER; > + struct mt_slot_state state = {0}; > > if (!slot) > return -EINVAL; > @@ -1058,13 +1146,42 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > * lift-off as userspace will not be aware > * of non-confidence, so we need to split > * it into 2 events: active MT_TOOL_PALM > - * and a separate liftoff. > + * and a separate lift off. > */ > active = true; > set_bit(slotnum, app->pending_palm_slots); > } > } > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > + mt_fill_slot_state(&state, slot); > + /* Check if every field in the slot is the same as before. > + * Some touchscreens report the same position for several > + * frames even though the finger is no longer on the surface. > + * This is actually being recognised as the finger remaining in > + * one position, which causes scrolling to be stopped. > + * Ignore such packets so that the scrolling continues > + * and the touchscreen reports the finger liftoff. > + */ > + if (!mt_is_slot_state_equal(&state, > + &app->mt_slot_state[slotnum])) { > + app->touch_change |= (active || > + app->mt_slot_state[slotnum].tip_state); > + app->repeated_counter = 0; > + } else { > +#define MT_MAX_REPEATED_DROPPED 5 > + if (app->repeated_counter < MT_MAX_REPEATED_DROPPED) { > + active = false; > + if (app->touched) > + app->touch_change = true; > + } else { > + app->touch_change = true; > + } > + app->repeated_counter++; > + } > + app->touched = active; > + app->mt_slot_state[slotnum] = state; > + } > input_mt_slot(input, slotnum); > input_mt_report_slot_state(input, tool, active); > if (active) { > @@ -1296,6 +1413,18 @@ static int mt_touch_input_configured(struct hid_device *hdev, > if (!app->pending_palm_slots) > return -ENOMEM; > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > + app->mt_slot_state = devm_kcalloc(&hi->input->dev, > + td->maxcontacts, > + sizeof(*app->mt_slot_state), > + GFP_KERNEL); > + if (!app->mt_slot_state) > + return -ENOMEM; > + mt_clear_slot_states(app->mt_slot_state, td->maxcontacts); > + app->repeated_counter = 0; > + } > + > + > ret = input_mt_init_slots(input, td->maxcontacts, app->mt_flags); > if (ret) > return ret; > @@ -1676,6 +1805,12 @@ static void mt_release_contacts(struct hid_device *hid) > > list_for_each_entry(application, &td->applications, list) { > application->num_received = 0; > + if (application->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > + application->touch_change = false; > + mt_clear_slot_states(application->mt_slot_state, > + td->maxcontacts); > + application->repeated_counter = 0; > + } > } > } > > @@ -2014,6 +2149,15 @@ static const struct hid_device_id mt_devices[] = { > MT_USB_DEVICE(USB_VENDOR_ID_ILITEK, > USB_DEVICE_ID_ILITEK_MULTITOUCH) }, > > + /* Ilitek 106E/F touchscreen */ > + { .driver_data = MT_CLS_ILITEK_016x, > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016E) }, > + > + { .driver_data = MT_CLS_ILITEK_016x, > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016F) }, > + > /* LG Melfas panel */ > { .driver_data = MT_CLS_LG, > HID_USB_DEVICE(USB_VENDOR_ID_LG, > -- > 2.35.1.1021.g381101b075-goog > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT 2022-04-05 11:24 ` Angela Czubak @ 2022-04-05 12:57 ` Benjamin Tissoires 2022-04-06 12:18 ` Angela Czubak 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Tissoires @ 2022-04-05 12:57 UTC (permalink / raw) To: Angela Czubak; +Cc: open list:HID CORE LAYER, Jiri Kosina, upstream Hi Angela, On Tue, Apr 5, 2022 at 1:25 PM Angela Czubak <acz@semihalf.com> wrote: > > Hi Benjamin, Hi Jiri, > > any thoughts on this patch? Sorry for taking too long to answer. I saw the patch coming in while coming back last week, and could not process it, and completely forgot about it. > If you believe that the userspace input stack should be a better place > to solve this kind of issue, please let me know. This is definitely not userspace to process those specificities, as far as I understand. However... I don't understand the bug very well :/ more inlined below: > > Regards, > Angela > > On Thu, Mar 31, 2022 at 4:45 PM Angela Czubak <acz@semihalf.com> wrote: > > > > Ilitek touchscreens 016E and 016F repeat last finger position after it has > > left the surface of the device. > > We send BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1 with the first repeated > > slot as this may mean that the finger has been lifted. > > The touchscreen is not actually too sensitive and this would > > modify the behaviour of the on-screen keyboard, for instance holding the > > backspace key might be sometimes interpreted as removing a single character > > only. Add counter of repeated frames and if it is significantly big then > > assume the finger actually remains stationary. This commit is really confusing to me. This description is mixing symptoms from before the patch, behaviour from after and userspace which doesn't mean a lot here not knowing what is the problem. What is the exact problem? What events the device is sending? What should be the expected kernel ouput? (bonus point for actually giving the events in the hid-recorder output format [0] ;-P ) FWIW, this driver is one of the few drivers in the HID stack to have extended tests in hid-tools[0]. I plan to merge that repo into the selftests subtree, but for now we need to use this external repo. So I'd be pleased to see new tests added for that quirk because it seems far from evident what is happening. > > > > Signed-off-by: Angela Czubak <acz@semihalf.com> > > --- > > > > Hi guys, > > > > Please let me know if you think there is some better solution. > > It felt to me that it would be more correct to solve it on kernel > > driver level since this issue is device specific, but perhaps > > there are some userspace input stacks that figured this problem out. > > With Chromium OS Input Stack the final BTN_TOUCH=0 event comes > > significantly late which causes hiccups or simply a scrolling fling > > not to happen, since the input stack thinks the finger is still > > in the same position and on the screen. If the device reports a wrong state, the kernel is the place to fix it. However, as mentioned above I do not understand what is wrong there by reading the description and the code. > > > > drivers/hid/hid-ids.h | 2 + > > drivers/hid/hid-multitouch.c | 150 ++++++++++++++++++++++++++++++++++- > > 2 files changed, 149 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > index 053853a891c5..cc85f547603c 100644 > > --- a/drivers/hid/hid-ids.h > > +++ b/drivers/hid/hid-ids.h > > @@ -630,6 +630,8 @@ > > > > #define USB_VENDOR_ID_ILITEK 0x222a > > #define USB_DEVICE_ID_ILITEK_MULTITOUCH 0x0001 > > +#define USB_DEVICE_ID_ILITEK_016E 0x016e > > +#define USB_DEVICE_ID_ILITEK_016F 0x016f > > > > #define USB_VENDOR_ID_INTEL_0 0x8086 > > #define USB_VENDOR_ID_INTEL_1 0x8087 > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > > index 99eabfb4145b..33cb5e5179c2 100644 > > --- a/drivers/hid/hid-multitouch.c > > +++ b/drivers/hid/hid-multitouch.c > > @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); > > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) > > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) > > #define MT_QUIRK_DISABLE_WAKEUP BIT(21) > > +#define MT_QUIRK_DROP_REPEATED_SLOT BIT(22) > > > > #define MT_INPUTMODE_TOUCHSCREEN 0x02 > > #define MT_INPUTMODE_TOUCHPAD 0x03 > > @@ -103,12 +104,22 @@ struct mt_usages { > > bool *confidence_state; /* is the touch made by a finger? */ > > }; > > > > +struct mt_slot_state { > > + __s32 x, y, cx, cy, p, w, h, a; > > + __s32 contactid; /* the device ContactID assigned to this slot */ > > + bool tip_state; /* is the touch valid? */ > > + bool inrange_state; /* is the finger in proximity of the sensor? */ > > + bool confidence_state; /* is the touch made by a finger? */ > > +}; This raises a red flag here. I spent quite some time removing internal slot states, and re-adding them means that something is wrong IMO. (though if we can not do something else, we might just re-add them). > > + > > struct mt_application { > > struct list_head list; > > unsigned int application; > > unsigned int report_id; > > struct list_head mt_usages; /* mt usages list */ > > > > + struct mt_slot_state *mt_slot_state; > > + > > __s32 quirks; > > > > __s32 *scantime; /* scantime reported */ > > @@ -116,6 +127,11 @@ struct mt_application { > > > > __s32 *raw_cc; /* contact count in the report */ > > int left_button_state; /* left button state */ > > + bool touch_change; /* if touch change detected */ > > + bool touched; /* if touch was present in the last > > + * message > > + */ So this is not per-touch, but per report? > > + int repeated_counter; /* how many times last frame repeated */ > > unsigned int mt_flags; /* flags to pass to input-mt */ > > > > unsigned long *pending_palm_slots; /* slots where we reported palm > > @@ -211,6 +227,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); > > #define MT_CLS_GOOGLE 0x0111 > > #define MT_CLS_RAZER_BLADE_STEALTH 0x0112 > > #define MT_CLS_SMART_TECH 0x0113 > > +#define MT_CLS_ILITEK_016x 0x0114 > > > > #define MT_DEFAULT_MAXCONTACT 10 > > #define MT_MAX_MAXCONTACT 250 > > @@ -386,6 +403,15 @@ static const struct mt_class mt_classes[] = { > > MT_QUIRK_CONTACT_CNT_ACCURATE | > > MT_QUIRK_SEPARATE_APP_REPORT, > > }, > > + { .name = MT_CLS_ILITEK_016x, > > + .quirks = MT_QUIRK_ALWAYS_VALID | > > + MT_QUIRK_IGNORE_DUPLICATES | > > + MT_QUIRK_HOVERING | > > + MT_QUIRK_CONTACT_CNT_ACCURATE | > > + MT_QUIRK_STICKY_FINGERS | > > + MT_QUIRK_WIN8_PTP_BUTTONS | > > + MT_QUIRK_DROP_REPEATED_SLOT, > > + .export_all_inputs = true }, > > { } > > }; > > > > @@ -783,7 +809,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, > > case HID_DG_CONFIDENCE: > > if ((cls->name == MT_CLS_WIN_8 || > > cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT || > > - cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP) && > > + cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP || > > + cls->name == MT_CLS_ILITEK_016x) && Not something you should care about, but this long test of spaghetti should likely be fixed into something way better at some point... > > (field->application == HID_DG_TOUCHPAD || > > field->application == HID_DG_TOUCHSCREEN)) > > app->quirks |= MT_QUIRK_CONFIDENCE; > > @@ -948,7 +975,13 @@ static void mt_sync_frame(struct mt_device *td, struct mt_application *app, > > input_event(input, EV_KEY, BTN_LEFT, app->left_button_state); > > > > input_mt_sync_frame(input); > > - input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > + if (app->touch_change) > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > + app->touch_change = false; Why filter out the timestamp? > > + } else { > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > + } > > input_sync(input); > > > > mt_release_pending_palms(td, app, input); > > @@ -993,6 +1026,60 @@ static int mt_touch_event(struct hid_device *hid, struct hid_field *field, > > return 1; > > } > > > > +static void mt_clear_slot_states(struct mt_slot_state *mt_slot_state, > > + int size) > > +{ > > + int i; > > + > > + for (i = 0; i < size; i++) > > + mt_slot_state[i].tip_state = 0; > > +} > > + > > +static void mt_fill_slot_state(struct mt_slot_state *mt_slot_state, > > + struct mt_usages *slot) > > +{ > > + mt_slot_state->x = *slot->x; > > + mt_slot_state->y = *slot->y; > > + mt_slot_state->cx = *slot->cx; > > + mt_slot_state->cy = *slot->cy; > > + mt_slot_state->p = *slot->p; > > + mt_slot_state->w = *slot->w; > > + mt_slot_state->h = *slot->h; > > + mt_slot_state->a = *slot->a; > > + mt_slot_state->contactid = *slot->contactid; > > + mt_slot_state->tip_state = *slot->tip_state; > > + mt_slot_state->inrange_state = *slot->inrange_state; > > + mt_slot_state->confidence_state = *slot->confidence_state; > > +} > > + > > +static bool mt_is_slot_state_equal(struct mt_slot_state *state1, > > + struct mt_slot_state *state2) > > +{ > > + if (state1->x != state2->x) > > + return false; > > + if (state1->y != state2->y) > > + return false; > > + if (state1->cx != state2->cx) > > + return false; > > + if (state1->cy != state2->cy) > > + return false; > > + if (state1->p != state2->p) > > + return false; > > + if (state1->w != state2->w) > > + return false; > > + if (state1->a != state2->a) > > + return false; > > + if (state1->contactid != state2->contactid) > > + return false; > > + if (state1->tip_state != state2->tip_state) > > + return false; > > + if (state1->inrange_state != state2->inrange_state) > > + return false; > > + if (state1->confidence_state != state2->confidence_state) > > + return false; > > + return true; > > +} The input stack already filters out duplicated events. So I am not sure we need to store the information here once again and test for it here. > > + > > static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > struct mt_application *app, > > struct mt_usages *slot) > > @@ -1005,6 +1092,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > int active; > > int slotnum; > > int tool = MT_TOOL_FINGER; > > + struct mt_slot_state state = {0}; > > > > if (!slot) > > return -EINVAL; > > @@ -1058,13 +1146,42 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > * lift-off as userspace will not be aware > > * of non-confidence, so we need to split > > * it into 2 events: active MT_TOOL_PALM > > - * and a separate liftoff. > > + * and a separate lift off. > > */ > > active = true; > > set_bit(slotnum, app->pending_palm_slots); > > } > > } > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > + mt_fill_slot_state(&state, slot); > > + /* Check if every field in the slot is the same as before. > > + * Some touchscreens report the same position for several > > + * frames even though the finger is no longer on the surface. > > + * This is actually being recognised as the finger remaining in > > + * one position, which causes scrolling to be stopped. > > + * Ignore such packets so that the scrolling continues > > + * and the touchscreen reports the finger liftoff. That part I understand better, but how is the device reporting such events? If the touch is no longer there, we should have a bit that says it is lifted, so we should simply just ignore the touch, no? > > + */ > > + if (!mt_is_slot_state_equal(&state, > > + &app->mt_slot_state[slotnum])) { > > + app->touch_change |= (active || > > + app->mt_slot_state[slotnum].tip_state); > > + app->repeated_counter = 0; > > + } else { > > +#define MT_MAX_REPEATED_DROPPED 5 > > + if (app->repeated_counter < MT_MAX_REPEATED_DROPPED) { > > + active = false; > > + if (app->touched) > > + app->touch_change = true; > > + } else { > > + app->touch_change = true; > > + } > > + app->repeated_counter++; This is specific to one device, but I have seen device filtering in hardware that reports the same steady data as long as the touch is present. So 5 reports with the same data seem quite short. Also, we do have devices which tend to forget to release slots, and that's why we have MT_QUIRK_STICKY_FINGERS which release touches after a while. > > + } > > + app->touched = active; > > + app->mt_slot_state[slotnum] = state; > > + } > > input_mt_slot(input, slotnum); > > input_mt_report_slot_state(input, tool, active); > > if (active) { > > @@ -1296,6 +1413,18 @@ static int mt_touch_input_configured(struct hid_device *hdev, > > if (!app->pending_palm_slots) > > return -ENOMEM; > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > + app->mt_slot_state = devm_kcalloc(&hi->input->dev, > > + td->maxcontacts, > > + sizeof(*app->mt_slot_state), > > + GFP_KERNEL); > > + if (!app->mt_slot_state) > > + return -ENOMEM; > > + mt_clear_slot_states(app->mt_slot_state, td->maxcontacts); > > + app->repeated_counter = 0; > > + } > > + > > + > > ret = input_mt_init_slots(input, td->maxcontacts, app->mt_flags); > > if (ret) > > return ret; > > @@ -1676,6 +1805,12 @@ static void mt_release_contacts(struct hid_device *hid) > > > > list_for_each_entry(application, &td->applications, list) { > > application->num_received = 0; > > + if (application->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > + application->touch_change = false; > > + mt_clear_slot_states(application->mt_slot_state, > > + td->maxcontacts); > > + application->repeated_counter = 0; > > + } > > } > > } > > > > @@ -2014,6 +2149,15 @@ static const struct hid_device_id mt_devices[] = { > > MT_USB_DEVICE(USB_VENDOR_ID_ILITEK, > > USB_DEVICE_ID_ILITEK_MULTITOUCH) }, > > > > + /* Ilitek 106E/F touchscreen */ > > + { .driver_data = MT_CLS_ILITEK_016x, > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016E) }, > > + > > + { .driver_data = MT_CLS_ILITEK_016x, > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016F) }, > > + > > /* LG Melfas panel */ > > { .driver_data = MT_CLS_LG, > > HID_USB_DEVICE(USB_VENDOR_ID_LG, > > -- > > 2.35.1.1021.g381101b075-goog > > > Cheers, Benjamin [0] https://gitlab.freedesktop.org/libevdev/hid-tools ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT 2022-04-05 12:57 ` Benjamin Tissoires @ 2022-04-06 12:18 ` Angela Czubak 2022-04-06 13:35 ` Benjamin Tissoires 0 siblings, 1 reply; 9+ messages in thread From: Angela Czubak @ 2022-04-06 12:18 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: open list:HID CORE LAYER, Jiri Kosina, upstream . On Tue, Apr 5, 2022 at 2:57 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi Angela, > > On Tue, Apr 5, 2022 at 1:25 PM Angela Czubak <acz@semihalf.com> wrote: > > > > Hi Benjamin, Hi Jiri, > > > > any thoughts on this patch? > > Sorry for taking too long to answer. I saw the patch coming in while > coming back last week, and could not process it, and completely forgot > about it. > > > If you believe that the userspace input stack should be a better place > > to solve this kind of issue, please let me know. > > This is definitely not userspace to process those specificities, as > far as I understand. However... I don't understand the bug very well > :/ > > more inlined below: > > > > > Regards, > > Angela > > > > On Thu, Mar 31, 2022 at 4:45 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > Ilitek touchscreens 016E and 016F repeat last finger position after it has > > > left the surface of the device. > > > We send BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1 with the first repeated > > > slot as this may mean that the finger has been lifted. > > > The touchscreen is not actually too sensitive and this would > > > modify the behaviour of the on-screen keyboard, for instance holding the > > > backspace key might be sometimes interpreted as removing a single character > > > only. Add counter of repeated frames and if it is significantly big then > > > assume the finger actually remains stationary. > > This commit is really confusing to me. This description is mixing > symptoms from before the patch, behaviour from after and userspace > which doesn't mean a lot here not knowing what is the problem. > > What is the exact problem? The device delays reporting finger lift off, instead it repeats its last report with only the scantime updated. I would interpret such events as meaning that the finger is present but has stopped moving, which is not the case, actually. When it comes to ChromeOS userspace input stack it seems that those events are actually ignored, but the problem is that the final event, i.e. the finger lit off is reported significantly late, so if the user is moving their finger to scroll the contents of the screen it immediately stops once the finger leaves the surface. However, the desired behaviour is for the scroll to continue, i.e. they should observe something called fling. The problem is that since this last event is delayed the scrolling stops. Because the last event is so late, our window, over which we calculate scroll speed, consists of just one single event, i.e. we cannot calculate any movement velocity so we assume it is zero. I tried to increase the window, but it appears that the delay is large enough for a human eye to notice that the scrolling stops (as there are no new events for a while) and then it restarts again (once the actual lift off event is noticed, BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1). I find it more difficult to solve at userspace level and we would like the fling (i.e. scrolling that continues after the finger has left the surface) to be fluent. > What events the device is sending? "Raw" events: https://gist.github.com/semihalf-czubak-angela/026072c013b4f883cb6adb7460b4d6ca So as you can see it repeats the last position etc. and the only thing changing is the scantime. This translates to input events (not that the data below is from another run, but you get the idea): https://gist.github.com/semihalf-czubak-angela/b957f7e464772bbdd95ddd814e84e5d9 > What should be the expected kernel ouput? > Well, ideally: https://gist.github.com/semihalf-czubak-angela/4dd52c177372f8f0c2b4e5ada841ea95 So what helps is if Event: time 1643827301.463888, type 3 (EV_ABS), code 53 (ABS_MT_POSITION_X), value 4817 Event: time 1643827301.463888, type 3 (EV_ABS), code 54 (ABS_MT_POSITION_Y), value 777 Event: time 1643827301.463888, type 3 (EV_ABS), code 0 (ABS_X), value 4817 Event: time 1643827301.463888, type 3 (EV_ABS), code 1 (ABS_Y), value 777 Event: time 1643827301.463888, type 4 (EV_MSC), code 5 (MSC_TIMESTAMP), value 408000 Event: time 1643827301.463888, -------------- SYN_REPORT ------------ Event: time 1643827301.479920, type 4 (EV_MSC), code 5 (MSC_TIMESTAMP), value 416000 Event: time 1643827301.479920, -------------- SYN_REPORT ------------ Event: time 1643827301.496921, type 4 (EV_MSC), code 5 (MSC_TIMESTAMP), value 424000 Event: time 1643827301.496921, -------------- SYN_REPORT ------------ Event: time 1643827301.517923, type 4 (EV_MSC), code 5 (MSC_TIMESTAMP), value 432000 Event: time 1643827301.517923, -------------- SYN_REPORT ------------ Event: time 1643827301.535920, type 4 (EV_MSC), code 5 (MSC_TIMESTAMP), value 440000 Event: time 1643827301.535920, -------------- SYN_REPORT ------------ Event: time 1643827301.554943, type 3 (EV_ABS), code 57 (ABS_MT_TRACKING_ID), value -1 Event: time 1643827301.554943, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0 Event: time 1643827301.554943, type 4 (EV_MSC), code 5 (MSC_TIMESTAMP), value 448000 Event: time 1643827301.554943, -------------- SYN_REPORT ------------ becomes Event: time 1643827301.463888, type 3 (EV_ABS), code 53 (ABS_MT_POSITION_X), value 4817 Event: time 1643827301.463888, type 3 (EV_ABS), code 54 (ABS_MT_POSITION_Y), value 777 Event: time 1643827301.463888, type 3 (EV_ABS), code 0 (ABS_X), value 4817 Event: time 1643827301.463888, type 3 (EV_ABS), code 1 (ABS_Y), value 777 Event: time 1643827301.463888, type 4 (EV_MSC), code 5 (MSC_TIMESTAMP), value 408000 Event: time 1643827301.463888, -------------- SYN_REPORT ------------ Event: time 1643827301.479920, type 3 (EV_ABS), code 57 (ABS_MT_TRACKING_ID), value -1 Event: time 1643827301.479920, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0 Event: time 1643827301.479920, type 4 (EV_MSC), code 5 (MSC_TIMESTAMP), value 416000 Event: time 1643827301.479920, -------------- SYN_REPORT ------------ > (bonus point for actually giving the events in the hid-recorder output > format [0] ;-P ) > Here you are: https://gist.github.com/semihalf-czubak-angela/eaaf73459c5c50f716016839fc368ab0 > FWIW, this driver is one of the few drivers in the HID stack to have > extended tests in hid-tools[0]. > I plan to merge that repo into the selftests subtree, but for now we > need to use this external repo. > > So I'd be pleased to see new tests added for that quirk because it > seems far from evident what is happening. > Ack, I will prepare something. > > > > > > Signed-off-by: Angela Czubak <acz@semihalf.com> > > > --- > > > > > > Hi guys, > > > > > > Please let me know if you think there is some better solution. > > > It felt to me that it would be more correct to solve it on kernel > > > driver level since this issue is device specific, but perhaps > > > there are some userspace input stacks that figured this problem out. > > > With Chromium OS Input Stack the final BTN_TOUCH=0 event comes > > > significantly late which causes hiccups or simply a scrolling fling > > > not to happen, since the input stack thinks the finger is still > > > in the same position and on the screen. > > If the device reports a wrong state, the kernel is the place to fix > it. However, as mentioned above I do not understand what is wrong > there by reading the description and the code. > > > > > > > drivers/hid/hid-ids.h | 2 + > > > drivers/hid/hid-multitouch.c | 150 ++++++++++++++++++++++++++++++++++- > > > 2 files changed, 149 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > > index 053853a891c5..cc85f547603c 100644 > > > --- a/drivers/hid/hid-ids.h > > > +++ b/drivers/hid/hid-ids.h > > > @@ -630,6 +630,8 @@ > > > > > > #define USB_VENDOR_ID_ILITEK 0x222a > > > #define USB_DEVICE_ID_ILITEK_MULTITOUCH 0x0001 > > > +#define USB_DEVICE_ID_ILITEK_016E 0x016e > > > +#define USB_DEVICE_ID_ILITEK_016F 0x016f > > > > > > #define USB_VENDOR_ID_INTEL_0 0x8086 > > > #define USB_VENDOR_ID_INTEL_1 0x8087 > > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > > > index 99eabfb4145b..33cb5e5179c2 100644 > > > --- a/drivers/hid/hid-multitouch.c > > > +++ b/drivers/hid/hid-multitouch.c > > > @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); > > > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) > > > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) > > > #define MT_QUIRK_DISABLE_WAKEUP BIT(21) > > > +#define MT_QUIRK_DROP_REPEATED_SLOT BIT(22) > > > > > > #define MT_INPUTMODE_TOUCHSCREEN 0x02 > > > #define MT_INPUTMODE_TOUCHPAD 0x03 > > > @@ -103,12 +104,22 @@ struct mt_usages { > > > bool *confidence_state; /* is the touch made by a finger? */ > > > }; > > > > > > +struct mt_slot_state { > > > + __s32 x, y, cx, cy, p, w, h, a; > > > + __s32 contactid; /* the device ContactID assigned to this slot */ > > > + bool tip_state; /* is the touch valid? */ > > > + bool inrange_state; /* is the finger in proximity of the sensor? */ > > > + bool confidence_state; /* is the touch made by a finger? */ > > > +}; > > This raises a red flag here. I spent quite some time removing internal > slot states, and re-adding them means that something is wrong IMO. > (though if we can not do something else, we might just re-add them). > I wanted to compare against the report contents excluding the scantime. Perhaps I could do that on the hid report level, this way just seemed easier to implement. > > > + > > > struct mt_application { > > > struct list_head list; > > > unsigned int application; > > > unsigned int report_id; > > > struct list_head mt_usages; /* mt usages list */ > > > > > > + struct mt_slot_state *mt_slot_state; > > > + > > > __s32 quirks; > > > > > > __s32 *scantime; /* scantime reported */ > > > @@ -116,6 +127,11 @@ struct mt_application { > > > > > > __s32 *raw_cc; /* contact count in the report */ > > > int left_button_state; /* left button state */ > > > + bool touch_change; /* if touch change detected */ > > > + bool touched; /* if touch was present in the last > > > + * message > > > + */ > > So this is not per-touch, but per report? > I suppose it is actually per report, it is just that implementation per slot seemed easier. I could not reproduce the issue when using two fingers. > > > + int repeated_counter; /* how many times last frame repeated */ > > > unsigned int mt_flags; /* flags to pass to input-mt */ > > > > > > unsigned long *pending_palm_slots; /* slots where we reported palm > > > @@ -211,6 +227,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); > > > #define MT_CLS_GOOGLE 0x0111 > > > #define MT_CLS_RAZER_BLADE_STEALTH 0x0112 > > > #define MT_CLS_SMART_TECH 0x0113 > > > +#define MT_CLS_ILITEK_016x 0x0114 > > > > > > #define MT_DEFAULT_MAXCONTACT 10 > > > #define MT_MAX_MAXCONTACT 250 > > > @@ -386,6 +403,15 @@ static const struct mt_class mt_classes[] = { > > > MT_QUIRK_CONTACT_CNT_ACCURATE | > > > MT_QUIRK_SEPARATE_APP_REPORT, > > > }, > > > + { .name = MT_CLS_ILITEK_016x, > > > + .quirks = MT_QUIRK_ALWAYS_VALID | > > > + MT_QUIRK_IGNORE_DUPLICATES | > > > + MT_QUIRK_HOVERING | > > > + MT_QUIRK_CONTACT_CNT_ACCURATE | > > > + MT_QUIRK_STICKY_FINGERS | > > > + MT_QUIRK_WIN8_PTP_BUTTONS | > > > + MT_QUIRK_DROP_REPEATED_SLOT, > > > + .export_all_inputs = true }, > > > { } > > > }; > > > > > > @@ -783,7 +809,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, > > > case HID_DG_CONFIDENCE: > > > if ((cls->name == MT_CLS_WIN_8 || > > > cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT || > > > - cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP) && > > > + cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP || > > > + cls->name == MT_CLS_ILITEK_016x) && > > Not something you should care about, but this long test of spaghetti > should likely be fixed into something way better at some point... > > > > (field->application == HID_DG_TOUCHPAD || > > > field->application == HID_DG_TOUCHSCREEN)) > > > app->quirks |= MT_QUIRK_CONFIDENCE; > > > @@ -948,7 +975,13 @@ static void mt_sync_frame(struct mt_device *td, struct mt_application *app, > > > input_event(input, EV_KEY, BTN_LEFT, app->left_button_state); > > > > > > input_mt_sync_frame(input); > > > - input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > + if (app->touch_change) > > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > + app->touch_change = false; > > Why filter out the timestamp? > I suppose lonely timestamps should not do any harm, I just wanted to avoid evtest etc. output where there is no change but the timestamp, it is kind of also remainder of my first approach where I just tried filtering this events out and see waiting for BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1 would be enough (it wasn't). > > > + } else { > > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > + } > > > input_sync(input); > > > > > > mt_release_pending_palms(td, app, input); > > > @@ -993,6 +1026,60 @@ static int mt_touch_event(struct hid_device *hid, struct hid_field *field, > > > return 1; > > > } > > > > > > +static void mt_clear_slot_states(struct mt_slot_state *mt_slot_state, > > > + int size) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < size; i++) > > > + mt_slot_state[i].tip_state = 0; > > > +} > > > + > > > +static void mt_fill_slot_state(struct mt_slot_state *mt_slot_state, > > > + struct mt_usages *slot) > > > +{ > > > + mt_slot_state->x = *slot->x; > > > + mt_slot_state->y = *slot->y; > > > + mt_slot_state->cx = *slot->cx; > > > + mt_slot_state->cy = *slot->cy; > > > + mt_slot_state->p = *slot->p; > > > + mt_slot_state->w = *slot->w; > > > + mt_slot_state->h = *slot->h; > > > + mt_slot_state->a = *slot->a; > > > + mt_slot_state->contactid = *slot->contactid; > > > + mt_slot_state->tip_state = *slot->tip_state; > > > + mt_slot_state->inrange_state = *slot->inrange_state; > > > + mt_slot_state->confidence_state = *slot->confidence_state; > > > +} > > > + > > > +static bool mt_is_slot_state_equal(struct mt_slot_state *state1, > > > + struct mt_slot_state *state2) > > > +{ > > > + if (state1->x != state2->x) > > > + return false; > > > + if (state1->y != state2->y) > > > + return false; > > > + if (state1->cx != state2->cx) > > > + return false; > > > + if (state1->cy != state2->cy) > > > + return false; > > > + if (state1->p != state2->p) > > > + return false; > > > + if (state1->w != state2->w) > > > + return false; > > > + if (state1->a != state2->a) > > > + return false; > > > + if (state1->contactid != state2->contactid) > > > + return false; > > > + if (state1->tip_state != state2->tip_state) > > > + return false; > > > + if (state1->inrange_state != state2->inrange_state) > > > + return false; > > > + if (state1->confidence_state != state2->confidence_state) > > > + return false; > > > + return true; > > > +} > > The input stack already filters out duplicated events. So I am not > sure we need to store the information here once again and test for it > here. > What I want to do is to "prematurely" report that the finger has been lifted. It is not that much about solely filtering out the duplicated events, I rather want to interpret such an event as a finger lift off. > > > + > > > static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > struct mt_application *app, > > > struct mt_usages *slot) > > > @@ -1005,6 +1092,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > int active; > > > int slotnum; > > > int tool = MT_TOOL_FINGER; > > > + struct mt_slot_state state = {0}; > > > > > > if (!slot) > > > return -EINVAL; > > > @@ -1058,13 +1146,42 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > * lift-off as userspace will not be aware > > > * of non-confidence, so we need to split > > > * it into 2 events: active MT_TOOL_PALM > > > - * and a separate liftoff. > > > + * and a separate lift off. > > > */ > > > active = true; > > > set_bit(slotnum, app->pending_palm_slots); > > > } > > > } > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > + mt_fill_slot_state(&state, slot); > > > + /* Check if every field in the slot is the same as before. > > > + * Some touchscreens report the same position for several > > > + * frames even though the finger is no longer on the surface. > > > + * This is actually being recognised as the finger remaining in > > > + * one position, which causes scrolling to be stopped. > > > + * Ignore such packets so that the scrolling continues > > > + * and the touchscreen reports the finger liftoff. > > That part I understand better, but how is the device reporting such events? > If the touch is no longer there, we should have a bit that says it is > lifted, so we should simply just ignore the touch, no? > Well, I would actually like the touch panel manufacturers to answer that :-) Based on some comments in our source code it seems that some devices might delay synthesis of lift-off to reduce risks of noisy release, but I am not sure if it explains why they produce events in between. > > > > + */ > > > + if (!mt_is_slot_state_equal(&state, > > > + &app->mt_slot_state[slotnum])) { > > > + app->touch_change |= (active || > > > + app->mt_slot_state[slotnum].tip_state); > > > + app->repeated_counter = 0; > > > + } else { > > > +#define MT_MAX_REPEATED_DROPPED 5 > > > + if (app->repeated_counter < MT_MAX_REPEATED_DROPPED) { > > > + active = false; > > > + if (app->touched) > > > + app->touch_change = true; > > > + } else { > > > + app->touch_change = true; > > > + } > > > + app->repeated_counter++; > > This is specific to one device, but I have seen device filtering in > hardware that reports the same steady data as long as the touch is > present. So 5 reports with the same data seem quite short. > It can be a larger number, I just believe that in this case it was enough. > Also, we do have devices which tend to forget to release slots, and > that's why we have MT_QUIRK_STICKY_FINGERS which release touches after > a while. Yeah, but the thing is that this specific device actually reports that the finger is still there, even though the finger has been lifted :-) I believe that MT_QUIRK_STICKY_FINGERS works when the device somehow fails to send a new report within some time after the previous one, so it does not solve the issue for us. > > > > + } > > > + app->touched = active; > > > + app->mt_slot_state[slotnum] = state; > > > + } > > > input_mt_slot(input, slotnum); > > > input_mt_report_slot_state(input, tool, active); > > > if (active) { > > > @@ -1296,6 +1413,18 @@ static int mt_touch_input_configured(struct hid_device *hdev, > > > if (!app->pending_palm_slots) > > > return -ENOMEM; > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > + app->mt_slot_state = devm_kcalloc(&hi->input->dev, > > > + td->maxcontacts, > > > + sizeof(*app->mt_slot_state), > > > + GFP_KERNEL); > > > + if (!app->mt_slot_state) > > > + return -ENOMEM; > > > + mt_clear_slot_states(app->mt_slot_state, td->maxcontacts); > > > + app->repeated_counter = 0; > > > + } > > > + > > > + > > > ret = input_mt_init_slots(input, td->maxcontacts, app->mt_flags); > > > if (ret) > > > return ret; > > > @@ -1676,6 +1805,12 @@ static void mt_release_contacts(struct hid_device *hid) > > > > > > list_for_each_entry(application, &td->applications, list) { > > > application->num_received = 0; > > > + if (application->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > + application->touch_change = false; > > > + mt_clear_slot_states(application->mt_slot_state, > > > + td->maxcontacts); > > > + application->repeated_counter = 0; > > > + } > > > } > > > } > > > > > > @@ -2014,6 +2149,15 @@ static const struct hid_device_id mt_devices[] = { > > > MT_USB_DEVICE(USB_VENDOR_ID_ILITEK, > > > USB_DEVICE_ID_ILITEK_MULTITOUCH) }, > > > > > > + /* Ilitek 106E/F touchscreen */ > > > + { .driver_data = MT_CLS_ILITEK_016x, > > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016E) }, > > > + > > > + { .driver_data = MT_CLS_ILITEK_016x, > > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016F) }, > > > + > > > /* LG Melfas panel */ > > > { .driver_data = MT_CLS_LG, > > > HID_USB_DEVICE(USB_VENDOR_ID_LG, > > > -- > > > 2.35.1.1021.g381101b075-goog > > > > > > > Cheers, > Benjamin > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT 2022-04-06 12:18 ` Angela Czubak @ 2022-04-06 13:35 ` Benjamin Tissoires 2022-04-08 9:48 ` Angela Czubak 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Tissoires @ 2022-04-06 13:35 UTC (permalink / raw) To: Angela Czubak; +Cc: open list:HID CORE LAYER, Jiri Kosina, upstream On Wed, Apr 6, 2022 at 2:19 PM Angela Czubak <acz@semihalf.com> wrote: > > . > > On Tue, Apr 5, 2022 at 2:57 PM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > Hi Angela, > > > > On Tue, Apr 5, 2022 at 1:25 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > Hi Benjamin, Hi Jiri, > > > > > > any thoughts on this patch? > > > > Sorry for taking too long to answer. I saw the patch coming in while > > coming back last week, and could not process it, and completely forgot > > about it. > > > > > If you believe that the userspace input stack should be a better place > > > to solve this kind of issue, please let me know. > > > > This is definitely not userspace to process those specificities, as > > far as I understand. However... I don't understand the bug very well > > :/ > > > > more inlined below: > > > > > > > > Regards, > > > Angela > > > > > > On Thu, Mar 31, 2022 at 4:45 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > > > Ilitek touchscreens 016E and 016F repeat last finger position after it has > > > > left the surface of the device. > > > > We send BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1 with the first repeated > > > > slot as this may mean that the finger has been lifted. > > > > The touchscreen is not actually too sensitive and this would > > > > modify the behaviour of the on-screen keyboard, for instance holding the > > > > backspace key might be sometimes interpreted as removing a single character > > > > only. Add counter of repeated frames and if it is significantly big then > > > > assume the finger actually remains stationary. > > > > This commit is really confusing to me. This description is mixing > > symptoms from before the patch, behaviour from after and userspace > > which doesn't mean a lot here not knowing what is the problem. > > > > What is the exact problem? > > The device delays reporting finger lift off, instead it repeats its > last report with only the scantime updated. > I would interpret such events as meaning that the finger is present > but has stopped moving, which > is not the case, actually. > > When it comes to ChromeOS userspace input stack it seems that those > events are actually ignored, but the > problem is that the final event, i.e. the finger lit off is reported > significantly late, so if the user is moving > their finger to scroll the contents of the screen it immediately stops > once the finger leaves the surface. > However, the desired behaviour is for the scroll to continue, i.e. > they should observe something called fling. > The problem is that since this last event is delayed the scrolling stops. > Because the last event is so late, our window, over which we calculate > scroll speed, consists of just one > single event, i.e. we cannot calculate any movement velocity so we > assume it is zero. > I tried to increase the window, but it appears that the delay is large > enough for a human eye to notice that > the scrolling stops (as there are no new events for a while) and then > it restarts again (once the actual > lift off event is noticed, BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1). > > I find it more difficult to solve at userspace level and we would like > the fling (i.e. scrolling that continues > after the finger has left the surface) to be fluent. OK, thanks for the explanation. There are still a few bits that are confusing to me: > > > What events the device is sending? > > "Raw" events: > > https://gist.github.com/semihalf-czubak-angela/026072c013b4f883cb6adb7460b4d6ca > > So as you can see it repeats the last position etc. and the only thing > changing is the scantime. > > This translates to input events (not that the data below is from > another run, but you get the idea): > > https://gist.github.com/semihalf-czubak-angela/b957f7e464772bbdd95ddd814e84e5d9 But in both of these records, the last finger is reported 5 times, which corresponds to the value of MT_MAX_REPEATED_DROPPED. Given that the device sends one report every 20 millisecs, you end up at the exact same 0.1 seconds of delay, which makes me wonder about the usefulness of the patch. > > > What should be the expected kernel ouput? > > > > Well, ideally: > https://gist.github.com/semihalf-czubak-angela/4dd52c177372f8f0c2b4e5ada841ea95 > > So what helps is if > > Event: time 1643827301.463888, type 3 (EV_ABS), code 53 > (ABS_MT_POSITION_X), value 4817 > Event: time 1643827301.463888, type 3 (EV_ABS), code 54 > (ABS_MT_POSITION_Y), value 777 > Event: time 1643827301.463888, type 3 (EV_ABS), code 0 (ABS_X), value 4817 > Event: time 1643827301.463888, type 3 (EV_ABS), code 1 (ABS_Y), value 777 > Event: time 1643827301.463888, type 4 (EV_MSC), code 5 > (MSC_TIMESTAMP), value 408000 > Event: time 1643827301.463888, -------------- SYN_REPORT ------------ > Event: time 1643827301.479920, type 4 (EV_MSC), code 5 > (MSC_TIMESTAMP), value 416000 > Event: time 1643827301.479920, -------------- SYN_REPORT ------------ > Event: time 1643827301.496921, type 4 (EV_MSC), code 5 > (MSC_TIMESTAMP), value 424000 > Event: time 1643827301.496921, -------------- SYN_REPORT ------------ > Event: time 1643827301.517923, type 4 (EV_MSC), code 5 > (MSC_TIMESTAMP), value 432000 > Event: time 1643827301.517923, -------------- SYN_REPORT ------------ > Event: time 1643827301.535920, type 4 (EV_MSC), code 5 > (MSC_TIMESTAMP), value 440000 > Event: time 1643827301.535920, -------------- SYN_REPORT ------------ > Event: time 1643827301.554943, type 3 (EV_ABS), code 57 > (ABS_MT_TRACKING_ID), value -1 > Event: time 1643827301.554943, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0 > Event: time 1643827301.554943, type 4 (EV_MSC), code 5 > (MSC_TIMESTAMP), value 448000 > Event: time 1643827301.554943, -------------- SYN_REPORT ------------ > > becomes > > Event: time 1643827301.463888, type 3 (EV_ABS), code 53 > (ABS_MT_POSITION_X), value 4817 > Event: time 1643827301.463888, type 3 (EV_ABS), code 54 > (ABS_MT_POSITION_Y), value 777 > Event: time 1643827301.463888, type 3 (EV_ABS), code 0 (ABS_X), value 4817 > Event: time 1643827301.463888, type 3 (EV_ABS), code 1 (ABS_Y), value 777 > Event: time 1643827301.463888, type 4 (EV_MSC), code 5 > (MSC_TIMESTAMP), value 408000 > Event: time 1643827301.463888, -------------- SYN_REPORT ------------ > Event: time 1643827301.479920, type 3 (EV_ABS), code 57 > (ABS_MT_TRACKING_ID), value -1 > Event: time 1643827301.479920, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0 > Event: time 1643827301.479920, type 4 (EV_MSC), code 5 > (MSC_TIMESTAMP), value 416000 > Event: time 1643827301.479920, -------------- SYN_REPORT ------------ Well, here, the timestamp says that you want the touch to come as soon as there are 2 identical values, because it comes right after the last modified event. This is doable, but that also means you might create false releases while processing the events. > > > > (bonus point for actually giving the events in the hid-recorder output > > format [0] ;-P ) > > > > Here you are: https://gist.github.com/semihalf-czubak-angela/eaaf73459c5c50f716016839fc368ab0 Heh, thanks :) > > > FWIW, this driver is one of the few drivers in the HID stack to have > > extended tests in hid-tools[0]. > > I plan to merge that repo into the selftests subtree, but for now we > > need to use this external repo. > > > > So I'd be pleased to see new tests added for that quirk because it > > seems far from evident what is happening. > > > > Ack, I will prepare something. > > > > > > > > > Signed-off-by: Angela Czubak <acz@semihalf.com> > > > > --- > > > > > > > > Hi guys, > > > > > > > > Please let me know if you think there is some better solution. > > > > It felt to me that it would be more correct to solve it on kernel > > > > driver level since this issue is device specific, but perhaps > > > > there are some userspace input stacks that figured this problem out. > > > > With Chromium OS Input Stack the final BTN_TOUCH=0 event comes > > > > significantly late which causes hiccups or simply a scrolling fling > > > > not to happen, since the input stack thinks the finger is still > > > > in the same position and on the screen. > > > > If the device reports a wrong state, the kernel is the place to fix > > it. However, as mentioned above I do not understand what is wrong > > there by reading the description and the code. > > > > > > > > > > drivers/hid/hid-ids.h | 2 + > > > > drivers/hid/hid-multitouch.c | 150 ++++++++++++++++++++++++++++++++++- > > > > 2 files changed, 149 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > > > index 053853a891c5..cc85f547603c 100644 > > > > --- a/drivers/hid/hid-ids.h > > > > +++ b/drivers/hid/hid-ids.h > > > > @@ -630,6 +630,8 @@ > > > > > > > > #define USB_VENDOR_ID_ILITEK 0x222a > > > > #define USB_DEVICE_ID_ILITEK_MULTITOUCH 0x0001 > > > > +#define USB_DEVICE_ID_ILITEK_016E 0x016e > > > > +#define USB_DEVICE_ID_ILITEK_016F 0x016f > > > > > > > > #define USB_VENDOR_ID_INTEL_0 0x8086 > > > > #define USB_VENDOR_ID_INTEL_1 0x8087 > > > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > > > > index 99eabfb4145b..33cb5e5179c2 100644 > > > > --- a/drivers/hid/hid-multitouch.c > > > > +++ b/drivers/hid/hid-multitouch.c > > > > @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); > > > > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) > > > > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) > > > > #define MT_QUIRK_DISABLE_WAKEUP BIT(21) > > > > +#define MT_QUIRK_DROP_REPEATED_SLOT BIT(22) > > > > > > > > #define MT_INPUTMODE_TOUCHSCREEN 0x02 > > > > #define MT_INPUTMODE_TOUCHPAD 0x03 > > > > @@ -103,12 +104,22 @@ struct mt_usages { > > > > bool *confidence_state; /* is the touch made by a finger? */ > > > > }; > > > > > > > > +struct mt_slot_state { > > > > + __s32 x, y, cx, cy, p, w, h, a; > > > > + __s32 contactid; /* the device ContactID assigned to this slot */ > > > > + bool tip_state; /* is the touch valid? */ > > > > + bool inrange_state; /* is the finger in proximity of the sensor? */ > > > > + bool confidence_state; /* is the touch made by a finger? */ > > > > +}; > > > > This raises a red flag here. I spent quite some time removing internal > > slot states, and re-adding them means that something is wrong IMO. > > (though if we can not do something else, we might just re-add them). > > > > I wanted to compare against the report contents excluding the scantime. > Perhaps I could do that on the hid report level, this way just seemed > easier to implement. You can also use input_mt_get_value() and compare with the current value before sending the value to the input stack. This way you do not need to store the values once again. > > > > > + > > > > struct mt_application { > > > > struct list_head list; > > > > unsigned int application; > > > > unsigned int report_id; > > > > struct list_head mt_usages; /* mt usages list */ > > > > > > > > + struct mt_slot_state *mt_slot_state; > > > > + > > > > __s32 quirks; > > > > > > > > __s32 *scantime; /* scantime reported */ > > > > @@ -116,6 +127,11 @@ struct mt_application { > > > > > > > > __s32 *raw_cc; /* contact count in the report */ > > > > int left_button_state; /* left button state */ > > > > + bool touch_change; /* if touch change detected */ > > > > + bool touched; /* if touch was present in the last > > > > + * message > > > > + */ > > > > So this is not per-touch, but per report? > > > > I suppose it is actually per report, it is just that implementation > per slot seemed easier. > I could not reproduce the issue when using two fingers. Which makes a lot of sense: when you still have one finger on the screen, the sensitive sensors are way much capable of detecting ghosts because they have a baseline to compare too. So I would suggest you enable that code only when you have 1 finger left and that this finger is still. > > > > > + int repeated_counter; /* how many times last frame repeated */ > > > > unsigned int mt_flags; /* flags to pass to input-mt */ > > > > > > > > unsigned long *pending_palm_slots; /* slots where we reported palm > > > > @@ -211,6 +227,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); > > > > #define MT_CLS_GOOGLE 0x0111 > > > > #define MT_CLS_RAZER_BLADE_STEALTH 0x0112 > > > > #define MT_CLS_SMART_TECH 0x0113 > > > > +#define MT_CLS_ILITEK_016x 0x0114 > > > > > > > > #define MT_DEFAULT_MAXCONTACT 10 > > > > #define MT_MAX_MAXCONTACT 250 > > > > @@ -386,6 +403,15 @@ static const struct mt_class mt_classes[] = { > > > > MT_QUIRK_CONTACT_CNT_ACCURATE | > > > > MT_QUIRK_SEPARATE_APP_REPORT, > > > > }, > > > > + { .name = MT_CLS_ILITEK_016x, > > > > + .quirks = MT_QUIRK_ALWAYS_VALID | > > > > + MT_QUIRK_IGNORE_DUPLICATES | > > > > + MT_QUIRK_HOVERING | > > > > + MT_QUIRK_CONTACT_CNT_ACCURATE | > > > > + MT_QUIRK_STICKY_FINGERS | > > > > + MT_QUIRK_WIN8_PTP_BUTTONS | > > > > + MT_QUIRK_DROP_REPEATED_SLOT, > > > > + .export_all_inputs = true }, > > > > { } > > > > }; > > > > > > > > @@ -783,7 +809,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, > > > > case HID_DG_CONFIDENCE: > > > > if ((cls->name == MT_CLS_WIN_8 || > > > > cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT || > > > > - cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP) && > > > > + cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP || > > > > + cls->name == MT_CLS_ILITEK_016x) && > > > > Not something you should care about, but this long test of spaghetti > > should likely be fixed into something way better at some point... > > > > > > (field->application == HID_DG_TOUCHPAD || > > > > field->application == HID_DG_TOUCHSCREEN)) > > > > app->quirks |= MT_QUIRK_CONFIDENCE; > > > > @@ -948,7 +975,13 @@ static void mt_sync_frame(struct mt_device *td, struct mt_application *app, > > > > input_event(input, EV_KEY, BTN_LEFT, app->left_button_state); > > > > > > > > input_mt_sync_frame(input); > > > > - input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > + if (app->touch_change) > > > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > + app->touch_change = false; > > > > Why filter out the timestamp? > > > > I suppose lonely timestamps should not do any harm, I just wanted to > avoid evtest etc. output > where there is no change but the timestamp, it is kind of also > remainder of my first approach where > I just tried filtering this events out and see waiting for BTN_TOUCH=0 > and ABS_MT_TRACKING_ID=-1 > would be enough (it wasn't). So I wouldn't filter out the timestamps. Not sure how ChromeOS stack behaves when we forward timestamps without a touch, but it should be capable of ignoring them. > > > > > + } else { > > > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > + } > > > > input_sync(input); > > > > > > > > mt_release_pending_palms(td, app, input); > > > > @@ -993,6 +1026,60 @@ static int mt_touch_event(struct hid_device *hid, struct hid_field *field, > > > > return 1; > > > > } > > > > > > > > +static void mt_clear_slot_states(struct mt_slot_state *mt_slot_state, > > > > + int size) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = 0; i < size; i++) > > > > + mt_slot_state[i].tip_state = 0; > > > > +} > > > > + > > > > +static void mt_fill_slot_state(struct mt_slot_state *mt_slot_state, > > > > + struct mt_usages *slot) > > > > +{ > > > > + mt_slot_state->x = *slot->x; > > > > + mt_slot_state->y = *slot->y; > > > > + mt_slot_state->cx = *slot->cx; > > > > + mt_slot_state->cy = *slot->cy; > > > > + mt_slot_state->p = *slot->p; > > > > + mt_slot_state->w = *slot->w; > > > > + mt_slot_state->h = *slot->h; > > > > + mt_slot_state->a = *slot->a; > > > > + mt_slot_state->contactid = *slot->contactid; > > > > + mt_slot_state->tip_state = *slot->tip_state; > > > > + mt_slot_state->inrange_state = *slot->inrange_state; > > > > + mt_slot_state->confidence_state = *slot->confidence_state; > > > > +} > > > > + > > > > +static bool mt_is_slot_state_equal(struct mt_slot_state *state1, > > > > + struct mt_slot_state *state2) > > > > +{ > > > > + if (state1->x != state2->x) > > > > + return false; > > > > + if (state1->y != state2->y) > > > > + return false; > > > > + if (state1->cx != state2->cx) > > > > + return false; > > > > + if (state1->cy != state2->cy) > > > > + return false; > > > > + if (state1->p != state2->p) > > > > + return false; > > > > + if (state1->w != state2->w) > > > > + return false; > > > > + if (state1->a != state2->a) > > > > + return false; > > > > + if (state1->contactid != state2->contactid) > > > > + return false; > > > > + if (state1->tip_state != state2->tip_state) > > > > + return false; > > > > + if (state1->inrange_state != state2->inrange_state) > > > > + return false; > > > > + if (state1->confidence_state != state2->confidence_state) > > > > + return false; > > > > + return true; > > > > +} > > > > The input stack already filters out duplicated events. So I am not > > sure we need to store the information here once again and test for it > > here. > > > > What I want to do is to "prematurely" report that the finger has been lifted. > It is not that much about solely filtering out the duplicated events, > I rather want to interpret > such an event as a finger lift off. Yes, but see my remark above. Given that the input stack already caches the values and that it is validated by the evdev recording you shared, you should be able to apply the quirk just before calling the various input_event() and comparing the current value of this last finger with the ones in the input stack. This way, you do not need to keep tabs on the various fingers. > > > > > + > > > > static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > struct mt_application *app, > > > > struct mt_usages *slot) > > > > @@ -1005,6 +1092,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > int active; > > > > int slotnum; > > > > int tool = MT_TOOL_FINGER; > > > > + struct mt_slot_state state = {0}; > > > > > > > > if (!slot) > > > > return -EINVAL; > > > > @@ -1058,13 +1146,42 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > * lift-off as userspace will not be aware > > > > * of non-confidence, so we need to split > > > > * it into 2 events: active MT_TOOL_PALM > > > > - * and a separate liftoff. > > > > + * and a separate lift off. > > > > */ > > > > active = true; > > > > set_bit(slotnum, app->pending_palm_slots); > > > > } > > > > } > > > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > + mt_fill_slot_state(&state, slot); > > > > + /* Check if every field in the slot is the same as before. > > > > + * Some touchscreens report the same position for several > > > > + * frames even though the finger is no longer on the surface. > > > > + * This is actually being recognised as the finger remaining in > > > > + * one position, which causes scrolling to be stopped. > > > > + * Ignore such packets so that the scrolling continues > > > > + * and the touchscreen reports the finger liftoff. > > > > That part I understand better, but how is the device reporting such events? > > If the touch is no longer there, we should have a bit that says it is > > lifted, so we should simply just ignore the touch, no? > > > > Well, I would actually like the touch panel manufacturers to answer that :-) > Based on some comments in our source code it seems that some devices > might delay synthesis of lift-off to reduce risks of noisy release, but I am not > sure if it explains why they produce events in between. They produce events in between because they have to, or they are not following the spec :) I also think the main reason is that if they forget about sending an event *and* that the touch continues to move after a few events, they will see a release happening on the touch because that's what STICKY_FINGER does (it's a copy of the behaviour from Windows FWIW). > > > > > > > + */ > > > > + if (!mt_is_slot_state_equal(&state, > > > > + &app->mt_slot_state[slotnum])) { > > > > + app->touch_change |= (active || > > > > + app->mt_slot_state[slotnum].tip_state); > > > > + app->repeated_counter = 0; > > > > + } else { > > > > +#define MT_MAX_REPEATED_DROPPED 5 > > > > + if (app->repeated_counter < MT_MAX_REPEATED_DROPPED) { > > > > + active = false; > > > > + if (app->touched) > > > > + app->touch_change = true; > > > > + } else { > > > > + app->touch_change = true; > > > > + } > > > > + app->repeated_counter++; > > > > This is specific to one device, but I have seen device filtering in > > hardware that reports the same steady data as long as the touch is > > present. So 5 reports with the same data seem quite short. > > > > It can be a larger number, I just believe that in this case it was enough. All 3 (or 4 maybe) recordings you shared showed that we received only 5 duplications of events. So there is a case to be made around whether we need this patch. What would be interesting is whether you can "emulate" that situation without releasing the finger. If the raw data is noisy (to some extent of course) and it's hard to have exactly twice the same sample for a touch, then maybe we can assume that as soon as we get identical values we can release. Maybe the false positive in that situation would be better handled by the user that missing the fling. > > > Also, we do have devices which tend to forget to release slots, and > > that's why we have MT_QUIRK_STICKY_FINGERS which release touches after > > a while. > > Yeah, but the thing is that this specific device actually reports that > the finger is still there, > even though the finger has been lifted :-) > I believe that MT_QUIRK_STICKY_FINGERS works when the device somehow > fails to send a new report > within some time after the previous one, so it does not solve the issue for us. It's not exactly the same case, but it still feels similar enough to maybe be reused. Can you please give a shot at the following patch (and change the value of MT_MAX_REPEATED_DROPPED between 1 to 5)? It doesn't use MT_QUIRK_STICKY_FINGERS but should be simnilar to what you have here in a shorter version. --- diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 0dece608c023..cff4088e564a 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -77,6 +77,8 @@ MODULE_LICENSE("GPL"); #define MT_BUTTONTYPE_CLICKPAD 0 +#define MT_MAX_REPEATED_DROPPED 3 + enum latency_mode { HID_LATENCY_NORMAL = 0, HID_LATENCY_HIGH = 1, @@ -128,6 +130,7 @@ struct mt_application { * 1 means we should use a serial protocol * > 1 means hybrid (multitouch) protocol */ + unsigned int repeated_touches; /* used when MT_QUIRK_DROP_REPEATED_SLOT */ __s32 dev_time; /* the scan time provided by the device */ unsigned long jiffies; /* the frame's jiffies */ @@ -1065,6 +1068,19 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, } } + /* TODO: add test for the new quirk so we don't apply this for all devices */ + if (active && app->num_expected == 1) { + struct input_mt_slot *i_slot = &mt->slots[slotnum]; + + if (input_mt_get_value(i_slot, ABS_MT_POSITION_X) == *slot->x && + input_mt_get_value(i_slot, ABS_MT_POSITION_Y) == *slot->y) { + if (++app->repeated_touches >= MT_MAX_REPEATED_DROPPED) + active = false; + } else { + app->repeated_touches = 0; + } + } + input_mt_slot(input, slotnum); input_mt_report_slot_state(input, tool, active); if (active) { --- Cheers, Benjamin > > > > > > > > > > + } > > > > + app->touched = active; > > > > + app->mt_slot_state[slotnum] = state; > > > > + } > > > > input_mt_slot(input, slotnum); > > > > input_mt_report_slot_state(input, tool, active); > > > > if (active) { > > > > @@ -1296,6 +1413,18 @@ static int mt_touch_input_configured(struct hid_device *hdev, > > > > if (!app->pending_palm_slots) > > > > return -ENOMEM; > > > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > + app->mt_slot_state = devm_kcalloc(&hi->input->dev, > > > > + td->maxcontacts, > > > > + sizeof(*app->mt_slot_state), > > > > + GFP_KERNEL); > > > > + if (!app->mt_slot_state) > > > > + return -ENOMEM; > > > > + mt_clear_slot_states(app->mt_slot_state, td->maxcontacts); > > > > + app->repeated_counter = 0; > > > > + } > > > > + > > > > + > > > > ret = input_mt_init_slots(input, td->maxcontacts, app->mt_flags); > > > > if (ret) > > > > return ret; > > > > @@ -1676,6 +1805,12 @@ static void mt_release_contacts(struct hid_device *hid) > > > > > > > > list_for_each_entry(application, &td->applications, list) { > > > > application->num_received = 0; > > > > + if (application->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > + application->touch_change = false; > > > > + mt_clear_slot_states(application->mt_slot_state, > > > > + td->maxcontacts); > > > > + application->repeated_counter = 0; > > > > + } > > > > } > > > > } > > > > > > > > @@ -2014,6 +2149,15 @@ static const struct hid_device_id mt_devices[] = { > > > > MT_USB_DEVICE(USB_VENDOR_ID_ILITEK, > > > > USB_DEVICE_ID_ILITEK_MULTITOUCH) }, > > > > > > > > + /* Ilitek 106E/F touchscreen */ > > > > + { .driver_data = MT_CLS_ILITEK_016x, > > > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016E) }, > > > > + > > > > + { .driver_data = MT_CLS_ILITEK_016x, > > > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016F) }, > > > > + > > > > /* LG Melfas panel */ > > > > { .driver_data = MT_CLS_LG, > > > > HID_USB_DEVICE(USB_VENDOR_ID_LG, > > > > -- > > > > 2.35.1.1021.g381101b075-goog > > > > > > > > > > > Cheers, > > Benjamin > > > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools > > > ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT 2022-04-06 13:35 ` Benjamin Tissoires @ 2022-04-08 9:48 ` Angela Czubak 2022-04-21 9:31 ` Benjamin Tissoires 0 siblings, 1 reply; 9+ messages in thread From: Angela Czubak @ 2022-04-08 9:48 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: open list:HID CORE LAYER, Jiri Kosina, upstream Hi Benjamin, On Wed, Apr 6, 2022 at 3:35 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > On Wed, Apr 6, 2022 at 2:19 PM Angela Czubak <acz@semihalf.com> wrote: > > > > . > > > > On Tue, Apr 5, 2022 at 2:57 PM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > Hi Angela, > > > > > > On Tue, Apr 5, 2022 at 1:25 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > > > Hi Benjamin, Hi Jiri, > > > > > > > > any thoughts on this patch? > > > > > > Sorry for taking too long to answer. I saw the patch coming in while > > > coming back last week, and could not process it, and completely forgot > > > about it. > > > > > > > If you believe that the userspace input stack should be a better place > > > > to solve this kind of issue, please let me know. > > > > > > This is definitely not userspace to process those specificities, as > > > far as I understand. However... I don't understand the bug very well > > > :/ > > > > > > more inlined below: > > > > > > > > > > > Regards, > > > > Angela > > > > > > > > On Thu, Mar 31, 2022 at 4:45 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > > > > > Ilitek touchscreens 016E and 016F repeat last finger position after it has > > > > > left the surface of the device. > > > > > We send BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1 with the first repeated > > > > > slot as this may mean that the finger has been lifted. > > > > > The touchscreen is not actually too sensitive and this would > > > > > modify the behaviour of the on-screen keyboard, for instance holding the > > > > > backspace key might be sometimes interpreted as removing a single character > > > > > only. Add counter of repeated frames and if it is significantly big then > > > > > assume the finger actually remains stationary. > > > > > > This commit is really confusing to me. This description is mixing > > > symptoms from before the patch, behaviour from after and userspace > > > which doesn't mean a lot here not knowing what is the problem. > > > > > > What is the exact problem? > > > > The device delays reporting finger lift off, instead it repeats its > > last report with only the scantime updated. > > I would interpret such events as meaning that the finger is present > > but has stopped moving, which > > is not the case, actually. > > > > When it comes to ChromeOS userspace input stack it seems that those > > events are actually ignored, but the > > problem is that the final event, i.e. the finger lit off is reported > > significantly late, so if the user is moving > > their finger to scroll the contents of the screen it immediately stops > > once the finger leaves the surface. > > However, the desired behaviour is for the scroll to continue, i.e. > > they should observe something called fling. > > The problem is that since this last event is delayed the scrolling stops. > > Because the last event is so late, our window, over which we calculate > > scroll speed, consists of just one > > single event, i.e. we cannot calculate any movement velocity so we > > assume it is zero. > > I tried to increase the window, but it appears that the delay is large > > enough for a human eye to notice that > > the scrolling stops (as there are no new events for a while) and then > > it restarts again (once the actual > > lift off event is noticed, BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1). > > > > I find it more difficult to solve at userspace level and we would like > > the fling (i.e. scrolling that continues > > after the finger has left the surface) to be fluent. > > OK, thanks for the explanation. There are still a few bits that are > confusing to me: > > > > > > What events the device is sending? > > > > "Raw" events: > > > > https://gist.github.com/semihalf-czubak-angela/026072c013b4f883cb6adb7460b4d6ca > > > > So as you can see it repeats the last position etc. and the only thing > > changing is the scantime. > > > > This translates to input events (not that the data below is from > > another run, but you get the idea): > > > > https://gist.github.com/semihalf-czubak-angela/b957f7e464772bbdd95ddd814e84e5d9 > > But in both of these records, the last finger is reported 5 times, which > corresponds to the value of MT_MAX_REPEATED_DROPPED. > Given that the device sends one report every 20 millisecs, you end up at > the exact same 0.1 seconds of delay, which makes me wonder about the > usefulness of the patch. > Well, let me rephrase then: this 0.1 is unfortunately very noticeable to a human eye. I know it shouldn't be much, but it actually causes the fling to stop, whereas a smooth behaviour/movement is expected. And with the patch the lift of is actually reported with the first repeated slot, it is just that after MT_MAX_REPEATED_DROPPED the kernel actually starts sending the slot/touch information again. > > > > > What should be the expected kernel ouput? > > > > > > > Well, ideally: > > https://gist.github.com/semihalf-czubak-angela/4dd52c177372f8f0c2b4e5ada841ea95 > > > > So what helps is if > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 53 > > (ABS_MT_POSITION_X), value 4817 > > Event: time 1643827301.463888, type 3 (EV_ABS), code 54 > > (ABS_MT_POSITION_Y), value 777 > > Event: time 1643827301.463888, type 3 (EV_ABS), code 0 (ABS_X), value 4817 > > Event: time 1643827301.463888, type 3 (EV_ABS), code 1 (ABS_Y), value 777 > > Event: time 1643827301.463888, type 4 (EV_MSC), code 5 > > (MSC_TIMESTAMP), value 408000 > > Event: time 1643827301.463888, -------------- SYN_REPORT ------------ > > Event: time 1643827301.479920, type 4 (EV_MSC), code 5 > > (MSC_TIMESTAMP), value 416000 > > Event: time 1643827301.479920, -------------- SYN_REPORT ------------ > > Event: time 1643827301.496921, type 4 (EV_MSC), code 5 > > (MSC_TIMESTAMP), value 424000 > > Event: time 1643827301.496921, -------------- SYN_REPORT ------------ > > Event: time 1643827301.517923, type 4 (EV_MSC), code 5 > > (MSC_TIMESTAMP), value 432000 > > Event: time 1643827301.517923, -------------- SYN_REPORT ------------ > > Event: time 1643827301.535920, type 4 (EV_MSC), code 5 > > (MSC_TIMESTAMP), value 440000 > > Event: time 1643827301.535920, -------------- SYN_REPORT ------------ > > Event: time 1643827301.554943, type 3 (EV_ABS), code 57 > > (ABS_MT_TRACKING_ID), value -1 > > Event: time 1643827301.554943, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0 > > Event: time 1643827301.554943, type 4 (EV_MSC), code 5 > > (MSC_TIMESTAMP), value 448000 > > Event: time 1643827301.554943, -------------- SYN_REPORT ------------ > > > > becomes > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 53 > > (ABS_MT_POSITION_X), value 4817 > > Event: time 1643827301.463888, type 3 (EV_ABS), code 54 > > (ABS_MT_POSITION_Y), value 777 > > Event: time 1643827301.463888, type 3 (EV_ABS), code 0 (ABS_X), value 4817 > > Event: time 1643827301.463888, type 3 (EV_ABS), code 1 (ABS_Y), value 777 > > Event: time 1643827301.463888, type 4 (EV_MSC), code 5 > > (MSC_TIMESTAMP), value 408000 > > Event: time 1643827301.463888, -------------- SYN_REPORT ------------ > > Event: time 1643827301.479920, type 3 (EV_ABS), code 57 > > (ABS_MT_TRACKING_ID), value -1 > > Event: time 1643827301.479920, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0 > > Event: time 1643827301.479920, type 4 (EV_MSC), code 5 > > (MSC_TIMESTAMP), value 416000 > > Event: time 1643827301.479920, -------------- SYN_REPORT ------------ > > Well, here, the timestamp says that you want the touch to come as soon > as there are 2 identical values, because it comes right after the last > modified event. This is doable, but that also means you might create > false releases while processing the events. > I am aware of that, hence I want to assume that if this repeats enough times the finger is actually stationary. Ideally I would like to have the fix in the firmware, but a patch in linux kernel seems easier for me :) > > > > > > > (bonus point for actually giving the events in the hid-recorder output > > > format [0] ;-P ) > > > > > > > Here you are: https://gist.github.com/semihalf-czubak-angela/eaaf73459c5c50f716016839fc368ab0 > > Heh, thanks :) > > > > > > FWIW, this driver is one of the few drivers in the HID stack to have > > > extended tests in hid-tools[0]. > > > I plan to merge that repo into the selftests subtree, but for now we > > > need to use this external repo. > > > > > > So I'd be pleased to see new tests added for that quirk because it > > > seems far from evident what is happening. > > > > > > > Ack, I will prepare something. > > > > > > > > > > > > Signed-off-by: Angela Czubak <acz@semihalf.com> > > > > > --- > > > > > > > > > > Hi guys, > > > > > > > > > > Please let me know if you think there is some better solution. > > > > > It felt to me that it would be more correct to solve it on kernel > > > > > driver level since this issue is device specific, but perhaps > > > > > there are some userspace input stacks that figured this problem out. > > > > > With Chromium OS Input Stack the final BTN_TOUCH=0 event comes > > > > > significantly late which causes hiccups or simply a scrolling fling > > > > > not to happen, since the input stack thinks the finger is still > > > > > in the same position and on the screen. > > > > > > If the device reports a wrong state, the kernel is the place to fix > > > it. However, as mentioned above I do not understand what is wrong > > > there by reading the description and the code. > > > > > > > > > > > > > drivers/hid/hid-ids.h | 2 + > > > > > drivers/hid/hid-multitouch.c | 150 ++++++++++++++++++++++++++++++++++- > > > > > 2 files changed, 149 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > > > > index 053853a891c5..cc85f547603c 100644 > > > > > --- a/drivers/hid/hid-ids.h > > > > > +++ b/drivers/hid/hid-ids.h > > > > > @@ -630,6 +630,8 @@ > > > > > > > > > > #define USB_VENDOR_ID_ILITEK 0x222a > > > > > #define USB_DEVICE_ID_ILITEK_MULTITOUCH 0x0001 > > > > > +#define USB_DEVICE_ID_ILITEK_016E 0x016e > > > > > +#define USB_DEVICE_ID_ILITEK_016F 0x016f > > > > > > > > > > #define USB_VENDOR_ID_INTEL_0 0x8086 > > > > > #define USB_VENDOR_ID_INTEL_1 0x8087 > > > > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > > > > > index 99eabfb4145b..33cb5e5179c2 100644 > > > > > --- a/drivers/hid/hid-multitouch.c > > > > > +++ b/drivers/hid/hid-multitouch.c > > > > > @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); > > > > > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) > > > > > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) > > > > > #define MT_QUIRK_DISABLE_WAKEUP BIT(21) > > > > > +#define MT_QUIRK_DROP_REPEATED_SLOT BIT(22) > > > > > > > > > > #define MT_INPUTMODE_TOUCHSCREEN 0x02 > > > > > #define MT_INPUTMODE_TOUCHPAD 0x03 > > > > > @@ -103,12 +104,22 @@ struct mt_usages { > > > > > bool *confidence_state; /* is the touch made by a finger? */ > > > > > }; > > > > > > > > > > +struct mt_slot_state { > > > > > + __s32 x, y, cx, cy, p, w, h, a; > > > > > + __s32 contactid; /* the device ContactID assigned to this slot */ > > > > > + bool tip_state; /* is the touch valid? */ > > > > > + bool inrange_state; /* is the finger in proximity of the sensor? */ > > > > > + bool confidence_state; /* is the touch made by a finger? */ > > > > > +}; > > > > > > This raises a red flag here. I spent quite some time removing internal > > > slot states, and re-adding them means that something is wrong IMO. > > > (though if we can not do something else, we might just re-add them). > > > > > > > I wanted to compare against the report contents excluding the scantime. > > Perhaps I could do that on the hid report level, this way just seemed > > easier to implement. > > You can also use input_mt_get_value() and compare with the current value > before sending the value to the input stack. This way you do not need to > store the values once again. Thanks for suggesting that! > > > > > > > > + > > > > > struct mt_application { > > > > > struct list_head list; > > > > > unsigned int application; > > > > > unsigned int report_id; > > > > > struct list_head mt_usages; /* mt usages list */ > > > > > > > > > > + struct mt_slot_state *mt_slot_state; > > > > > + > > > > > __s32 quirks; > > > > > > > > > > __s32 *scantime; /* scantime reported */ > > > > > @@ -116,6 +127,11 @@ struct mt_application { > > > > > > > > > > __s32 *raw_cc; /* contact count in the report */ > > > > > int left_button_state; /* left button state */ > > > > > + bool touch_change; /* if touch change detected */ > > > > > + bool touched; /* if touch was present in the last > > > > > + * message > > > > > + */ > > > > > > So this is not per-touch, but per report? > > > > > > > I suppose it is actually per report, it is just that implementation > > per slot seemed easier. > > I could not reproduce the issue when using two fingers. > > Which makes a lot of sense: when you still have one finger on the > screen, the sensitive sensors are way much capable of detecting ghosts > because they have a baseline to compare too. > So I would suggest you enable that code only when you have 1 finger left > and that this finger is still. > > > > > > > > + int repeated_counter; /* how many times last frame repeated */ > > > > > unsigned int mt_flags; /* flags to pass to input-mt */ > > > > > > > > > > unsigned long *pending_palm_slots; /* slots where we reported palm > > > > > @@ -211,6 +227,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); > > > > > #define MT_CLS_GOOGLE 0x0111 > > > > > #define MT_CLS_RAZER_BLADE_STEALTH 0x0112 > > > > > #define MT_CLS_SMART_TECH 0x0113 > > > > > +#define MT_CLS_ILITEK_016x 0x0114 > > > > > > > > > > #define MT_DEFAULT_MAXCONTACT 10 > > > > > #define MT_MAX_MAXCONTACT 250 > > > > > @@ -386,6 +403,15 @@ static const struct mt_class mt_classes[] = { > > > > > MT_QUIRK_CONTACT_CNT_ACCURATE | > > > > > MT_QUIRK_SEPARATE_APP_REPORT, > > > > > }, > > > > > + { .name = MT_CLS_ILITEK_016x, > > > > > + .quirks = MT_QUIRK_ALWAYS_VALID | > > > > > + MT_QUIRK_IGNORE_DUPLICATES | > > > > > + MT_QUIRK_HOVERING | > > > > > + MT_QUIRK_CONTACT_CNT_ACCURATE | > > > > > + MT_QUIRK_STICKY_FINGERS | > > > > > + MT_QUIRK_WIN8_PTP_BUTTONS | > > > > > + MT_QUIRK_DROP_REPEATED_SLOT, > > > > > + .export_all_inputs = true }, > > > > > { } > > > > > }; > > > > > > > > > > @@ -783,7 +809,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, > > > > > case HID_DG_CONFIDENCE: > > > > > if ((cls->name == MT_CLS_WIN_8 || > > > > > cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT || > > > > > - cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP) && > > > > > + cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP || > > > > > + cls->name == MT_CLS_ILITEK_016x) && > > > > > > Not something you should care about, but this long test of spaghetti > > > should likely be fixed into something way better at some point... > > > > > > > > (field->application == HID_DG_TOUCHPAD || > > > > > field->application == HID_DG_TOUCHSCREEN)) > > > > > app->quirks |= MT_QUIRK_CONFIDENCE; > > > > > @@ -948,7 +975,13 @@ static void mt_sync_frame(struct mt_device *td, struct mt_application *app, > > > > > input_event(input, EV_KEY, BTN_LEFT, app->left_button_state); > > > > > > > > > > input_mt_sync_frame(input); > > > > > - input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > + if (app->touch_change) > > > > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > > + app->touch_change = false; > > > > > > Why filter out the timestamp? > > > > > > > I suppose lonely timestamps should not do any harm, I just wanted to > > avoid evtest etc. output > > where there is no change but the timestamp, it is kind of also > > remainder of my first approach where > > I just tried filtering this events out and see waiting for BTN_TOUCH=0 > > and ABS_MT_TRACKING_ID=-1 > > would be enough (it wasn't). > > So I wouldn't filter out the timestamps. Not sure how ChromeOS stack > behaves when we forward timestamps without a touch, but it should be > capable of ignoring them. Ack. > > > > > > > > + } else { > > > > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > > + } > > > > > input_sync(input); > > > > > > > > > > mt_release_pending_palms(td, app, input); > > > > > @@ -993,6 +1026,60 @@ static int mt_touch_event(struct hid_device *hid, struct hid_field *field, > > > > > return 1; > > > > > } > > > > > > > > > > +static void mt_clear_slot_states(struct mt_slot_state *mt_slot_state, > > > > > + int size) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + for (i = 0; i < size; i++) > > > > > + mt_slot_state[i].tip_state = 0; > > > > > +} > > > > > + > > > > > +static void mt_fill_slot_state(struct mt_slot_state *mt_slot_state, > > > > > + struct mt_usages *slot) > > > > > +{ > > > > > + mt_slot_state->x = *slot->x; > > > > > + mt_slot_state->y = *slot->y; > > > > > + mt_slot_state->cx = *slot->cx; > > > > > + mt_slot_state->cy = *slot->cy; > > > > > + mt_slot_state->p = *slot->p; > > > > > + mt_slot_state->w = *slot->w; > > > > > + mt_slot_state->h = *slot->h; > > > > > + mt_slot_state->a = *slot->a; > > > > > + mt_slot_state->contactid = *slot->contactid; > > > > > + mt_slot_state->tip_state = *slot->tip_state; > > > > > + mt_slot_state->inrange_state = *slot->inrange_state; > > > > > + mt_slot_state->confidence_state = *slot->confidence_state; > > > > > +} > > > > > + > > > > > +static bool mt_is_slot_state_equal(struct mt_slot_state *state1, > > > > > + struct mt_slot_state *state2) > > > > > +{ > > > > > + if (state1->x != state2->x) > > > > > + return false; > > > > > + if (state1->y != state2->y) > > > > > + return false; > > > > > + if (state1->cx != state2->cx) > > > > > + return false; > > > > > + if (state1->cy != state2->cy) > > > > > + return false; > > > > > + if (state1->p != state2->p) > > > > > + return false; > > > > > + if (state1->w != state2->w) > > > > > + return false; > > > > > + if (state1->a != state2->a) > > > > > + return false; > > > > > + if (state1->contactid != state2->contactid) > > > > > + return false; > > > > > + if (state1->tip_state != state2->tip_state) > > > > > + return false; > > > > > + if (state1->inrange_state != state2->inrange_state) > > > > > + return false; > > > > > + if (state1->confidence_state != state2->confidence_state) > > > > > + return false; > > > > > + return true; > > > > > +} > > > > > > The input stack already filters out duplicated events. So I am not > > > sure we need to store the information here once again and test for it > > > here. > > > > > > > What I want to do is to "prematurely" report that the finger has been lifted. > > It is not that much about solely filtering out the duplicated events, > > I rather want to interpret > > such an event as a finger lift off. > > Yes, but see my remark above. Given that the input stack already caches > the values and that it is validated by the evdev recording you shared, > you should be able to apply the quirk just before calling the various > input_event() and comparing the current value of this last finger with > the ones in the input stack. This way, you do not need to keep tabs on > the various fingers. > > > > > > > > + > > > > > static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > > struct mt_application *app, > > > > > struct mt_usages *slot) > > > > > @@ -1005,6 +1092,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > > int active; > > > > > int slotnum; > > > > > int tool = MT_TOOL_FINGER; > > > > > + struct mt_slot_state state = {0}; > > > > > > > > > > if (!slot) > > > > > return -EINVAL; > > > > > @@ -1058,13 +1146,42 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > > * lift-off as userspace will not be aware > > > > > * of non-confidence, so we need to split > > > > > * it into 2 events: active MT_TOOL_PALM > > > > > - * and a separate liftoff. > > > > > + * and a separate lift off. > > > > > */ > > > > > active = true; > > > > > set_bit(slotnum, app->pending_palm_slots); > > > > > } > > > > > } > > > > > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > + mt_fill_slot_state(&state, slot); > > > > > + /* Check if every field in the slot is the same as before. > > > > > + * Some touchscreens report the same position for several > > > > > + * frames even though the finger is no longer on the surface. > > > > > + * This is actually being recognised as the finger remaining in > > > > > + * one position, which causes scrolling to be stopped. > > > > > + * Ignore such packets so that the scrolling continues > > > > > + * and the touchscreen reports the finger liftoff. > > > > > > That part I understand better, but how is the device reporting such events? > > > If the touch is no longer there, we should have a bit that says it is > > > lifted, so we should simply just ignore the touch, no? > > > > > > > Well, I would actually like the touch panel manufacturers to answer that :-) > > Based on some comments in our source code it seems that some devices > > might delay synthesis of lift-off to reduce risks of noisy release, but I am not > > sure if it explains why they produce events in between. > > They produce events in between because they have to, or they are not > following the spec :) > I also think the main reason is that if they forget about sending an > event *and* that the touch continues to move after a few events, they > will see a release happening on the touch because that's what > STICKY_FINGER does (it's a copy of the behaviour from Windows FWIW). > > > > > > > > > > > + */ > > > > > + if (!mt_is_slot_state_equal(&state, > > > > > + &app->mt_slot_state[slotnum])) { > > > > > + app->touch_change |= (active || > > > > > + app->mt_slot_state[slotnum].tip_state); > > > > > + app->repeated_counter = 0; > > > > > + } else { > > > > > +#define MT_MAX_REPEATED_DROPPED 5 > > > > > + if (app->repeated_counter < MT_MAX_REPEATED_DROPPED) { > > > > > + active = false; > > > > > + if (app->touched) > > > > > + app->touch_change = true; > > > > > + } else { > > > > > + app->touch_change = true; > > > > > + } > > > > > + app->repeated_counter++; > > > > > > This is specific to one device, but I have seen device filtering in > > > hardware that reports the same steady data as long as the touch is > > > present. So 5 reports with the same data seem quite short. > > > > > > > It can be a larger number, I just believe that in this case it was enough. > > All 3 (or 4 maybe) recordings you shared showed that we received only 5 > duplications of events. So there is a case to be made around whether we > need this patch. > > What would be interesting is whether you can "emulate" that situation > without releasing the finger. If the raw data is noisy (to some extent > of course) and it's hard to have exactly twice the same sample for a > touch, then maybe we can assume that as soon as we get identical values > we can release. Maybe the false positive in that situation would be > better handled by the user that missing the fling. > Actually, I am able to receive the same report if I am very still and try not to move my finger. I wouldn't say it is easy but it is definitely possible and hence i wanted to use this upper bound so that using an on screen keyboard is not annoying (i.e. I want to be able to hold backspace key and erase more than just one sign). > > > > > Also, we do have devices which tend to forget to release slots, and > > > that's why we have MT_QUIRK_STICKY_FINGERS which release touches after > > > a while. > > > > Yeah, but the thing is that this specific device actually reports that > > the finger is still there, > > even though the finger has been lifted :-) > > I believe that MT_QUIRK_STICKY_FINGERS works when the device somehow > > fails to send a new report > > within some time after the previous one, so it does not solve the issue for us. > > It's not exactly the same case, but it still feels similar enough to > maybe be reused. > I am a little bit lost; MT_CLS_ILITEK_016x contains MT_QUIRK_STICKY_FINGERS. Based on the logs I provided, do you think that it does not work as expected? > Can you please give a shot at the following patch (and change the value > of MT_MAX_REPEATED_DROPPED between 1 to 5)? > It doesn't use MT_QUIRK_STICKY_FINGERS but should be simnilar to what > you have here in a shorter version. > > --- > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > index 0dece608c023..cff4088e564a 100644 > --- a/drivers/hid/hid-multitouch.c > +++ b/drivers/hid/hid-multitouch.c > @@ -77,6 +77,8 @@ MODULE_LICENSE("GPL"); > > #define MT_BUTTONTYPE_CLICKPAD 0 > > +#define MT_MAX_REPEATED_DROPPED 3 > + > enum latency_mode { > HID_LATENCY_NORMAL = 0, > HID_LATENCY_HIGH = 1, > @@ -128,6 +130,7 @@ struct mt_application { > * 1 means we should use a serial protocol > * > 1 means hybrid (multitouch) protocol > */ > + unsigned int repeated_touches; /* used when MT_QUIRK_DROP_REPEATED_SLOT */ > > __s32 dev_time; /* the scan time provided by the device */ > unsigned long jiffies; /* the frame's jiffies */ > @@ -1065,6 +1068,19 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > } > } > > + /* TODO: add test for the new quirk so we don't apply this for all devices */ > + if (active && app->num_expected == 1) { > + struct input_mt_slot *i_slot = &mt->slots[slotnum]; > + > + if (input_mt_get_value(i_slot, ABS_MT_POSITION_X) == *slot->x && > + input_mt_get_value(i_slot, ABS_MT_POSITION_Y) == *slot->y) { > + if (++app->repeated_touches >= MT_MAX_REPEATED_DROPPED) > + active = false; > + } else { > + app->repeated_touches = 0; > + } > + } > + > input_mt_slot(input, slotnum); > input_mt_report_slot_state(input, tool, active); > if (active) { > --- > Observed results: MT_MAX_REPEATED_DROPPED = 1: It solves this issue, no hiccup visible, but continuing to press a key on the screen does not produce the right effect, for instance backspace erases a single character only MT_MAX_REPEATED_DROPPED = 2: Same as above MT_MAX_REPEATED_DROPPED = 3: Small hiccup noticeable, problems with backspace as above MT_MAX_REPEATED_DROPPED = 4: Small hiccup noticeable, problems with backspace as above MT_MAX_REPEATED_DROPPED = 5: Fling does not work (due to 0.1s delay and UI constants assuming that it is too long time to still calculate velocity) + problems with backspace > Cheers, > Benjamin > > > > > > > > > > > > > > > > > + } > > > > > + app->touched = active; > > > > > + app->mt_slot_state[slotnum] = state; > > > > > + } > > > > > input_mt_slot(input, slotnum); > > > > > input_mt_report_slot_state(input, tool, active); > > > > > if (active) { > > > > > @@ -1296,6 +1413,18 @@ static int mt_touch_input_configured(struct hid_device *hdev, > > > > > if (!app->pending_palm_slots) > > > > > return -ENOMEM; > > > > > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > + app->mt_slot_state = devm_kcalloc(&hi->input->dev, > > > > > + td->maxcontacts, > > > > > + sizeof(*app->mt_slot_state), > > > > > + GFP_KERNEL); > > > > > + if (!app->mt_slot_state) > > > > > + return -ENOMEM; > > > > > + mt_clear_slot_states(app->mt_slot_state, td->maxcontacts); > > > > > + app->repeated_counter = 0; > > > > > + } > > > > > + > > > > > + > > > > > ret = input_mt_init_slots(input, td->maxcontacts, app->mt_flags); > > > > > if (ret) > > > > > return ret; > > > > > @@ -1676,6 +1805,12 @@ static void mt_release_contacts(struct hid_device *hid) > > > > > > > > > > list_for_each_entry(application, &td->applications, list) { > > > > > application->num_received = 0; > > > > > + if (application->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > + application->touch_change = false; > > > > > + mt_clear_slot_states(application->mt_slot_state, > > > > > + td->maxcontacts); > > > > > + application->repeated_counter = 0; > > > > > + } > > > > > } > > > > > } > > > > > > > > > > @@ -2014,6 +2149,15 @@ static const struct hid_device_id mt_devices[] = { > > > > > MT_USB_DEVICE(USB_VENDOR_ID_ILITEK, > > > > > USB_DEVICE_ID_ILITEK_MULTITOUCH) }, > > > > > > > > > > + /* Ilitek 106E/F touchscreen */ > > > > > + { .driver_data = MT_CLS_ILITEK_016x, > > > > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > > > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016E) }, > > > > > + > > > > > + { .driver_data = MT_CLS_ILITEK_016x, > > > > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > > > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016F) }, > > > > > + > > > > > /* LG Melfas panel */ > > > > > { .driver_data = MT_CLS_LG, > > > > > HID_USB_DEVICE(USB_VENDOR_ID_LG, > > > > > -- > > > > > 2.35.1.1021.g381101b075-goog > > > > > > > > > > > > > > > Cheers, > > > Benjamin > > > > > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT 2022-04-08 9:48 ` Angela Czubak @ 2022-04-21 9:31 ` Benjamin Tissoires 2022-04-21 11:30 ` Angela Czubak 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Tissoires @ 2022-04-21 9:31 UTC (permalink / raw) To: Angela Czubak; +Cc: open list:HID CORE LAYER, Jiri Kosina, upstream Hi Angela, [sorry for the delay here, I wanted to think about it twice and switched to another task in the end and forgot to answer] On Fri, Apr 8, 2022 at 11:48 AM Angela Czubak <acz@semihalf.com> wrote: > > Hi Benjamin, > > On Wed, Apr 6, 2022 at 3:35 PM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > On Wed, Apr 6, 2022 at 2:19 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > . > > > > > > On Tue, Apr 5, 2022 at 2:57 PM Benjamin Tissoires > > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > Hi Angela, > > > > > > > > On Tue, Apr 5, 2022 at 1:25 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > > > > > Hi Benjamin, Hi Jiri, > > > > > > > > > > any thoughts on this patch? > > > > > > > > Sorry for taking too long to answer. I saw the patch coming in while > > > > coming back last week, and could not process it, and completely forgot > > > > about it. > > > > > > > > > If you believe that the userspace input stack should be a better place > > > > > to solve this kind of issue, please let me know. > > > > > > > > This is definitely not userspace to process those specificities, as > > > > far as I understand. However... I don't understand the bug very well > > > > :/ > > > > > > > > more inlined below: > > > > > > > > > > > > > > Regards, > > > > > Angela > > > > > > > > > > On Thu, Mar 31, 2022 at 4:45 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > > > > > > > Ilitek touchscreens 016E and 016F repeat last finger position after it has > > > > > > left the surface of the device. > > > > > > We send BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1 with the first repeated > > > > > > slot as this may mean that the finger has been lifted. > > > > > > The touchscreen is not actually too sensitive and this would > > > > > > modify the behaviour of the on-screen keyboard, for instance holding the > > > > > > backspace key might be sometimes interpreted as removing a single character > > > > > > only. Add counter of repeated frames and if it is significantly big then > > > > > > assume the finger actually remains stationary. > > > > > > > > This commit is really confusing to me. This description is mixing > > > > symptoms from before the patch, behaviour from after and userspace > > > > which doesn't mean a lot here not knowing what is the problem. > > > > > > > > What is the exact problem? > > > > > > The device delays reporting finger lift off, instead it repeats its > > > last report with only the scantime updated. > > > I would interpret such events as meaning that the finger is present > > > but has stopped moving, which > > > is not the case, actually. > > > > > > When it comes to ChromeOS userspace input stack it seems that those > > > events are actually ignored, but the > > > problem is that the final event, i.e. the finger lit off is reported > > > significantly late, so if the user is moving > > > their finger to scroll the contents of the screen it immediately stops > > > once the finger leaves the surface. > > > However, the desired behaviour is for the scroll to continue, i.e. > > > they should observe something called fling. > > > The problem is that since this last event is delayed the scrolling stops. > > > Because the last event is so late, our window, over which we calculate > > > scroll speed, consists of just one > > > single event, i.e. we cannot calculate any movement velocity so we > > > assume it is zero. > > > I tried to increase the window, but it appears that the delay is large > > > enough for a human eye to notice that > > > the scrolling stops (as there are no new events for a while) and then > > > it restarts again (once the actual > > > lift off event is noticed, BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1). > > > > > > I find it more difficult to solve at userspace level and we would like > > > the fling (i.e. scrolling that continues > > > after the finger has left the surface) to be fluent. > > > > OK, thanks for the explanation. There are still a few bits that are > > confusing to me: > > > > > > > > > What events the device is sending? > > > > > > "Raw" events: > > > > > > https://gist.github.com/semihalf-czubak-angela/026072c013b4f883cb6adb7460b4d6ca > > > > > > So as you can see it repeats the last position etc. and the only thing > > > changing is the scantime. > > > > > > This translates to input events (not that the data below is from > > > another run, but you get the idea): > > > > > > https://gist.github.com/semihalf-czubak-angela/b957f7e464772bbdd95ddd814e84e5d9 > > > > But in both of these records, the last finger is reported 5 times, which > > corresponds to the value of MT_MAX_REPEATED_DROPPED. > > Given that the device sends one report every 20 millisecs, you end up at > > the exact same 0.1 seconds of delay, which makes me wonder about the > > usefulness of the patch. > > > > Well, let me rephrase then: this 0.1 is unfortunately very noticeable > to a human eye. > I know it shouldn't be much, but it actually causes the fling to stop, > whereas a smooth behaviour/movement > is expected. > And with the patch the lift of is actually reported with the first > repeated slot, it is just that after > MT_MAX_REPEATED_DROPPED the kernel actually starts sending the > slot/touch information again. > > > > > > > > What should be the expected kernel ouput? > > > > > > > > > > Well, ideally: > > > https://gist.github.com/semihalf-czubak-angela/4dd52c177372f8f0c2b4e5ada841ea95 > > > > > > So what helps is if > > > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 53 > > > (ABS_MT_POSITION_X), value 4817 > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 54 > > > (ABS_MT_POSITION_Y), value 777 > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 0 (ABS_X), value 4817 > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 1 (ABS_Y), value 777 > > > Event: time 1643827301.463888, type 4 (EV_MSC), code 5 > > > (MSC_TIMESTAMP), value 408000 > > > Event: time 1643827301.463888, -------------- SYN_REPORT ------------ > > > Event: time 1643827301.479920, type 4 (EV_MSC), code 5 > > > (MSC_TIMESTAMP), value 416000 > > > Event: time 1643827301.479920, -------------- SYN_REPORT ------------ > > > Event: time 1643827301.496921, type 4 (EV_MSC), code 5 > > > (MSC_TIMESTAMP), value 424000 > > > Event: time 1643827301.496921, -------------- SYN_REPORT ------------ > > > Event: time 1643827301.517923, type 4 (EV_MSC), code 5 > > > (MSC_TIMESTAMP), value 432000 > > > Event: time 1643827301.517923, -------------- SYN_REPORT ------------ > > > Event: time 1643827301.535920, type 4 (EV_MSC), code 5 > > > (MSC_TIMESTAMP), value 440000 > > > Event: time 1643827301.535920, -------------- SYN_REPORT ------------ > > > Event: time 1643827301.554943, type 3 (EV_ABS), code 57 > > > (ABS_MT_TRACKING_ID), value -1 > > > Event: time 1643827301.554943, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0 > > > Event: time 1643827301.554943, type 4 (EV_MSC), code 5 > > > (MSC_TIMESTAMP), value 448000 > > > Event: time 1643827301.554943, -------------- SYN_REPORT ------------ > > > > > > becomes > > > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 53 > > > (ABS_MT_POSITION_X), value 4817 > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 54 > > > (ABS_MT_POSITION_Y), value 777 > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 0 (ABS_X), value 4817 > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 1 (ABS_Y), value 777 > > > Event: time 1643827301.463888, type 4 (EV_MSC), code 5 > > > (MSC_TIMESTAMP), value 408000 > > > Event: time 1643827301.463888, -------------- SYN_REPORT ------------ > > > Event: time 1643827301.479920, type 3 (EV_ABS), code 57 > > > (ABS_MT_TRACKING_ID), value -1 > > > Event: time 1643827301.479920, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0 > > > Event: time 1643827301.479920, type 4 (EV_MSC), code 5 > > > (MSC_TIMESTAMP), value 416000 > > > Event: time 1643827301.479920, -------------- SYN_REPORT ------------ > > > > Well, here, the timestamp says that you want the touch to come as soon > > as there are 2 identical values, because it comes right after the last > > modified event. This is doable, but that also means you might create > > false releases while processing the events. > > > > I am aware of that, hence I want to assume that if this repeats enough > times the finger is actually stationary. > Ideally I would like to have the fix in the firmware, but a patch in > linux kernel seems easier for me :) > > > > > > > > > > > (bonus point for actually giving the events in the hid-recorder output > > > > format [0] ;-P ) > > > > > > > > > > Here you are: https://gist.github.com/semihalf-czubak-angela/eaaf73459c5c50f716016839fc368ab0 > > > > Heh, thanks :) > > > > > > > > > FWIW, this driver is one of the few drivers in the HID stack to have > > > > extended tests in hid-tools[0]. > > > > I plan to merge that repo into the selftests subtree, but for now we > > > > need to use this external repo. > > > > > > > > So I'd be pleased to see new tests added for that quirk because it > > > > seems far from evident what is happening. > > > > > > > > > > Ack, I will prepare something. > > > > > > > > > > > > > > > Signed-off-by: Angela Czubak <acz@semihalf.com> > > > > > > --- > > > > > > > > > > > > Hi guys, > > > > > > > > > > > > Please let me know if you think there is some better solution. > > > > > > It felt to me that it would be more correct to solve it on kernel > > > > > > driver level since this issue is device specific, but perhaps > > > > > > there are some userspace input stacks that figured this problem out. > > > > > > With Chromium OS Input Stack the final BTN_TOUCH=0 event comes > > > > > > significantly late which causes hiccups or simply a scrolling fling > > > > > > not to happen, since the input stack thinks the finger is still > > > > > > in the same position and on the screen. > > > > > > > > If the device reports a wrong state, the kernel is the place to fix > > > > it. However, as mentioned above I do not understand what is wrong > > > > there by reading the description and the code. > > > > > > > > > > > > > > > > drivers/hid/hid-ids.h | 2 + > > > > > > drivers/hid/hid-multitouch.c | 150 ++++++++++++++++++++++++++++++++++- > > > > > > 2 files changed, 149 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > > > > > index 053853a891c5..cc85f547603c 100644 > > > > > > --- a/drivers/hid/hid-ids.h > > > > > > +++ b/drivers/hid/hid-ids.h > > > > > > @@ -630,6 +630,8 @@ > > > > > > > > > > > > #define USB_VENDOR_ID_ILITEK 0x222a > > > > > > #define USB_DEVICE_ID_ILITEK_MULTITOUCH 0x0001 > > > > > > +#define USB_DEVICE_ID_ILITEK_016E 0x016e > > > > > > +#define USB_DEVICE_ID_ILITEK_016F 0x016f > > > > > > > > > > > > #define USB_VENDOR_ID_INTEL_0 0x8086 > > > > > > #define USB_VENDOR_ID_INTEL_1 0x8087 > > > > > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > > > > > > index 99eabfb4145b..33cb5e5179c2 100644 > > > > > > --- a/drivers/hid/hid-multitouch.c > > > > > > +++ b/drivers/hid/hid-multitouch.c > > > > > > @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); > > > > > > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) > > > > > > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) > > > > > > #define MT_QUIRK_DISABLE_WAKEUP BIT(21) > > > > > > +#define MT_QUIRK_DROP_REPEATED_SLOT BIT(22) > > > > > > > > > > > > #define MT_INPUTMODE_TOUCHSCREEN 0x02 > > > > > > #define MT_INPUTMODE_TOUCHPAD 0x03 > > > > > > @@ -103,12 +104,22 @@ struct mt_usages { > > > > > > bool *confidence_state; /* is the touch made by a finger? */ > > > > > > }; > > > > > > > > > > > > +struct mt_slot_state { > > > > > > + __s32 x, y, cx, cy, p, w, h, a; > > > > > > + __s32 contactid; /* the device ContactID assigned to this slot */ > > > > > > + bool tip_state; /* is the touch valid? */ > > > > > > + bool inrange_state; /* is the finger in proximity of the sensor? */ > > > > > > + bool confidence_state; /* is the touch made by a finger? */ > > > > > > +}; > > > > > > > > This raises a red flag here. I spent quite some time removing internal > > > > slot states, and re-adding them means that something is wrong IMO. > > > > (though if we can not do something else, we might just re-add them). > > > > > > > > > > I wanted to compare against the report contents excluding the scantime. > > > Perhaps I could do that on the hid report level, this way just seemed > > > easier to implement. > > > > You can also use input_mt_get_value() and compare with the current value > > before sending the value to the input stack. This way you do not need to > > store the values once again. > > Thanks for suggesting that! > > > > > > > > > > > > + > > > > > > struct mt_application { > > > > > > struct list_head list; > > > > > > unsigned int application; > > > > > > unsigned int report_id; > > > > > > struct list_head mt_usages; /* mt usages list */ > > > > > > > > > > > > + struct mt_slot_state *mt_slot_state; > > > > > > + > > > > > > __s32 quirks; > > > > > > > > > > > > __s32 *scantime; /* scantime reported */ > > > > > > @@ -116,6 +127,11 @@ struct mt_application { > > > > > > > > > > > > __s32 *raw_cc; /* contact count in the report */ > > > > > > int left_button_state; /* left button state */ > > > > > > + bool touch_change; /* if touch change detected */ > > > > > > + bool touched; /* if touch was present in the last > > > > > > + * message > > > > > > + */ > > > > > > > > So this is not per-touch, but per report? > > > > > > > > > > I suppose it is actually per report, it is just that implementation > > > per slot seemed easier. > > > I could not reproduce the issue when using two fingers. > > > > Which makes a lot of sense: when you still have one finger on the > > screen, the sensitive sensors are way much capable of detecting ghosts > > because they have a baseline to compare too. > > So I would suggest you enable that code only when you have 1 finger left > > and that this finger is still. > > > > > > > > > > > + int repeated_counter; /* how many times last frame repeated */ > > > > > > unsigned int mt_flags; /* flags to pass to input-mt */ > > > > > > > > > > > > unsigned long *pending_palm_slots; /* slots where we reported palm > > > > > > @@ -211,6 +227,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); > > > > > > #define MT_CLS_GOOGLE 0x0111 > > > > > > #define MT_CLS_RAZER_BLADE_STEALTH 0x0112 > > > > > > #define MT_CLS_SMART_TECH 0x0113 > > > > > > +#define MT_CLS_ILITEK_016x 0x0114 > > > > > > > > > > > > #define MT_DEFAULT_MAXCONTACT 10 > > > > > > #define MT_MAX_MAXCONTACT 250 > > > > > > @@ -386,6 +403,15 @@ static const struct mt_class mt_classes[] = { > > > > > > MT_QUIRK_CONTACT_CNT_ACCURATE | > > > > > > MT_QUIRK_SEPARATE_APP_REPORT, > > > > > > }, > > > > > > + { .name = MT_CLS_ILITEK_016x, > > > > > > + .quirks = MT_QUIRK_ALWAYS_VALID | > > > > > > + MT_QUIRK_IGNORE_DUPLICATES | > > > > > > + MT_QUIRK_HOVERING | > > > > > > + MT_QUIRK_CONTACT_CNT_ACCURATE | > > > > > > + MT_QUIRK_STICKY_FINGERS | > > > > > > + MT_QUIRK_WIN8_PTP_BUTTONS | > > > > > > + MT_QUIRK_DROP_REPEATED_SLOT, > > > > > > + .export_all_inputs = true }, > > > > > > { } > > > > > > }; > > > > > > > > > > > > @@ -783,7 +809,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, > > > > > > case HID_DG_CONFIDENCE: > > > > > > if ((cls->name == MT_CLS_WIN_8 || > > > > > > cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT || > > > > > > - cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP) && > > > > > > + cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP || > > > > > > + cls->name == MT_CLS_ILITEK_016x) && > > > > > > > > Not something you should care about, but this long test of spaghetti > > > > should likely be fixed into something way better at some point... > > > > > > > > > > (field->application == HID_DG_TOUCHPAD || > > > > > > field->application == HID_DG_TOUCHSCREEN)) > > > > > > app->quirks |= MT_QUIRK_CONFIDENCE; > > > > > > @@ -948,7 +975,13 @@ static void mt_sync_frame(struct mt_device *td, struct mt_application *app, > > > > > > input_event(input, EV_KEY, BTN_LEFT, app->left_button_state); > > > > > > > > > > > > input_mt_sync_frame(input); > > > > > > - input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > > + if (app->touch_change) > > > > > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > > > + app->touch_change = false; > > > > > > > > Why filter out the timestamp? > > > > > > > > > > I suppose lonely timestamps should not do any harm, I just wanted to > > > avoid evtest etc. output > > > where there is no change but the timestamp, it is kind of also > > > remainder of my first approach where > > > I just tried filtering this events out and see waiting for BTN_TOUCH=0 > > > and ABS_MT_TRACKING_ID=-1 > > > would be enough (it wasn't). > > > > So I wouldn't filter out the timestamps. Not sure how ChromeOS stack > > behaves when we forward timestamps without a touch, but it should be > > capable of ignoring them. > > Ack. > > > > > > > > > > > > + } else { > > > > > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > > > + } > > > > > > input_sync(input); > > > > > > > > > > > > mt_release_pending_palms(td, app, input); > > > > > > @@ -993,6 +1026,60 @@ static int mt_touch_event(struct hid_device *hid, struct hid_field *field, > > > > > > return 1; > > > > > > } > > > > > > > > > > > > +static void mt_clear_slot_states(struct mt_slot_state *mt_slot_state, > > > > > > + int size) > > > > > > +{ > > > > > > + int i; > > > > > > + > > > > > > + for (i = 0; i < size; i++) > > > > > > + mt_slot_state[i].tip_state = 0; > > > > > > +} > > > > > > + > > > > > > +static void mt_fill_slot_state(struct mt_slot_state *mt_slot_state, > > > > > > + struct mt_usages *slot) > > > > > > +{ > > > > > > + mt_slot_state->x = *slot->x; > > > > > > + mt_slot_state->y = *slot->y; > > > > > > + mt_slot_state->cx = *slot->cx; > > > > > > + mt_slot_state->cy = *slot->cy; > > > > > > + mt_slot_state->p = *slot->p; > > > > > > + mt_slot_state->w = *slot->w; > > > > > > + mt_slot_state->h = *slot->h; > > > > > > + mt_slot_state->a = *slot->a; > > > > > > + mt_slot_state->contactid = *slot->contactid; > > > > > > + mt_slot_state->tip_state = *slot->tip_state; > > > > > > + mt_slot_state->inrange_state = *slot->inrange_state; > > > > > > + mt_slot_state->confidence_state = *slot->confidence_state; > > > > > > +} > > > > > > + > > > > > > +static bool mt_is_slot_state_equal(struct mt_slot_state *state1, > > > > > > + struct mt_slot_state *state2) > > > > > > +{ > > > > > > + if (state1->x != state2->x) > > > > > > + return false; > > > > > > + if (state1->y != state2->y) > > > > > > + return false; > > > > > > + if (state1->cx != state2->cx) > > > > > > + return false; > > > > > > + if (state1->cy != state2->cy) > > > > > > + return false; > > > > > > + if (state1->p != state2->p) > > > > > > + return false; > > > > > > + if (state1->w != state2->w) > > > > > > + return false; > > > > > > + if (state1->a != state2->a) > > > > > > + return false; > > > > > > + if (state1->contactid != state2->contactid) > > > > > > + return false; > > > > > > + if (state1->tip_state != state2->tip_state) > > > > > > + return false; > > > > > > + if (state1->inrange_state != state2->inrange_state) > > > > > > + return false; > > > > > > + if (state1->confidence_state != state2->confidence_state) > > > > > > + return false; > > > > > > + return true; > > > > > > +} > > > > > > > > The input stack already filters out duplicated events. So I am not > > > > sure we need to store the information here once again and test for it > > > > here. > > > > > > > > > > What I want to do is to "prematurely" report that the finger has been lifted. > > > It is not that much about solely filtering out the duplicated events, > > > I rather want to interpret > > > such an event as a finger lift off. > > > > Yes, but see my remark above. Given that the input stack already caches > > the values and that it is validated by the evdev recording you shared, > > you should be able to apply the quirk just before calling the various > > input_event() and comparing the current value of this last finger with > > the ones in the input stack. This way, you do not need to keep tabs on > > the various fingers. > > > > > > > > > > > + > > > > > > static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > > > struct mt_application *app, > > > > > > struct mt_usages *slot) > > > > > > @@ -1005,6 +1092,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > > > int active; > > > > > > int slotnum; > > > > > > int tool = MT_TOOL_FINGER; > > > > > > + struct mt_slot_state state = {0}; > > > > > > > > > > > > if (!slot) > > > > > > return -EINVAL; > > > > > > @@ -1058,13 +1146,42 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > > > * lift-off as userspace will not be aware > > > > > > * of non-confidence, so we need to split > > > > > > * it into 2 events: active MT_TOOL_PALM > > > > > > - * and a separate liftoff. > > > > > > + * and a separate lift off. > > > > > > */ > > > > > > active = true; > > > > > > set_bit(slotnum, app->pending_palm_slots); > > > > > > } > > > > > > } > > > > > > > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > > + mt_fill_slot_state(&state, slot); > > > > > > + /* Check if every field in the slot is the same as before. > > > > > > + * Some touchscreens report the same position for several > > > > > > + * frames even though the finger is no longer on the surface. > > > > > > + * This is actually being recognised as the finger remaining in > > > > > > + * one position, which causes scrolling to be stopped. > > > > > > + * Ignore such packets so that the scrolling continues > > > > > > + * and the touchscreen reports the finger liftoff. > > > > > > > > That part I understand better, but how is the device reporting such events? > > > > If the touch is no longer there, we should have a bit that says it is > > > > lifted, so we should simply just ignore the touch, no? > > > > > > > > > > Well, I would actually like the touch panel manufacturers to answer that :-) > > > Based on some comments in our source code it seems that some devices > > > might delay synthesis of lift-off to reduce risks of noisy release, but I am not > > > sure if it explains why they produce events in between. > > > > They produce events in between because they have to, or they are not > > following the spec :) > > I also think the main reason is that if they forget about sending an > > event *and* that the touch continues to move after a few events, they > > will see a release happening on the touch because that's what > > STICKY_FINGER does (it's a copy of the behaviour from Windows FWIW). > > > > > > > > > > > > > > > + */ > > > > > > + if (!mt_is_slot_state_equal(&state, > > > > > > + &app->mt_slot_state[slotnum])) { > > > > > > + app->touch_change |= (active || > > > > > > + app->mt_slot_state[slotnum].tip_state); > > > > > > + app->repeated_counter = 0; > > > > > > + } else { > > > > > > +#define MT_MAX_REPEATED_DROPPED 5 > > > > > > + if (app->repeated_counter < MT_MAX_REPEATED_DROPPED) { > > > > > > + active = false; > > > > > > + if (app->touched) > > > > > > + app->touch_change = true; > > > > > > + } else { > > > > > > + app->touch_change = true; > > > > > > + } > > > > > > + app->repeated_counter++; > > > > > > > > This is specific to one device, but I have seen device filtering in > > > > hardware that reports the same steady data as long as the touch is > > > > present. So 5 reports with the same data seem quite short. > > > > > > > > > > It can be a larger number, I just believe that in this case it was enough. > > > > All 3 (or 4 maybe) recordings you shared showed that we received only 5 > > duplications of events. So there is a case to be made around whether we > > need this patch. > > > > What would be interesting is whether you can "emulate" that situation > > without releasing the finger. If the raw data is noisy (to some extent > > of course) and it's hard to have exactly twice the same sample for a > > touch, then maybe we can assume that as soon as we get identical values > > we can release. Maybe the false positive in that situation would be > > better handled by the user that missing the fling. > > > > Actually, I am able to receive the same report if I am very still and > try not to move my finger. > I wouldn't say it is easy but it is definitely possible and hence i > wanted to use this upper bound > so that using an on screen keyboard is not annoying (i.e. I want to be > able to hold backspace key > and erase more than just one sign). Unfortunately, this is a deal breaker for me. You are trying to solve 2 userspace issues in the kernel, where we don't have context. So whatever you are choosing to implement, you are breaking one use case. The fact that nothing separates the output between "touch is holding still" and "touch released" means that the kernel will have to assume one or the other, and this will likely break other userspace applications. I am sorry but I do not see how to fix that in the kernel: - for the backspace key, this is a userspace issue. If the finger is holding still on a key, some autorepeat needs to be emulated and the kernel has nothing to say here - for the fling, well, sorry I don't have a good solution but you'll probably have to quirk the device in userspace in a similar manner that you are doing in kernel space: in pseudo code: If fling is started and the device is this bogus one and if there is only one finger and the event consists in just one timestamp without changes in X/Y, assume release and filter out any further events for this slot. This way, you have the context (we started scrolling on a page), and you are not stepping on a drawing application toes for instance. Cheers, Benjamin > > > > > > > > Also, we do have devices which tend to forget to release slots, and > > > > that's why we have MT_QUIRK_STICKY_FINGERS which release touches after > > > > a while. > > > > > > Yeah, but the thing is that this specific device actually reports that > > > the finger is still there, > > > even though the finger has been lifted :-) > > > I believe that MT_QUIRK_STICKY_FINGERS works when the device somehow > > > fails to send a new report > > > within some time after the previous one, so it does not solve the issue for us. > > > > It's not exactly the same case, but it still feels similar enough to > > maybe be reused. > > > > I am a little bit lost; MT_CLS_ILITEK_016x contains MT_QUIRK_STICKY_FINGERS. > Based on the logs I provided, do you think that it does not work as expected? > > > Can you please give a shot at the following patch (and change the value > > of MT_MAX_REPEATED_DROPPED between 1 to 5)? > > It doesn't use MT_QUIRK_STICKY_FINGERS but should be simnilar to what > > you have here in a shorter version. > > > > --- > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > > index 0dece608c023..cff4088e564a 100644 > > --- a/drivers/hid/hid-multitouch.c > > +++ b/drivers/hid/hid-multitouch.c > > @@ -77,6 +77,8 @@ MODULE_LICENSE("GPL"); > > > > #define MT_BUTTONTYPE_CLICKPAD 0 > > > > +#define MT_MAX_REPEATED_DROPPED 3 > > + > > enum latency_mode { > > HID_LATENCY_NORMAL = 0, > > HID_LATENCY_HIGH = 1, > > @@ -128,6 +130,7 @@ struct mt_application { > > * 1 means we should use a serial protocol > > * > 1 means hybrid (multitouch) protocol > > */ > > + unsigned int repeated_touches; /* used when MT_QUIRK_DROP_REPEATED_SLOT */ > > > > __s32 dev_time; /* the scan time provided by the device */ > > unsigned long jiffies; /* the frame's jiffies */ > > @@ -1065,6 +1068,19 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > } > > } > > > > + /* TODO: add test for the new quirk so we don't apply this for all devices */ > > + if (active && app->num_expected == 1) { > > + struct input_mt_slot *i_slot = &mt->slots[slotnum]; > > + > > + if (input_mt_get_value(i_slot, ABS_MT_POSITION_X) == *slot->x && > > + input_mt_get_value(i_slot, ABS_MT_POSITION_Y) == *slot->y) { > > + if (++app->repeated_touches >= MT_MAX_REPEATED_DROPPED) > > + active = false; > > + } else { > > + app->repeated_touches = 0; > > + } > > + } > > + > > input_mt_slot(input, slotnum); > > input_mt_report_slot_state(input, tool, active); > > if (active) { > > --- > > > > Observed results: > MT_MAX_REPEATED_DROPPED = 1: It solves this issue, no hiccup visible, > but continuing to press a key on the screen > does not produce the right effect, for instance backspace erases a > single character only > MT_MAX_REPEATED_DROPPED = 2: Same as above > MT_MAX_REPEATED_DROPPED = 3: Small hiccup noticeable, problems with > backspace as above > MT_MAX_REPEATED_DROPPED = 4: Small hiccup noticeable, problems with > backspace as above > MT_MAX_REPEATED_DROPPED = 5: Fling does not work (due to 0.1s delay > and UI constants assuming that it is too long > time to still calculate velocity) + problems with backspace > > > > > > Cheers, > > Benjamin > > > > > > > > > > > > > > > > > > > > > > > > + } > > > > > > + app->touched = active; > > > > > > + app->mt_slot_state[slotnum] = state; > > > > > > + } > > > > > > input_mt_slot(input, slotnum); > > > > > > input_mt_report_slot_state(input, tool, active); > > > > > > if (active) { > > > > > > @@ -1296,6 +1413,18 @@ static int mt_touch_input_configured(struct hid_device *hdev, > > > > > > if (!app->pending_palm_slots) > > > > > > return -ENOMEM; > > > > > > > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > > + app->mt_slot_state = devm_kcalloc(&hi->input->dev, > > > > > > + td->maxcontacts, > > > > > > + sizeof(*app->mt_slot_state), > > > > > > + GFP_KERNEL); > > > > > > + if (!app->mt_slot_state) > > > > > > + return -ENOMEM; > > > > > > + mt_clear_slot_states(app->mt_slot_state, td->maxcontacts); > > > > > > + app->repeated_counter = 0; > > > > > > + } > > > > > > + > > > > > > + > > > > > > ret = input_mt_init_slots(input, td->maxcontacts, app->mt_flags); > > > > > > if (ret) > > > > > > return ret; > > > > > > @@ -1676,6 +1805,12 @@ static void mt_release_contacts(struct hid_device *hid) > > > > > > > > > > > > list_for_each_entry(application, &td->applications, list) { > > > > > > application->num_received = 0; > > > > > > + if (application->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > > + application->touch_change = false; > > > > > > + mt_clear_slot_states(application->mt_slot_state, > > > > > > + td->maxcontacts); > > > > > > + application->repeated_counter = 0; > > > > > > + } > > > > > > } > > > > > > } > > > > > > > > > > > > @@ -2014,6 +2149,15 @@ static const struct hid_device_id mt_devices[] = { > > > > > > MT_USB_DEVICE(USB_VENDOR_ID_ILITEK, > > > > > > USB_DEVICE_ID_ILITEK_MULTITOUCH) }, > > > > > > > > > > > > + /* Ilitek 106E/F touchscreen */ > > > > > > + { .driver_data = MT_CLS_ILITEK_016x, > > > > > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > > > > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016E) }, > > > > > > + > > > > > > + { .driver_data = MT_CLS_ILITEK_016x, > > > > > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > > > > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016F) }, > > > > > > + > > > > > > /* LG Melfas panel */ > > > > > > { .driver_data = MT_CLS_LG, > > > > > > HID_USB_DEVICE(USB_VENDOR_ID_LG, > > > > > > -- > > > > > > 2.35.1.1021.g381101b075-goog > > > > > > > > > > > > > > > > > > > Cheers, > > > > Benjamin > > > > > > > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT 2022-04-21 9:31 ` Benjamin Tissoires @ 2022-04-21 11:30 ` Angela Czubak 2022-04-22 8:55 ` Benjamin Tissoires 0 siblings, 1 reply; 9+ messages in thread From: Angela Czubak @ 2022-04-21 11:30 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: open list:HID CORE LAYER, Jiri Kosina, upstream On Thu, Apr 21, 2022 at 11:31 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi Angela, > > [sorry for the delay here, I wanted to think about it twice and > switched to another task in the end and forgot to answer] > > On Fri, Apr 8, 2022 at 11:48 AM Angela Czubak <acz@semihalf.com> wrote: > > > > Hi Benjamin, > > > > On Wed, Apr 6, 2022 at 3:35 PM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > > > On Wed, Apr 6, 2022 at 2:19 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > > > . > > > > > > > > On Tue, Apr 5, 2022 at 2:57 PM Benjamin Tissoires > > > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > Hi Angela, > > > > > > > > > > On Tue, Apr 5, 2022 at 1:25 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > > > > > > > Hi Benjamin, Hi Jiri, > > > > > > > > > > > > any thoughts on this patch? > > > > > > > > > > Sorry for taking too long to answer. I saw the patch coming in while > > > > > coming back last week, and could not process it, and completely forgot > > > > > about it. > > > > > > > > > > > If you believe that the userspace input stack should be a better place > > > > > > to solve this kind of issue, please let me know. > > > > > > > > > > This is definitely not userspace to process those specificities, as > > > > > far as I understand. However... I don't understand the bug very well > > > > > :/ > > > > > > > > > > more inlined below: > > > > > > > > > > > > > > > > > Regards, > > > > > > Angela > > > > > > > > > > > > On Thu, Mar 31, 2022 at 4:45 PM Angela Czubak <acz@semihalf.com> wrote: > > > > > > > > > > > > > > Ilitek touchscreens 016E and 016F repeat last finger position after it has > > > > > > > left the surface of the device. > > > > > > > We send BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1 with the first repeated > > > > > > > slot as this may mean that the finger has been lifted. > > > > > > > The touchscreen is not actually too sensitive and this would > > > > > > > modify the behaviour of the on-screen keyboard, for instance holding the > > > > > > > backspace key might be sometimes interpreted as removing a single character > > > > > > > only. Add counter of repeated frames and if it is significantly big then > > > > > > > assume the finger actually remains stationary. > > > > > > > > > > This commit is really confusing to me. This description is mixing > > > > > symptoms from before the patch, behaviour from after and userspace > > > > > which doesn't mean a lot here not knowing what is the problem. > > > > > > > > > > What is the exact problem? > > > > > > > > The device delays reporting finger lift off, instead it repeats its > > > > last report with only the scantime updated. > > > > I would interpret such events as meaning that the finger is present > > > > but has stopped moving, which > > > > is not the case, actually. > > > > > > > > When it comes to ChromeOS userspace input stack it seems that those > > > > events are actually ignored, but the > > > > problem is that the final event, i.e. the finger lit off is reported > > > > significantly late, so if the user is moving > > > > their finger to scroll the contents of the screen it immediately stops > > > > once the finger leaves the surface. > > > > However, the desired behaviour is for the scroll to continue, i.e. > > > > they should observe something called fling. > > > > The problem is that since this last event is delayed the scrolling stops. > > > > Because the last event is so late, our window, over which we calculate > > > > scroll speed, consists of just one > > > > single event, i.e. we cannot calculate any movement velocity so we > > > > assume it is zero. > > > > I tried to increase the window, but it appears that the delay is large > > > > enough for a human eye to notice that > > > > the scrolling stops (as there are no new events for a while) and then > > > > it restarts again (once the actual > > > > lift off event is noticed, BTN_TOUCH=0 and ABS_MT_TRACKING_ID=-1). > > > > > > > > I find it more difficult to solve at userspace level and we would like > > > > the fling (i.e. scrolling that continues > > > > after the finger has left the surface) to be fluent. > > > > > > OK, thanks for the explanation. There are still a few bits that are > > > confusing to me: > > > > > > > > > > > > What events the device is sending? > > > > > > > > "Raw" events: > > > > > > > > https://gist.github.com/semihalf-czubak-angela/026072c013b4f883cb6adb7460b4d6ca > > > > > > > > So as you can see it repeats the last position etc. and the only thing > > > > changing is the scantime. > > > > > > > > This translates to input events (not that the data below is from > > > > another run, but you get the idea): > > > > > > > > https://gist.github.com/semihalf-czubak-angela/b957f7e464772bbdd95ddd814e84e5d9 > > > > > > But in both of these records, the last finger is reported 5 times, which > > > corresponds to the value of MT_MAX_REPEATED_DROPPED. > > > Given that the device sends one report every 20 millisecs, you end up at > > > the exact same 0.1 seconds of delay, which makes me wonder about the > > > usefulness of the patch. > > > > > > > Well, let me rephrase then: this 0.1 is unfortunately very noticeable > > to a human eye. > > I know it shouldn't be much, but it actually causes the fling to stop, > > whereas a smooth behaviour/movement > > is expected. > > And with the patch the lift of is actually reported with the first > > repeated slot, it is just that after > > MT_MAX_REPEATED_DROPPED the kernel actually starts sending the > > slot/touch information again. > > > > > > > > > > > What should be the expected kernel ouput? > > > > > > > > > > > > > Well, ideally: > > > > https://gist.github.com/semihalf-czubak-angela/4dd52c177372f8f0c2b4e5ada841ea95 > > > > > > > > So what helps is if > > > > > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 53 > > > > (ABS_MT_POSITION_X), value 4817 > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 54 > > > > (ABS_MT_POSITION_Y), value 777 > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 0 (ABS_X), value 4817 > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 1 (ABS_Y), value 777 > > > > Event: time 1643827301.463888, type 4 (EV_MSC), code 5 > > > > (MSC_TIMESTAMP), value 408000 > > > > Event: time 1643827301.463888, -------------- SYN_REPORT ------------ > > > > Event: time 1643827301.479920, type 4 (EV_MSC), code 5 > > > > (MSC_TIMESTAMP), value 416000 > > > > Event: time 1643827301.479920, -------------- SYN_REPORT ------------ > > > > Event: time 1643827301.496921, type 4 (EV_MSC), code 5 > > > > (MSC_TIMESTAMP), value 424000 > > > > Event: time 1643827301.496921, -------------- SYN_REPORT ------------ > > > > Event: time 1643827301.517923, type 4 (EV_MSC), code 5 > > > > (MSC_TIMESTAMP), value 432000 > > > > Event: time 1643827301.517923, -------------- SYN_REPORT ------------ > > > > Event: time 1643827301.535920, type 4 (EV_MSC), code 5 > > > > (MSC_TIMESTAMP), value 440000 > > > > Event: time 1643827301.535920, -------------- SYN_REPORT ------------ > > > > Event: time 1643827301.554943, type 3 (EV_ABS), code 57 > > > > (ABS_MT_TRACKING_ID), value -1 > > > > Event: time 1643827301.554943, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0 > > > > Event: time 1643827301.554943, type 4 (EV_MSC), code 5 > > > > (MSC_TIMESTAMP), value 448000 > > > > Event: time 1643827301.554943, -------------- SYN_REPORT ------------ > > > > > > > > becomes > > > > > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 53 > > > > (ABS_MT_POSITION_X), value 4817 > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 54 > > > > (ABS_MT_POSITION_Y), value 777 > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 0 (ABS_X), value 4817 > > > > Event: time 1643827301.463888, type 3 (EV_ABS), code 1 (ABS_Y), value 777 > > > > Event: time 1643827301.463888, type 4 (EV_MSC), code 5 > > > > (MSC_TIMESTAMP), value 408000 > > > > Event: time 1643827301.463888, -------------- SYN_REPORT ------------ > > > > Event: time 1643827301.479920, type 3 (EV_ABS), code 57 > > > > (ABS_MT_TRACKING_ID), value -1 > > > > Event: time 1643827301.479920, type 1 (EV_KEY), code 330 (BTN_TOUCH), value 0 > > > > Event: time 1643827301.479920, type 4 (EV_MSC), code 5 > > > > (MSC_TIMESTAMP), value 416000 > > > > Event: time 1643827301.479920, -------------- SYN_REPORT ------------ > > > > > > Well, here, the timestamp says that you want the touch to come as soon > > > as there are 2 identical values, because it comes right after the last > > > modified event. This is doable, but that also means you might create > > > false releases while processing the events. > > > > > > > I am aware of that, hence I want to assume that if this repeats enough > > times the finger is actually stationary. > > Ideally I would like to have the fix in the firmware, but a patch in > > linux kernel seems easier for me :) > > > > > > > > > > > > > > > (bonus point for actually giving the events in the hid-recorder output > > > > > format [0] ;-P ) > > > > > > > > > > > > > Here you are: https://gist.github.com/semihalf-czubak-angela/eaaf73459c5c50f716016839fc368ab0 > > > > > > Heh, thanks :) > > > > > > > > > > > > FWIW, this driver is one of the few drivers in the HID stack to have > > > > > extended tests in hid-tools[0]. > > > > > I plan to merge that repo into the selftests subtree, but for now we > > > > > need to use this external repo. > > > > > > > > > > So I'd be pleased to see new tests added for that quirk because it > > > > > seems far from evident what is happening. > > > > > > > > > > > > > Ack, I will prepare something. > > > > > > > > > > > > > > > > > > Signed-off-by: Angela Czubak <acz@semihalf.com> > > > > > > > --- > > > > > > > > > > > > > > Hi guys, > > > > > > > > > > > > > > Please let me know if you think there is some better solution. > > > > > > > It felt to me that it would be more correct to solve it on kernel > > > > > > > driver level since this issue is device specific, but perhaps > > > > > > > there are some userspace input stacks that figured this problem out. > > > > > > > With Chromium OS Input Stack the final BTN_TOUCH=0 event comes > > > > > > > significantly late which causes hiccups or simply a scrolling fling > > > > > > > not to happen, since the input stack thinks the finger is still > > > > > > > in the same position and on the screen. > > > > > > > > > > If the device reports a wrong state, the kernel is the place to fix > > > > > it. However, as mentioned above I do not understand what is wrong > > > > > there by reading the description and the code. > > > > > > > > > > > > > > > > > > > drivers/hid/hid-ids.h | 2 + > > > > > > > drivers/hid/hid-multitouch.c | 150 ++++++++++++++++++++++++++++++++++- > > > > > > > 2 files changed, 149 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > > > > > > index 053853a891c5..cc85f547603c 100644 > > > > > > > --- a/drivers/hid/hid-ids.h > > > > > > > +++ b/drivers/hid/hid-ids.h > > > > > > > @@ -630,6 +630,8 @@ > > > > > > > > > > > > > > #define USB_VENDOR_ID_ILITEK 0x222a > > > > > > > #define USB_DEVICE_ID_ILITEK_MULTITOUCH 0x0001 > > > > > > > +#define USB_DEVICE_ID_ILITEK_016E 0x016e > > > > > > > +#define USB_DEVICE_ID_ILITEK_016F 0x016f > > > > > > > > > > > > > > #define USB_VENDOR_ID_INTEL_0 0x8086 > > > > > > > #define USB_VENDOR_ID_INTEL_1 0x8087 > > > > > > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > > > > > > > index 99eabfb4145b..33cb5e5179c2 100644 > > > > > > > --- a/drivers/hid/hid-multitouch.c > > > > > > > +++ b/drivers/hid/hid-multitouch.c > > > > > > > @@ -71,6 +71,7 @@ MODULE_LICENSE("GPL"); > > > > > > > #define MT_QUIRK_SEPARATE_APP_REPORT BIT(19) > > > > > > > #define MT_QUIRK_FORCE_MULTI_INPUT BIT(20) > > > > > > > #define MT_QUIRK_DISABLE_WAKEUP BIT(21) > > > > > > > +#define MT_QUIRK_DROP_REPEATED_SLOT BIT(22) > > > > > > > > > > > > > > #define MT_INPUTMODE_TOUCHSCREEN 0x02 > > > > > > > #define MT_INPUTMODE_TOUCHPAD 0x03 > > > > > > > @@ -103,12 +104,22 @@ struct mt_usages { > > > > > > > bool *confidence_state; /* is the touch made by a finger? */ > > > > > > > }; > > > > > > > > > > > > > > +struct mt_slot_state { > > > > > > > + __s32 x, y, cx, cy, p, w, h, a; > > > > > > > + __s32 contactid; /* the device ContactID assigned to this slot */ > > > > > > > + bool tip_state; /* is the touch valid? */ > > > > > > > + bool inrange_state; /* is the finger in proximity of the sensor? */ > > > > > > > + bool confidence_state; /* is the touch made by a finger? */ > > > > > > > +}; > > > > > > > > > > This raises a red flag here. I spent quite some time removing internal > > > > > slot states, and re-adding them means that something is wrong IMO. > > > > > (though if we can not do something else, we might just re-add them). > > > > > > > > > > > > > I wanted to compare against the report contents excluding the scantime. > > > > Perhaps I could do that on the hid report level, this way just seemed > > > > easier to implement. > > > > > > You can also use input_mt_get_value() and compare with the current value > > > before sending the value to the input stack. This way you do not need to > > > store the values once again. > > > > Thanks for suggesting that! > > > > > > > > > > > > > > > > + > > > > > > > struct mt_application { > > > > > > > struct list_head list; > > > > > > > unsigned int application; > > > > > > > unsigned int report_id; > > > > > > > struct list_head mt_usages; /* mt usages list */ > > > > > > > > > > > > > > + struct mt_slot_state *mt_slot_state; > > > > > > > + > > > > > > > __s32 quirks; > > > > > > > > > > > > > > __s32 *scantime; /* scantime reported */ > > > > > > > @@ -116,6 +127,11 @@ struct mt_application { > > > > > > > > > > > > > > __s32 *raw_cc; /* contact count in the report */ > > > > > > > int left_button_state; /* left button state */ > > > > > > > + bool touch_change; /* if touch change detected */ > > > > > > > + bool touched; /* if touch was present in the last > > > > > > > + * message > > > > > > > + */ > > > > > > > > > > So this is not per-touch, but per report? > > > > > > > > > > > > > I suppose it is actually per report, it is just that implementation > > > > per slot seemed easier. > > > > I could not reproduce the issue when using two fingers. > > > > > > Which makes a lot of sense: when you still have one finger on the > > > screen, the sensitive sensors are way much capable of detecting ghosts > > > because they have a baseline to compare too. > > > So I would suggest you enable that code only when you have 1 finger left > > > and that this finger is still. > > > > > > > > > > > > > > + int repeated_counter; /* how many times last frame repeated */ > > > > > > > unsigned int mt_flags; /* flags to pass to input-mt */ > > > > > > > > > > > > > > unsigned long *pending_palm_slots; /* slots where we reported palm > > > > > > > @@ -211,6 +227,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app); > > > > > > > #define MT_CLS_GOOGLE 0x0111 > > > > > > > #define MT_CLS_RAZER_BLADE_STEALTH 0x0112 > > > > > > > #define MT_CLS_SMART_TECH 0x0113 > > > > > > > +#define MT_CLS_ILITEK_016x 0x0114 > > > > > > > > > > > > > > #define MT_DEFAULT_MAXCONTACT 10 > > > > > > > #define MT_MAX_MAXCONTACT 250 > > > > > > > @@ -386,6 +403,15 @@ static const struct mt_class mt_classes[] = { > > > > > > > MT_QUIRK_CONTACT_CNT_ACCURATE | > > > > > > > MT_QUIRK_SEPARATE_APP_REPORT, > > > > > > > }, > > > > > > > + { .name = MT_CLS_ILITEK_016x, > > > > > > > + .quirks = MT_QUIRK_ALWAYS_VALID | > > > > > > > + MT_QUIRK_IGNORE_DUPLICATES | > > > > > > > + MT_QUIRK_HOVERING | > > > > > > > + MT_QUIRK_CONTACT_CNT_ACCURATE | > > > > > > > + MT_QUIRK_STICKY_FINGERS | > > > > > > > + MT_QUIRK_WIN8_PTP_BUTTONS | > > > > > > > + MT_QUIRK_DROP_REPEATED_SLOT, > > > > > > > + .export_all_inputs = true }, > > > > > > > { } > > > > > > > }; > > > > > > > > > > > > > > @@ -783,7 +809,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, > > > > > > > case HID_DG_CONFIDENCE: > > > > > > > if ((cls->name == MT_CLS_WIN_8 || > > > > > > > cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT || > > > > > > > - cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP) && > > > > > > > + cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP || > > > > > > > + cls->name == MT_CLS_ILITEK_016x) && > > > > > > > > > > Not something you should care about, but this long test of spaghetti > > > > > should likely be fixed into something way better at some point... > > > > > > > > > > > > (field->application == HID_DG_TOUCHPAD || > > > > > > > field->application == HID_DG_TOUCHSCREEN)) > > > > > > > app->quirks |= MT_QUIRK_CONFIDENCE; > > > > > > > @@ -948,7 +975,13 @@ static void mt_sync_frame(struct mt_device *td, struct mt_application *app, > > > > > > > input_event(input, EV_KEY, BTN_LEFT, app->left_button_state); > > > > > > > > > > > > > > input_mt_sync_frame(input); > > > > > > > - input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > > > + if (app->touch_change) > > > > > > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > > > > + app->touch_change = false; > > > > > > > > > > Why filter out the timestamp? > > > > > > > > > > > > > I suppose lonely timestamps should not do any harm, I just wanted to > > > > avoid evtest etc. output > > > > where there is no change but the timestamp, it is kind of also > > > > remainder of my first approach where > > > > I just tried filtering this events out and see waiting for BTN_TOUCH=0 > > > > and ABS_MT_TRACKING_ID=-1 > > > > would be enough (it wasn't). > > > > > > So I wouldn't filter out the timestamps. Not sure how ChromeOS stack > > > behaves when we forward timestamps without a touch, but it should be > > > capable of ignoring them. > > > > Ack. > > > > > > > > > > > > > > > > + } else { > > > > > > > + input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp); > > > > > > > + } > > > > > > > input_sync(input); > > > > > > > > > > > > > > mt_release_pending_palms(td, app, input); > > > > > > > @@ -993,6 +1026,60 @@ static int mt_touch_event(struct hid_device *hid, struct hid_field *field, > > > > > > > return 1; > > > > > > > } > > > > > > > > > > > > > > +static void mt_clear_slot_states(struct mt_slot_state *mt_slot_state, > > > > > > > + int size) > > > > > > > +{ > > > > > > > + int i; > > > > > > > + > > > > > > > + for (i = 0; i < size; i++) > > > > > > > + mt_slot_state[i].tip_state = 0; > > > > > > > +} > > > > > > > + > > > > > > > +static void mt_fill_slot_state(struct mt_slot_state *mt_slot_state, > > > > > > > + struct mt_usages *slot) > > > > > > > +{ > > > > > > > + mt_slot_state->x = *slot->x; > > > > > > > + mt_slot_state->y = *slot->y; > > > > > > > + mt_slot_state->cx = *slot->cx; > > > > > > > + mt_slot_state->cy = *slot->cy; > > > > > > > + mt_slot_state->p = *slot->p; > > > > > > > + mt_slot_state->w = *slot->w; > > > > > > > + mt_slot_state->h = *slot->h; > > > > > > > + mt_slot_state->a = *slot->a; > > > > > > > + mt_slot_state->contactid = *slot->contactid; > > > > > > > + mt_slot_state->tip_state = *slot->tip_state; > > > > > > > + mt_slot_state->inrange_state = *slot->inrange_state; > > > > > > > + mt_slot_state->confidence_state = *slot->confidence_state; > > > > > > > +} > > > > > > > + > > > > > > > +static bool mt_is_slot_state_equal(struct mt_slot_state *state1, > > > > > > > + struct mt_slot_state *state2) > > > > > > > +{ > > > > > > > + if (state1->x != state2->x) > > > > > > > + return false; > > > > > > > + if (state1->y != state2->y) > > > > > > > + return false; > > > > > > > + if (state1->cx != state2->cx) > > > > > > > + return false; > > > > > > > + if (state1->cy != state2->cy) > > > > > > > + return false; > > > > > > > + if (state1->p != state2->p) > > > > > > > + return false; > > > > > > > + if (state1->w != state2->w) > > > > > > > + return false; > > > > > > > + if (state1->a != state2->a) > > > > > > > + return false; > > > > > > > + if (state1->contactid != state2->contactid) > > > > > > > + return false; > > > > > > > + if (state1->tip_state != state2->tip_state) > > > > > > > + return false; > > > > > > > + if (state1->inrange_state != state2->inrange_state) > > > > > > > + return false; > > > > > > > + if (state1->confidence_state != state2->confidence_state) > > > > > > > + return false; > > > > > > > + return true; > > > > > > > +} > > > > > > > > > > The input stack already filters out duplicated events. So I am not > > > > > sure we need to store the information here once again and test for it > > > > > here. > > > > > > > > > > > > > What I want to do is to "prematurely" report that the finger has been lifted. > > > > It is not that much about solely filtering out the duplicated events, > > > > I rather want to interpret > > > > such an event as a finger lift off. > > > > > > Yes, but see my remark above. Given that the input stack already caches > > > the values and that it is validated by the evdev recording you shared, > > > you should be able to apply the quirk just before calling the various > > > input_event() and comparing the current value of this last finger with > > > the ones in the input stack. This way, you do not need to keep tabs on > > > the various fingers. > > > > > > > > > > > > > > + > > > > > > > static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > > > > struct mt_application *app, > > > > > > > struct mt_usages *slot) > > > > > > > @@ -1005,6 +1092,7 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > > > > int active; > > > > > > > int slotnum; > > > > > > > int tool = MT_TOOL_FINGER; > > > > > > > + struct mt_slot_state state = {0}; > > > > > > > > > > > > > > if (!slot) > > > > > > > return -EINVAL; > > > > > > > @@ -1058,13 +1146,42 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > > > > > * lift-off as userspace will not be aware > > > > > > > * of non-confidence, so we need to split > > > > > > > * it into 2 events: active MT_TOOL_PALM > > > > > > > - * and a separate liftoff. > > > > > > > + * and a separate lift off. > > > > > > > */ > > > > > > > active = true; > > > > > > > set_bit(slotnum, app->pending_palm_slots); > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > > > + mt_fill_slot_state(&state, slot); > > > > > > > + /* Check if every field in the slot is the same as before. > > > > > > > + * Some touchscreens report the same position for several > > > > > > > + * frames even though the finger is no longer on the surface. > > > > > > > + * This is actually being recognised as the finger remaining in > > > > > > > + * one position, which causes scrolling to be stopped. > > > > > > > + * Ignore such packets so that the scrolling continues > > > > > > > + * and the touchscreen reports the finger liftoff. > > > > > > > > > > That part I understand better, but how is the device reporting such events? > > > > > If the touch is no longer there, we should have a bit that says it is > > > > > lifted, so we should simply just ignore the touch, no? > > > > > > > > > > > > > Well, I would actually like the touch panel manufacturers to answer that :-) > > > > Based on some comments in our source code it seems that some devices > > > > might delay synthesis of lift-off to reduce risks of noisy release, but I am not > > > > sure if it explains why they produce events in between. > > > > > > They produce events in between because they have to, or they are not > > > following the spec :) > > > I also think the main reason is that if they forget about sending an > > > event *and* that the touch continues to move after a few events, they > > > will see a release happening on the touch because that's what > > > STICKY_FINGER does (it's a copy of the behaviour from Windows FWIW). > > > > > > > > > > > > > > > > > > > + */ > > > > > > > + if (!mt_is_slot_state_equal(&state, > > > > > > > + &app->mt_slot_state[slotnum])) { > > > > > > > + app->touch_change |= (active || > > > > > > > + app->mt_slot_state[slotnum].tip_state); > > > > > > > + app->repeated_counter = 0; > > > > > > > + } else { > > > > > > > +#define MT_MAX_REPEATED_DROPPED 5 > > > > > > > + if (app->repeated_counter < MT_MAX_REPEATED_DROPPED) { > > > > > > > + active = false; > > > > > > > + if (app->touched) > > > > > > > + app->touch_change = true; > > > > > > > + } else { > > > > > > > + app->touch_change = true; > > > > > > > + } > > > > > > > + app->repeated_counter++; > > > > > > > > > > This is specific to one device, but I have seen device filtering in > > > > > hardware that reports the same steady data as long as the touch is > > > > > present. So 5 reports with the same data seem quite short. > > > > > > > > > > > > > It can be a larger number, I just believe that in this case it was enough. > > > > > > All 3 (or 4 maybe) recordings you shared showed that we received only 5 > > > duplications of events. So there is a case to be made around whether we > > > need this patch. > > > > > > What would be interesting is whether you can "emulate" that situation > > > without releasing the finger. If the raw data is noisy (to some extent > > > of course) and it's hard to have exactly twice the same sample for a > > > touch, then maybe we can assume that as soon as we get identical values > > > we can release. Maybe the false positive in that situation would be > > > better handled by the user that missing the fling. > > > > > > > Actually, I am able to receive the same report if I am very still and > > try not to move my finger. > > I wouldn't say it is easy but it is definitely possible and hence i > > wanted to use this upper bound > > so that using an on screen keyboard is not annoying (i.e. I want to be > > able to hold backspace key > > and erase more than just one sign). > > Unfortunately, this is a deal breaker for me. You are trying to solve > 2 userspace issues in the kernel, where we don't have context. So > whatever you are choosing to implement, you are breaking one use case. > > The fact that nothing separates the output between "touch is holding > still" and "touch released" means that the kernel will have to assume > one or the other, and this will likely break other userspace > applications. > > I am sorry but I do not see how to fix that in the kernel: > - for the backspace key, this is a userspace issue. If the finger is > holding still on a key, some autorepeat needs to be emulated and the > kernel has nothing to say here > - for the fling, well, sorry I don't have a good solution but you'll > probably have to quirk the device in userspace in a similar manner > that you are doing in kernel space: > in pseudo code: > > If fling is started and the device is this bogus one and if there is > only one finger and the event consists in just one timestamp without > changes in X/Y, assume release and filter out any further events for > this slot. > > This way, you have the context (we started scrolling on a page), and > you are not stepping on a drawing application toes for instance. > Thanks for the response. Fortunately the vendor was able to resolve the issue in the new FW, so I believe that no kernel patch will be necessary :) In case it is not enough I will try and apply fixes in the user space. Regards, Angela > Cheers, > Benjamin > > > > > > > > > > > > Also, we do have devices which tend to forget to release slots, and > > > > > that's why we have MT_QUIRK_STICKY_FINGERS which release touches after > > > > > a while. > > > > > > > > Yeah, but the thing is that this specific device actually reports that > > > > the finger is still there, > > > > even though the finger has been lifted :-) > > > > I believe that MT_QUIRK_STICKY_FINGERS works when the device somehow > > > > fails to send a new report > > > > within some time after the previous one, so it does not solve the issue for us. > > > > > > It's not exactly the same case, but it still feels similar enough to > > > maybe be reused. > > > > > > > I am a little bit lost; MT_CLS_ILITEK_016x contains MT_QUIRK_STICKY_FINGERS. > > Based on the logs I provided, do you think that it does not work as expected? > > > > > Can you please give a shot at the following patch (and change the value > > > of MT_MAX_REPEATED_DROPPED between 1 to 5)? > > > It doesn't use MT_QUIRK_STICKY_FINGERS but should be simnilar to what > > > you have here in a shorter version. > > > > > > --- > > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > > > index 0dece608c023..cff4088e564a 100644 > > > --- a/drivers/hid/hid-multitouch.c > > > +++ b/drivers/hid/hid-multitouch.c > > > @@ -77,6 +77,8 @@ MODULE_LICENSE("GPL"); > > > > > > #define MT_BUTTONTYPE_CLICKPAD 0 > > > > > > +#define MT_MAX_REPEATED_DROPPED 3 > > > + > > > enum latency_mode { > > > HID_LATENCY_NORMAL = 0, > > > HID_LATENCY_HIGH = 1, > > > @@ -128,6 +130,7 @@ struct mt_application { > > > * 1 means we should use a serial protocol > > > * > 1 means hybrid (multitouch) protocol > > > */ > > > + unsigned int repeated_touches; /* used when MT_QUIRK_DROP_REPEATED_SLOT */ > > > > > > __s32 dev_time; /* the scan time provided by the device */ > > > unsigned long jiffies; /* the frame's jiffies */ > > > @@ -1065,6 +1068,19 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input, > > > } > > > } > > > > > > + /* TODO: add test for the new quirk so we don't apply this for all devices */ > > > + if (active && app->num_expected == 1) { > > > + struct input_mt_slot *i_slot = &mt->slots[slotnum]; > > > + > > > + if (input_mt_get_value(i_slot, ABS_MT_POSITION_X) == *slot->x && > > > + input_mt_get_value(i_slot, ABS_MT_POSITION_Y) == *slot->y) { > > > + if (++app->repeated_touches >= MT_MAX_REPEATED_DROPPED) > > > + active = false; > > > + } else { > > > + app->repeated_touches = 0; > > > + } > > > + } > > > + > > > input_mt_slot(input, slotnum); > > > input_mt_report_slot_state(input, tool, active); > > > if (active) { > > > --- > > > > > > > Observed results: > > MT_MAX_REPEATED_DROPPED = 1: It solves this issue, no hiccup visible, > > but continuing to press a key on the screen > > does not produce the right effect, for instance backspace erases a > > single character only > > MT_MAX_REPEATED_DROPPED = 2: Same as above > > MT_MAX_REPEATED_DROPPED = 3: Small hiccup noticeable, problems with > > backspace as above > > MT_MAX_REPEATED_DROPPED = 4: Small hiccup noticeable, problems with > > backspace as above > > MT_MAX_REPEATED_DROPPED = 5: Fling does not work (due to 0.1s delay > > and UI constants assuming that it is too long > > time to still calculate velocity) + problems with backspace > > > > > > > > > > > Cheers, > > > Benjamin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + } > > > > > > > + app->touched = active; > > > > > > > + app->mt_slot_state[slotnum] = state; > > > > > > > + } > > > > > > > input_mt_slot(input, slotnum); > > > > > > > input_mt_report_slot_state(input, tool, active); > > > > > > > if (active) { > > > > > > > @@ -1296,6 +1413,18 @@ static int mt_touch_input_configured(struct hid_device *hdev, > > > > > > > if (!app->pending_palm_slots) > > > > > > > return -ENOMEM; > > > > > > > > > > > > > > + if (app->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > > > + app->mt_slot_state = devm_kcalloc(&hi->input->dev, > > > > > > > + td->maxcontacts, > > > > > > > + sizeof(*app->mt_slot_state), > > > > > > > + GFP_KERNEL); > > > > > > > + if (!app->mt_slot_state) > > > > > > > + return -ENOMEM; > > > > > > > + mt_clear_slot_states(app->mt_slot_state, td->maxcontacts); > > > > > > > + app->repeated_counter = 0; > > > > > > > + } > > > > > > > + > > > > > > > + > > > > > > > ret = input_mt_init_slots(input, td->maxcontacts, app->mt_flags); > > > > > > > if (ret) > > > > > > > return ret; > > > > > > > @@ -1676,6 +1805,12 @@ static void mt_release_contacts(struct hid_device *hid) > > > > > > > > > > > > > > list_for_each_entry(application, &td->applications, list) { > > > > > > > application->num_received = 0; > > > > > > > + if (application->quirks & MT_QUIRK_DROP_REPEATED_SLOT) { > > > > > > > + application->touch_change = false; > > > > > > > + mt_clear_slot_states(application->mt_slot_state, > > > > > > > + td->maxcontacts); > > > > > > > + application->repeated_counter = 0; > > > > > > > + } > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > @@ -2014,6 +2149,15 @@ static const struct hid_device_id mt_devices[] = { > > > > > > > MT_USB_DEVICE(USB_VENDOR_ID_ILITEK, > > > > > > > USB_DEVICE_ID_ILITEK_MULTITOUCH) }, > > > > > > > > > > > > > > + /* Ilitek 106E/F touchscreen */ > > > > > > > + { .driver_data = MT_CLS_ILITEK_016x, > > > > > > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > > > > > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016E) }, > > > > > > > + > > > > > > > + { .driver_data = MT_CLS_ILITEK_016x, > > > > > > > + HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH_WIN_8, > > > > > > > + USB_VENDOR_ID_ILITEK, USB_DEVICE_ID_ILITEK_016F) }, > > > > > > > + > > > > > > > /* LG Melfas panel */ > > > > > > > { .driver_data = MT_CLS_LG, > > > > > > > HID_USB_DEVICE(USB_VENDOR_ID_LG, > > > > > > > -- > > > > > > > 2.35.1.1021.g381101b075-goog > > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > Benjamin > > > > > > > > > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT 2022-04-21 11:30 ` Angela Czubak @ 2022-04-22 8:55 ` Benjamin Tissoires 0 siblings, 0 replies; 9+ messages in thread From: Benjamin Tissoires @ 2022-04-22 8:55 UTC (permalink / raw) To: Angela Czubak; +Cc: open list:HID CORE LAYER, Jiri Kosina, upstream On Thu, Apr 21, 2022 at 1:31 PM Angela Czubak <acz@semihalf.com> wrote: [...] > Thanks for the response. Fortunately the vendor was able to resolve the > issue in the new FW, so I believe that no kernel patch will be necessary :) > In case it is not enough I will try and apply fixes in the user space. > Great! Glad to see that this is resolved. Cheers, Benjamin ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-04-22 8:56 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-31 14:45 [PATCH] HID: multitouch: Implement MT_QUIRK_DROP_REPEATED_SLOT Angela Czubak 2022-04-05 11:24 ` Angela Czubak 2022-04-05 12:57 ` Benjamin Tissoires 2022-04-06 12:18 ` Angela Czubak 2022-04-06 13:35 ` Benjamin Tissoires 2022-04-08 9:48 ` Angela Czubak 2022-04-21 9:31 ` Benjamin Tissoires 2022-04-21 11:30 ` Angela Czubak 2022-04-22 8:55 ` Benjamin Tissoires
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.