Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753923AbaJaQvn (ORCPT ); Fri, 31 Oct 2014 12:51:43 -0400 Received: from mail-ie0-f182.google.com ([209.85.223.182]:35175 "EHLO mail-ie0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752065AbaJaQvl (ORCPT ); Fri, 31 Oct 2014 12:51:41 -0400 Date: Fri, 31 Oct 2014 09:51:36 -0700 From: Dmitry Torokhov To: Benjamin Tissoires Cc: Henrik Rydberg , Hans de Goede , Peter Hutterer , linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, Daniel Kurtz , Chung-yih Wang Subject: Re: [PATCH 1/2] Input: synaptics: Use in-kernel tracking for reporting mt data Message-ID: <20141031165136.GB32331@dtor-ws> References: <1414693987-6059-1-git-send-email-benjamin.tissoires@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1414693987-6059-1-git-send-email-benjamin.tissoires@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 30, 2014 at 02:33:06PM -0400, Benjamin Tissoires wrote: > The current code tries to consider all states and transitions to properly > detect which finger is attached to which slot. The code is quite huge > and difficult to read. > If the sensor manages to group the touch points but is not reliable in > giving tracking ids, we can simply use the kernel tracking method. Note > that it is already used by Cr-48 Chromebooks. > > Incidentaly, this fixes a bug reported by Peter Hutterer: > """ > on the Lenovo T440, run: > evemu-record /dev/input/event4 | grep BTN_ > > then put one, two, three, two fingers down > when you go from 3 to 2 fingers the driver sends a spurious BTN_TOUCH 0 > event: > > E: 0.000000 0001 014a 0001 # EV_KEY / BTN_TOUCH 1 > E: 0.000000 0001 0145 0001 # EV_KEY / BTN_TOOL_FINGER 1 > E: 0.770008 0001 0145 0000 # EV_KEY / BTN_TOOL_FINGER 0 > E: 0.770008 0001 014d 0001 # EV_KEY / BTN_TOOL_DOUBLETAP 1 > E: 1.924716 0001 014d 0000 # EV_KEY / BTN_TOOL_DOUBLETAP 0 > E: 1.924716 0001 014e 0001 # EV_KEY / BTN_TOOL_TRIPLETAP 1 > > .. changing from 3 to 2 fingers now > > E: 3.152641 0001 014a 0000 # EV_KEY / BTN_TOUCH 0 > E: 3.152641 0001 014d 0001 # EV_KEY / BTN_TOOL_DOUBLETAP 1 > E: 3.152641 0001 014e 0000 # EV_KEY / BTN_TOOL_TRIPLETAP 0 > E: 3.176948 0001 014a 0001 # EV_KEY / BTN_TOUCH 1 > > quick look in the kernel shows it's caused by hw.z going to 0 for a packet, > so probably a firmware bug. either way, it makes it hard to track BTN_TOUCH > as signal that at least one finger is down. > """ > > The in-kernel tracking is enough to remove this spurious BTN_TOUCH 0. > > Signed-off-by: Benjamin Tissoires > --- > > Hi Dmitry, > > I started working on that for 2 other bug reports > https://bugs.freedesktop.org/show_bug.cgi?id=81278 > and > https://bugs.freedesktop.org/show_bug.cgi?id=76722 > > I thought the cursor jumps could be fixed by the in-kernel tracking, but the > tracking needs a little bit more work to filter them out (patches to follow soon). > > From a user perspective, this patch does not change anything to what the user > previously had. It also fixes Peter's bug that's why I decide to send this out > by itself (removing ~350 lines of code and fixing bugs is always nice). > > I think the cursor jump fixes will need more bikeshedding in input-mt.c (I am > *really* bad at designing APIs), so I'll send it later as an RFC. Daniel and Chung-yih were working on the driver so let's see if they have a moment... > > Cheers, > Benjamin > > drivers/input/mouse/synaptics.c | 397 ++++------------------------------------ > drivers/input/mouse/synaptics.h | 18 +- > 2 files changed, 34 insertions(+), 381 deletions(-) > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c > index 9031a0a..fd89249 100644 > --- a/drivers/input/mouse/synaptics.c > +++ b/drivers/input/mouse/synaptics.c > @@ -569,14 +569,6 @@ static void synaptics_pt_create(struct psmouse *psmouse) > * Functions to interpret the absolute mode packets > ****************************************************************************/ > > -static void synaptics_mt_state_set(struct synaptics_mt_state *state, int count, > - int sgm, int agm) > -{ > - state->count = count; > - state->sgm = sgm; > - state->agm = agm; > -} > - > static void synaptics_parse_agm(const unsigned char buf[], > struct synaptics_data *priv, > struct synaptics_hw_state *hw) > @@ -595,16 +587,13 @@ static void synaptics_parse_agm(const unsigned char buf[], > break; > > case 2: > - /* AGM-CONTACT packet: (count, sgm, agm) */ > - synaptics_mt_state_set(&agm->mt_state, buf[1], buf[2], buf[4]); > + /* AGM-CONTACT packet: we are only interested in the count */ > + priv->agm_count = buf[1]; > break; > > default: > break; > } > - > - /* Record that at least one AGM has been received since last SGM */ > - priv->agm_pending = true; > } > > static bool is_forcepad; > @@ -798,388 +787,68 @@ static void synaptics_report_buttons(struct psmouse *psmouse, > input_report_key(dev, BTN_0 + i, hw->ext_buttons & (1 << i)); > } > > -static void synaptics_report_slot(struct input_dev *dev, int slot, > - const struct synaptics_hw_state *hw) > -{ > - input_mt_slot(dev, slot); > - input_mt_report_slot_state(dev, MT_TOOL_FINGER, (hw != NULL)); > - if (!hw) > - return; > - > - input_report_abs(dev, ABS_MT_POSITION_X, hw->x); > - input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y)); > - input_report_abs(dev, ABS_MT_PRESSURE, hw->z); > -} > - > static void synaptics_report_mt_data(struct psmouse *psmouse, > - struct synaptics_mt_state *mt_state, > - const struct synaptics_hw_state *sgm) > + const struct synaptics_hw_state *sgm, > + int num_fingers) > { > struct input_dev *dev = psmouse->dev; > struct synaptics_data *priv = psmouse->private; > - struct synaptics_hw_state *agm = &priv->agm; > - struct synaptics_mt_state *old = &priv->mt_state; > + const struct synaptics_hw_state *hw[2] = { sgm, &priv->agm }; > + struct input_mt_pos pos[2]; > + int slot[2], nsemi, i; > > - switch (mt_state->count) { > - case 0: > - synaptics_report_slot(dev, 0, NULL); > - synaptics_report_slot(dev, 1, NULL); > - break; > - case 1: > - if (mt_state->sgm == -1) { > - synaptics_report_slot(dev, 0, NULL); > - synaptics_report_slot(dev, 1, NULL); > - } else if (mt_state->sgm == 0) { > - synaptics_report_slot(dev, 0, sgm); > - synaptics_report_slot(dev, 1, NULL); > - } else { > - synaptics_report_slot(dev, 0, NULL); > - synaptics_report_slot(dev, 1, sgm); > - } > - break; > - default: > - /* > - * If the finger slot contained in SGM is valid, and either > - * hasn't changed, or is new, or the old SGM has now moved to > - * AGM, then report SGM in MTB slot 0. > - * Otherwise, empty MTB slot 0. > - */ > - if (mt_state->sgm != -1 && > - (mt_state->sgm == old->sgm || > - old->sgm == -1 || mt_state->agm == old->sgm)) > - synaptics_report_slot(dev, 0, sgm); > - else > - synaptics_report_slot(dev, 0, NULL); > + nsemi = clamp_val(num_fingers, 0, 2); > > - /* > - * If the finger slot contained in AGM is valid, and either > - * hasn't changed, or is new, then report AGM in MTB slot 1. > - * Otherwise, empty MTB slot 1. > - * > - * However, in the case where the AGM is new, make sure that > - * that it is either the same as the old SGM, or there was no > - * SGM. > - * > - * Otherwise, if the SGM was just 1, and the new AGM is 2, then > - * the new AGM will keep the old SGM's tracking ID, which can > - * cause apparent drumroll. This happens if in the following > - * valid finger sequence: > - * > - * Action SGM AGM (MTB slot:Contact) > - * 1. Touch contact 0 (0:0) > - * 2. Touch contact 1 (0:0, 1:1) > - * 3. Lift contact 0 (1:1) > - * 4. Touch contacts 2,3 (0:2, 1:3) > - * > - * In step 4, contact 3, in AGM must not be given the same > - * tracking ID as contact 1 had in step 3. To avoid this, > - * the first agm with contact 3 is dropped and slot 1 is > - * invalidated (tracking ID = -1). > - */ > - if (mt_state->agm != -1 && > - (mt_state->agm == old->agm || > - (old->agm == -1 && > - (old->sgm == -1 || mt_state->agm == old->sgm)))) > - synaptics_report_slot(dev, 1, agm); > - else > - synaptics_report_slot(dev, 1, NULL); > - break; > + for (i = 0; i < nsemi; i++) { > + pos[i].x = hw[i]->x; > + pos[i].y = synaptics_invert_y(hw[i]->y); > } > > + input_mt_assign_slots(dev, slot, pos, nsemi); > + > + for (i = 0; i < nsemi; i++) { > + input_mt_slot(dev, slot[i]); > + input_mt_report_slot_state(dev, MT_TOOL_FINGER, true); > + input_report_abs(dev, ABS_MT_POSITION_X, pos[i].x); > + input_report_abs(dev, ABS_MT_POSITION_Y, pos[i].y); > + input_report_abs(dev, ABS_MT_PRESSURE, hw[i]->z); > + } > + > + input_mt_drop_unused(dev); > + > /* Don't use active slot count to generate BTN_TOOL events. */ > input_mt_report_pointer_emulation(dev, false); > > /* Send the number of fingers reported by touchpad itself. */ > - input_mt_report_finger_count(dev, mt_state->count); > + input_mt_report_finger_count(dev, num_fingers); > > synaptics_report_buttons(psmouse, sgm); > > input_sync(dev); > } > > -/* Handle case where mt_state->count = 0 */ > -static void synaptics_image_sensor_0f(struct synaptics_data *priv, > - struct synaptics_mt_state *mt_state) > -{ > - synaptics_mt_state_set(mt_state, 0, -1, -1); > - priv->mt_state_lost = false; > -} > - > -/* Handle case where mt_state->count = 1 */ > -static void synaptics_image_sensor_1f(struct synaptics_data *priv, > - struct synaptics_mt_state *mt_state) > -{ > - struct synaptics_hw_state *agm = &priv->agm; > - struct synaptics_mt_state *old = &priv->mt_state; > - > - /* > - * If the last AGM was (0,0,0), and there is only one finger left, > - * then we absolutely know that SGM contains slot 0, and all other > - * fingers have been removed. > - */ > - if (priv->agm_pending && agm->z == 0) { > - synaptics_mt_state_set(mt_state, 1, 0, -1); > - priv->mt_state_lost = false; > - return; > - } > - > - switch (old->count) { > - case 0: > - synaptics_mt_state_set(mt_state, 1, 0, -1); > - break; > - case 1: > - /* > - * If mt_state_lost, then the previous transition was 3->1, > - * and SGM now contains either slot 0 or 1, but we don't know > - * which. So, we just assume that the SGM now contains slot 1. > - * > - * If pending AGM and either: > - * (a) the previous SGM slot contains slot 0, or > - * (b) there was no SGM slot > - * then, the SGM now contains slot 1 > - * > - * Case (a) happens with very rapid "drum roll" gestures, where > - * slot 0 finger is lifted and a new slot 1 finger touches > - * within one reporting interval. > - * > - * Case (b) happens if initially two or more fingers tap > - * briefly, and all but one lift before the end of the first > - * reporting interval. > - * > - * (In both these cases, slot 0 will becomes empty, so SGM > - * contains slot 1 with the new finger) > - * > - * Else, if there was no previous SGM, it now contains slot 0. > - * > - * Otherwise, SGM still contains the same slot. > - */ > - if (priv->mt_state_lost || > - (priv->agm_pending && old->sgm <= 0)) > - synaptics_mt_state_set(mt_state, 1, 1, -1); > - else if (old->sgm == -1) > - synaptics_mt_state_set(mt_state, 1, 0, -1); > - break; > - case 2: > - /* > - * If mt_state_lost, we don't know which finger SGM contains. > - * > - * So, report 1 finger, but with both slots empty. > - * We will use slot 1 on subsequent 1->1 > - */ > - if (priv->mt_state_lost) { > - synaptics_mt_state_set(mt_state, 1, -1, -1); > - break; > - } > - /* > - * Since the last AGM was NOT (0,0,0), it was the finger in > - * slot 0 that has been removed. > - * So, SGM now contains previous AGM's slot, and AGM is now > - * empty. > - */ > - synaptics_mt_state_set(mt_state, 1, old->agm, -1); > - break; > - case 3: > - /* > - * Since last AGM was not (0,0,0), we don't know which finger > - * is left. > - * > - * So, report 1 finger, but with both slots empty. > - * We will use slot 1 on subsequent 1->1 > - */ > - synaptics_mt_state_set(mt_state, 1, -1, -1); > - priv->mt_state_lost = true; > - break; > - case 4: > - case 5: > - /* mt_state was updated by AGM-CONTACT packet */ > - break; > - } > -} > - > -/* Handle case where mt_state->count = 2 */ > -static void synaptics_image_sensor_2f(struct synaptics_data *priv, > - struct synaptics_mt_state *mt_state) > -{ > - struct synaptics_mt_state *old = &priv->mt_state; > - > - switch (old->count) { > - case 0: > - synaptics_mt_state_set(mt_state, 2, 0, 1); > - break; > - case 1: > - /* > - * If previous SGM contained slot 1 or higher, SGM now contains > - * slot 0 (the newly touching finger) and AGM contains SGM's > - * previous slot. > - * > - * Otherwise, SGM still contains slot 0 and AGM now contains > - * slot 1. > - */ > - if (old->sgm >= 1) > - synaptics_mt_state_set(mt_state, 2, 0, old->sgm); > - else > - synaptics_mt_state_set(mt_state, 2, 0, 1); > - break; > - case 2: > - /* > - * If mt_state_lost, SGM now contains either finger 1 or 2, but > - * we don't know which. > - * So, we just assume that the SGM contains slot 0 and AGM 1. > - */ > - if (priv->mt_state_lost) > - synaptics_mt_state_set(mt_state, 2, 0, 1); > - /* > - * Otherwise, use the same mt_state, since it either hasn't > - * changed, or was updated by a recently received AGM-CONTACT > - * packet. > - */ > - break; > - case 3: > - /* > - * 3->2 transitions have two unsolvable problems: > - * 1) no indication is given which finger was removed > - * 2) no way to tell if agm packet was for finger 3 > - * before 3->2, or finger 2 after 3->2. > - * > - * So, report 2 fingers, but empty all slots. > - * We will guess slots [0,1] on subsequent 2->2. > - */ > - synaptics_mt_state_set(mt_state, 2, -1, -1); > - priv->mt_state_lost = true; > - break; > - case 4: > - case 5: > - /* mt_state was updated by AGM-CONTACT packet */ > - break; > - } > -} > - > -/* Handle case where mt_state->count = 3 */ > -static void synaptics_image_sensor_3f(struct synaptics_data *priv, > - struct synaptics_mt_state *mt_state) > -{ > - struct synaptics_mt_state *old = &priv->mt_state; > - > - switch (old->count) { > - case 0: > - synaptics_mt_state_set(mt_state, 3, 0, 2); > - break; > - case 1: > - /* > - * If previous SGM contained slot 2 or higher, SGM now contains > - * slot 0 (one of the newly touching fingers) and AGM contains > - * SGM's previous slot. > - * > - * Otherwise, SGM now contains slot 0 and AGM contains slot 2. > - */ > - if (old->sgm >= 2) > - synaptics_mt_state_set(mt_state, 3, 0, old->sgm); > - else > - synaptics_mt_state_set(mt_state, 3, 0, 2); > - break; > - case 2: > - /* > - * If the AGM previously contained slot 3 or higher, then the > - * newly touching finger is in the lowest available slot. > - * > - * If SGM was previously 1 or higher, then the new SGM is > - * now slot 0 (with a new finger), otherwise, the new finger > - * is now in a hidden slot between 0 and AGM's slot. > - * > - * In all such cases, the SGM now contains slot 0, and the AGM > - * continues to contain the same slot as before. > - */ > - if (old->agm >= 3) { > - synaptics_mt_state_set(mt_state, 3, 0, old->agm); > - break; > - } > - > - /* > - * After some 3->1 and all 3->2 transitions, we lose track > - * of which slot is reported by SGM and AGM. > - * > - * For 2->3 in this state, report 3 fingers, but empty all > - * slots, and we will guess (0,2) on a subsequent 0->3. > - * > - * To userspace, the resulting transition will look like: > - * 2:[0,1] -> 3:[-1,-1] -> 3:[0,2] > - */ > - if (priv->mt_state_lost) { > - synaptics_mt_state_set(mt_state, 3, -1, -1); > - break; > - } > - > - /* > - * If the (SGM,AGM) really previously contained slots (0, 1), > - * then we cannot know what slot was just reported by the AGM, > - * because the 2->3 transition can occur either before or after > - * the AGM packet. Thus, this most recent AGM could contain > - * either the same old slot 1 or the new slot 2. > - * Subsequent AGMs will be reporting slot 2. > - * > - * To userspace, the resulting transition will look like: > - * 2:[0,1] -> 3:[0,-1] -> 3:[0,2] > - */ > - synaptics_mt_state_set(mt_state, 3, 0, -1); > - break; > - case 3: > - /* > - * If, for whatever reason, the previous agm was invalid, > - * Assume SGM now contains slot 0, AGM now contains slot 2. > - */ > - if (old->agm <= 2) > - synaptics_mt_state_set(mt_state, 3, 0, 2); > - /* > - * mt_state either hasn't changed, or was updated by a recently > - * received AGM-CONTACT packet. > - */ > - break; > - > - case 4: > - case 5: > - /* mt_state was updated by AGM-CONTACT packet */ > - break; > - } > -} > - > -/* Handle case where mt_state->count = 4, or = 5 */ > -static void synaptics_image_sensor_45f(struct synaptics_data *priv, > - struct synaptics_mt_state *mt_state) > -{ > - /* mt_state was updated correctly by AGM-CONTACT packet */ > - priv->mt_state_lost = false; > -} > - > static void synaptics_image_sensor_process(struct psmouse *psmouse, > struct synaptics_hw_state *sgm) > { > struct synaptics_data *priv = psmouse->private; > - struct synaptics_hw_state *agm = &priv->agm; > - struct synaptics_mt_state mt_state; > - > - /* Initialize using current mt_state (as updated by last agm) */ > - mt_state = agm->mt_state; > + int num_fingers; > > /* > * Update mt_state using the new finger count and current mt_state. > */ > if (sgm->z == 0) > - synaptics_image_sensor_0f(priv, &mt_state); > + num_fingers = 0; > else if (sgm->w >= 4) > - synaptics_image_sensor_1f(priv, &mt_state); > + num_fingers = 1; > else if (sgm->w == 0) > - synaptics_image_sensor_2f(priv, &mt_state); > - else if (sgm->w == 1 && mt_state.count <= 3) > - synaptics_image_sensor_3f(priv, &mt_state); > + num_fingers = 2; > + else if (sgm->w == 1) > + num_fingers = priv->agm_count ? priv->agm_count : 3; > else > - synaptics_image_sensor_45f(priv, &mt_state); > + num_fingers = 4; > > /* Send resulting input events to user space */ > - synaptics_report_mt_data(psmouse, &mt_state, sgm); > - > - /* Store updated mt_state */ > - priv->mt_state = agm->mt_state = mt_state; > - priv->agm_pending = false; > + synaptics_report_mt_data(psmouse, sgm, num_fingers); > } > > static void synaptics_profile_sensor_process(struct psmouse *psmouse, > @@ -1439,7 +1108,7 @@ static void set_input_params(struct psmouse *psmouse, > ABS_MT_POSITION_Y); > /* Image sensors can report per-contact pressure */ > input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0); > - input_mt_init_slots(dev, 2, INPUT_MT_POINTER); > + input_mt_init_slots(dev, 2, INPUT_MT_POINTER | INPUT_MT_TRACK); > > /* Image sensors can signal 4 and 5 finger clicks */ > __set_bit(BTN_TOOL_QUADTAP, dev->keybit); > diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h > index 1bd01f2..6faf9bb 100644 > --- a/drivers/input/mouse/synaptics.h > +++ b/drivers/input/mouse/synaptics.h > @@ -119,16 +119,6 @@ > #define SYN_REDUCED_FILTER_FUZZ 8 > > /* > - * A structure to describe which internal touchpad finger slots are being > - * reported in raw packets. > - */ > -struct synaptics_mt_state { > - int count; /* num fingers being tracked */ > - int sgm; /* which slot is reported by sgm pkt */ > - int agm; /* which slot is reported by agm pkt*/ > -}; > - > -/* > * A structure to describe the state of the touchpad hardware (buttons and pad) > */ > struct synaptics_hw_state { > @@ -143,9 +133,6 @@ struct synaptics_hw_state { > unsigned int down:1; > unsigned char ext_buttons; > signed char scroll; > - > - /* As reported in last AGM-CONTACT packets */ > - struct synaptics_mt_state mt_state; > }; > > struct synaptics_data { > @@ -170,15 +157,12 @@ struct synaptics_data { > > struct serio *pt_port; /* Pass-through serio port */ > > - struct synaptics_mt_state mt_state; /* Current mt finger state */ > - bool mt_state_lost; /* mt_state may be incorrect */ > - > /* > * Last received Advanced Gesture Mode (AGM) packet. An AGM packet > * contains position data for a second contact, at half resolution. > */ > struct synaptics_hw_state agm; > - bool agm_pending; /* new AGM packet received */ > + unsigned int agm_count; /* finger count reported by agm */ > > /* ForcePad handling */ > unsigned long press_start; > -- > 2.1.0 > -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/