[patch 0/5] Bug fix and win8 for hid-multitouch

[patch 0/5] Bug fix and win8 for hid-multitouch

[benjamin.tissoires]

Hi Guys,

The first patch fixes the bug I discovered lately. This bug was related to the out
of bound bitfield test, so it concerns every known devices. Ideally, it should go
to upstream-fixes, but this would requires additional adaptations.
Jiri, How do you want to proceed?

The last 4 patches are the beginning of the support of Win8 devices. It's only the
beginning as the specification is much more precise than the Win7, and we will need
to do more tuning in the future. However, Win8 devices are retro-compatible with
Win7, so they will work out of the box though.

Cheers,
Benjamin

[PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[benjamin.tissoires]

From: Benjamin Tissoires <benjamin.tissoires@enac.fr>

The previous implementation introduced a randomness in the splitting
of the different touches reported by the device. This version is more
robust as we don't rely on hi->input->absbit, but on our own structure.

This also prepares hid-multitouch to better support Win8 devices.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 drivers/hid/hid-multitouch.c |   58 +++++++++++++++++++++++++++++++++--------
 1 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 84bb32e..c6ffb05 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -70,9 +70,16 @@ struct mt_class {
 	bool is_indirect;	/* true for touchpads */
 };
 
+struct mt_fields {
+	unsigned usages[HID_MAX_FIELDS];
+	unsigned int length;
+};
+
 struct mt_device {
 	struct mt_slot curdata;	/* placeholder of incoming data */
 	struct mt_class mtclass;	/* our mt device class */
+	struct mt_fields *fields;	/* temporary placeholder for storing the
+					   multitouch fields */
 	unsigned last_field_index;	/* last field index of the report */
 	unsigned last_slot_field;	/* the last field of a slot */
 	__s8 inputmode;		/* InputMode HID feature, -1 if non-existent */
@@ -275,11 +282,15 @@ static void set_abs(struct input_dev *input, unsigned int code,
 	input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
 }
 
-static void set_last_slot_field(struct hid_usage *usage, struct mt_device *td,
+static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
 		struct hid_input *hi)
 {
-	if (!test_bit(usage->hid, hi->input->absbit))
-		td->last_slot_field = usage->hid;
+	struct mt_fields *f = td->fields;
+
+	if (f->length >= HID_MAX_FIELDS)
+		return;
+
+	f->usages[f->length++] = usage->hid;
 }
 
 static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
@@ -330,7 +341,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_X, field, cls->sn_move);
-			set_last_slot_field(usage, td, hi);
+			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_GD_Y:
@@ -340,7 +351,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_Y, field, cls->sn_move);
-			set_last_slot_field(usage, td, hi);
+			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
 		}
@@ -349,24 +360,24 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	case HID_UP_DIGITIZER:
 		switch (usage->hid) {
 		case HID_DG_INRANGE:
-			set_last_slot_field(usage, td, hi);
+			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONFIDENCE:
-			set_last_slot_field(usage, td, hi);
+			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_TIPSWITCH:
 			hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
 			input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
-			set_last_slot_field(usage, td, hi);
+			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTID:
 			if (!td->maxcontacts)
 				td->maxcontacts = MT_DEFAULT_MAXCONTACT;
 			input_mt_init_slots(hi->input, td->maxcontacts);
-			td->last_slot_field = usage->hid;
+			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			td->touches_by_report++;
 			return 1;
@@ -375,7 +386,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 					EV_ABS, ABS_MT_TOUCH_MAJOR);
 			set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field,
 				cls->sn_width);
-			set_last_slot_field(usage, td, hi);
+			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_HEIGHT:
@@ -385,7 +396,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_height);
 			input_set_abs_params(hi->input,
 					ABS_MT_ORIENTATION, 0, 1, 0, 0);
-			set_last_slot_field(usage, td, hi);
+			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_TIPPRESSURE:
@@ -396,7 +407,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_PRESSURE, field,
 				cls->sn_pressure);
-			set_last_slot_field(usage, td, hi);
+			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTCOUNT:
@@ -650,6 +661,16 @@ static void mt_post_parse_default_settings(struct mt_device *td)
 	td->mtclass.quirks = quirks;
 }
 
+static void mt_post_parse(struct mt_device *td)
+{
+	struct mt_fields *f = td->fields;
+
+	if (td->touches_by_report > 0) {
+		int field_count_per_touch = f->length / td->touches_by_report;
+		td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
+	}
+}
+
 static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	int ret, i;
@@ -680,6 +701,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	td->maxcontact_report_id = -1;
 	hid_set_drvdata(hdev, td);
 
+	td->fields = kzalloc(sizeof(struct mt_fields), GFP_KERNEL);
+	if (!td->fields) {
+		dev_err(&hdev->dev, "cannot allocate multitouch fields data\n");
+		ret = -ENOMEM;
+		goto fail;
+	}
+
 	ret = hid_parse(hdev);
 	if (ret != 0)
 		goto fail;
@@ -688,6 +716,8 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret)
 		goto fail;
 
+	mt_post_parse(td);
+
 	if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
 		mt_post_parse_default_settings(td);
 
@@ -705,9 +735,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	mt_set_maxcontacts(hdev);
 	mt_set_input_mode(hdev);
 
+	kfree(td->fields);
+	td->fields = NULL;
+
 	return 0;
 
 fail:
+	kfree(td->fields);
 	kfree(td);
 	return ret;
 }
-- 
1.7.7.6

[PATCH 2/5] HID: hid-multitouch: get maxcontacts also from logical_max value

[benjamin.tissoires]

From: Benjamin Tissoires <benjamin.tissoires@enac.fr>

Win8 devices are required to present the feature "Maximum Contact Number".
If the current value is 0, then, the driver can get the actual supported
contact count by seeing the logical_max.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 drivers/hid/hid-multitouch.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c6ffb05..e205d1e 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -265,6 +265,8 @@ static void mt_feature_mapping(struct hid_device *hdev,
 	case HID_DG_CONTACTMAX:
 		td->maxcontact_report_id = field->report->id;
 		td->maxcontacts = field->value[0];
+		if (!td->maxcontacts)
+			td->maxcontacts = field->logical_maximum;
 		if (td->mtclass.maxcontacts)
 			/* check if the maxcontacts is given by the class */
 			td->maxcontacts = td->mtclass.maxcontacts;
-- 
1.7.7.6

[PATCH 3/5] HID: hid-multitouch: support arrays for the split of the touches in a report

[benjamin.tissoires]

From: Benjamin Tissoires <benjamin.tissoires@enac.fr>

Win8 certification introduced the ability to transmit two X and two Y per
touch. The specification precises that it must be in an array, with a
report count == 2.

This test guarantees that we split the touches on the last element
in this array.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 drivers/hid/hid-multitouch.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index e205d1e..3ee22ec 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -589,7 +589,10 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 			return 0;
 		}
 
-		if (usage->hid == td->last_slot_field)
+		if (usage->hid == td->last_slot_field &&
+			usage == &field->usage[field->maxusage - 1])
+			/* we only take into account the last report
+			 * of a field if report_count > 1 */
 			mt_complete_slot(td);
 
 		if (field->index == td->last_field_index
-- 
1.7.7.6

[PATCH 4/5] input: Introduce MT_CENTER_X and MT_CENTER_Y

[benjamin.tissoires]

From: Benjamin Tissoires <benjamin.tissoires@enac.fr>

Microsoft published a new version of their multitouch specification.
They introduced a new multitouch field: MT_CENTER_X|Y.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 Documentation/input/multi-touch-protocol.txt |   14 ++++++++++++--
 include/linux/input.h                        |    8 +++++---
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/Documentation/input/multi-touch-protocol.txt b/Documentation/input/multi-touch-protocol.txt
index 543101c..289951c 100644
--- a/Documentation/input/multi-touch-protocol.txt
+++ b/Documentation/input/multi-touch-protocol.txt
@@ -254,11 +254,21 @@ between. In such cases, the range of ABS_MT_ORIENTATION should be [0, 1]
 
 ABS_MT_POSITION_X
 
-The surface X coordinate of the center of the touching ellipse.
+The surface X coordinate of the point the user intended to touch.
 
 ABS_MT_POSITION_Y
 
-The surface Y coordinate of the center of the touching ellipse.
+The surface Y coordinate of the point the user intended to touch.
+
+ABS_MT_CENTER_X
+
+The surface X coordinate of the center of the touching ellipse. This field is
+introduced by Windows 8 multitouch protocol.
+
+ABS_MT_CENTER_Y
+
+The surface Y coordinate of the center of the touching ellipse. This field is
+introduced by Windows 8 multitouch protocol.
 
 ABS_MT_TOOL_TYPE
 
diff --git a/include/linux/input.h b/include/linux/input.h
index a816714..5b0dfe2 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -806,18 +806,20 @@ struct input_keymap_entry {
 #define ABS_MT_WIDTH_MAJOR	0x32	/* Major axis of approaching ellipse */
 #define ABS_MT_WIDTH_MINOR	0x33	/* Minor axis (omit if circular) */
 #define ABS_MT_ORIENTATION	0x34	/* Ellipse orientation */
-#define ABS_MT_POSITION_X	0x35	/* Center X ellipse position */
-#define ABS_MT_POSITION_Y	0x36	/* Center Y ellipse position */
+#define ABS_MT_POSITION_X	0x35	/* X the user intended to touch */
+#define ABS_MT_POSITION_Y	0x36	/* Y the user intended to touch */
 #define ABS_MT_TOOL_TYPE	0x37	/* Type of touching device */
 #define ABS_MT_BLOB_ID		0x38	/* Group a set of packets as a blob */
 #define ABS_MT_TRACKING_ID	0x39	/* Unique ID of initiated contact */
 #define ABS_MT_PRESSURE		0x3a	/* Pressure on contact area */
 #define ABS_MT_DISTANCE		0x3b	/* Contact hover distance */
+#define ABS_MT_CENTER_X		0x3c	/* Center X ellipse position */
+#define ABS_MT_CENTER_Y		0x3d	/* Center Y ellipse position */
 
 #ifdef __KERNEL__
 /* Implementation details, userspace should not care about these */
 #define ABS_MT_FIRST		ABS_MT_TOUCH_MAJOR
-#define ABS_MT_LAST		ABS_MT_DISTANCE
+#define ABS_MT_LAST		ABS_MT_CENTER_Y
 #endif
 
 #define ABS_MAX			0x3f
-- 
1.7.7.6

[PATCH 5/5] HID: hid-multitouch: support T and C for win8 devices

[benjamin.tissoires]

From: Benjamin Tissoires <benjamin.tissoires@enac.fr>

Win8 input specification clarifies the X and Y sent by devices.
It distincts the position where the user wants to Touch (T) from
the center of the ellipsoide (C). This patch enable supports for this
distinction in hid-multitouch.

We recognize Win8 certified devices from their vendor field 0xff0000c5
where Microsoft put a signed blob in the report to check if the device
passed the certification.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 drivers/hid/hid-multitouch.c |   77 +++++++++++++++++++++++++++++++++++++++---
 1 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 3ee22ec..48c8576 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -51,9 +51,10 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_VALID_IS_INRANGE	(1 << 5)
 #define MT_QUIRK_VALID_IS_CONFIDENCE	(1 << 6)
 #define MT_QUIRK_SLOT_IS_CONTACTID_MINUS_ONE	(1 << 8)
+#define MT_QUIRK_WIN_8_CERTIFIED	(1 << 9)
 
 struct mt_slot {
-	__s32 x, y, p, w, h;
+	__s32 x, y, cx, cy, p, w, h;
 	__s32 contactid;	/* the device ContactID assigned to this slot */
 	bool touch_state;	/* is the touch valid? */
 	bool seen_in_this_frame;/* has this slot been updated */
@@ -71,7 +72,7 @@ struct mt_class {
 };
 
 struct mt_fields {
-	unsigned usages[HID_MAX_FIELDS];
+	struct hid_usage *usages[HID_MAX_FIELDS];
 	unsigned int length;
 };
 
@@ -272,9 +273,14 @@ static void mt_feature_mapping(struct hid_device *hdev,
 			td->maxcontacts = td->mtclass.maxcontacts;
 
 		break;
+	case 0xff0000c5:
+		if (field->report_count == 256 && field->report_size == 8)
+			td->mtclass.quirks |= MT_QUIRK_WIN_8_CERTIFIED;
+		break;
 	}
 }
 
+
 static void set_abs(struct input_dev *input, unsigned int code,
 		struct hid_field *field, int snratio)
 {
@@ -292,7 +298,7 @@ static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
 	if (f->length >= HID_MAX_FIELDS)
 		return;
 
-	f->usages[f->length++] = usage->hid;
+	f->usages[f->length++] = usage;
 }
 
 static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
@@ -343,6 +349,9 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_X, field, cls->sn_move);
+			if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED)
+				set_abs(hi->input, ABS_MT_CENTER_X, field,
+					cls->sn_move);
 			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
@@ -353,6 +362,9 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_Y, field, cls->sn_move);
+			if (cls->quirks & MT_QUIRK_WIN_8_CERTIFIED)
+				set_abs(hi->input, ABS_MT_CENTER_Y, field,
+					cls->sn_move);
 			mt_store_field(usage, td, hi);
 			td->last_field_index = field->index;
 			return 1;
@@ -515,6 +527,12 @@ static void mt_emit_event(struct mt_device *td, struct input_dev *input)
 
 			input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x);
 			input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y);
+			if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED) {
+				input_event(input, EV_ABS, ABS_MT_CENTER_X,
+					s->cx);
+				input_event(input, EV_ABS, ABS_MT_CENTER_Y,
+					s->cy);
+			}
 			input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
 			input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
 			input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
@@ -561,10 +579,14 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 			td->curdata.p = value;
 			break;
 		case HID_GD_X:
-			td->curdata.x = value;
+			if (usage->code == ABS_MT_POSITION_X)
+				td->curdata.x = value;
+			td->curdata.cx = value;
 			break;
 		case HID_GD_Y:
-			td->curdata.y = value;
+			if (usage->code == ABS_MT_POSITION_Y)
+				td->curdata.y = value;
+			td->curdata.cy = value;
 			break;
 		case HID_DG_WIDTH:
 			td->curdata.w = value;
@@ -666,6 +688,47 @@ static void mt_post_parse_default_settings(struct mt_device *td)
 	td->mtclass.quirks = quirks;
 }
 
+static void mt_post_parse_win8(struct mt_device *td)
+{
+	struct mt_fields *f = td->fields;
+	int field_count_per_touch = f->length / td->touches_by_report;
+	int i;
+
+	int position_x_index = -1;
+	int position_y_index = -1;
+	int center_x_index = -1;
+	int center_y_index = -1;
+
+	for (i = 0; i < field_count_per_touch; i++) {
+		switch (f->usages[i]->hid) {
+		case HID_GD_X:
+			if (position_x_index < 0)
+				position_x_index = i;
+			else
+				center_x_index = i;
+			break;
+		case HID_GD_Y:
+			if (position_y_index < 0)
+				position_y_index = i;
+			else
+				center_y_index = i;
+			break;
+		}
+	}
+
+	if (center_x_index < 0 || center_y_index < 0)
+		/* center X and center y are not provided */
+		return;
+
+	for (i = 0; i < td->touches_by_report; i++) {
+		int cur_touch_index = i * field_count_per_touch;
+		struct hid_usage **usages = &f->usages[cur_touch_index];
+
+		usages[center_x_index]->code = ABS_MT_CENTER_X;
+		usages[center_y_index]->code = ABS_MT_CENTER_Y;
+	}
+}
+
 static void mt_post_parse(struct mt_device *td)
 {
 	struct mt_fields *f = td->fields;
@@ -673,6 +736,10 @@ static void mt_post_parse(struct mt_device *td)
 	if (td->touches_by_report > 0) {
 		int field_count_per_touch = f->length / td->touches_by_report;
 		td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
+
+		if (td->mtclass.quirks & MT_QUIRK_WIN_8_CERTIFIED)
+			mt_post_parse_win8(td);
+
 	}
 }
 
-- 
1.7.7.6

Re: [PATCH 4/5] input: Introduce MT_CENTER_X and MT_CENTER_Y

[Henrik Rydberg]

Hi Benjamin, Stephane,

> Microsoft published a new version of their multitouch specification.
> They introduced a new multitouch field: MT_CENTER_X|Y.

I would like a bit of elaboration here, since I am not convinced the
mapping should be directly translated to the MT protocol.

Clearly, the basic idea to be able to model an assymmetric tool is
good. Without an angle description, however, there seems to be a
mismatch in the degrees of freedom. In win8, one can place the hot
spot in a corner of the rectangle, but one cannot make the rectangle
into an ellipse in that direction. In linux, one can put the ellipse
in the wanted direction, but the hot spot is always in the center. To
first order, both models allow for finger-rotation information, but in
different ways. Adding more parameters requires a bit more
thought. Perhaps one should use the newly found win8 parameters to
reshape the pointing ellipse to start with.

Thanks,
Henrik

Re: [PATCH 4/5] input: Introduce MT_CENTER_X and MT_CENTER_Y

[Benjamin Tissoires]

Hi Henrik,

Well, this is the mail I intended to send yesterday before I run out
of internet connection. This may be useless, but, I'll send it
anyway....

On Fri, May 4, 2012 at 3:48 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin, Stephane,
>
>> Microsoft published a new version of their multitouch specification.
>> They introduced a new multitouch field: MT_CENTER_X|Y.
>
> I would like a bit of elaboration here, since I am not convinced the
> mapping should be directly translated to the MT protocol.
>
> Clearly, the basic idea to be able to model an assymmetric tool is
> good. Without an angle description, however, there seems to be a
> mismatch in the degrees of freedom. In win8, one can place the hot
> spot in a corner of the rectangle, but one cannot make the rectangle
> into an ellipse in that direction.

That's not the interpretation I made of the spec.
http://msdn.microsoft.com/library/windows/hardware/br259100

Win8 is a total respin of the multitouch protocol (though backward
compatible). It asks for a lot more reliability and performances for
the devices.
>From what I understood, the T point (the touch) can be an arbitrary
point within the ellipse. Furthermore, the ellipse can be oriented in
an arbitrary rotation (like linux) because they introduced the hid
field Azimuth (0x3f) that is the "Counter-clockwise rotation of the
cursor around the Z axis".

To sum up, there are two kind of information:
- The touch point (where the user wants to touch)
- The elliptic shape of the touch (width, height, center, azimuth, pressure)

I think these two information are interesting as most of the time, the
center of the ellipse is _not_ the point where the user wants to
touch. The best thing to test that is to see how many time you miss
the pixel you want to touch in the current implementation.

I also wanted to publish these 2 last patches to raise the discussion,
and to show that the fix of the randomness in the splitting of the
touches within the reports was compatible with new kinds of devices.

We should focus for now (3.5) on the first 3 patches, and let aside
the 2 last for the next version, when everyone agrees.

Cheers,
Benjamin


> In linux, one can put the ellipse
> in the wanted direction, but the hot spot is always in the center. To
> first order, both models allow for finger-rotation information, but in
> different ways. Adding more parameters requires a bit more
> thought. Perhaps one should use the newly found win8 parameters to
> reshape the pointing ellipse to start with.
>
> Thanks,
> Henrik

Re: [PATCH 4/5] input: Introduce MT_CENTER_X and MT_CENTER_Y

[Henrik Rydberg]

Hi Benjamin,

> > I would like a bit of elaboration here, since I am not convinced the
> > mapping should be directly translated to the MT protocol.
> >
> > Clearly, the basic idea to be able to model an assymmetric tool is
> > good. Without an angle description, however, there seems to be a
> > mismatch in the degrees of freedom. In win8, one can place the hot
> > spot in a corner of the rectangle, but one cannot make the rectangle
> > into an ellipse in that direction.
> 
> That's not the interpretation I made of the spec.
> http://msdn.microsoft.com/library/windows/hardware/br259100

Yep, we are referring to the same file.

> Win8 is a total respin of the multitouch protocol (though backward
> compatible). It asks for a lot more reliability and performances for
> the devices.
> From what I understood, the T point (the touch) can be an arbitrary
> point within the ellipse. Furthermore, the ellipse can be oriented in
> an arbitrary rotation (like linux) because they introduced the hid
> field Azimuth (0x3f) that is the "Counter-clockwise rotation of the
> cursor around the Z axis".

The document is a bit unclear on what object azimuth is referring to,
and whether it correlates with the C-T vector, but essentially, there
is a lot more details, I agree.

> To sum up, there are two kind of information:
> - The touch point (where the user wants to touch)
> - The elliptic shape of the touch (width, height, center, azimuth, pressure)

Width and height still refer to the bounding box, that much is clear
from the document. The shape modelled by azimuth is not quite that
clear.

> I think these two information are interesting as most of the time, the
> center of the ellipse is _not_ the point where the user wants to
> touch. The best thing to test that is to see how many time you miss
> the pixel you want to touch in the current implementation.

No argument here; separating the touch and shape points is a great
improvement. It also gives directional information in a simpler manner
than via the shape angle.

> I also wanted to publish these 2 last patches to raise the discussion,
> and to show that the fix of the randomness in the splitting of the
> touches within the reports was compatible with new kinds of devices.
> 
> We should focus for now (3.5) on the first 3 patches, and let aside
> the 2 last for the next version, when everyone agrees.

Yes, it may well be that the userland discussion carries over into
3.6, and we can certainly treat the hid enablement separately.

More to come on the shape modelling in the other thread.

Thanks,
Henrik

Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Henrik Rydberg]

Hi Benjamin,

> The previous implementation introduced a randomness in the splitting
> of the different touches reported by the device. This version is more
> robust as we don't rely on hi->input->absbit, but on our own structure.
> 
> This also prepares hid-multitouch to better support Win8 devices.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> ---
>  drivers/hid/hid-multitouch.c |   58 +++++++++++++++++++++++++++++++++--------
>  1 files changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 84bb32e..c6ffb05 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -70,9 +70,16 @@ struct mt_class {
>  	bool is_indirect;	/* true for touchpads */
>  };
>  
> +struct mt_fields {
> +	unsigned usages[HID_MAX_FIELDS];
> +	unsigned int length;
> +};
> +
>  struct mt_device {
>  	struct mt_slot curdata;	/* placeholder of incoming data */
>  	struct mt_class mtclass;	/* our mt device class */
> +	struct mt_fields *fields;	/* temporary placeholder for storing the
> +					   multitouch fields */

Why not skip the pointer here?

>  	unsigned last_field_index;	/* last field index of the report */
>  	unsigned last_slot_field;	/* the last field of a slot */
>  	__s8 inputmode;		/* InputMode HID feature, -1 if non-existent */
> @@ -275,11 +282,15 @@ static void set_abs(struct input_dev *input, unsigned int code,
>  	input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
>  }
>  
> -static void set_last_slot_field(struct hid_usage *usage, struct mt_device *td,
> +static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
>  		struct hid_input *hi)

How about adding the last_field_index here also, using a function like

static void mt_store_field(struct mt_device *td, struct hid_input *hi,
       	    		struct hid_field *field, struct hid_usage *usage)

>  {
> -	if (!test_bit(usage->hid, hi->input->absbit))
> -		td->last_slot_field = usage->hid;
> +	struct mt_fields *f = td->fields;

And inserting td->last_field_index = field->index here.

> +
> +	if (f->length >= HID_MAX_FIELDS)
> +		return;
> +
> +	f->usages[f->length++] = usage->hid;
>  }
>  
>  static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> @@ -330,7 +341,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  				cls->sn_move);
>  			/* touchscreen emulation */
>  			set_abs(hi->input, ABS_X, field, cls->sn_move);
> -			set_last_slot_field(usage, td, hi);
> +			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_GD_Y:
> @@ -340,7 +351,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  				cls->sn_move);
>  			/* touchscreen emulation */
>  			set_abs(hi->input, ABS_Y, field, cls->sn_move);
> -			set_last_slot_field(usage, td, hi);
> +			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
>  		}
> @@ -349,24 +360,24 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  	case HID_UP_DIGITIZER:
>  		switch (usage->hid) {
>  		case HID_DG_INRANGE:
> -			set_last_slot_field(usage, td, hi);
> +			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_DG_CONFIDENCE:
> -			set_last_slot_field(usage, td, hi);
> +			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_DG_TIPSWITCH:
>  			hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
>  			input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
> -			set_last_slot_field(usage, td, hi);
> +			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_DG_CONTACTID:
>  			if (!td->maxcontacts)
>  				td->maxcontacts = MT_DEFAULT_MAXCONTACT;
>  			input_mt_init_slots(hi->input, td->maxcontacts);
> -			td->last_slot_field = usage->hid;
> +			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			td->touches_by_report++;
>  			return 1;
> @@ -375,7 +386,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  					EV_ABS, ABS_MT_TOUCH_MAJOR);
>  			set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field,
>  				cls->sn_width);
> -			set_last_slot_field(usage, td, hi);
> +			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_DG_HEIGHT:
> @@ -385,7 +396,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  				cls->sn_height);
>  			input_set_abs_params(hi->input,
>  					ABS_MT_ORIENTATION, 0, 1, 0, 0);
> -			set_last_slot_field(usage, td, hi);
> +			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_DG_TIPPRESSURE:
> @@ -396,7 +407,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  			/* touchscreen emulation */
>  			set_abs(hi->input, ABS_PRESSURE, field,
>  				cls->sn_pressure);
> -			set_last_slot_field(usage, td, hi);
> +			mt_store_field(usage, td, hi);
>  			td->last_field_index = field->index;
>  			return 1;
>  		case HID_DG_CONTACTCOUNT:
> @@ -650,6 +661,16 @@ static void mt_post_parse_default_settings(struct mt_device *td)
>  	td->mtclass.quirks = quirks;
>  }
>  
> +static void mt_post_parse(struct mt_device *td)
> +{
> +	struct mt_fields *f = td->fields;
> +
> +	if (td->touches_by_report > 0) {
> +		int field_count_per_touch = f->length / td->touches_by_report;
> +		td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
> +	}
> +}
> +
>  static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
>  	int ret, i;
> @@ -680,6 +701,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	td->maxcontact_report_id = -1;
>  	hid_set_drvdata(hdev, td);
>  
> +	td->fields = kzalloc(sizeof(struct mt_fields), GFP_KERNEL);
> +	if (!td->fields) {
> +		dev_err(&hdev->dev, "cannot allocate multitouch fields data\n");
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +

This can be skipped.

>  	ret = hid_parse(hdev);
>  	if (ret != 0)
>  		goto fail;
> @@ -688,6 +716,8 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	if (ret)
>  		goto fail;
>  
> +	mt_post_parse(td);
> +
>  	if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
>  		mt_post_parse_default_settings(td);
>  
> @@ -705,9 +735,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	mt_set_maxcontacts(hdev);
>  	mt_set_input_mode(hdev);
>  
> +	kfree(td->fields);
> +	td->fields = NULL;
> +

Ditto.

>  	return 0;
>  
>  fail:
> +	kfree(td->fields);

Ditto.

>  	kfree(td);
>  	return ret;
>  }
> -- 
> 1.7.7.6
> 

Thanks,
Henrik

Re: [PATCH 2/5] HID: hid-multitouch: get maxcontacts also from logical_max value

[Henrik Rydberg]

Hi,

> Win8 devices are required to present the feature "Maximum Contact Number".
> If the current value is 0, then, the driver can get the actual supported
> contact count by seeing the logical_max.

And for win7, it is zero?

> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> ---
>  drivers/hid/hid-multitouch.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index c6ffb05..e205d1e 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -265,6 +265,8 @@ static void mt_feature_mapping(struct hid_device *hdev,
>  	case HID_DG_CONTACTMAX:
>  		td->maxcontact_report_id = field->report->id;
>  		td->maxcontacts = field->value[0];
> +		if (!td->maxcontacts)
> +			td->maxcontacts = field->logical_maximum;
>  		if (td->mtclass.maxcontacts)
>  			/* check if the maxcontacts is given by the class */
>  			td->maxcontacts = td->mtclass.maxcontacts;
> -- 
> 1.7.7.6
> 

Thanks,
Henrik

Re: [patch 0/5] Bug fix and win8 for hid-multitouch

[Jiri Kosina]

On Fri, 4 May 2012, benjamin.tissoires wrote:

> The first patch fixes the bug I discovered lately. This bug was related to the out
> of bound bitfield test, so it concerns every known devices. Ideally, it should go
> to upstream-fixes, but this would requires additional adaptations.
> Jiri, How do you want to proceed?

When did the bug get introduced, please? i.e. is it a regression?

As we are getting close to 3.5 merge window being open, maybe the best 
thing to do would be just push it there once it's open, and then 
eventually backport it for stable@ if necessary.

> The last 4 patches are the beginning of the support of Win8 devices. 
> It's only the beginning as the specification is much more precise than 
> the Win7, and we will need to do more tuning in the future. However, 
> Win8 devices are retro-compatible with Win7, so they will work out of 
> the box though.

I understand that Henrik had some valid concerns, so I will wait for a 
second respin and Henrik's ack, ok?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [patch 0/5] Bug fix and win8 for hid-multitouch

[Benjamin Tissoires]

Hello Jiri,

On Wed, May 9, 2012 at 4:39 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Fri, 4 May 2012, benjamin.tissoires wrote:
>
>> The first patch fixes the bug I discovered lately. This bug was related to the out
>> of bound bitfield test, so it concerns every known devices. Ideally, it should go
>> to upstream-fixes, but this would requires additional adaptations.
>> Jiri, How do you want to proceed?
>
> When did the bug get introduced, please? i.e. is it a regression?

Well, it's not exactly a regression: it's always been there... ;-)

More seriously, the bug was introduced in the commit
"HID: multitouch: fix handling of buggy reports descriptors for Dell
ST2220T" (in kernel 3.3)
The thing is that this bug fix was not good and was working by miracle.

>
> As we are getting close to 3.5 merge window being open, maybe the best
> thing to do would be just push it there once it's open, and then
> eventually backport it for stable@ if necessary.

That seems fair.

>
>> The last 4 patches are the beginning of the support of Win8 devices.
>> It's only the beginning as the specification is much more precise than
>> the Win7, and we will need to do more tuning in the future. However,
>> Win8 devices are retro-compatible with Win7, so they will work out of
>> the box though.
>
> I understand that Henrik had some valid concerns, so I will wait for a
> second respin and Henrik's ack, ok?

ok,
Thanks,
Benjamin

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs

Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Benjamin Tissoires]

Hi Henrik,

thanks for the review.
Some comments inlined:

On Sun, May 6, 2012 at 9:01 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> The previous implementation introduced a randomness in the splitting
>> of the different touches reported by the device. This version is more
>> robust as we don't rely on hi->input->absbit, but on our own structure.
>>
>> This also prepares hid-multitouch to better support Win8 devices.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>> ---
>>  drivers/hid/hid-multitouch.c |   58 +++++++++++++++++++++++++++++++++--------
>>  1 files changed, 46 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 84bb32e..c6ffb05 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -70,9 +70,16 @@ struct mt_class {
>>       bool is_indirect;       /* true for touchpads */
>>  };
>>
>> +struct mt_fields {
>> +     unsigned usages[HID_MAX_FIELDS];
>> +     unsigned int length;
>> +};
>> +
>>  struct mt_device {
>>       struct mt_slot curdata; /* placeholder of incoming data */
>>       struct mt_class mtclass;        /* our mt device class */
>> +     struct mt_fields *fields;       /* temporary placeholder for storing the
>> +                                        multitouch fields */
>
> Why not skip the pointer here?

well, the idea was to keep the memory footprint low. As these values
are only needed at init, then I freed them once I finished using them.
I can of course skip the pointer, but in that case, wouldn't the
struct declaration be worthless?

>
>>       unsigned last_field_index;      /* last field index of the report */
>>       unsigned last_slot_field;       /* the last field of a slot */
>>       __s8 inputmode;         /* InputMode HID feature, -1 if non-existent */
>> @@ -275,11 +282,15 @@ static void set_abs(struct input_dev *input, unsigned int code,
>>       input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
>>  }
>>
>> -static void set_last_slot_field(struct hid_usage *usage, struct mt_device *td,
>> +static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
>>               struct hid_input *hi)
>
> How about adding the last_field_index here also, using a function like

I'm not a big fan of this idea.
last_field_index represent the last mt field, was it local or global
(i.e. per touch or global to the report).
mt_store_field stores only the local (per-touch) information to be
able to divide the array by the number of touch. Adding the global
items there too will force us to introduce a switch to exclude global
items.

Cheers,
Benjamin

>
> static void mt_store_field(struct mt_device *td, struct hid_input *hi,
>                        struct hid_field *field, struct hid_usage *usage)
>
>>  {
>> -     if (!test_bit(usage->hid, hi->input->absbit))
>> -             td->last_slot_field = usage->hid;
>> +     struct mt_fields *f = td->fields;
>
> And inserting td->last_field_index = field->index here.
>
>> +
>> +     if (f->length >= HID_MAX_FIELDS)
>> +             return;
>> +
>> +     f->usages[f->length++] = usage->hid;
>>  }
>>
>>  static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>> @@ -330,7 +341,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>                               cls->sn_move);
>>                       /* touchscreen emulation */
>>                       set_abs(hi->input, ABS_X, field, cls->sn_move);
>> -                     set_last_slot_field(usage, td, hi);
>> +                     mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_GD_Y:
>> @@ -340,7 +351,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>                               cls->sn_move);
>>                       /* touchscreen emulation */
>>                       set_abs(hi->input, ABS_Y, field, cls->sn_move);
>> -                     set_last_slot_field(usage, td, hi);
>> +                     mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               }
>> @@ -349,24 +360,24 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>       case HID_UP_DIGITIZER:
>>               switch (usage->hid) {
>>               case HID_DG_INRANGE:
>> -                     set_last_slot_field(usage, td, hi);
>> +                     mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_DG_CONFIDENCE:
>> -                     set_last_slot_field(usage, td, hi);
>> +                     mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_DG_TIPSWITCH:
>>                       hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
>>                       input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
>> -                     set_last_slot_field(usage, td, hi);
>> +                     mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_DG_CONTACTID:
>>                       if (!td->maxcontacts)
>>                               td->maxcontacts = MT_DEFAULT_MAXCONTACT;
>>                       input_mt_init_slots(hi->input, td->maxcontacts);
>> -                     td->last_slot_field = usage->hid;
>> +                     mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       td->touches_by_report++;
>>                       return 1;
>> @@ -375,7 +386,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>                                       EV_ABS, ABS_MT_TOUCH_MAJOR);
>>                       set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field,
>>                               cls->sn_width);
>> -                     set_last_slot_field(usage, td, hi);
>> +                     mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_DG_HEIGHT:
>> @@ -385,7 +396,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>                               cls->sn_height);
>>                       input_set_abs_params(hi->input,
>>                                       ABS_MT_ORIENTATION, 0, 1, 0, 0);
>> -                     set_last_slot_field(usage, td, hi);
>> +                     mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_DG_TIPPRESSURE:
>> @@ -396,7 +407,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>                       /* touchscreen emulation */
>>                       set_abs(hi->input, ABS_PRESSURE, field,
>>                               cls->sn_pressure);
>> -                     set_last_slot_field(usage, td, hi);
>> +                     mt_store_field(usage, td, hi);
>>                       td->last_field_index = field->index;
>>                       return 1;
>>               case HID_DG_CONTACTCOUNT:
>> @@ -650,6 +661,16 @@ static void mt_post_parse_default_settings(struct mt_device *td)
>>       td->mtclass.quirks = quirks;
>>  }
>>
>> +static void mt_post_parse(struct mt_device *td)
>> +{
>> +     struct mt_fields *f = td->fields;
>> +
>> +     if (td->touches_by_report > 0) {
>> +             int field_count_per_touch = f->length / td->touches_by_report;
>> +             td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
>> +     }
>> +}
>> +
>>  static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>  {
>>       int ret, i;
>> @@ -680,6 +701,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>       td->maxcontact_report_id = -1;
>>       hid_set_drvdata(hdev, td);
>>
>> +     td->fields = kzalloc(sizeof(struct mt_fields), GFP_KERNEL);
>> +     if (!td->fields) {
>> +             dev_err(&hdev->dev, "cannot allocate multitouch fields data\n");
>> +             ret = -ENOMEM;
>> +             goto fail;
>> +     }
>> +
>
> This can be skipped.
>
>>       ret = hid_parse(hdev);
>>       if (ret != 0)
>>               goto fail;
>> @@ -688,6 +716,8 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>       if (ret)
>>               goto fail;
>>
>> +     mt_post_parse(td);
>> +
>>       if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
>>               mt_post_parse_default_settings(td);
>>
>> @@ -705,9 +735,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>       mt_set_maxcontacts(hdev);
>>       mt_set_input_mode(hdev);
>>
>> +     kfree(td->fields);
>> +     td->fields = NULL;
>> +
>
> Ditto.
>
>>       return 0;
>>
>>  fail:
>> +     kfree(td->fields);
>
> Ditto.
>
>>       kfree(td);
>>       return ret;
>>  }
>> --
>> 1.7.7.6
>>
>
> Thanks,
> Henrik

Re: [PATCH 2/5] HID: hid-multitouch: get maxcontacts also from logical_max value

[Benjamin Tissoires]

On Sun, May 6, 2012 at 9:03 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi,
>
>> Win8 devices are required to present the feature "Maximum Contact Number".
>> If the current value is 0, then, the driver can get the actual supported
>> contact count by seeing the logical_max.
>
> And for win7, it is zero?

Well, the truth is that the Win8 specification formally describes the
values here. And to get the certification, hardware makers have to put
the right value in logical_max.
TBH, I don't care that much now with win7 devices. Most of them are a
piece of crap (not true dual fingers, problems in hid reports
descriptors, etc...), but they just work (we made the necessary
things). With the introduction of Win8, hardware makers will have to
*certify* their devices, and thus, the Win8 driver is much less
tolerant. I really think that we are going to see more and more win8
devices, whereas win7 devices will fade out.

I had to add this patch because I have a win8 device that has the
value associated to this field at 0, and it's the first I saw with
this behavior.

Cheers,
Benjamin

>
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>> ---
>>  drivers/hid/hid-multitouch.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index c6ffb05..e205d1e 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -265,6 +265,8 @@ static void mt_feature_mapping(struct hid_device *hdev,
>>       case HID_DG_CONTACTMAX:
>>               td->maxcontact_report_id = field->report->id;
>>               td->maxcontacts = field->value[0];
>> +             if (!td->maxcontacts)
>> +                     td->maxcontacts = field->logical_maximum;
>>               if (td->mtclass.maxcontacts)
>>                       /* check if the maxcontacts is given by the class */
>>                       td->maxcontacts = td->mtclass.maxcontacts;
>> --
>> 1.7.7.6
>>
>
> Thanks,
> Henrik

Re: [PATCH 2/5] HID: hid-multitouch: get maxcontacts also from logical_max value

[Henrik Rydberg]

Hi Benjamin,

> >> Win8 devices are required to present the feature "Maximum Contact Number".
> >> If the current value is 0, then, the driver can get the actual supported
> >> contact count by seeing the logical_max.
> >
> > And for win7, it is zero?
> 
> Well, the truth is that the Win8 specification formally describes the
> values here. And to get the certification, hardware makers have to put
> the right value in logical_max.
> TBH, I don't care that much now with win7 devices. Most of them are a
> piece of crap (not true dual fingers, problems in hid reports
> descriptors, etc...), but they just work (we made the necessary
> things). With the introduction of Win8, hardware makers will have to
> *certify* their devices, and thus, the Win8 driver is much less
> tolerant. I really think that we are going to see more and more win8
> devices, whereas win7 devices will fade out.
> 
> I had to add this patch because I have a win8 device that has the
> value associated to this field at 0, and it's the first I saw with
> this behavior.

As long as all existing devices are unaffected, it's fine, hence the question.

Thanks,
Henrik

Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Henrik Rydberg]

> > Why not skip the pointer here?
> 
> well, the idea was to keep the memory footprint low. As these values
> are only needed at init, then I freed them once I finished using them.
> I can of course skip the pointer, but in that case, wouldn't the
> struct declaration be worthless?

My bad, I misread the placement of the free() statement. I was also
concerned about memory, since HID is big enough a memory hog as it
is. Barring the added complexity of this patch, it now makes more
sense.

    Acked-by: Henrik Rydberg <rydberg@euromail.se>

Thanks,
Henrik

Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Jiri Kosina]

On Wed, 9 May 2012, Henrik Rydberg wrote:

> > well, the idea was to keep the memory footprint low. As these values
> > are only needed at init, then I freed them once I finished using them.
> > I can of course skip the pointer, but in that case, wouldn't the
> > struct declaration be worthless?
> 
> My bad, I misread the placement of the free() statement. I was also
> concerned about memory, since HID is big enough a memory hog as it
> is. Barring the added complexity of this patch, it now makes more
> sense.
> 
>     Acked-by: Henrik Rydberg <rydberg@euromail.se>

drivers/hid/hid-multitouch.c: In function ‘mt_post_parse’:
drivers/hid/hid-multitouch.c:673: error: invalid type argument of ‘->’ (have ‘unsigned int’)
make[1]: *** [drivers/hid/hid-multitouch.o] Error 1

I believe that

	td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;

should be

	td->last_slot_field = f->usages[field_count_per_touch - 1];

but I leave this up to you guys to fix.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Benjamin Tissoires]

On Thu, May 10, 2012 at 10:31 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 9 May 2012, Henrik Rydberg wrote:
>
>> > well, the idea was to keep the memory footprint low. As these values
>> > are only needed at init, then I freed them once I finished using them.
>> > I can of course skip the pointer, but in that case, wouldn't the
>> > struct declaration be worthless?
>>
>> My bad, I misread the placement of the free() statement. I was also
>> concerned about memory, since HID is big enough a memory hog as it
>> is. Barring the added complexity of this patch, it now makes more
>> sense.
>>
>>     Acked-by: Henrik Rydberg <rydberg@euromail.se>
>
> drivers/hid/hid-multitouch.c: In function ‘mt_post_parse’:
> drivers/hid/hid-multitouch.c:673: error: invalid type argument of ‘->’ (have ‘unsigned int’)
> make[1]: *** [drivers/hid/hid-multitouch.o] Error 1
>
> I believe that
>
>        td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
>
> should be
>
>        td->last_slot_field = f->usages[field_count_per_touch - 1];

Ouch, indeed. the "->hid" is extra. When I split the patch 1 and 5, I
missed that one.

I tested it without the ->hid, and it's good. Do I need to resend the patch?

Cheers,
Benjamin

>
> but I leave this up to you guys to fix.
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs

Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Jiri Kosina]

On Thu, 10 May 2012, Benjamin Tissoires wrote:

> >> > well, the idea was to keep the memory footprint low. As these values
> >> > are only needed at init, then I freed them once I finished using them.
> >> > I can of course skip the pointer, but in that case, wouldn't the
> >> > struct declaration be worthless?
> >>
> >> My bad, I misread the placement of the free() statement. I was also
> >> concerned about memory, since HID is big enough a memory hog as it
> >> is. Barring the added complexity of this patch, it now makes more
> >> sense.
> >>
> >>     Acked-by: Henrik Rydberg <rydberg@euromail.se>
> >
> > drivers/hid/hid-multitouch.c: In function ‘mt_post_parse’:
> > drivers/hid/hid-multitouch.c:673: error: invalid type argument of ‘->’ (have ‘unsigned int’)
> > make[1]: *** [drivers/hid/hid-multitouch.o] Error 1
> >
> > I believe that
> >
> >        td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
> >
> > should be
> >
> >        td->last_slot_field = f->usages[field_count_per_touch - 1];
> 
> Ouch, indeed. the "->hid" is extra. When I split the patch 1 and 5, I
> missed that one.
> 
> I tested it without the ->hid, and it's good. Do I need to resend the patch?

No, that's fine, I have fixed that up and applied.

I am not applying the rest of the patchset yet.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/5] HID: hid-multitouch: get maxcontacts also from logical_max value

[Benjamin Tissoires]

On Wed, May 9, 2012 at 9:46 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> >> Win8 devices are required to present the feature "Maximum Contact Number".
>> >> If the current value is 0, then, the driver can get the actual supported
>> >> contact count by seeing the logical_max.
>> >
>> > And for win7, it is zero?
>>
>> Well, the truth is that the Win8 specification formally describes the
>> values here. And to get the certification, hardware makers have to put
>> the right value in logical_max.
>> TBH, I don't care that much now with win7 devices. Most of them are a
>> piece of crap (not true dual fingers, problems in hid reports
>> descriptors, etc...), but they just work (we made the necessary
>> things). With the introduction of Win8, hardware makers will have to
>> *certify* their devices, and thus, the Win8 driver is much less
>> tolerant. I really think that we are going to see more and more win8
>> devices, whereas win7 devices will fade out.
>>
>> I had to add this patch because I have a win8 device that has the
>> value associated to this field at 0, and it's the first I saw with
>> this behavior.
>
> As long as all existing devices are unaffected, it's fine, hence the question.

I checked all the reports descriptors that I have.
2 devices (one Stantum and one Irtouch) present an unrealistic
logical_max value (255). The thing is that if this logical_max is
false, and if the value is not provided, then I don't know how could I
retrieve the right value beside introducing a MT_CLS...

Henrik, do you think that 255 is two much for the slots?

Thanks,
Benjamin

>
> Thanks,
> Henrik

Re: [PATCH 2/5] HID: hid-multitouch: get maxcontacts also from logical_max value

[Henrik Rydberg]

> > As long as all existing devices are unaffected, it's fine, hence the question.
> 
> I checked all the reports descriptors that I have.
> 2 devices (one Stantum and one Irtouch) present an unrealistic
> logical_max value (255). The thing is that if this logical_max is
> false, and if the value is not provided, then I don't know how could I
> retrieve the right value beside introducing a MT_CLS...
> 
> Henrik, do you think that 255 is two much for the slots?

It is large enough for us to start worrying about how we manage
memory, not to mention throughput. Since we do not really have devices
of that type yet, how about adding a MT_MAX_MAXCONTACT next to
MT_DEFAULT_MAXCONTACT, and use it as a sanity check, like so:

	if (!td->maxcontacts && field->logical_maximum <= MT_MAX_MAXCONTACT)
		td->maxcontacts = field->logical_maximum;

Then the default (10) would be picked for the suspect devices, just as
it is today.

Thanks,
Henrik

RE: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Drews, Paul]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Benjamin Tissoires
> Sent: Wednesday, May 09, 2012 12:04 PM
> To: Henrik Rydberg
> Cc: Dmitry Torokhov; Jiri Kosina; Stephane Chatty; linux-input@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection
> 

> On Sun, May 6, 2012 at 9:01 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> > Hi Benjamin,
> >
> >> The previous implementation introduced a randomness in the splitting
> >> of the different touches reported by the device. This version is more
> >> robust as we don't rely on hi->input->absbit, but on our own structure.
> >>

> >> +
> >>  struct mt_device {
> >>       struct mt_slot curdata; /* placeholder of incoming data */
> >>       struct mt_class mtclass;        /* our mt device class */
> >> +     struct mt_fields *fields;       /* temporary placeholder for storing the
> >> +                                        multitouch fields */
> >
> > Why not skip the pointer here?
> 
> well, the idea was to keep the memory footprint low. As these values
> are only needed at init, then I freed them once I finished using them.
> I can of course skip the pointer, but in that case, wouldn't the
> struct declaration be worthless?
> 
> >

> >>
> >> +static void mt_post_parse(struct mt_device *td)
> >> +{
> >> +     struct mt_fields *f = td->fields;
> >> +
> >> +     if (td->touches_by_report > 0) {
> >> +             int field_count_per_touch = f->length / td->touches_by_report;
> >> +             td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
> >> +     }
> >> +}
> >> +

It sounds as though:

() Reviewers are a little uncomfortable with the memory footprint and
    allocation/free
() The patch as it stands relies on the pattern of "usage" values repeating
    for each touch, and deeming the last one in the repetition pattern to
    be the last-slot-field marker.

If this is the case, how about avoiding storing all the slot-field values
and just detecting the point of repetition to use the most-recently-seen
usage value as the last-slot-field marker.  I have been successfully using
the patch below based on this notion.  It took the failure rate from about
1-per-10 boots to 250+ boots with no failures on an Atmel MaXTouch.
I don't have others to try it with, including the "buggy" one that led
to all this trouble in the first place.

Patch follows:
==========================================================
>From 242f6773babe0fc0215764abbeeeff6510f3ca92 Mon Sep 17 00:00:00 2001
From: Paul Drews <paul.drews@intel.com>
Date: Wed, 16 May 2012 11:15:00 -0700
Subject: [PATCH] Repair detection of last slot in multitouch reports

The logic for detecting the last per-touch slot in a
multitouch report erroneously used hid usage values (large
numbers such as 0xd0032) as indices into the smaller absbit
bitmap (with bit indexes up to 0x3f).  This caused
intermittent failures in the configuration of the last-slot
value leading to stale x,y coordinates being reported in
multi-touch input events.  It also carried the risk of a
segmentation fault due to the out-of-range bitmap index.

This patch takes a different approach of detecting the last
per-touch slot:  when the hid usage value wraps around to
the first hid usage value we have seen already, we must be
looking at the slots for the next touch of a multi-touch
report, so the last hid usage value we have seen so far must
be the last per-touch value.

Issue: AIA-446
Change-Id: Ic1872998502874298bb60705df9bd2fc70de1738
Signed-off-by: Paul Drews <paul.drews@intel.com>
---
 drivers/hid/hid-multitouch.c |   39 ++++++++++++++++++++++++++-------------
 1 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2e6d187..226f828 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -75,6 +75,9 @@ struct mt_device {
 	struct mt_class mtclass;	/* our mt device class */
 	unsigned last_field_index;	/* last field index of the report */
 	unsigned last_slot_field;	/* the last field of a slot */
+	bool last_slot_field_found;	/* last_slot_field has full init */
+	unsigned first_slot_field;
+	bool first_slot_field_found;	/* for detecting wrap to next touch */
 	__s8 inputmode;		/* InputMode HID feature, -1 if non-existent */
 	__s8 maxcontact_report_id;	/* Maximum Contact Number HID feature,
 				   -1 if non-existent */
@@ -275,11 +278,21 @@ static void set_abs(struct input_dev *input, unsigned int code,
 	input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
 }
 
-static void set_last_slot_field(struct hid_usage *usage, struct mt_device *td,
-		struct hid_input *hi)
+static void update_last_slot_field(struct hid_usage *usage,
+		struct mt_device *td)
 {
-	if (!test_bit(usage->hid, hi->input->absbit))
-		td->last_slot_field = usage->hid;
+	if (!td->last_slot_field_found) {
+		if (td->first_slot_field_found) {
+			if (td->last_slot_field == usage->hid)
+				td->last_slot_field_found = true; /* wrapped */
+			else
+				td->last_slot_field = usage->hid;
+		} else {
+			td->first_slot_field = usage->hid;
+			td->first_slot_field_found = true;
+			td->last_slot_field = usage->hid;
+		}
+	}
 }
 
 static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
@@ -340,7 +353,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_X, field, cls->sn_move);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_GD_Y:
@@ -350,7 +363,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_Y, field, cls->sn_move);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		}
@@ -359,24 +372,24 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	case HID_UP_DIGITIZER:
 		switch (usage->hid) {
 		case HID_DG_INRANGE:
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONFIDENCE:
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_TIPSWITCH:
 			hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
 			input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTID:
 			if (!td->maxcontacts)
 				td->maxcontacts = MT_DEFAULT_MAXCONTACT;
 			input_mt_init_slots(hi->input, td->maxcontacts);
-			td->last_slot_field = usage->hid;
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			td->touches_by_report++;
 			return 1;
@@ -385,7 +398,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 					EV_ABS, ABS_MT_TOUCH_MAJOR);
 			set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field,
 				cls->sn_width);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_HEIGHT:
@@ -395,7 +408,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_height);
 			input_set_abs_params(hi->input,
 					ABS_MT_ORIENTATION, 0, 1, 0, 0);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_TIPPRESSURE:
@@ -406,7 +419,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_PRESSURE, field,
 				cls->sn_pressure);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTCOUNT:
-- 
1.7.3.4
==========================================================

Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Benjamin Tissoires]

Hi Paul,

On Fri, May 18, 2012 at 11:14 PM, Drews, Paul <paul.drews@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>> owner@vger.kernel.org] On Behalf Of Benjamin Tissoires
>> Sent: Wednesday, May 09, 2012 12:04 PM
>> To: Henrik Rydberg
>> Cc: Dmitry Torokhov; Jiri Kosina; Stephane Chatty; linux-input@vger.kernel.org;
>> linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection
>>
>
>> On Sun, May 6, 2012 at 9:01 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > Hi Benjamin,
>> >
>> >> The previous implementation introduced a randomness in the splitting
>> >> of the different touches reported by the device. This version is more
>> >> robust as we don't rely on hi->input->absbit, but on our own structure.
>> >>
>
>> >> +
>> >>  struct mt_device {
>> >>       struct mt_slot curdata; /* placeholder of incoming data */
>> >>       struct mt_class mtclass;        /* our mt device class */
>> >> +     struct mt_fields *fields;       /* temporary placeholder for storing the
>> >> +                                        multitouch fields */
>> >
>> > Why not skip the pointer here?
>>
>> well, the idea was to keep the memory footprint low. As these values
>> are only needed at init, then I freed them once I finished using them.
>> I can of course skip the pointer, but in that case, wouldn't the
>> struct declaration be worthless?
>>
>> >
>
>> >>
>> >> +static void mt_post_parse(struct mt_device *td)
>> >> +{
>> >> +     struct mt_fields *f = td->fields;
>> >> +
>> >> +     if (td->touches_by_report > 0) {
>> >> +             int field_count_per_touch = f->length / td->touches_by_report;
>> >> +             td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
>> >> +     }
>> >> +}
>> >> +
>
> It sounds as though:
>
> () Reviewers are a little uncomfortable with the memory footprint and
>    allocation/free
> () The patch as it stands relies on the pattern of "usage" values repeating
>    for each touch, and deeming the last one in the repetition pattern to
>    be the last-slot-field marker.
>
> If this is the case, how about avoiding storing all the slot-field values
> and just detecting the point of repetition to use the most-recently-seen
> usage value as the last-slot-field marker.  I have been successfully using
> the patch below based on this notion.  It took the failure rate from about
> 1-per-10 boots to 250+ boots with no failures on an Atmel MaXTouch.
> I don't have others to try it with, including the "buggy" one that led
> to all this trouble in the first place.

Thank you very much for this patch. However, Jiri already applied mine
with the allocation/free mechanism.

You're idea is good but it has one big problem with Win8 devices:
As we can have 2 X and 2 Y per touch report, if these dual-X reporting
or dual-Y reporting is present in the report, we will stop at the
second X or the second Y seen, which will lead to a buggy touchscreen
(the first touch won't get it's second coordinate). However, without
this particularity, the patch would have worked ;-)

If the Win8 norm has came earlier, the initial implementation that
relies on the collection would have suffice, but some hardware makers
made a bad use of it, leading us to stop using this, and relying on a
more brutal approach.

I found a little problem in the patch too:

>
> Patch follows:
> ==========================================================
> From 242f6773babe0fc0215764abbeeeff6510f3ca92 Mon Sep 17 00:00:00 2001
> From: Paul Drews <paul.drews@intel.com>
> Date: Wed, 16 May 2012 11:15:00 -0700
> Subject: [PATCH] Repair detection of last slot in multitouch reports
>
> The logic for detecting the last per-touch slot in a
> multitouch report erroneously used hid usage values (large
> numbers such as 0xd0032) as indices into the smaller absbit
> bitmap (with bit indexes up to 0x3f).  This caused
> intermittent failures in the configuration of the last-slot
> value leading to stale x,y coordinates being reported in
> multi-touch input events.  It also carried the risk of a
> segmentation fault due to the out-of-range bitmap index.
>
> This patch takes a different approach of detecting the last
> per-touch slot:  when the hid usage value wraps around to
> the first hid usage value we have seen already, we must be
> looking at the slots for the next touch of a multi-touch
> report, so the last hid usage value we have seen so far must
> be the last per-touch value.
>
> Issue: AIA-446
> Change-Id: Ic1872998502874298bb60705df9bd2fc70de1738
> Signed-off-by: Paul Drews <paul.drews@intel.com>
> ---
>  drivers/hid/hid-multitouch.c |   39 ++++++++++++++++++++++++++-------------
>  1 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 2e6d187..226f828 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -75,6 +75,9 @@ struct mt_device {
>        struct mt_class mtclass;        /* our mt device class */
>        unsigned last_field_index;      /* last field index of the report */
>        unsigned last_slot_field;       /* the last field of a slot */
> +       bool last_slot_field_found;     /* last_slot_field has full init */
> +       unsigned first_slot_field;
> +       bool first_slot_field_found;    /* for detecting wrap to next touch */
>        __s8 inputmode;         /* InputMode HID feature, -1 if non-existent */
>        __s8 maxcontact_report_id;      /* Maximum Contact Number HID feature,
>                                   -1 if non-existent */
> @@ -275,11 +278,21 @@ static void set_abs(struct input_dev *input, unsigned int code,
>        input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
>  }
>
> -static void set_last_slot_field(struct hid_usage *usage, struct mt_device *td,
> -               struct hid_input *hi)
> +static void update_last_slot_field(struct hid_usage *usage,
> +               struct mt_device *td)
>  {
> -       if (!test_bit(usage->hid, hi->input->absbit))
> -               td->last_slot_field = usage->hid;
> +       if (!td->last_slot_field_found) {
> +               if (td->first_slot_field_found) {
> +                       if (td->last_slot_field == usage->hid)

I'm sure you wanted to put here:
                       if (td->first_slot_field == usage->hid)

Cheers,
Benjamin

> +                               td->last_slot_field_found = true; /* wrapped */
> +                       else
> +                               td->last_slot_field = usage->hid;
> +               } else {
> +                       td->first_slot_field = usage->hid;
> +                       td->first_slot_field_found = true;
> +                       td->last_slot_field = usage->hid;
> +               }
> +       }
>  }
>
>  static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> @@ -340,7 +353,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>                                cls->sn_move);
>                        /* touchscreen emulation */
>                        set_abs(hi->input, ABS_X, field, cls->sn_move);
> -                       set_last_slot_field(usage, td, hi);
> +                       update_last_slot_field(usage, td);
>                        td->last_field_index = field->index;
>                        return 1;
>                case HID_GD_Y:
> @@ -350,7 +363,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>                                cls->sn_move);
>                        /* touchscreen emulation */
>                        set_abs(hi->input, ABS_Y, field, cls->sn_move);
> -                       set_last_slot_field(usage, td, hi);
> +                       update_last_slot_field(usage, td);
>                        td->last_field_index = field->index;
>                        return 1;
>                }
> @@ -359,24 +372,24 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>        case HID_UP_DIGITIZER:
>                switch (usage->hid) {
>                case HID_DG_INRANGE:
> -                       set_last_slot_field(usage, td, hi);
> +                       update_last_slot_field(usage, td);
>                        td->last_field_index = field->index;
>                        return 1;
>                case HID_DG_CONFIDENCE:
> -                       set_last_slot_field(usage, td, hi);
> +                       update_last_slot_field(usage, td);
>                        td->last_field_index = field->index;
>                        return 1;
>                case HID_DG_TIPSWITCH:
>                        hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
>                        input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
> -                       set_last_slot_field(usage, td, hi);
> +                       update_last_slot_field(usage, td);
>                        td->last_field_index = field->index;
>                        return 1;
>                case HID_DG_CONTACTID:
>                        if (!td->maxcontacts)
>                                td->maxcontacts = MT_DEFAULT_MAXCONTACT;
>                        input_mt_init_slots(hi->input, td->maxcontacts);
> -                       td->last_slot_field = usage->hid;
> +                       update_last_slot_field(usage, td);
>                        td->last_field_index = field->index;
>                        td->touches_by_report++;
>                        return 1;
> @@ -385,7 +398,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>                                        EV_ABS, ABS_MT_TOUCH_MAJOR);
>                        set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field,
>                                cls->sn_width);
> -                       set_last_slot_field(usage, td, hi);
> +                       update_last_slot_field(usage, td);
>                        td->last_field_index = field->index;
>                        return 1;
>                case HID_DG_HEIGHT:
> @@ -395,7 +408,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>                                cls->sn_height);
>                        input_set_abs_params(hi->input,
>                                        ABS_MT_ORIENTATION, 0, 1, 0, 0);
> -                       set_last_slot_field(usage, td, hi);
> +                       update_last_slot_field(usage, td);
>                        td->last_field_index = field->index;
>                        return 1;
>                case HID_DG_TIPPRESSURE:
> @@ -406,7 +419,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>                        /* touchscreen emulation */
>                        set_abs(hi->input, ABS_PRESSURE, field,
>                                cls->sn_pressure);
> -                       set_last_slot_field(usage, td, hi);
> +                       update_last_slot_field(usage, td);
>                        td->last_field_index = field->index;
>                        return 1;
>                case HID_DG_CONTACTCOUNT:
> --
> 1.7.3.4
> ==========================================================

RE: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Drews, Paul]

> -static void set_last_slot_field(struct hid_usage *usage, struct mt_device *td,
> +static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
>  		struct hid_input *hi)
>  {
> -	if (!test_bit(usage->hid, hi->input->absbit))
> -		td->last_slot_field = usage->hid;
> +	struct mt_fields *f = td->fields;
> +
> +	if (f->length >= HID_MAX_FIELDS)
> +		return;
> +
> +	f->usages[f->length++] = usage->hid;
>  }

The "hi" formal-parameter is no longer used, can go away.

Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Drews, Paul]

Hi Benjamin,

> Hi Paul,
> 
> On Fri, May 18, 2012 at 11:14 PM, Drews, Paul <paul.drews@intel.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> >> owner@vger.kernel.org] On Behalf Of Benjamin Tissoires
> >> Sent: Wednesday, May 09, 2012 12:04 PM
> >> To: Henrik Rydberg
> >> Cc: Dmitry Torokhov; Jiri Kosina; Stephane Chatty; linux-
> input@vger.kernel.org;
> >> linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection
> >>
...
> > If this is the case, how about avoiding storing all the slot-field values
> > and just detecting the point of repetition to use the most-recently-seen
> > usage value as the last-slot-field marker.  I have been successfully using
> > the patch below based on this notion.  It took the failure rate from about
> > 1-per-10 boots to 250+ boots with no failures on an Atmel MaXTouch.
> > I don't have others to try it with, including the "buggy" one that led
> > to all this trouble in the first place.
> 
> Thank you very much for this patch. However, Jiri already applied mine
> with the allocation/free mechanism.
> 
> You're idea is good but it has one big problem with Win8 devices:
> As we can have 2 X and 2 Y per touch report, if these dual-X reporting
> or dual-Y reporting is present in the report, we will stop at the
> second X or the second Y seen, which will lead to a buggy touchscreen
> (the first touch won't get it's second coordinate). However, without
> this particularity, the patch would have worked ;-)
> 
> If the Win8 norm has came earlier, the initial implementation that
> relies on the collection would have suffice, but some hardware makers
> made a bad use of it, leading us to stop using this, and relying on a
> more brutal approach.

Oops.  Didn't know about that.  If the first item is duplicated somewhere
in the sequence, that's a fatal problem for my approach.

> > +static void update_last_slot_field(struct hid_usage *usage,
> > +               struct mt_device *td)
> >  {
> > -       if (!test_bit(usage->hid, hi->input->absbit))
> > -               td->last_slot_field = usage->hid;
> > +       if (!td->last_slot_field_found) {
> > +               if (td->first_slot_field_found) {
> > +                       if (td->last_slot_field == usage->hid)
> 
> I'm sure you wanted to put here:
>                        if (td->first_slot_field == usage->hid)
> 
> Cheers,
> Benjamin

Good catch.  And as you point out, irrelevant since your patch is in
linux-next already.  I tested your commit 3ac36d1 from there with
a 3.4 (final) kernel on a Atmel MaXTouch Digitizer tablet and it is
working fine.

Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection

[Drews, Paul]

>>> The previous implementation introduced a randomness in the splitting
>>> of the different touches reported by the device. This version is more
>>> robust as we don't rely on hi->input->absbit, but on our own structure.
>>>
>>> This also prepares hid-multitouch to better support Win8 devices.
>>>
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>
>>>  struct mt_device {
>>>       struct mt_slot curdata; /* placeholder of incoming data */
>>>       struct mt_class mtclass;        /* our mt device class */
>>> +     struct mt_fields *fields;       /* temporary placeholder for storing the
>>> +                                        multitouch fields */
>>
>> Why not skip the pointer here?
>
>well, the idea was to keep the memory footprint low. As these values
>are only needed at init, then I freed them once I finished using them.
>I can of course skip the pointer, but in that case, wouldn't the
>struct declaration be worthless?
>
>
>>> +static void mt_post_parse(struct mt_device *td)
>>> +{
>>> +     struct mt_fields *f = td->fields;
>>> +
>>> +     if (td->touches_by_report > 0) {
>>> +             int field_count_per_touch = f->length / td->touches_by_report;
>>> +             td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
>>> +     }
>>> +}
>>> +
>

The patch as it stands more-or-less depends on the group of usage->hid
values repeating for each touch in the multi-touch report, and choosing
the last usage->hid seen in the first group as the ultimate last_slot_field
value.  A suggestion: as long as we're relying on this group repetition
anyway, why not take advantage of the repetition wrap-around to
detect the last_slot_field without having to allocate memory and store
everything?  I've been using the following patch that does it this way
with an Atmel MaXTouch Digitizer (3EB:211C).

Prior to this patch I was getting a MTBF of about 1 failure in 10 boots
due to the out-of-range bitmap lookup coming up with an unlucky
result and making the wrong last_slot_field conclusion.  Symptom:
touch events get reported to user-space with previous x,y coordinates.
Also confirmed using a printk to instrument the kernel for this.

With this patch, I have tested beyond 10X the MTBF on 3.4-rc7 with no failures.
I don't have a touchscreen other than that Atmel to test with.  Will this
method work with the buggy touchscreen that the original patch was
intended to fix?

Patch follows:
========================================================
>From 9ff29221247f6a3531f4b7939898fe708aa96830 Mon Sep 17 00:00:00 2001
From: Paul Drews <paul.drews@intel.com>
Date: Wed, 16 May 2012 11:15:00 -0700
Subject: [PATCH] Repair detection of last slot in multitouch reports

The logic for detecting the last per-touch slot in a
multitouch report erroneously used hid usage values (large
numbers such as 0xd0032) as indices into the smaller absbit
bitmap (with bit indexes up to 0x3f).  This caused
intermittent failures in the configuration of the last-slot
value leading to stale x,y coordinates being reported in
multi-touch input events.  It also carried the risk of a
segmentation fault due to the out-of-range bitmap index.

This patch takes a different approach of detecting the last
per-touch slot:  when the hid usage value wraps around to
the first hid usage value we have seen already, we must be
looking at the slots for the next touch of a multi-touch
report, so the last hid usage value we have seen so far must
be the last per-touch value.

Signed-off-by: Paul Drews <paul.drews@intel.com>
---
 drivers/hid/hid-multitouch.c |   39 ++++++++++++++++++++++++++-------------
 1 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2e6d187..226f828 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -75,6 +75,9 @@ struct mt_device {
 	struct mt_class mtclass;	/* our mt device class */
 	unsigned last_field_index;	/* last field index of the report */
 	unsigned last_slot_field;	/* the last field of a slot */
+	bool last_slot_field_found;	/* last_slot_field has full init */
+	unsigned first_slot_field;
+	bool first_slot_field_found;	/* for detecting wrap to next touch */
 	__s8 inputmode;		/* InputMode HID feature, -1 if non-existent */
 	__s8 maxcontact_report_id;	/* Maximum Contact Number HID feature,
 				   -1 if non-existent */
@@ -275,11 +278,21 @@ static void set_abs(struct input_dev *input, unsigned int code,
 	input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
 }
 
-static void set_last_slot_field(struct hid_usage *usage, struct mt_device *td,
-		struct hid_input *hi)
+static void update_last_slot_field(struct hid_usage *usage,
+		struct mt_device *td)
 {
-	if (!test_bit(usage->hid, hi->input->absbit))
-		td->last_slot_field = usage->hid;
+	if (!td->last_slot_field_found) {
+		if (td->first_slot_field_found) {
+			if (td->last_slot_field == usage->hid)
+				td->last_slot_field_found = true; /* wrapped */
+			else
+				td->last_slot_field = usage->hid;
+		} else {
+			td->first_slot_field = usage->hid;
+			td->first_slot_field_found = true;
+			td->last_slot_field = usage->hid;
+		}
+	}
 }
 
 static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
@@ -340,7 +353,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_X, field, cls->sn_move);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_GD_Y:
@@ -350,7 +363,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_move);
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_Y, field, cls->sn_move);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		}
@@ -359,24 +372,24 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	case HID_UP_DIGITIZER:
 		switch (usage->hid) {
 		case HID_DG_INRANGE:
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONFIDENCE:
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_TIPSWITCH:
 			hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
 			input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTID:
 			if (!td->maxcontacts)
 				td->maxcontacts = MT_DEFAULT_MAXCONTACT;
 			input_mt_init_slots(hi->input, td->maxcontacts);
-			td->last_slot_field = usage->hid;
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			td->touches_by_report++;
 			return 1;
@@ -385,7 +398,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 					EV_ABS, ABS_MT_TOUCH_MAJOR);
 			set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field,
 				cls->sn_width);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_HEIGHT:
@@ -395,7 +408,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				cls->sn_height);
 			input_set_abs_params(hi->input,
 					ABS_MT_ORIENTATION, 0, 1, 0, 0);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_TIPPRESSURE:
@@ -406,7 +419,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			/* touchscreen emulation */
 			set_abs(hi->input, ABS_PRESSURE, field,
 				cls->sn_pressure);
-			set_last_slot_field(usage, td, hi);
+			update_last_slot_field(usage, td);
 			td->last_field_index = field->index;
 			return 1;
 		case HID_DG_CONTACTCOUNT:
-- 
1.7.3.4
========================================================