[PATCH 0/9 v2] Synaptics image sensor support

[PATCH 0/9 v2] Synaptics image sensor support

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Hello,

This patch set (against next) is intended to add support for synaptics
"image sensor" touchpads.

Patches 1-3 clean up the current driver slightly and prepare for the image
sensor patches which follow.

Patches 4-7 add up to 3 finger support for image sensor touchpads.
Image sensors do not suffer from the finger tracking issues that plagued
the earlier "profile sensors", and which required the invention of "semi-mt"
(Semi-mt reports a bounding box around two fingers instead of the fingers
themselves).  Instead, the image sensors report the actual positions of two
fingers using the same "Advanced Gesture Mode".  This driver uses two MT-B slots
to report these two fingers to userspace.  In addition, it will also report
the total number of fingers using BTN_TOOL_*TAP EV_KEY events.
Userspace drivers should be aware that the number of fingers reported via
BTN_TOOL_*TAP can be greater than the total number MT-B slots with non-negative
track_ids.  Upon opening the device node, userspace should query the maximum
values supported ABS_MT_SLOT, and note the number of supported BTN_TOOL_*TAP
events.

Patches 8-9 adds up to 5 finger support.
In fact, the image sensor touchpads actually track 5 fingers while reporting
just 2 finger positions.  These patches add support for properly tracking the
reported slots through 4 and 5 finger transitions, while always reporting two of
them via 2 MT-B slots.  In addition, a new event, EV_KEY/BTN_TOOL_QUINTTAP, is
added to the event subsystem to allow reporting up to 5 fingers.

These patches are similar to, and inspired by, a similar patchset recently
submitted by Derek Foreman and Daniel Stone.  However, it is not directly built
upon, nor is it compatible with, those patches.

Thanks,
Daniel

Daniel Kurtz (9):
  Input: synaptics - refactor y inversion
  Input: synaptics - refactor agm packet parsing
  Input: synaptics - refactor initialization of abs position axes
  Input: synaptics - add image sensor support
  Input: synaptics - decode AGM packet types
  Input: synaptics - process finger (<=3) transitions
  Input: synaptics - improved 2->3 finger transition handling
  Input: add BTN_TOOL_QUINTTAP for reporting 5 fingers on touchpad
  Input: synaptics - process finger (<=5) transitions

 drivers/input/input-mt.c        |    1 +
 drivers/input/mouse/synaptics.c |  466 ++++++++++++++++++++++++++++++++++++---
 drivers/input/mouse/synaptics.h |   27 ++-
 include/linux/input.h           |    1 +
 4 files changed, 458 insertions(+), 37 deletions(-)

-- 
1.7.3.1

[PATCH 1/9 v2] Input: synaptics - refactor y inversion

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Synaptics touchpads report increasing y from bottom to top.
This is inverted from normal userspace "top of screen is 0" coordinates.
Thus, the kernel driver reports inverted y coordinates to userspace.

This patch refactors this inversion.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 5538fc6..297e88f 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -44,6 +44,16 @@
 #define YMIN_NOMINAL 1408
 #define YMAX_NOMINAL 4448
 
+/*
+ * Synaptics touchpads report the y coordinate from bottom to top, which is
+ * opposite from what userspace expects.
+ * This function is used to invert y before reporting.
+ */
+static int invert_y(int y)
+{
+	return YMAX_NOMINAL + YMIN_NOMINAL - y;
+}
+
 
 /*****************************************************************************
  *	Stuff we need even when we do not want native Synaptics support
@@ -502,8 +512,7 @@ static void synaptics_report_semi_mt_slot(struct input_dev *dev, int slot,
 	input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
 	if (active) {
 		input_report_abs(dev, ABS_MT_POSITION_X, x);
-		input_report_abs(dev, ABS_MT_POSITION_Y,
-				 YMAX_NOMINAL + YMIN_NOMINAL - y);
+		input_report_abs(dev, ABS_MT_POSITION_Y, invert_y(y));
 	}
 }
 
@@ -597,7 +606,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 
 	if (num_fingers > 0) {
 		input_report_abs(dev, ABS_X, hw.x);
-		input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
+		input_report_abs(dev, ABS_Y, invert_y(hw.y));
 	}
 	input_report_abs(dev, ABS_PRESSURE, hw.z);
 
-- 
1.7.3.1

[PATCH 2/9 v2] Input: synaptics - refactor agm packet parsing

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

When a Synaptics touchpad is in "AGM" mode, and multiple fingers are
detected, the touchpad sends alternating "Advanced Gesture Mode" (AGM) and
"Simple Gesture Mode" (SGM) packets.
  The AGM packets have w=2, and contain reduced resolution finger data.
  The SGM packets have w={0,1} and contain full resolution finger data.

Refactor the parsing of agm packets to its own function, and rename the
synaptics_data.mt field to .agm to indicate that it contains the contents of
the last agm packet.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   19 ++++++++++++++-----
 drivers/input/mouse/synaptics.h |    6 +++++-
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 297e88f..e966894 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -419,6 +419,17 @@ static void synaptics_pt_create(struct psmouse *psmouse)
  *	Functions to interpret the absolute mode packets
  ****************************************************************************/
 
+static void synaptics_parse_agm(const unsigned char buf[],
+				struct synaptics_data *priv)
+{
+	struct synaptics_hw_state *agm = &priv->agm;
+
+	/* Gesture packet: (x, y, z) at half resolution */
+	agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
+	agm->y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
+	agm->z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
+}
+
 static int synaptics_parse_hw_state(const unsigned char buf[],
 				    struct synaptics_data *priv,
 				    struct synaptics_hw_state *hw)
@@ -453,10 +464,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 		}
 
 		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
-			/* Gesture packet: (x, y, z) at half resolution */
-			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
-			priv->mt.y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
-			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
+			synaptics_parse_agm(buf, priv);
 			return 1;
 		}
 
@@ -595,7 +603,8 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	}
 
 	if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
-		synaptics_report_semi_mt_data(dev, &hw, &priv->mt, num_fingers);
+		synaptics_report_semi_mt_data(dev, &hw, &priv->agm,
+					      num_fingers);
 
 	/* Post events
 	 * BTN_TOUCH has to be first as mousedev relies on it when doing
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index a4394e1..da8e969 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -146,7 +146,11 @@ struct synaptics_data {
 
 	struct serio *pt_port;			/* Pass-through serio port */
 
-	struct synaptics_hw_state mt;		/* current gesture packet */
+	/*
+	 * 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;
 };
 
 void synaptics_module_init(void);
-- 
1.7.3.1

[PATCH 3/9 v2] Input: synaptics - refactor initialization of abs position axes

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   44 +++++++++++++++++---------------------
 1 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index e966894..30d05fd 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -712,39 +712,38 @@ static psmouse_ret_t synaptics_process_byte(struct psmouse *psmouse)
 /*****************************************************************************
  *	Driver initialization/cleanup functions
  ****************************************************************************/
+static void set_abs_position_params(struct input_dev *dev,
+				    struct synaptics_data *priv, int x_code,
+				    int y_code)
+{
+	int x_min = priv->x_min ?: XMIN_NOMINAL;
+	int x_max = priv->x_max ?: XMAX_NOMINAL;
+	int y_min = priv->y_min ?: YMIN_NOMINAL;
+	int y_max = priv->y_max ?: YMAX_NOMINAL;
+	int fuzz = SYN_CAP_REDUCED_FILTERING(priv->ext_cap_0c) ?
+			SYN_REDUCED_FILTER_FUZZ : 0;
+
+	input_set_abs_params(dev, x_code, x_min, x_max, fuzz, 0);
+	input_set_abs_params(dev, y_code, y_min, y_max, fuzz, 0);
+	input_abs_set_res(dev, x_code, priv->x_res);
+	input_abs_set_res(dev, y_code, priv->y_res);
+}
+
 static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 {
 	int i;
-	int fuzz = SYN_CAP_REDUCED_FILTERING(priv->ext_cap_0c) ?
-			SYN_REDUCED_FILTER_FUZZ : 0;
 
 	__set_bit(INPUT_PROP_POINTER, dev->propbit);
 
 	__set_bit(EV_ABS, dev->evbit);
-	input_set_abs_params(dev, ABS_X,
-			     priv->x_min ?: XMIN_NOMINAL,
-			     priv->x_max ?: XMAX_NOMINAL,
-			     fuzz, 0);
-	input_set_abs_params(dev, ABS_Y,
-			     priv->y_min ?: YMIN_NOMINAL,
-			     priv->y_max ?: YMAX_NOMINAL,
-			     fuzz, 0);
+	set_abs_position_params(dev, priv, ABS_X, ABS_Y);
 	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
 
 	if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
 		__set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
 		input_mt_init_slots(dev, 2);
-		input_set_abs_params(dev, ABS_MT_POSITION_X,
-				     priv->x_min ?: XMIN_NOMINAL,
-				     priv->x_max ?: XMAX_NOMINAL,
-				     fuzz, 0);
-		input_set_abs_params(dev, ABS_MT_POSITION_Y,
-				     priv->y_min ?: YMIN_NOMINAL,
-				     priv->y_max ?: YMAX_NOMINAL,
-				     fuzz, 0);
-
-		input_abs_set_res(dev, ABS_MT_POSITION_X, priv->x_res);
-		input_abs_set_res(dev, ABS_MT_POSITION_Y, priv->y_res);
+		set_abs_position_params(dev, priv, ABS_MT_POSITION_X,
+					ABS_MT_POSITION_Y);
 	}
 
 	if (SYN_CAP_PALMDETECT(priv->capabilities))
@@ -777,9 +776,6 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 	__clear_bit(REL_X, dev->relbit);
 	__clear_bit(REL_Y, dev->relbit);
 
-	input_abs_set_res(dev, ABS_X, priv->x_res);
-	input_abs_set_res(dev, ABS_Y, priv->y_res);
-
 	if (SYN_CAP_CLICKPAD(priv->ext_cap_0c)) {
 		__set_bit(INPUT_PROP_BUTTONPAD, dev->propbit);
 		/* Clickpads report only left button */
-- 
1.7.3.1

[PATCH 4/9 v2] Input: synaptics - add image sensor support

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Synaptics makes (at least) two kinds of touchpad sensors:
 * Older pads use a profile sensor that could only infer the location
   of individual fingers based on the projection of their profiles
   onto row and column sensors.
 * Newer pads use an image sensor that can track true finger position
   using a two-dimensional sensor grid.

Both sensor types support an "Advanced Gesture Mode":
 When multiple fingers are detected, the touchpad sends alternating
 "Advanced Gesture Mode" (AGM) and "Simple Gesture Mode" (SGM)
 packets.
 The AGM packets have w=2, and contain reduced resolution finger data
 The SGM packets have w={0,1} and contain full resolution finger data

Profile sensors try to report the "upper" (larger y value) finger in
 the SGM packet, and the lower (smaller y value) in the AGM packet.
 However, due to the nature of the profile sensor, they easily get
 confused when fingers cross, and can start reporting the x-coordinate
 of one with the y-coordinate of the other.  Thus, for profile
 sensors, "semi-mt" was created, which reports a "bounding box"
 created by pairing min and max coordinates of the two pairs of
 reported fingers.

Image sensors can report the actual coordinates of two of the fingers
 present.  This patch detects if the touchpad is an image sensor and
 reports finger data using the MT-B protocol.

NOTE: This patch only adds partial support for 2-finger gestures.
      The proper interpretation of the slot contents when more than
      two fingers are present is left to later patches.  Also,
      handling of 'number of fingers' transitions is incomplete.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |  104 +++++++++++++++++++++++++++++++++++++-
 drivers/input/mouse/synaptics.h |    3 +
 2 files changed, 104 insertions(+), 3 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 30d05fd..a9960d2 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -304,7 +304,8 @@ static int synaptics_set_advanced_gesture_mode(struct psmouse *psmouse)
 	static unsigned char param = 0xc8;
 	struct synaptics_data *priv = psmouse->private;
 
-	if (!SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
+	if (!(SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) ||
+			SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)))
 		return 0;
 
 	if (psmouse_sliced_command(psmouse, SYN_QUE_MODEL))
@@ -463,7 +464,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
 		}
 
-		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
+		if ((SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)
+				|| SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
+				&& hw->w == 2) {
 			synaptics_parse_agm(buf, priv);
 			return 1;
 		}
@@ -543,6 +546,87 @@ static void synaptics_report_semi_mt_data(struct input_dev *dev,
 	}
 }
 
+static void synaptics_report_slot_empty(struct input_dev *dev, int slot)
+{
+	input_mt_slot(dev, slot);
+	input_mt_report_slot_state(dev, MT_TOOL_FINGER, false);
+}
+
+static void synaptics_report_slot_sgm(struct input_dev *dev, int slot,
+				      const struct synaptics_hw_state *sgm)
+{
+	input_mt_slot(dev, slot);
+	input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
+	input_report_abs(dev, ABS_MT_POSITION_X, sgm->x);
+	input_report_abs(dev, ABS_MT_POSITION_Y, invert_y(sgm->y));
+	input_report_abs(dev, ABS_MT_PRESSURE, sgm->z);
+	/* SGM can sometimes contain valid width */
+	if (sgm->w >= 4)
+		input_report_abs(dev, ABS_MT_TOUCH_MAJOR, sgm->w);
+}
+
+static void synaptics_report_slot_agm(struct input_dev *dev, int slot,
+				      const struct synaptics_hw_state *agm)
+{
+	input_mt_slot(dev, slot);
+	input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
+	input_report_abs(dev, ABS_MT_POSITION_X, agm->x);
+	input_report_abs(dev, ABS_MT_POSITION_Y, invert_y(agm->y));
+	input_report_abs(dev, ABS_MT_PRESSURE, agm->z);
+}
+
+static void synaptics_report_mt(struct psmouse *psmouse,
+				int count,
+				const struct synaptics_hw_state *sgm)
+{
+	struct input_dev *dev = psmouse->dev;
+	struct synaptics_data *priv = psmouse->private;
+	struct synaptics_hw_state *agm = &priv->agm;
+
+	switch (count) {
+	case 0:
+		synaptics_report_slot_empty(dev, 0);
+		synaptics_report_slot_empty(dev, 1);
+		break;
+	case 1:
+		synaptics_report_slot_sgm(dev, 0, sgm);
+		synaptics_report_slot_empty(dev, 1);
+		break;
+	case 2:
+	case 3: /* Fall-through case */
+		synaptics_report_slot_sgm(dev, 0, sgm);
+		synaptics_report_slot_agm(dev, 1, agm);
+		break;
+	}
+
+	/* 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, count);
+
+	input_report_key(dev, BTN_LEFT, sgm->left);
+	input_sync(dev);
+}
+
+static void synaptics_image_sensor_process(struct psmouse *psmouse,
+					   struct synaptics_hw_state *sgm)
+{
+	int count;
+
+	if (sgm->z == 0)
+		count = 0;
+	else if (sgm->w >= 4)
+		count = 1;
+	else if (sgm->w == 0)
+		count = 2;
+	else
+		count = 3;
+
+	/* Send resulting input events to user space */
+	synaptics_report_mt(psmouse, count, sgm);
+}
+
 /*
  *  called for each full received packet from the touchpad
  */
@@ -558,6 +642,11 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	if (synaptics_parse_hw_state(psmouse->packet, priv, &hw))
 		return;
 
+	if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) {
+		synaptics_image_sensor_process(psmouse, &hw);
+		return;
+	}
+
 	if (hw.scroll) {
 		priv->scroll += hw.scroll;
 
@@ -739,7 +828,16 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 	set_abs_position_params(dev, priv, ABS_X, ABS_Y);
 	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
 
-	if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
+	if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) {
+		input_mt_init_slots(dev, 2);
+		set_abs_position_params(dev, priv, ABS_MT_POSITION_X,
+					ABS_MT_POSITION_Y);
+		/* Image sensors can report per-contact pressure */
+		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
+		/* Image sensors can sometimes report per-contact width */
+		input_set_abs_params(dev, ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
+	} else if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
+		/* Non-image sensors with AGM use semi-mt */
 		__set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
 		input_mt_init_slots(dev, 2);
 		set_abs_position_params(dev, priv, ABS_MT_POSITION_X,
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index da8e969..34296b9 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -74,6 +74,8 @@
  * 2	0x04	reduced filtering	firmware does less filtering on
  *					position data, driver should watch
  *					for noise.
+ * 2	0x08	image sensor		image sensor tracks 5 fingers, but only
+ *					reports 2.
  * 2	0x20	report min		query 0x0f gives min coord reported
  */
 #define SYN_CAP_CLICKPAD(ex0c)		((ex0c) & 0x100000) /* 1-button ClickPad */
@@ -82,6 +84,7 @@
 #define SYN_CAP_MIN_DIMENSIONS(ex0c)	((ex0c) & 0x000200)
 #define SYN_CAP_ADV_GESTURE(ex0c)	((ex0c) & 0x080000)
 #define SYN_CAP_REDUCED_FILTERING(ex0c)	((ex0c) & 0x000400)
+#define SYN_CAP_IMAGE_SENSOR(ex0c)	((ex0c) & 0x000800)
 
 /* synaptics modes query bits */
 #define SYN_MODE_ABSOLUTE(m)		((m) & (1 << 7))
-- 
1.7.3.1

[PATCH 5/9 v2] Input: synaptics - decode AGM packet types

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

A Synaptics image sensor tracks 5 fingers, but can only report 2.

The algorithm for choosing which 2 fingers to report and in which packet:
  Touchpad maintains 5 slots, numbered 0 to 4
  Initially all slots are empty
  As new fingers are detected, assign them to the lowest available slots
  The touchpad always reports:
    SGM: lowest numbered non-empty slot
    AGM: highest numbered non-empty slot, if there is one

In addition, these touchpads have a special AGM packet type which reports
the number of fingers currently being tracked, and which finger is in
each of the two slots.  Unfortunately, these "TYPE=2" packets are only used
when more than 3 fingers are being tracked.  When less than 4 fingers
are present, the 'w' value must be used to track how many fingers are
present, and knowing which fingers are being reported is much more
difficult, if not impossible.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   38 +++++++++++++++++++++++++++++++-------
 drivers/input/mouse/synaptics.h |   14 +++++++++++++-
 2 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index a9960d2..ff8c839 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -420,15 +420,39 @@ 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_data *priv,
+				struct synaptics_hw_state *hw)
 {
 	struct synaptics_hw_state *agm = &priv->agm;
+	int agm_packet_type;
+
+	agm_packet_type = (buf[5] & 0x30) >> 4;
+	switch (agm_packet_type) {
+	case 1:
+		/* Gesture packet: (x, y, z) half resolution */
+		agm->w = hw->w;
+		agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
+		agm->y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
+		agm->z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
+		break;
 
-	/* Gesture packet: (x, y, z) at half resolution */
-	agm->x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
-	agm->y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
-	agm->z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
+	case 2:
+		/* AGM-CONTACT packet: (count, sgm, agm) */
+		synaptics_mt_state_set(&agm->mt_state, buf[1], buf[2], buf[4]);
+		break;
+
+	default:
+		break;
+	}
 }
 
 static int synaptics_parse_hw_state(const unsigned char buf[],
@@ -467,7 +491,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 		if ((SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)
 				|| SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
 				&& hw->w == 2) {
-			synaptics_parse_agm(buf, priv);
+			synaptics_parse_agm(buf, priv, hw);
 			return 1;
 		}
 
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 34296b9..0c63357 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -115,9 +115,18 @@
 #define SYN_REDUCED_FILTER_FUZZ		8
 
 /*
- * A structure to describe the state of the touchpad hardware (buttons and pad)
+ * 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 {
 	int x;
 	int y;
@@ -130,6 +139,9 @@ 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 {
-- 
1.7.3.1

[PATCH 6/9 v2] Input: synaptics - process finger (<=3) transitions

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Synaptics image sensor touchpads track 5 fingers, but only report 2.

This patch attempts to deal with some idiosyncrasies of these touchpads:

 * When there are 3 or more fingers, only two are reported.
 * The number of fingers can change at any time, but is only reported in
   SGM packets, thus at a number-of-fingers change, it is not possible
   to tell whether the AGM finger is for the original or new number of
   fingers.
 * When the number of fingers changes from 2->3 it is not
   possible to tell which of the 2 fingers are now reported.
 * When number of fingers changes from 3->2 it is often not possible to
   tell which finger was removed, and which are now being reported.

When 2 or more packets are present on the touchpad, the kernel reports
exactly two MT-B slots containing the position data for the two fingers
reported by the touchpad.
In addition, it reports the total number of fingers using one of the
EV_KEY/BTN_TOOL_*TAP events.  Thus, this is a hybrid singletouch/MT-B scheme.

Userspace can detect this condition by noting that the driver supports
more EV_KEY/BTN_TOOL_*TAP events than its maximum EV_ABS/ABS_MT_SLOT.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |  229 ++++++++++++++++++++++++++++++++++++---
 drivers/input/mouse/synaptics.h |    3 +
 2 files changed, 216 insertions(+), 16 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index ff8c839..b626b98 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -453,6 +453,9 @@ static void synaptics_parse_agm(const unsigned char buf[],
 	default:
 		break;
 	}
+
+	/* Record that at least one AGM has been received since last SGM */
+	priv->agm_pending = true;
 }
 
 static int synaptics_parse_hw_state(const unsigned char buf[],
@@ -600,26 +603,44 @@ static void synaptics_report_slot_agm(struct input_dev *dev, int slot,
 }
 
 static void synaptics_report_mt(struct psmouse *psmouse,
-				int count,
+				struct synaptics_mt_state *mt_state,
 				const struct synaptics_hw_state *sgm)
 {
 	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;
 
-	switch (count) {
+	switch (mt_state->count) {
 	case 0:
 		synaptics_report_slot_empty(dev, 0);
 		synaptics_report_slot_empty(dev, 1);
 		break;
 	case 1:
-		synaptics_report_slot_sgm(dev, 0, sgm);
-		synaptics_report_slot_empty(dev, 1);
+		if (mt_state->sgm == 0) {
+			synaptics_report_slot_sgm(dev, 0, sgm);
+			synaptics_report_slot_empty(dev, 1);
+		} else {
+			synaptics_report_slot_empty(dev, 0);
+			synaptics_report_slot_sgm(dev, 1, sgm);
+		}
 		break;
-	case 2:
-	case 3: /* Fall-through case */
-		synaptics_report_slot_sgm(dev, 0, sgm);
-		synaptics_report_slot_agm(dev, 1, agm);
+	default:
+		/*
+		 * For all other finger counts, report sgm in 0 and agm in 1,
+		 * but only if the sgm/agm is reporting the same finger.
+		 * If either reported finger has changed, invalidate its slot's
+		 * old tracking id, instead.
+		 */
+		if ((old->sgm == -1) || (old->sgm == mt_state->sgm))
+			synaptics_report_slot_sgm(dev, 0, sgm);
+		else
+			synaptics_report_slot_empty(dev, 0);
+
+		if ((old->agm == -1) || (old->agm == mt_state->agm))
+			synaptics_report_slot_agm(dev, 1, agm);
+		else
+			synaptics_report_slot_empty(dev, 1);
 		break;
 	}
 
@@ -627,28 +648,204 @@ static void synaptics_report_mt(struct psmouse *psmouse,
 	input_mt_report_pointer_emulation(dev, false);
 
 	/* Send the number of fingers reported by touchpad itself. */
-	input_mt_report_finger_count(dev, count);
+	input_mt_report_finger_count(dev, mt_state->count);
 
 	input_report_key(dev, BTN_LEFT, sgm->left);
 	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);
+}
+
+/* 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 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);
+		return;
+	}
+
+	switch (old->count) {
+	case 0:
+		synaptics_mt_state_set(mt_state, 1, 0, -1);
+		break;
+	case 1:
+		/*
+		 * If pending AGM and either:
+		 *   (a) the previous SGM slot contains slot 0, or
+		 *   (b) there was no SGM slot
+		 * then SGM now contains slot 1
+		 *
+		 *  (a) The "SGM contains slot 0" case happens with very rapid
+		 * "drum roll" gestures, where slot 0 finger is lifted and a
+		 * new slot 1 finger touches within one reporting interval.
+		 *
+		 *  (b) The "no SGM slot" case 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 become empty, and SGM
+		 * contains a new finger in slot 1)
+		 *
+		 * Else, if there was no previous SGM, it now contains slot 0.
+		 *
+		 * Otherwise, SGM still contains the same slot.
+		 */
+
+		if (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:
+		/*
+		 * 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, empty all slots. We will guess slot 0 on subsequent 1->1
+		 */
+		synaptics_mt_state_set(mt_state, 0, -1, -1);
+		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:
+		/*
+		 * mt_state 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, empty all slots. We will guess slots [0,1] on
+		 * subsequent 2->2
+		 */
+		synaptics_mt_state_set(mt_state, 0, -1, -1);
+		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:
+		/*
+		 * On 2->3 transitions, we are given no indication which finger
+		 * was added.
+		 * We don't even know what finger the current AGM packet
+		 * contained.
+		 *
+		 * So, empty all slots. They get filled on a subsequent 3->3
+		 */
+		synaptics_mt_state_set(mt_state, 0, -1, -1);
+		break;
+	case 3:
+		/*
+		 * mt_state either hasn't changed, or was updated by a recently
+		 * received AGM-CONTACT packet.
+		 */
+		break;
+	}
+}
+
 static void synaptics_image_sensor_process(struct psmouse *psmouse,
 					   struct synaptics_hw_state *sgm)
 {
-	int count;
+	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;
 
+	/*
+	 * Update mt_state using the new finger count and current mt_state.
+	 */
 	if (sgm->z == 0)
-		count = 0;
+		synaptics_image_sensor_0f(priv, &mt_state);
 	else if (sgm->w >= 4)
-		count = 1;
+		synaptics_image_sensor_1f(priv, &mt_state);
 	else if (sgm->w == 0)
-		count = 2;
-	else
-		count = 3;
+		synaptics_image_sensor_2f(priv, &mt_state);
+	else if (sgm->w == 1)
+		synaptics_image_sensor_3f(priv, &mt_state);
 
 	/* Send resulting input events to user space */
-	synaptics_report_mt(psmouse, count, sgm);
+	synaptics_report_mt(psmouse, &mt_state, sgm);
+
+	/* Store updated mt_state */
+	priv->mt_state = agm->mt_state = mt_state;
+	priv->agm_pending = false;
 }
 
 /*
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 0c63357..87be1fe 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -161,11 +161,14 @@ struct synaptics_data {
 
 	struct serio *pt_port;			/* Pass-through serio port */
 
+	struct synaptics_mt_state mt_state;	/* Current mt finger state */
+
 	/*
 	 * 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 */
 };
 
 void synaptics_module_init(void);
-- 
1.7.3.1

[PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

As long as we know which slots are currently contained in SGM and AGM
packets, it is possible to track the slot 0 finger when transitioning from
2->3 fingers.  This is the case when fingers are being added one at a
time, 1->2->3.

However, when fingers are removed, we sometimes lose track of which slots
are contained in SGM and AGM. In particular, when transitioning from 3->2
and sometimes 3->1.  In both of these cases, we can no longer track slot 0
during 2->3 transitions.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   33 +++++++++++++++++++++++++++------
 drivers/input/mouse/synaptics.h |    1 +
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index b626b98..893e567 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -659,6 +659,7 @@ 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 */
@@ -726,6 +727,7 @@ static void synaptics_image_sensor_1f(struct synaptics_data *priv,
 		 * So, empty all slots. We will guess slot 0 on subsequent 1->1
 		 */
 		synaptics_mt_state_set(mt_state, 0, -1, -1);
+		priv->mt_state_lost = true;
 		break;
 	}
 }
@@ -771,6 +773,7 @@ static void synaptics_image_sensor_2f(struct synaptics_data *priv,
 		 * subsequent 2->2
 		 */
 		synaptics_mt_state_set(mt_state, 0, -1, -1);
+		priv->mt_state_lost = true;
 		break;
 	}
 }
@@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
 		break;
 	case 2:
 		/*
-		 * On 2->3 transitions, we are given no indication which finger
-		 * was added.
-		 * We don't even know what finger the current AGM packet
-		 * contained.
+		 * After some 3->1 and all 3->2 transitions, we lose track
+		 * of which slot is reported by sgm and agm.
 		 *
-		 * So, empty all slots. They get filled on a subsequent 3->3
+		 * For 2->3 in this state, empty all slots, and we will guess
+		 * (0,1) on a subsequent 0->3.
+		 *
+		 * To userspace, the resulting transition will look like:
+		 *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
 		 */
-		synaptics_mt_state_set(mt_state, 0, -1, -1);
+		if (priv->mt_state_lost) {
+			synaptics_mt_state_set(mt_state, 0, -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] -> 1:[0,-1] -> 3:[0,2]
+		 */
+		synaptics_mt_state_set(mt_state, 1, 0, -1);
 		break;
 	case 3:
 		/*
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 87be1fe..e3edfea 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -162,6 +162,7 @@ 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
-- 
1.7.3.1

[PATCH 8/9 v2] Input: add BTN_TOOL_QUINTTAP for reporting 5 fingers on touchpad

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/input-mt.c |    1 +
 include/linux/input.h    |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index c48c81f..9150ee7 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -117,6 +117,7 @@ void input_mt_report_finger_count(struct input_dev *dev, int count)
 	input_event(dev, EV_KEY, BTN_TOOL_DOUBLETAP, count == 2);
 	input_event(dev, EV_KEY, BTN_TOOL_TRIPLETAP, count == 3);
 	input_event(dev, EV_KEY, BTN_TOOL_QUADTAP, count == 4);
+	input_event(dev, EV_KEY, BTN_TOOL_QUINTTAP, count == 5);
 }
 EXPORT_SYMBOL(input_mt_report_finger_count);
 
diff --git a/include/linux/input.h b/include/linux/input.h
index 068784e..4de4b46 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -503,6 +503,7 @@ struct input_keymap_entry {
 #define BTN_TOOL_FINGER		0x145
 #define BTN_TOOL_MOUSE		0x146
 #define BTN_TOOL_LENS		0x147
+#define BTN_TOOL_QUINTTAP	0x148	/* Five fingers on trackpad */
 #define BTN_TOUCH		0x14a
 #define BTN_STYLUS		0x14b
 #define BTN_STYLUS2		0x14c
-- 
1.7.3.1

[PATCH 9/9 v2] Input: synaptics - process finger (<=5) transitions

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Synaptics image sensor touchpads track up to 5 fingers, but only report 2.
They use a special "TYPE=2" (AGM-CONTACT) packet type that reports
the number of tracked fingers and which finger is reported in the SGM
and AGM packets.

With this new packet type, it is possible to tell userspace when 4 or 5
fingers are touching.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   44 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 43 insertions(+), 1 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 893e567..2a1e05f 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -729,6 +729,10 @@ static void synaptics_image_sensor_1f(struct synaptics_data *priv,
 		synaptics_mt_state_set(mt_state, 0, -1, -1);
 		priv->mt_state_lost = true;
 		break;
+	case 4:
+	case 5:
+		/* mt_state was updated by AGM-CONTACT packet */
+		break;
 	}
 }
 
@@ -775,6 +779,10 @@ static void synaptics_image_sensor_2f(struct synaptics_data *priv,
 		synaptics_mt_state_set(mt_state, 0, -1, -1);
 		priv->mt_state_lost = true;
 		break;
+	case 4:
+	case 5:
+		/* mt_state was updated by AGM-CONTACT packet */
+		break;
 	}
 }
 
@@ -803,6 +811,22 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
 		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.
 		 *
@@ -836,9 +860,22 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
 		 * 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)
 {
@@ -858,8 +895,10 @@ static void synaptics_image_sensor_process(struct psmouse *psmouse,
 		synaptics_image_sensor_1f(priv, &mt_state);
 	else if (sgm->w == 0)
 		synaptics_image_sensor_2f(priv, &mt_state);
-	else if (sgm->w == 1)
+	else if (sgm->w == 1 && mt_state.count <= 3)
 		synaptics_image_sensor_3f(priv, &mt_state);
+	else
+		synaptics_image_sensor_45f(priv, &mt_state);
 
 	/* Send resulting input events to user space */
 	synaptics_report_mt(psmouse, &mt_state, sgm);
@@ -1078,6 +1117,9 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
 		/* Image sensors can sometimes report per-contact width */
 		input_set_abs_params(dev, ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
+		/* Image sensors can signal 4 and 5 finger clicks */
+		__set_bit(BTN_TOOL_QUADTAP, dev->keybit);
+		__set_bit(BTN_TOOL_QUINTTAP, dev->keybit);
 	} else if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
 		/* Non-image sensors with AGM use semi-mt */
 		__set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
-- 
1.7.3.1

Re: [PATCH 1/9 v2] Input: synaptics - refactor y inversion

[Chase Douglas]

On 07/20/2011 06:38 AM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Synaptics touchpads report increasing y from bottom to top.
> This is inverted from normal userspace "top of screen is 0" coordinates.
> Thus, the kernel driver reports inverted y coordinates to userspace.
> 
> This patch refactors this inversion.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Acked-by: Chase Douglas <chase.douglas@canonical.com>

Re: [PATCH 2/9 v2] Input: synaptics - refactor agm packet parsing

[Chase Douglas]

On 07/20/2011 06:38 AM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> When a Synaptics touchpad is in "AGM" mode, and multiple fingers are
> detected, the touchpad sends alternating "Advanced Gesture Mode" (AGM) and
> "Simple Gesture Mode" (SGM) packets.
>   The AGM packets have w=2, and contain reduced resolution finger data.
>   The SGM packets have w={0,1} and contain full resolution finger data.
> 
> Refactor the parsing of agm packets to its own function, and rename the
> synaptics_data.mt field to .agm to indicate that it contains the contents of
> the last agm packet.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Acked-by: Chase Douglas <chase.douglas@canonical.com>

Re: [PATCH 3/9 v2] Input: synaptics - refactor initialization of abs position axes

[Chase Douglas]

On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Acked-by: Chase Douglas <chase.douglas@canonical.com>

Re: [PATCH 8/9 v2] Input: add BTN_TOOL_QUINTTAP for reporting 5 fingers on touchpad

[Chase Douglas]

On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Meh, I guess... No more after 5!

Acked-by: Chase Douglas <chase.douglas@canonical.com>

Re: [PATCH 9/9 v2] Input: synaptics - process finger (<=5) transitions

[Chase Douglas]

On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Synaptics image sensor touchpads track up to 5 fingers, but only report 2.
> They use a special "TYPE=2" (AGM-CONTACT) packet type that reports
> the number of tracked fingers and which finger is reported in the SGM
> and AGM packets.
> 
> With this new packet type, it is possible to tell userspace when 4 or 5
> fingers are touching.

Maybe I'm blind, but I don't see where the QUADTAP and QUINTAP values
are set in the events. I see where the bits are set during
initialization, but not during use.

> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/mouse/synaptics.c |   44 ++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 43 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 893e567..2a1e05f 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -729,6 +729,10 @@ static void synaptics_image_sensor_1f(struct synaptics_data *priv,
>  		synaptics_mt_state_set(mt_state, 0, -1, -1);
>  		priv->mt_state_lost = true;
>  		break;
> +	case 4:
> +	case 5:
> +		/* mt_state was updated by AGM-CONTACT packet */
> +		break;
>  	}
>  }
>  
> @@ -775,6 +779,10 @@ static void synaptics_image_sensor_2f(struct synaptics_data *priv,
>  		synaptics_mt_state_set(mt_state, 0, -1, -1);
>  		priv->mt_state_lost = true;
>  		break;
> +	case 4:
> +	case 5:
> +		/* mt_state was updated by AGM-CONTACT packet */
> +		break;
>  	}
>  }
>  
> @@ -803,6 +811,22 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>  		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.
>  		 *
> @@ -836,9 +860,22 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>  		 * 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)
>  {
> @@ -858,8 +895,10 @@ static void synaptics_image_sensor_process(struct psmouse *psmouse,
>  		synaptics_image_sensor_1f(priv, &mt_state);
>  	else if (sgm->w == 0)
>  		synaptics_image_sensor_2f(priv, &mt_state);
> -	else if (sgm->w == 1)
> +	else if (sgm->w == 1 && mt_state.count <= 3)
>  		synaptics_image_sensor_3f(priv, &mt_state);
> +	else
> +		synaptics_image_sensor_45f(priv, &mt_state);
>  
>  	/* Send resulting input events to user space */
>  	synaptics_report_mt(psmouse, &mt_state, sgm);
> @@ -1078,6 +1117,9 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>  		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
>  		/* Image sensors can sometimes report per-contact width */
>  		input_set_abs_params(dev, ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
> +		/* Image sensors can signal 4 and 5 finger clicks */
> +		__set_bit(BTN_TOOL_QUADTAP, dev->keybit);
> +		__set_bit(BTN_TOOL_QUINTTAP, dev->keybit);
>  	} else if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
>  		/* Non-image sensors with AGM use semi-mt */
>  		__set_bit(INPUT_PROP_SEMI_MT, dev->propbit);

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Chase Douglas]

On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> As long as we know which slots are currently contained in SGM and AGM
> packets, it is possible to track the slot 0 finger when transitioning from
> 2->3 fingers.  This is the case when fingers are being added one at a
> time, 1->2->3.
> 
> However, when fingers are removed, we sometimes lose track of which slots
> are contained in SGM and AGM. In particular, when transitioning from 3->2
> and sometimes 3->1.  In both of these cases, we can no longer track slot 0
> during 2->3 transitions.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/mouse/synaptics.c |   33 +++++++++++++++++++++++++++------
>  drivers/input/mouse/synaptics.h |    1 +
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index b626b98..893e567 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -659,6 +659,7 @@ 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 */
> @@ -726,6 +727,7 @@ static void synaptics_image_sensor_1f(struct synaptics_data *priv,
>  		 * So, empty all slots. We will guess slot 0 on subsequent 1->1
>  		 */
>  		synaptics_mt_state_set(mt_state, 0, -1, -1);
> +		priv->mt_state_lost = true;
>  		break;
>  	}
>  }
> @@ -771,6 +773,7 @@ static void synaptics_image_sensor_2f(struct synaptics_data *priv,
>  		 * subsequent 2->2
>  		 */
>  		synaptics_mt_state_set(mt_state, 0, -1, -1);
> +		priv->mt_state_lost = true;
>  		break;
>  	}
>  }
> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>  		break;
>  	case 2:
>  		/*
> -		 * On 2->3 transitions, we are given no indication which finger
> -		 * was added.
> -		 * We don't even know what finger the current AGM packet
> -		 * contained.
> +		 * After some 3->1 and all 3->2 transitions, we lose track
> +		 * of which slot is reported by sgm and agm.
>  		 *
> -		 * So, empty all slots. They get filled on a subsequent 3->3
> +		 * For 2->3 in this state, empty all slots, and we will guess
> +		 * (0,1) on a subsequent 0->3.
> +		 *
> +		 * To userspace, the resulting transition will look like:
> +		 *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]

I don't think this should be allowed. We shouldn't be giving userspace
wrong information. One could argue that userspace could watch for these
transitions, but then I would argue that the driver should be handling
this :).

I don't know what the best resolution to this issue is, but any
transition in the number of fingers must be accurate. In uTouch, we
watch for touch count transitions for "continuation" gestures.

>  		 */
> -		synaptics_mt_state_set(mt_state, 0, -1, -1);
> +		if (priv->mt_state_lost) {
> +			synaptics_mt_state_set(mt_state, 0, -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] -> 1:[0,-1] -> 3:[0,2]
> +		 */
> +		synaptics_mt_state_set(mt_state, 1, 0, -1);
>  		break;
>  	case 3:
>  		/*
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index 87be1fe..e3edfea 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -162,6 +162,7 @@ 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

Re: [PATCH 6/9 v2] Input: synaptics - process finger (<=3) transitions

[Chase Douglas]

On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Synaptics image sensor touchpads track 5 fingers, but only report 2.
> 
> This patch attempts to deal with some idiosyncrasies of these touchpads:
> 
>  * When there are 3 or more fingers, only two are reported.
>  * The number of fingers can change at any time, but is only reported in
>    SGM packets, thus at a number-of-fingers change, it is not possible
>    to tell whether the AGM finger is for the original or new number of
>    fingers.
>  * When the number of fingers changes from 2->3 it is not
>    possible to tell which of the 2 fingers are now reported.
>  * When number of fingers changes from 3->2 it is often not possible to
>    tell which finger was removed, and which are now being reported.
> 
> When 2 or more packets are present on the touchpad, the kernel reports
                 ^touches or fingers

Re: [PATCH 0/9 v2] Synaptics image sensor support

[Chase Douglas]

On 07/20/2011 06:38 AM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> Patches 4-7 add up to 3 finger support for image sensor touchpads.
> Image sensors do not suffer from the finger tracking issues that plagued
> the earlier "profile sensors", and which required the invention of "semi-mt"
> (Semi-mt reports a bounding box around two fingers instead of the fingers
> themselves).  Instead, the image sensors report the actual positions of two
> fingers using the same "Advanced Gesture Mode".  This driver uses two MT-B slots
> to report these two fingers to userspace.  In addition, it will also report
> the total number of fingers using BTN_TOOL_*TAP EV_KEY events.
> Userspace drivers should be aware that the number of fingers reported via
> BTN_TOOL_*TAP can be greater than the total number MT-B slots with non-negative
> track_ids.  Upon opening the device node, userspace should query the maximum
> values supported ABS_MT_SLOT, and note the number of supported BTN_TOOL_*TAP
> events.

I would still rather see a new property bit for devices that track and
report different numbers of touches. I think it is worthwhile to be
explicit here rather than leave it up to voodoo magic in userpace drivers.

This and the touch count transition issue I brought up in a reply to
patch 7 are my only concerns with this part of the patchset.

BTW, I found the commit messages and the code to be very comprehensible.
Good job on cleaning them up!

-- Chase

Re: [PATCH 9/9 v2] Input: synaptics - process finger (<=5) transitions

[Daniel Kurtz]

Hi Chase,

Thanks for all of your reviews!

On Sat, Jul 23, 2011 at 9:02 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
>
> On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> >
> > Synaptics image sensor touchpads track up to 5 fingers, but only report 2.
> > They use a special "TYPE=2" (AGM-CONTACT) packet type that reports
> > the number of tracked fingers and which finger is reported in the SGM
> > and AGM packets.
> >
> > With this new packet type, it is possible to tell userspace when 4 or 5
> > fingers are touching.
>
> Maybe I'm blind, but I don't see where the QUADTAP and QUINTAP values
> are set in the events. I see where the bits are set during
> initialization, but not during use.

It's subtle.
The firmware actually report 4/5 in the AGM-CONTACT packet, which the
agm packet parser sets directly in mt_state->count.
This is then reported to userspace when synaptics_report_mt() calls
input_mt_report_finger_count(dev, mt_state->count), which now supports
QUINTTAP (see patch #8).

-Daniel

>
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > ---
> >  drivers/input/mouse/synaptics.c |   44 ++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 43 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index 893e567..2a1e05f 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -729,6 +729,10 @@ static void synaptics_image_sensor_1f(struct synaptics_data *priv,
> >               synaptics_mt_state_set(mt_state, 0, -1, -1);
> >               priv->mt_state_lost = true;
> >               break;
> > +     case 4:
> > +     case 5:
> > +             /* mt_state was updated by AGM-CONTACT packet */
> > +             break;
> >       }
> >  }
> >
> > @@ -775,6 +779,10 @@ static void synaptics_image_sensor_2f(struct synaptics_data *priv,
> >               synaptics_mt_state_set(mt_state, 0, -1, -1);
> >               priv->mt_state_lost = true;
> >               break;
> > +     case 4:
> > +     case 5:
> > +             /* mt_state was updated by AGM-CONTACT packet */
> > +             break;
> >       }
> >  }
> >
> > @@ -803,6 +811,22 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
> >               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.
> >                *
> > @@ -836,9 +860,22 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
> >                * 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)
> >  {
> > @@ -858,8 +895,10 @@ static void synaptics_image_sensor_process(struct psmouse *psmouse,
> >               synaptics_image_sensor_1f(priv, &mt_state);
> >       else if (sgm->w == 0)
> >               synaptics_image_sensor_2f(priv, &mt_state);
> > -     else if (sgm->w == 1)
> > +     else if (sgm->w == 1 && mt_state.count <= 3)
> >               synaptics_image_sensor_3f(priv, &mt_state);
> > +     else
> > +             synaptics_image_sensor_45f(priv, &mt_state);
> >
> >       /* Send resulting input events to user space */
> >       synaptics_report_mt(psmouse, &mt_state, sgm);
> > @@ -1078,6 +1117,9 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
> >               input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> >               /* Image sensors can sometimes report per-contact width */
> >               input_set_abs_params(dev, ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
> > +             /* Image sensors can signal 4 and 5 finger clicks */
> > +             __set_bit(BTN_TOOL_QUADTAP, dev->keybit);
> > +             __set_bit(BTN_TOOL_QUINTTAP, dev->keybit);
> >       } else if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
> >               /* Non-image sensors with AGM use semi-mt */
> >               __set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
>

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Daniel Kurtz]

Hi Chase,

On Sat, Jul 23, 2011 at 9:07 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
>> From: Daniel Kurtz <djkurtz@chromium.org>
>>
>> As long as we know which slots are currently contained in SGM and AGM
>> packets, it is possible to track the slot 0 finger when transitioning from
>> 2->3 fingers.  This is the case when fingers are being added one at a
>> time, 1->2->3.
>>
>> However, when fingers are removed, we sometimes lose track of which slots
>> are contained in SGM and AGM. In particular, when transitioning from 3->2
>> and sometimes 3->1.  In both of these cases, we can no longer track slot 0
>> during 2->3 transitions.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/input/mouse/synaptics.c |   33 +++++++++++++++++++++++++++------
>>  drivers/input/mouse/synaptics.h |    1 +
>>  2 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index b626b98..893e567 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -659,6 +659,7 @@ 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 */
>> @@ -726,6 +727,7 @@ static void synaptics_image_sensor_1f(struct synaptics_data *priv,
>>                * So, empty all slots. We will guess slot 0 on subsequent 1->1
>>                */
>>               synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +             priv->mt_state_lost = true;
>>               break;
>>       }
>>  }
>> @@ -771,6 +773,7 @@ static void synaptics_image_sensor_2f(struct synaptics_data *priv,
>>                * subsequent 2->2
>>                */
>>               synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +             priv->mt_state_lost = true;
>>               break;
>>       }
>>  }
>> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>>               break;
>>       case 2:
>>               /*
>> -              * On 2->3 transitions, we are given no indication which finger
>> -              * was added.
>> -              * We don't even know what finger the current AGM packet
>> -              * contained.
>> +              * After some 3->1 and all 3->2 transitions, we lose track
>> +              * of which slot is reported by sgm and agm.
>>                *
>> -              * So, empty all slots. They get filled on a subsequent 3->3
>> +              * For 2->3 in this state, empty all slots, and we will guess
>> +              * (0,1) on a subsequent 0->3.
>> +              *
>> +              * To userspace, the resulting transition will look like:
>> +              *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
>
> I don't think this should be allowed. We shouldn't be giving userspace
> wrong information. One could argue that userspace could watch for these
> transitions, but then I would argue that the driver should be handling
> this :).
>
> I don't know what the best resolution to this issue is, but any
> transition in the number of fingers must be accurate. In uTouch, we
> watch for touch count transitions for "continuation" gestures.

So, you want the count to be accurate.
But, during these transitions, there is not enough information to
guarantee all of the following at the same time:
 (1) finger count
 (2) track_id
 (3) finger position

Would you prefer an implementation that continued to report count (via
BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
these cases where it could not determine the correct position or track_id
to report?

It seems like it would be more work for userspace to handle this new way
than the simulated number-of-touch transitions, where the transient
states are all normal valid states.

-Daniel

>
>>                */
>> -             synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +             if (priv->mt_state_lost) {
>> +                     synaptics_mt_state_set(mt_state, 0, -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] -> 1:[0,-1] -> 3:[0,2]
>> +              */
>> +             synaptics_mt_state_set(mt_state, 1, 0, -1);
>>               break;
>>       case 3:
>>               /*
>> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
>> index 87be1fe..e3edfea 100644
>> --- a/drivers/input/mouse/synaptics.h
>> +++ b/drivers/input/mouse/synaptics.h
>> @@ -162,6 +162,7 @@ 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
>
>

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Daniel Kurtz]

Hi Chase,

On Sat, Jul 23, 2011 at 9:07 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
>> From: Daniel Kurtz <djkurtz@chromium.org>
>>
>> As long as we know which slots are currently contained in SGM and AGM
>> packets, it is possible to track the slot 0 finger when transitioning from
>> 2->3 fingers.  This is the case when fingers are being added one at a
>> time, 1->2->3.
>>
>> However, when fingers are removed, we sometimes lose track of which slots
>> are contained in SGM and AGM. In particular, when transitioning from 3->2
>> and sometimes 3->1.  In both of these cases, we can no longer track slot 0
>> during 2->3 transitions.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/input/mouse/synaptics.c |   33 +++++++++++++++++++++++++++------
>>  drivers/input/mouse/synaptics.h |    1 +
>>  2 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index b626b98..893e567 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -659,6 +659,7 @@ 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 */
>> @@ -726,6 +727,7 @@ static void synaptics_image_sensor_1f(struct synaptics_data *priv,
>>                * So, empty all slots. We will guess slot 0 on subsequent 1->1
>>                */
>>               synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +             priv->mt_state_lost = true;
>>               break;
>>       }
>>  }
>> @@ -771,6 +773,7 @@ static void synaptics_image_sensor_2f(struct synaptics_data *priv,
>>                * subsequent 2->2
>>                */
>>               synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +             priv->mt_state_lost = true;
>>               break;
>>       }
>>  }
>> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>>               break;
>>       case 2:
>>               /*
>> -              * On 2->3 transitions, we are given no indication which finger
>> -              * was added.
>> -              * We don't even know what finger the current AGM packet
>> -              * contained.
>> +              * After some 3->1 and all 3->2 transitions, we lose track
>> +              * of which slot is reported by sgm and agm.
>>                *
>> -              * So, empty all slots. They get filled on a subsequent 3->3
>> +              * For 2->3 in this state, empty all slots, and we will guess
>> +              * (0,1) on a subsequent 0->3.
>> +              *
>> +              * To userspace, the resulting transition will look like:
>> +              *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
>
> I don't think this should be allowed. We shouldn't be giving userspace
> wrong information. One could argue that userspace could watch for these
> transitions, but then I would argue that the driver should be handling
> this :).
>
> I don't know what the best resolution to this issue is, but any
> transition in the number of fingers must be accurate. In uTouch, we
> watch for touch count transitions for "continuation" gestures.

So, you want the count to be accurate.
But, during these transitions, there is not enough information to
guarantee all of the following at the same time:
 (1) finger count
 (2) track_id
 (3) finger position

Would you prefer an implementation that continued to report count (via
BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
these cases where it could not determine the correct position or track_id
to report?

It seems like it would be more work for userspace to handle this new way
than the simulated number-of-touch transitions, where the transient
states are all normal valid states.

-Daniel

>
>>                */
>> -             synaptics_mt_state_set(mt_state, 0, -1, -1);
>> +             if (priv->mt_state_lost) {
>> +                     synaptics_mt_state_set(mt_state, 0, -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] -> 1:[0,-1] -> 3:[0,2]
>> +              */
>> +             synaptics_mt_state_set(mt_state, 1, 0, -1);
>>               break;
>>       case 3:
>>               /*
>> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
>> index 87be1fe..e3edfea 100644
>> --- a/drivers/input/mouse/synaptics.h
>> +++ b/drivers/input/mouse/synaptics.h
>> @@ -162,6 +162,7 @@ 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/9 v2] Input: synaptics - refactor y inversion

[Dmitry Torokhov]

On Wed, Jul 20, 2011 at 09:38:58PM +0800, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Synaptics touchpads report increasing y from bottom to top.
> This is inverted from normal userspace "top of screen is 0" coordinates.
> Thus, the kernel driver reports inverted y coordinates to userspace.
> 
> This patch refactors this inversion.
> 

I really do not see the point...

Thanks.

-- 
Dmitry

Re: [PATCH 8/9 v2] Input: add BTN_TOOL_QUINTTAP for reporting 5 fingers on touchpad

[Dmitry Torokhov]

On Wed, Jul 20, 2011 at 09:39:05PM +0800, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 

Do we know what we want to do with this gesture? Because if not I'd
rather not merge it for now, until there are real users.

Thanks.

-- 
Dmitry

Re: [PATCH 8/9 v2] Input: add BTN_TOOL_QUINTTAP for reporting 5 fingers on touchpad

[Daniel Kurtz]

On Mon, Jul 25, 2011 at 4:29 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Wed, Jul 20, 2011 at 09:39:05PM +0800, djkurtz@chromium.org wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> >
>
> Do we know what we want to do with this gesture? Because if not I'd
> rather not merge it for now, until there are real users.

And do what instead?
Report 5 fingers with BTN_TOOL_QUADTAP?

> Thanks.
>
> --
> Dmitry

Re: [PATCH 8/9 v2] Input: add BTN_TOOL_QUINTTAP for reporting 5 fingers on touchpad

[Dmitry Torokhov]

On Mon, Jul 25, 2011 at 05:14:22PM +0800, Daniel Kurtz wrote:
> On Mon, Jul 25, 2011 at 4:29 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Wed, Jul 20, 2011 at 09:39:05PM +0800, djkurtz@chromium.org wrote:
> > > From: Daniel Kurtz <djkurtz@chromium.org>
> > >
> >
> > Do we know what we want to do with this gesture? Because if not I'd
> > rather not merge it for now, until there are real users.
> 
> And do what instead?
> Report 5 fingers with BTN_TOOL_QUADTAP?
> 

That, or nothing... The fact that the device can report 5 finger touch
does not mean that anybody cares about this.

-- 
Dmitry

Re: [PATCH 8/9 v2] Input: add BTN_TOOL_QUINTTAP for reporting 5 fingers on touchpad

[Daniel Kurtz]

Hi Dmitry, et al,

On Tue, Jul 26, 2011 at 2:16 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Mon, Jul 25, 2011 at 05:14:22PM +0800, Daniel Kurtz wrote:
> > On Mon, Jul 25, 2011 at 4:29 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Wed, Jul 20, 2011 at 09:39:05PM +0800, djkurtz@chromium.org wrote:
> > > > From: Daniel Kurtz <djkurtz@chromium.org>
> > > >
> > >
> > > Do we know what we want to do with this gesture? Because if not I'd
> > > rather not merge it for now, until there are real users.
> >
> > And do what instead?
> > Report 5 fingers with BTN_TOOL_QUADTAP?
> >
>
> That, or nothing... The fact that the device can report 5 finger touch
> does not mean that anybody cares about this.
> --
> Dmitry

The only current use of this is to detect "resting thumb + 4-finger-scroll".
"4-finger scroll" is a gesture supported by some applications and
operating systems.
"resting thumb" is when a clickpad user rests a finger (typically a
thumb), in the bottom left "click zone" in anticipation of
click+move=select gestures.
Thus, this 4-finger scroll is actually sometimes a 5-finger gesture.

Similarly, I work with many touchpads from different vendors, some of
which do actually provide 5 independent finger coordinates.  The
drivers for these touchpads truly send 5 MT-B slots when 5 fingers are
present.

Should these drivers perform input_mt_report_pointer_emulation()?
If so, should we add BTN_TOOL_QUINTTAP to allow them to report 5
fingers in emulation mode?
Or is that ridiculous, since emulation is only for old userspace
programs which wouldn't know what to do with QUINTTAP, anyway?

-Dan

Re: [PATCH 1/9 v2] Input: synaptics - refactor y inversion

[Daniel Kurtz]

On Mon, Jul 25, 2011 at 4:27 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Jul 20, 2011 at 09:38:58PM +0800, djkurtz@chromium.org wrote:
>> From: Daniel Kurtz <djkurtz@chromium.org>
>>
>> Synaptics touchpads report increasing y from bottom to top.
>> This is inverted from normal userspace "top of screen is 0" coordinates.
>> Thus, the kernel driver reports inverted y coordinates to userspace.
>>
>> This patch refactors this inversion.
>>
>
> I really do not see the point...

The point is that this inversion is done in 3 places in the driver.
Refactoring lets us document why this inversion is happening in just a
single place.

>
> Thanks.
>
> --
> Dmitry
>

Re: [PATCH 1/9 v2] Input: synaptics - refactor y inversion

[Chase Douglas]

On 07/25/2011 01:27 AM, Dmitry Torokhov wrote:
> On Wed, Jul 20, 2011 at 09:38:58PM +0800, djkurtz@chromium.org wrote:
>> From: Daniel Kurtz <djkurtz@chromium.org>
>>
>> Synaptics touchpads report increasing y from bottom to top.
>> This is inverted from normal userspace "top of screen is 0" coordinates.
>> Thus, the kernel driver reports inverted y coordinates to userspace.
>>
>> This patch refactors this inversion.
>>
> 
> I really do not see the point...

It's easier for me to read and comprehend, at least. I agree that it's
really really minor, but if it's in the middle of 10 other patches, then
I don't have a problem with it.

-- Chase

Re: [PATCH 8/9 v2] Input: add BTN_TOOL_QUINTTAP for reporting 5 fingers on touchpad

[Chase Douglas]

On 07/25/2011 11:16 AM, Dmitry Torokhov wrote:
> On Mon, Jul 25, 2011 at 05:14:22PM +0800, Daniel Kurtz wrote:
>> On Mon, Jul 25, 2011 at 4:29 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>>
>>> On Wed, Jul 20, 2011 at 09:39:05PM +0800, djkurtz@chromium.org wrote:
>>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>>
>>>
>>> Do we know what we want to do with this gesture? Because if not I'd
>>> rather not merge it for now, until there are real users.
>>
>> And do what instead?
>> Report 5 fingers with BTN_TOOL_QUADTAP?
>>
> 
> That, or nothing... The fact that the device can report 5 finger touch
> does not mean that anybody cares about this.

The uTouch gesture stack supports up to 5 touch gestures, and although
most of them are either hard to do on a trackpad or are not possible
with this driver, I can think of uses for 5 finger tap. Unity uses four
finger tap quite effectively I believe.

There is a meaningful limit to this: five fingers on a human hand. It
seems more arbitrary to stick the protocol limit at four fingers.

-- Chase

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Chase Douglas]

On 07/22/2011 09:36 PM, Daniel Kurtz wrote:
> On Sat, Jul 23, 2011 at 9:07 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>> On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>>>               break;
>>>       case 2:
>>>               /*
>>> -              * On 2->3 transitions, we are given no indication which finger
>>> -              * was added.
>>> -              * We don't even know what finger the current AGM packet
>>> -              * contained.
>>> +              * After some 3->1 and all 3->2 transitions, we lose track
>>> +              * of which slot is reported by sgm and agm.
>>>                *
>>> -              * So, empty all slots. They get filled on a subsequent 3->3
>>> +              * For 2->3 in this state, empty all slots, and we will guess
>>> +              * (0,1) on a subsequent 0->3.
>>> +              *
>>> +              * To userspace, the resulting transition will look like:
>>> +              *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
>>
>> I don't think this should be allowed. We shouldn't be giving userspace
>> wrong information. One could argue that userspace could watch for these
>> transitions, but then I would argue that the driver should be handling
>> this :).
>>
>> I don't know what the best resolution to this issue is, but any
>> transition in the number of fingers must be accurate. In uTouch, we
>> watch for touch count transitions for "continuation" gestures.
> 
> So, you want the count to be accurate.
> But, during these transitions, there is not enough information to
> guarantee all of the following at the same time:
>  (1) finger count
>  (2) track_id
>  (3) finger position

If I had to pick one to give up, it would be the tracking id. It carries
the least useful information for a device that you may not get the whole
touch stream. Semi-mt devices already forsake the tracking id value,
IIRC. The id is always incremented when you create a new slot, but when
you go from 2->3 fingers the ids in the slots stay the same even if the
third finger expands the bounding box.

> Would you prefer an implementation that continued to report count (via
> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
> these cases where it could not determine the correct position or track_id
> to report?

That may be doable, but I would prefer to just assume that tracking ids
are not valid when (tracked touches > reported touches).

> It seems like it would be more work for userspace to handle this new way
> than the simulated number-of-touch transitions, where the transient
> states are all normal valid states.

This harkens back to my earlier statements where I said this new
Synaptics protocol is worse than the previous one :).

I agree that the implementation you gave here might be trickier for
userspace, so I'd rather table it unless you feel that the "tracking ids
are meaningless!" solution won't work. If you think there are problems
with that approach, we can re-evaluate.

Thanks!

-- Chase

Re: [PATCH 9/9 v2] Input: synaptics - process finger (<=5) transitions

[Chase Douglas]

On 07/22/2011 09:11 PM, Daniel Kurtz wrote:
> On Sat, Jul 23, 2011 at 9:02 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>>
>> On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>
>>> Synaptics image sensor touchpads track up to 5 fingers, but only report 2.
>>> They use a special "TYPE=2" (AGM-CONTACT) packet type that reports
>>> the number of tracked fingers and which finger is reported in the SGM
>>> and AGM packets.
>>>
>>> With this new packet type, it is possible to tell userspace when 4 or 5
>>> fingers are touching.
>>
>> Maybe I'm blind, but I don't see where the QUADTAP and QUINTAP values
>> are set in the events. I see where the bits are set during
>> initialization, but not during use.
> 
> It's subtle.
> The firmware actually report 4/5 in the AGM-CONTACT packet, which the
> agm packet parser sets directly in mt_state->count.
> This is then reported to userspace when synaptics_report_mt() calls
> input_mt_report_finger_count(dev, mt_state->count), which now supports
> QUINTTAP (see patch #8).

Ahh, thanks! Conditional upon getting the previous patches merged in
some fashion:

Acked-by: Chase Douglas <chase.douglas@canonical.com>

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Daniel Kurtz]

On Wed, Jul 27, 2011 at 7:14 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On 07/22/2011 09:36 PM, Daniel Kurtz wrote:
>> On Sat, Jul 23, 2011 at 9:07 AM, Chase Douglas
>> <chase.douglas@canonical.com> wrote:
>>> On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
>>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>>>>               break;
>>>>       case 2:
>>>>               /*
>>>> -              * On 2->3 transitions, we are given no indication which finger
>>>> -              * was added.
>>>> -              * We don't even know what finger the current AGM packet
>>>> -              * contained.
>>>> +              * After some 3->1 and all 3->2 transitions, we lose track
>>>> +              * of which slot is reported by sgm and agm.
>>>>                *
>>>> -              * So, empty all slots. They get filled on a subsequent 3->3
>>>> +              * For 2->3 in this state, empty all slots, and we will guess
>>>> +              * (0,1) on a subsequent 0->3.
>>>> +              *
>>>> +              * To userspace, the resulting transition will look like:
>>>> +              *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
>>>
>>> I don't think this should be allowed. We shouldn't be giving userspace
>>> wrong information. One could argue that userspace could watch for these
>>> transitions, but then I would argue that the driver should be handling
>>> this :).
>>>
>>> I don't know what the best resolution to this issue is, but any
>>> transition in the number of fingers must be accurate. In uTouch, we
>>> watch for touch count transitions for "continuation" gestures.
>>
>> So, you want the count to be accurate.
>> But, during these transitions, there is not enough information to
>> guarantee all of the following at the same time:
>>  (1) finger count
>>  (2) track_id
>>  (3) finger position
>
> If I had to pick one to give up, it would be the tracking id. It carries
> the least useful information for a device that you may not get the whole
> touch stream. Semi-mt devices already forsake the tracking id value,
> IIRC. The id is always incremented when you create a new slot, but when
> you go from 2->3 fingers the ids in the slots stay the same even if the
> third finger expands the bounding box.

For synaptics profile sensors, adding additional fingers does not
change which fingers are reported.
You always get the first two fingers.
AFAICT, you cannot "expand the bounding box" by adding new fingers.
Thus, throwing away the track_id is irrelevant, since once the semi-mt
driver starts reporting 2+ fingers, the track_ids are fixed.

The image sensor does not work that way.
As you add new fingers, the trackpad will, under certain conditions,
switch to reporting the locations of these new fingers.
Changing track_id is how we report this to userspace.

>
>> Would you prefer an implementation that continued to report count (via
>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>> these cases where it could not determine the correct position or track_id
>> to report?
>
> That may be doable, but I would prefer to just assume that tracking ids
> are not valid when (tracked touches > reported touches).

Userspace is free to make this assumption, of course, but, in fact,
the image sensor trackpad actually does a pretty good job of tracking
the fingers - it just has serious issues reporting them!
Since a track_id change is how userspace is told that the identity of
the reported finger is changing, if the track_id of a finger position
datum is unknowable, I'd rather just discard it in the kernel than
report it to userspace with the wrong track_id.
Otherwise, the heuristics used in the userspace finger tracking
algorithms would need to be overly aggressively tuned to handle this
known error cases:
  2->3 and 3->2 finger transitions look like 2nd finger motion,
instead of reported finger changes.

>
>> It seems like it would be more work for userspace to handle this new way
>> than the simulated number-of-touch transitions, where the transient
>> states are all normal valid states.
>
> This harkens back to my earlier statements where I said this new
> Synaptics protocol is worse than the previous one :).
>
> I agree that the implementation you gave here might be trickier for
> userspace, so I'd rather table it unless you feel that the "tracking ids
> are meaningless!" solution won't work. If you think there are problems
> with that approach, we can re-evaluate.
>
> Thanks!
>
> -- Chase

Yes, I feel there are problems with this approach, as I tried to explain above.
Can you explain why you 'continuation gestures' can't handle 1->2->3
finger transitions looking like 1->2->1->3, and 3->2->3 looking like
3->2->0->3?

I think the only real point left to decide is what BTN_* events should
signify during these rare transition states:
  (a) the actually number of fingers on the pad,
  (b) the number of fingers being reported via the slots

The current patchset does (a).
We could do (b), if that would get these patches accepted sooner :)

Thanks!
-Dan

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Daniel Kurtz]

On Wed, Jul 27, 2011 at 7:14 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On 07/22/2011 09:36 PM, Daniel Kurtz wrote:
>> On Sat, Jul 23, 2011 at 9:07 AM, Chase Douglas
>> <chase.douglas@canonical.com> wrote:
>>> On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
>>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>>>>               break;
>>>>       case 2:
>>>>               /*
>>>> -              * On 2->3 transitions, we are given no indication which finger
>>>> -              * was added.
>>>> -              * We don't even know what finger the current AGM packet
>>>> -              * contained.
>>>> +              * After some 3->1 and all 3->2 transitions, we lose track
>>>> +              * of which slot is reported by sgm and agm.
>>>>                *
>>>> -              * So, empty all slots. They get filled on a subsequent 3->3
>>>> +              * For 2->3 in this state, empty all slots, and we will guess
>>>> +              * (0,1) on a subsequent 0->3.
>>>> +              *
>>>> +              * To userspace, the resulting transition will look like:
>>>> +              *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
>>>
>>> I don't think this should be allowed. We shouldn't be giving userspace
>>> wrong information. One could argue that userspace could watch for these
>>> transitions, but then I would argue that the driver should be handling
>>> this :).
>>>
>>> I don't know what the best resolution to this issue is, but any
>>> transition in the number of fingers must be accurate. In uTouch, we
>>> watch for touch count transitions for "continuation" gestures.
>>
>> So, you want the count to be accurate.
>> But, during these transitions, there is not enough information to
>> guarantee all of the following at the same time:
>>  (1) finger count
>>  (2) track_id
>>  (3) finger position
>
> If I had to pick one to give up, it would be the tracking id. It carries
> the least useful information for a device that you may not get the whole
> touch stream. Semi-mt devices already forsake the tracking id value,
> IIRC. The id is always incremented when you create a new slot, but when
> you go from 2->3 fingers the ids in the slots stay the same even if the
> third finger expands the bounding box.

For synaptics profile sensors, adding additional fingers does not
change which fingers are reported.
You always get the first two fingers.
AFAICT, you cannot "expand the bounding box" by adding new fingers.
Thus, throwing away the track_id is irrelevant, since once the semi-mt
driver starts reporting 2+ fingers, the track_ids are fixed.

The image sensor does not work that way.
As you add new fingers, the trackpad will, under certain conditions,
switch to reporting the locations of these new fingers.
Changing track_id is how we report this to userspace.

>
>> Would you prefer an implementation that continued to report count (via
>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>> these cases where it could not determine the correct position or track_id
>> to report?
>
> That may be doable, but I would prefer to just assume that tracking ids
> are not valid when (tracked touches > reported touches).

Userspace is free to make this assumption, of course, but, in fact,
the image sensor trackpad actually does a pretty good job of tracking
the fingers - it just has serious issues reporting them!
Since a track_id change is how userspace is told that the identity of
the reported finger is changing, if the track_id of a finger position
datum is unknowable, I'd rather just discard it in the kernel than
report it to userspace with the wrong track_id.
Otherwise, the heuristics used in the userspace finger tracking
algorithms would need to be overly aggressively tuned to handle this
known error cases:
  2->3 and 3->2 finger transitions look like 2nd finger motion,
instead of reported finger changes.

>
>> It seems like it would be more work for userspace to handle this new way
>> than the simulated number-of-touch transitions, where the transient
>> states are all normal valid states.
>
> This harkens back to my earlier statements where I said this new
> Synaptics protocol is worse than the previous one :).
>
> I agree that the implementation you gave here might be trickier for
> userspace, so I'd rather table it unless you feel that the "tracking ids
> are meaningless!" solution won't work. If you think there are problems
> with that approach, we can re-evaluate.
>
> Thanks!
>
> -- Chase

Yes, I feel there are problems with this approach, as I tried to explain above.
Can you explain why you 'continuation gestures' can't handle 1->2->3
finger transitions looking like 1->2->1->3, and 3->2->3 looking like
3->2->0->3?

I think the only real point left to decide is what BTN_* events should
signify during these rare transition states:
  (a) the actually number of fingers on the pad,
  (b) the number of fingers being reported via the slots

The current patchset does (a).
We could do (b), if that would get these patches accepted sooner :)

Thanks!
-Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Chase Douglas]

On 07/26/2011 09:48 PM, Daniel Kurtz wrote:
> On Wed, Jul 27, 2011 at 7:14 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>> On 07/22/2011 09:36 PM, Daniel Kurtz wrote:
>>> On Sat, Jul 23, 2011 at 9:07 AM, Chase Douglas
>>> <chase.douglas@canonical.com> wrote:
>>>> On 07/20/2011 06:39 AM, djkurtz@chromium.org wrote:
>>>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>>> @@ -800,14 +803,32 @@ static void synaptics_image_sensor_3f(struct synaptics_data *priv,
>>>>>               break;
>>>>>       case 2:
>>>>>               /*
>>>>> -              * On 2->3 transitions, we are given no indication which finger
>>>>> -              * was added.
>>>>> -              * We don't even know what finger the current AGM packet
>>>>> -              * contained.
>>>>> +              * After some 3->1 and all 3->2 transitions, we lose track
>>>>> +              * of which slot is reported by sgm and agm.
>>>>>                *
>>>>> -              * So, empty all slots. They get filled on a subsequent 3->3
>>>>> +              * For 2->3 in this state, empty all slots, and we will guess
>>>>> +              * (0,1) on a subsequent 0->3.
>>>>> +              *
>>>>> +              * To userspace, the resulting transition will look like:
>>>>> +              *    2:[0,1] -> 0:[-1,-1] -> 3:[0,2]
>>>>
>>>> I don't think this should be allowed. We shouldn't be giving userspace
>>>> wrong information. One could argue that userspace could watch for these
>>>> transitions, but then I would argue that the driver should be handling
>>>> this :).
>>>>
>>>> I don't know what the best resolution to this issue is, but any
>>>> transition in the number of fingers must be accurate. In uTouch, we
>>>> watch for touch count transitions for "continuation" gestures.
>>>
>>> So, you want the count to be accurate.
>>> But, during these transitions, there is not enough information to
>>> guarantee all of the following at the same time:
>>>  (1) finger count
>>>  (2) track_id
>>>  (3) finger position
>>
>> If I had to pick one to give up, it would be the tracking id. It carries
>> the least useful information for a device that you may not get the whole
>> touch stream. Semi-mt devices already forsake the tracking id value,
>> IIRC. The id is always incremented when you create a new slot, but when
>> you go from 2->3 fingers the ids in the slots stay the same even if the
>> third finger expands the bounding box.
> 
> For synaptics profile sensors, adding additional fingers does not
> change which fingers are reported.
> You always get the first two fingers.
> AFAICT, you cannot "expand the bounding box" by adding new fingers.
> Thus, throwing away the track_id is irrelevant, since once the semi-mt
> driver starts reporting 2+ fingers, the track_ids are fixed.

Hmmm... you're right. I was completely mistaken here. This changes some
of my thinking about how to handle these devices.

> The image sensor does not work that way.
> As you add new fingers, the trackpad will, under certain conditions,
> switch to reporting the locations of these new fingers.
> Changing track_id is how we report this to userspace.
> 
>>
>>> Would you prefer an implementation that continued to report count (via
>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>> these cases where it could not determine the correct position or track_id
>>> to report?
>>
>> That may be doable, but I would prefer to just assume that tracking ids
>> are not valid when (tracked touches > reported touches).
> 
> Userspace is free to make this assumption, of course, but, in fact,
> the image sensor trackpad actually does a pretty good job of tracking
> the fingers - it just has serious issues reporting them!
> Since a track_id change is how userspace is told that the identity of
> the reported finger is changing, if the track_id of a finger position
> datum is unknowable, I'd rather just discard it in the kernel than
> report it to userspace with the wrong track_id.
> Otherwise, the heuristics used in the userspace finger tracking
> algorithms would need to be overly aggressively tuned to handle this
> known error cases:
>   2->3 and 3->2 finger transitions look like 2nd finger motion,
> instead of reported finger changes.
> 
>>
>>> It seems like it would be more work for userspace to handle this new way
>>> than the simulated number-of-touch transitions, where the transient
>>> states are all normal valid states.
>>
>> This harkens back to my earlier statements where I said this new
>> Synaptics protocol is worse than the previous one :).
>>
>> I agree that the implementation you gave here might be trickier for
>> userspace, so I'd rather table it unless you feel that the "tracking ids
>> are meaningless!" solution won't work. If you think there are problems
>> with that approach, we can re-evaluate.
>>
>> Thanks!
>>
>> -- Chase
> 
> Yes, I feel there are problems with this approach, as I tried to explain above.
> Can you explain why you 'continuation gestures' can't handle 1->2->3
> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
> 3->2->0->3?
> 
> I think the only real point left to decide is what BTN_* events should
> signify during these rare transition states:
>   (a) the actually number of fingers on the pad,
>   (b) the number of fingers being reported via the slots
> 
> The current patchset does (a).
> We could do (b), if that would get these patches accepted sooner :)

I was thinking that the current patchset does (b). I think (a) is
better, and if it really works that way then I'm fine with it. It's hard
for me to keep track of the flow of the logic across the patches :).

That said, merging this patchset as is effectively means that the number
of slots is completely decoupled from the number of touches on the
device. Previously, one could say that the number of touches on the
device was equal to the number of open slots or more if all slots were
open. With this approach, we could have 0 slots open during a transition
when there are still touches down.

While the distinction makes sense for these synaptics devices, I don't
want the semantics to hold for full multitouch devices. Otherwise, we
would have to add many more BTN_*TAPs. If we go this route, we must have
a way to tell userspace that this is a special device where the number
of active touches can only be determined from BTN_*TAP. Thus, we would
need a property for this exception to normal behavior.

(PS: As I've thought more about it, I don't think we need the property I
was advocating for before. That property was for denoting that the
device tracks more than it reports. If we're going to get this complex
in the protocol, there's not much you can do with bitmask properties to
denote every specific special case.)

-- Chase

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Daniel Kurtz]

On Thu, Jul 28, 2011 at 5:13 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
[...]
>>>
>>>> Would you prefer an implementation that continued to report count (via
>>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>>> these cases where it could not determine the correct position or track_id
>>>> to report?
>>>
>>> That may be doable, but I would prefer to just assume that tracking ids
>>> are not valid when (tracked touches > reported touches).
>>
>> Userspace is free to make this assumption, of course, but, in fact,
>> the image sensor trackpad actually does a pretty good job of tracking
>> the fingers - it just has serious issues reporting them!
>> Since a track_id change is how userspace is told that the identity of
>> the reported finger is changing, if the track_id of a finger position
>> datum is unknowable, I'd rather just discard it in the kernel than
>> report it to userspace with the wrong track_id.
>> Otherwise, the heuristics used in the userspace finger tracking
>> algorithms would need to be overly aggressively tuned to handle this
>> known error cases:
>>   2->3 and 3->2 finger transitions look like 2nd finger motion,
>> instead of reported finger changes.
>>
>>>
>>>> It seems like it would be more work for userspace to handle this new way
>>>> than the simulated number-of-touch transitions, where the transient
>>>> states are all normal valid states.
>>>
>>> This harkens back to my earlier statements where I said this new
>>> Synaptics protocol is worse than the previous one :).
>>>
>>> I agree that the implementation you gave here might be trickier for
>>> userspace, so I'd rather table it unless you feel that the "tracking ids
>>> are meaningless!" solution won't work. If you think there are problems
>>> with that approach, we can re-evaluate.
>>>
>>> Thanks!
>>>
>>> -- Chase
>>
>> Yes, I feel there are problems with this approach, as I tried to explain above.
>> Can you explain why you 'continuation gestures' can't handle 1->2->3
>> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
>> 3->2->0->3?
>>
>> I think the only real point left to decide is what BTN_* events should
>> signify during these rare transition states:
>>   (a) the actually number of fingers on the pad,
>>   (b) the number of fingers being reported via the slots
>>
>> The current patchset does (a).
>> We could do (b), if that would get these patches accepted sooner :)
>
> I was thinking that the current patchset does (b). I think (a) is
> better, and if it really works that way then I'm fine with it. It's hard
> for me to keep track of the flow of the logic across the patches :).

Argh, my bad.  You are correct.  Current patchset does (b)!
It reports the number of active slots, not the number of touches.

In any case, I really don't know why you need (a).  We are talking
about some degenerate transitions here.  Your userspace driver should
work just fine with the (b) driver, it just loses some really
complicated continued gestures for hardware that can't support them.

>
> That said, merging this patchset as is effectively means that the number
> of slots is completely decoupled from the number of touches on the
> device. Previously, one could say that the number of touches on the
> device was equal to the number of open slots or more if all slots were
> open. With this approach, we could have 0 slots open during a transition
> when there are still touches down.
>
> While the distinction makes sense for these synaptics devices, I don't
> want the semantics to hold for full multitouch devices. Otherwise, we
> would have to add many more BTN_*TAPs. If we go this route, we must have
> a way to tell userspace that this is a special device where the number
> of active touches can only be determined from BTN_*TAP. Thus, we would
> need a property for this exception to normal behavior.

Henrik & Dmitry roughly suggested "do not define a new property; let
userspace detect max BTN_*TAP > ABS_MT_SLOT.max to indicate that
BTN_*TAP carries the total number of touches".  I wish they would
chime in on this patchset...

>
> (PS: As I've thought more about it, I don't think we need the property I
> was advocating for before. That property was for denoting that the
> device tracks more than it reports. If we're going to get this complex
> in the protocol, there's not much you can do with bitmask properties to
> denote every specific special case.)
>
> -- Chase

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Chase Douglas]

On 07/27/2011 06:00 PM, Daniel Kurtz wrote:
> On Thu, Jul 28, 2011 at 5:13 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
> [...]
>>>>
>>>>> Would you prefer an implementation that continued to report count (via
>>>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>>>> these cases where it could not determine the correct position or track_id
>>>>> to report?
>>>>
>>>> That may be doable, but I would prefer to just assume that tracking ids
>>>> are not valid when (tracked touches > reported touches).
>>>
>>> Userspace is free to make this assumption, of course, but, in fact,
>>> the image sensor trackpad actually does a pretty good job of tracking
>>> the fingers - it just has serious issues reporting them!
>>> Since a track_id change is how userspace is told that the identity of
>>> the reported finger is changing, if the track_id of a finger position
>>> datum is unknowable, I'd rather just discard it in the kernel than
>>> report it to userspace with the wrong track_id.
>>> Otherwise, the heuristics used in the userspace finger tracking
>>> algorithms would need to be overly aggressively tuned to handle this
>>> known error cases:
>>>   2->3 and 3->2 finger transitions look like 2nd finger motion,
>>> instead of reported finger changes.
>>>
>>>>
>>>>> It seems like it would be more work for userspace to handle this new way
>>>>> than the simulated number-of-touch transitions, where the transient
>>>>> states are all normal valid states.
>>>>
>>>> This harkens back to my earlier statements where I said this new
>>>> Synaptics protocol is worse than the previous one :).
>>>>
>>>> I agree that the implementation you gave here might be trickier for
>>>> userspace, so I'd rather table it unless you feel that the "tracking ids
>>>> are meaningless!" solution won't work. If you think there are problems
>>>> with that approach, we can re-evaluate.
>>>>
>>>> Thanks!
>>>>
>>>> -- Chase
>>>
>>> Yes, I feel there are problems with this approach, as I tried to explain above.
>>> Can you explain why you 'continuation gestures' can't handle 1->2->3
>>> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
>>> 3->2->0->3?
>>>
>>> I think the only real point left to decide is what BTN_* events should
>>> signify during these rare transition states:
>>>   (a) the actually number of fingers on the pad,
>>>   (b) the number of fingers being reported via the slots
>>>
>>> The current patchset does (a).
>>> We could do (b), if that would get these patches accepted sooner :)
>>
>> I was thinking that the current patchset does (b). I think (a) is
>> better, and if it really works that way then I'm fine with it. It's hard
>> for me to keep track of the flow of the logic across the patches :).
> 
> Argh, my bad.  You are correct.  Current patchset does (b)!
> It reports the number of active slots, not the number of touches.
> 
> In any case, I really don't know why you need (a).  We are talking
> about some degenerate transitions here.  Your userspace driver should
> work just fine with the (b) driver, it just loses some really
> complicated continued gestures for hardware that can't support them.

To give userspace incorrect information about the number of touches on
the device is always bad. Lets say the degenerate case is when you go
from two touches to three (maybe Synaptics doesn't do this, but someone
else might). When the user performs a three finger tap, we'd see two
touches down, two lifted up, three touches down, three lifted up in
short succession. Userspace is gonna get pretty confused about that :).

(Please don't make me try to come up with a use case we already have in
Unity that would be broken for Synaptics due to this. I could probably
find one, but it would take quite a bit of thinking. :)

>> That said, merging this patchset as is effectively means that the number
>> of slots is completely decoupled from the number of touches on the
>> device. Previously, one could say that the number of touches on the
>> device was equal to the number of open slots or more if all slots were
>> open. With this approach, we could have 0 slots open during a transition
>> when there are still touches down.
>>
>> While the distinction makes sense for these synaptics devices, I don't
>> want the semantics to hold for full multitouch devices. Otherwise, we
>> would have to add many more BTN_*TAPs. If we go this route, we must have
>> a way to tell userspace that this is a special device where the number
>> of active touches can only be determined from BTN_*TAP. Thus, we would
>> need a property for this exception to normal behavior.
> 
> Henrik & Dmitry roughly suggested "do not define a new property; let
> userspace detect max BTN_*TAP > ABS_MT_SLOT.max to indicate that
> BTN_*TAP carries the total number of touches".  I wish they would
> chime in on this patchset...

I think it sets a really bad precedent to obey the stated protocol in
most cases, but disregard it in others if specific conditions are met,
which are not documented and are not presented in a clear manner to
userspace. At the *very least*, this change would need documentation to
go in at the same time, but I strongly believe a property is merited here.

-- Chase

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Daniel Kurtz]

On Thu, Jul 28, 2011 at 10:07 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On 07/27/2011 06:00 PM, Daniel Kurtz wrote:
>> On Thu, Jul 28, 2011 at 5:13 AM, Chase Douglas
>> <chase.douglas@canonical.com> wrote:
>> [...]
>>>>>
>>>>>> Would you prefer an implementation that continued to report count (via
>>>>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>>>>> these cases where it could not determine the correct position or track_id
>>>>>> to report?
>>>>>
>>>>> That may be doable, but I would prefer to just assume that tracking ids
>>>>> are not valid when (tracked touches > reported touches).
>>>>
>>>> Userspace is free to make this assumption, of course, but, in fact,
>>>> the image sensor trackpad actually does a pretty good job of tracking
>>>> the fingers - it just has serious issues reporting them!
>>>> Since a track_id change is how userspace is told that the identity of
>>>> the reported finger is changing, if the track_id of a finger position
>>>> datum is unknowable, I'd rather just discard it in the kernel than
>>>> report it to userspace with the wrong track_id.
>>>> Otherwise, the heuristics used in the userspace finger tracking
>>>> algorithms would need to be overly aggressively tuned to handle this
>>>> known error cases:
>>>>   2->3 and 3->2 finger transitions look like 2nd finger motion,
>>>> instead of reported finger changes.
>>>>
>>>>>
>>>>>> It seems like it would be more work for userspace to handle this new way
>>>>>> than the simulated number-of-touch transitions, where the transient
>>>>>> states are all normal valid states.
>>>>>
>>>>> This harkens back to my earlier statements where I said this new
>>>>> Synaptics protocol is worse than the previous one :).
>>>>>
>>>>> I agree that the implementation you gave here might be trickier for
>>>>> userspace, so I'd rather table it unless you feel that the "tracking ids
>>>>> are meaningless!" solution won't work. If you think there are problems
>>>>> with that approach, we can re-evaluate.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> -- Chase
>>>>
>>>> Yes, I feel there are problems with this approach, as I tried to explain above.
>>>> Can you explain why you 'continuation gestures' can't handle 1->2->3
>>>> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
>>>> 3->2->0->3?
>>>>
>>>> I think the only real point left to decide is what BTN_* events should
>>>> signify during these rare transition states:
>>>>   (a) the actually number of fingers on the pad,
>>>>   (b) the number of fingers being reported via the slots
>>>>
>>>> The current patchset does (a).
>>>> We could do (b), if that would get these patches accepted sooner :)
>>>
>>> I was thinking that the current patchset does (b). I think (a) is
>>> better, and if it really works that way then I'm fine with it. It's hard
>>> for me to keep track of the flow of the logic across the patches :).
>>
>> Argh, my bad.  You are correct.  Current patchset does (b)!
>> It reports the number of active slots, not the number of touches.
>>
>> In any case, I really don't know why you need (a).  We are talking
>> about some degenerate transitions here.  Your userspace driver should
>> work just fine with the (b) driver, it just loses some really
>> complicated continued gestures for hardware that can't support them.
>
> To give userspace incorrect information about the number of touches on
> the device is always bad. Lets say the degenerate case is when you go
> from two touches to three (maybe Synaptics doesn't do this, but someone
> else might). When the user performs a three finger tap, we'd see two
> touches down, two lifted up, three touches down, three lifted up in
> short succession. Userspace is gonna get pretty confused about that :).
>
> (Please don't make me try to come up with a use case we already have in
> Unity that would be broken for Synaptics due to this. I could probably
> find one, but it would take quite a bit of thinking. :)

Just to be clear:

Debouncing num-fingers at the start of a tap works just fine.
For a 3f-tap, you would see one of:
  0->1->2->3
  0->2->3
  0->3

Not:
  0->1->2->0->3
  0->2->0->3

You only see 2->0->3 if there had previously been 3 fingers down, and
some of those fingers are removed:
  0->3->0*->2*->0*->3*
  0->3->0*->1*

Where the "*" states are when mt_state_lost = true.
So, with method (b), you might see 3->0->2->0 if the release of the
other two fingers was really slow (longer than 37.5 ms), or, more
likely on a tap, just 3->0.

In any case, I don't think we are making progress arguing (a) vs. (b).
Let me implement method (a), and upload for review.
I agree that it makes some sense to always accurately report number of
touches with the BTN_*, independent of number of slots.

A true MT-B userspace app would always do the right thing with the
slots, legacy apps can always do the right thing in legacy mode, and a
hybrid app get do 2-touch multitouch & use BTN_* to determine total
number of touches.

Was there anything else I should add/change while I'm at it?

You mention documentation below, was there something in particular
that you'd like to see documented better?  Where?

-Dan

>
>>> That said, merging this patchset as is effectively means that the number
>>> of slots is completely decoupled from the number of touches on the
>>> device. Previously, one could say that the number of touches on the
>>> device was equal to the number of open slots or more if all slots were
>>> open. With this approach, we could have 0 slots open during a transition
>>> when there are still touches down.
>>>
>>> While the distinction makes sense for these synaptics devices, I don't
>>> want the semantics to hold for full multitouch devices. Otherwise, we
>>> would have to add many more BTN_*TAPs. If we go this route, we must have
>>> a way to tell userspace that this is a special device where the number
>>> of active touches can only be determined from BTN_*TAP. Thus, we would
>>> need a property for this exception to normal behavior.
>>
>> Henrik & Dmitry roughly suggested "do not define a new property; let
>> userspace detect max BTN_*TAP > ABS_MT_SLOT.max to indicate that
>> BTN_*TAP carries the total number of touches".  I wish they would
>> chime in on this patchset...
>
> I think it sets a really bad precedent to obey the stated protocol in
> most cases, but disregard it in others if specific conditions are met,
> which are not documented and are not presented in a clear manner to
> userspace. At the *very least*, this change would need documentation to
> go in at the same time, but I strongly believe a property is merited here.
>
> -- Chase

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Daniel Kurtz]

On Thu, Jul 28, 2011 at 10:07 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On 07/27/2011 06:00 PM, Daniel Kurtz wrote:
>> On Thu, Jul 28, 2011 at 5:13 AM, Chase Douglas
>> <chase.douglas@canonical.com> wrote:
>> [...]
>>>>>
>>>>>> Would you prefer an implementation that continued to report count (via
>>>>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>>>>> these cases where it could not determine the correct position or track_id
>>>>>> to report?
>>>>>
>>>>> That may be doable, but I would prefer to just assume that tracking ids
>>>>> are not valid when (tracked touches > reported touches).
>>>>
>>>> Userspace is free to make this assumption, of course, but, in fact,
>>>> the image sensor trackpad actually does a pretty good job of tracking
>>>> the fingers - it just has serious issues reporting them!
>>>> Since a track_id change is how userspace is told that the identity of
>>>> the reported finger is changing, if the track_id of a finger position
>>>> datum is unknowable, I'd rather just discard it in the kernel than
>>>> report it to userspace with the wrong track_id.
>>>> Otherwise, the heuristics used in the userspace finger tracking
>>>> algorithms would need to be overly aggressively tuned to handle this
>>>> known error cases:
>>>>   2->3 and 3->2 finger transitions look like 2nd finger motion,
>>>> instead of reported finger changes.
>>>>
>>>>>
>>>>>> It seems like it would be more work for userspace to handle this new way
>>>>>> than the simulated number-of-touch transitions, where the transient
>>>>>> states are all normal valid states.
>>>>>
>>>>> This harkens back to my earlier statements where I said this new
>>>>> Synaptics protocol is worse than the previous one :).
>>>>>
>>>>> I agree that the implementation you gave here might be trickier for
>>>>> userspace, so I'd rather table it unless you feel that the "tracking ids
>>>>> are meaningless!" solution won't work. If you think there are problems
>>>>> with that approach, we can re-evaluate.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> -- Chase
>>>>
>>>> Yes, I feel there are problems with this approach, as I tried to explain above.
>>>> Can you explain why you 'continuation gestures' can't handle 1->2->3
>>>> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
>>>> 3->2->0->3?
>>>>
>>>> I think the only real point left to decide is what BTN_* events should
>>>> signify during these rare transition states:
>>>>   (a) the actually number of fingers on the pad,
>>>>   (b) the number of fingers being reported via the slots
>>>>
>>>> The current patchset does (a).
>>>> We could do (b), if that would get these patches accepted sooner :)
>>>
>>> I was thinking that the current patchset does (b). I think (a) is
>>> better, and if it really works that way then I'm fine with it. It's hard
>>> for me to keep track of the flow of the logic across the patches :).
>>
>> Argh, my bad.  You are correct.  Current patchset does (b)!
>> It reports the number of active slots, not the number of touches.
>>
>> In any case, I really don't know why you need (a).  We are talking
>> about some degenerate transitions here.  Your userspace driver should
>> work just fine with the (b) driver, it just loses some really
>> complicated continued gestures for hardware that can't support them.
>
> To give userspace incorrect information about the number of touches on
> the device is always bad. Lets say the degenerate case is when you go
> from two touches to three (maybe Synaptics doesn't do this, but someone
> else might). When the user performs a three finger tap, we'd see two
> touches down, two lifted up, three touches down, three lifted up in
> short succession. Userspace is gonna get pretty confused about that :).
>
> (Please don't make me try to come up with a use case we already have in
> Unity that would be broken for Synaptics due to this. I could probably
> find one, but it would take quite a bit of thinking. :)

Just to be clear:

Debouncing num-fingers at the start of a tap works just fine.
For a 3f-tap, you would see one of:
  0->1->2->3
  0->2->3
  0->3

Not:
  0->1->2->0->3
  0->2->0->3

You only see 2->0->3 if there had previously been 3 fingers down, and
some of those fingers are removed:
  0->3->0*->2*->0*->3*
  0->3->0*->1*

Where the "*" states are when mt_state_lost = true.
So, with method (b), you might see 3->0->2->0 if the release of the
other two fingers was really slow (longer than 37.5 ms), or, more
likely on a tap, just 3->0.

In any case, I don't think we are making progress arguing (a) vs. (b).
Let me implement method (a), and upload for review.
I agree that it makes some sense to always accurately report number of
touches with the BTN_*, independent of number of slots.

A true MT-B userspace app would always do the right thing with the
slots, legacy apps can always do the right thing in legacy mode, and a
hybrid app get do 2-touch multitouch & use BTN_* to determine total
number of touches.

Was there anything else I should add/change while I'm at it?

You mention documentation below, was there something in particular
that you'd like to see documented better?  Where?

-Dan

>
>>> That said, merging this patchset as is effectively means that the number
>>> of slots is completely decoupled from the number of touches on the
>>> device. Previously, one could say that the number of touches on the
>>> device was equal to the number of open slots or more if all slots were
>>> open. With this approach, we could have 0 slots open during a transition
>>> when there are still touches down.
>>>
>>> While the distinction makes sense for these synaptics devices, I don't
>>> want the semantics to hold for full multitouch devices. Otherwise, we
>>> would have to add many more BTN_*TAPs. If we go this route, we must have
>>> a way to tell userspace that this is a special device where the number
>>> of active touches can only be determined from BTN_*TAP. Thus, we would
>>> need a property for this exception to normal behavior.
>>
>> Henrik & Dmitry roughly suggested "do not define a new property; let
>> userspace detect max BTN_*TAP > ABS_MT_SLOT.max to indicate that
>> BTN_*TAP carries the total number of touches".  I wish they would
>> chime in on this patchset...
>
> I think it sets a really bad precedent to obey the stated protocol in
> most cases, but disregard it in others if specific conditions are met,
> which are not documented and are not presented in a clear manner to
> userspace. At the *very least*, this change would need documentation to
> go in at the same time, but I strongly believe a property is merited here.
>
> -- Chase
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/9 v2] Input: synaptics - improved 2->3 finger transition handling

[Chase Douglas]

On 07/28/2011 06:56 AM, Daniel Kurtz wrote:
> On Thu, Jul 28, 2011 at 10:07 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>> On 07/27/2011 06:00 PM, Daniel Kurtz wrote:
>>> On Thu, Jul 28, 2011 at 5:13 AM, Chase Douglas
>>> <chase.douglas@canonical.com> wrote:
>>> [...]
>>>>>>
>>>>>>> Would you prefer an implementation that continued to report count (via
>>>>>>> BTN_TOUCH*) correctly, but dropped down to 0 or 1 MT-B slots when for
>>>>>>> these cases where it could not determine the correct position or track_id
>>>>>>> to report?
>>>>>>
>>>>>> That may be doable, but I would prefer to just assume that tracking ids
>>>>>> are not valid when (tracked touches > reported touches).
>>>>>
>>>>> Userspace is free to make this assumption, of course, but, in fact,
>>>>> the image sensor trackpad actually does a pretty good job of tracking
>>>>> the fingers - it just has serious issues reporting them!
>>>>> Since a track_id change is how userspace is told that the identity of
>>>>> the reported finger is changing, if the track_id of a finger position
>>>>> datum is unknowable, I'd rather just discard it in the kernel than
>>>>> report it to userspace with the wrong track_id.
>>>>> Otherwise, the heuristics used in the userspace finger tracking
>>>>> algorithms would need to be overly aggressively tuned to handle this
>>>>> known error cases:
>>>>>   2->3 and 3->2 finger transitions look like 2nd finger motion,
>>>>> instead of reported finger changes.
>>>>>
>>>>>>
>>>>>>> It seems like it would be more work for userspace to handle this new way
>>>>>>> than the simulated number-of-touch transitions, where the transient
>>>>>>> states are all normal valid states.
>>>>>>
>>>>>> This harkens back to my earlier statements where I said this new
>>>>>> Synaptics protocol is worse than the previous one :).
>>>>>>
>>>>>> I agree that the implementation you gave here might be trickier for
>>>>>> userspace, so I'd rather table it unless you feel that the "tracking ids
>>>>>> are meaningless!" solution won't work. If you think there are problems
>>>>>> with that approach, we can re-evaluate.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> -- Chase
>>>>>
>>>>> Yes, I feel there are problems with this approach, as I tried to explain above.
>>>>> Can you explain why you 'continuation gestures' can't handle 1->2->3
>>>>> finger transitions looking like 1->2->1->3, and 3->2->3 looking like
>>>>> 3->2->0->3?
>>>>>
>>>>> I think the only real point left to decide is what BTN_* events should
>>>>> signify during these rare transition states:
>>>>>   (a) the actually number of fingers on the pad,
>>>>>   (b) the number of fingers being reported via the slots
>>>>>
>>>>> The current patchset does (a).
>>>>> We could do (b), if that would get these patches accepted sooner :)
>>>>
>>>> I was thinking that the current patchset does (b). I think (a) is
>>>> better, and if it really works that way then I'm fine with it. It's hard
>>>> for me to keep track of the flow of the logic across the patches :).
>>>
>>> Argh, my bad.  You are correct.  Current patchset does (b)!
>>> It reports the number of active slots, not the number of touches.
>>>
>>> In any case, I really don't know why you need (a).  We are talking
>>> about some degenerate transitions here.  Your userspace driver should
>>> work just fine with the (b) driver, it just loses some really
>>> complicated continued gestures for hardware that can't support them.
>>
>> To give userspace incorrect information about the number of touches on
>> the device is always bad. Lets say the degenerate case is when you go
>> from two touches to three (maybe Synaptics doesn't do this, but someone
>> else might). When the user performs a three finger tap, we'd see two
>> touches down, two lifted up, three touches down, three lifted up in
>> short succession. Userspace is gonna get pretty confused about that :).
>>
>> (Please don't make me try to come up with a use case we already have in
>> Unity that would be broken for Synaptics due to this. I could probably
>> find one, but it would take quite a bit of thinking. :)
> 
> Just to be clear:
> 
> Debouncing num-fingers at the start of a tap works just fine.
> For a 3f-tap, you would see one of:
>   0->1->2->3
>   0->2->3
>   0->3
> 
> Not:
>   0->1->2->0->3
>   0->2->0->3
> 
> You only see 2->0->3 if there had previously been 3 fingers down, and
> some of those fingers are removed:
>   0->3->0*->2*->0*->3*
>   0->3->0*->1*
> 
> Where the "*" states are when mt_state_lost = true.
> So, with method (b), you might see 3->0->2->0 if the release of the
> other two fingers was really slow (longer than 37.5 ms), or, more
> likely on a tap, just 3->0.
> 
> In any case, I don't think we are making progress arguing (a) vs. (b).
> Let me implement method (a), and upload for review.
> I agree that it makes some sense to always accurately report number of
> touches with the BTN_*, independent of number of slots.
> 
> A true MT-B userspace app would always do the right thing with the
> slots, legacy apps can always do the right thing in legacy mode, and a
> hybrid app get do 2-touch multitouch & use BTN_* to determine total
> number of touches.
> 
> Was there anything else I should add/change while I'm at it?

No, I think functionally I would be happy with the new version. I still
want the property bit :). Dmitry, can you weigh in here? What I remember
was you were disinclined towards a new property bit, but I'd like to see
a firm decision taken and the reasoning behind it.

> You mention documentation below, was there something in particular
> that you'd like to see documented better?  Where?

Documentation of anything that goes against the normal protocol should
be in Documentation/input/event-codes.txt and/or
Documentation/input/multi-touch-protocol.txt. The latter seems the most
appropriate place for this. If you want extra credit you could document
SEMI_MT as well, I think that slipped through the cracks; this isn't a
requirement for me, though :).

Thanks!

>>>> That said, merging this patchset as is effectively means that the number
>>>> of slots is completely decoupled from the number of touches on the
>>>> device. Previously, one could say that the number of touches on the
>>>> device was equal to the number of open slots or more if all slots were
>>>> open. With this approach, we could have 0 slots open during a transition
>>>> when there are still touches down.
>>>>
>>>> While the distinction makes sense for these synaptics devices, I don't
>>>> want the semantics to hold for full multitouch devices. Otherwise, we
>>>> would have to add many more BTN_*TAPs. If we go this route, we must have
>>>> a way to tell userspace that this is a special device where the number
>>>> of active touches can only be determined from BTN_*TAP. Thus, we would
>>>> need a property for this exception to normal behavior.
>>>
>>> Henrik & Dmitry roughly suggested "do not define a new property; let
>>> userspace detect max BTN_*TAP > ABS_MT_SLOT.max to indicate that
>>> BTN_*TAP carries the total number of touches".  I wish they would
>>> chime in on this patchset...
>>
>> I think it sets a really bad precedent to obey the stated protocol in
>> most cases, but disregard it in others if specific conditions are met,
>> which are not documented and are not presented in a clear manner to
>> userspace. At the *very least*, this change would need documentation to
>> go in at the same time, but I strongly believe a property is merited here.
>>
>> -- Chase
> 

Re: [PATCH 8/9 v2] Input: add BTN_TOOL_QUINTTAP for reporting 5 fingers on touchpad

[Henrik Rydberg]

> > That, or nothing... The fact that the device can report 5 finger touch
> > does not mean that anybody cares about this.
> > --
> > Dmitry
> 
> The only current use of this is to detect "resting thumb + 4-finger-scroll".
> "4-finger scroll" is a gesture supported by some applications and
> operating systems.
> "resting thumb" is when a clickpad user rests a finger (typically a
> thumb), in the bottom left "click zone" in anticipation of
> click+move=select gestures.
> Thus, this 4-finger scroll is actually sometimes a 5-finger gesture.

Which is actually the same reason QUADTAP was added originally... are
we entering a recursion here? ;-)

I tend to agree with Dmitry - without a clear usage, it makes sense to
instead wait to see which comes first - a five-finger usecase or a
true MT device that everybody can afford.

> Similarly, I work with many touchpads from different vendors, some of
> which do actually provide 5 independent finger coordinates.  The
> drivers for these touchpads truly send 5 MT-B slots when 5 fingers are
> present.
> 
> Should these drivers perform input_mt_report_pointer_emulation()?
> If so, should we add BTN_TOOL_QUINTTAP to allow them to report 5
> fingers in emulation mode?

Pointer emulation is a separate issue from the number of finger
reports.

> Or is that ridiculous, since emulation is only for old userspace
> programs which wouldn't know what to do with QUINTTAP, anyway?

I would say yes. ;-)

Cheers,
Henrik

Re: [PATCH 8/9 v2] Input: add BTN_TOOL_QUINTTAP for reporting 5 fingers on touchpad

[Henrik Rydberg]

> > That, or nothing... The fact that the device can report 5 finger touch
> > does not mean that anybody cares about this.
> > --
> > Dmitry
> 
> The only current use of this is to detect "resting thumb + 4-finger-scroll".
> "4-finger scroll" is a gesture supported by some applications and
> operating systems.
> "resting thumb" is when a clickpad user rests a finger (typically a
> thumb), in the bottom left "click zone" in anticipation of
> click+move=select gestures.
> Thus, this 4-finger scroll is actually sometimes a 5-finger gesture.

Which is actually the same reason QUADTAP was added originally... are
we entering a recursion here? ;-)

I tend to agree with Dmitry - without a clear usage, it makes sense to
instead wait to see which comes first - a five-finger usecase or a
true MT device that everybody can afford.

> Similarly, I work with many touchpads from different vendors, some of
> which do actually provide 5 independent finger coordinates.  The
> drivers for these touchpads truly send 5 MT-B slots when 5 fingers are
> present.
> 
> Should these drivers perform input_mt_report_pointer_emulation()?
> If so, should we add BTN_TOOL_QUINTTAP to allow them to report 5
> fingers in emulation mode?

Pointer emulation is a separate issue from the number of finger
reports.

> Or is that ridiculous, since emulation is only for old userspace
> programs which wouldn't know what to do with QUINTTAP, anyway?

I would say yes. ;-)

Cheers,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html