mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [patch 0/5] Bug fix and win8 for hid-multitouch
@ 2012-05-04 12:53 benjamin.tissoires
  2012-05-04 12:53 ` [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection benjamin.tissoires
                   ` (5 more replies)
  0 siblings, 6 replies; 27+ messages in thread
From: benjamin.tissoires @ 2012-05-04 12:53 UTC (permalink / raw)
  To: benjamin.tissoires, Dmitry Torokhov, Henrik Rydberg, Jiri Kosina,
	Stephane Chatty, linux-input, linux-kernel

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


^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection
@ 2012-05-16 20:34 Drews, Paul
  0 siblings, 0 replies; 27+ messages in thread
From: Drews, Paul @ 2012-05-16 20:34 UTC (permalink / raw)
  To: linux-kernel; +Cc: benjamin.tissoires

>>> 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
========================================================

^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection
@ 2012-05-23 20:27 Drews, Paul
  0 siblings, 0 replies; 27+ messages in thread
From: Drews, Paul @ 2012-05-23 20:27 UTC (permalink / raw)
  To: linux-kernel; +Cc: Benjamin Tissoires (benjamin.tissoires@gmail.com)

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread
end of thread, other threads:[~2012-05-23 20:28 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-04 12:53 [patch 0/5] Bug fix and win8 for hid-multitouch benjamin.tissoires
2012-05-04 12:53 ` [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection benjamin.tissoires
2012-05-06 19:01   ` Henrik Rydberg
2012-05-09 19:04     ` Benjamin Tissoires
2012-05-09 19:56       ` Henrik Rydberg
2012-05-10  8:31         ` Jiri Kosina
2012-05-10  9:32           ` Benjamin Tissoires
2012-05-10  9:37             ` Jiri Kosina
2012-05-18 21:14       ` Drews, Paul
2012-05-21 16:43         ` Benjamin Tissoires
2012-05-21 19:01   ` Drews, Paul
2012-05-04 12:53 ` [PATCH 2/5] HID: hid-multitouch: get maxcontacts also from logical_max value benjamin.tissoires
2012-05-06 19:03   ` Henrik Rydberg
2012-05-09 19:13     ` Benjamin Tissoires
2012-05-09 19:46       ` Henrik Rydberg
2012-05-10 12:15         ` Benjamin Tissoires
2012-05-10 12:46           ` Henrik Rydberg
2012-05-04 12:53 ` [PATCH 3/5] HID: hid-multitouch: support arrays for the split of the touches in a report benjamin.tissoires
2012-05-04 12:53 ` [PATCH 4/5] input: Introduce MT_CENTER_X and MT_CENTER_Y benjamin.tissoires
2012-05-04 13:48   ` Henrik Rydberg
2012-05-06 14:34     ` Benjamin Tissoires
2012-05-06 17:43       ` Henrik Rydberg
2012-05-04 12:53 ` [PATCH 5/5] HID: hid-multitouch: support T and C for win8 devices benjamin.tissoires
2012-05-09 14:39 ` [patch 0/5] Bug fix and win8 for hid-multitouch Jiri Kosina
2012-05-09 17:52   ` Benjamin Tissoires
2012-05-16 20:34 [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection Drews, Paul
2012-05-23 20:27 Drews, Paul

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome