Hi Paul, On Fri, May 18, 2012 at 11:14 PM, Drews, Paul <paul.drews@xxxxxxxxx> wrote: > > >> -----Original Message----- >> From: linux-kernel-owner@xxxxxxxxxxxxxxx [mailto:linux-kernel- >> owner@xxxxxxxxxxxxxxx] 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@xxxxxxxxxxxxxxx; >> linux-kernel@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection >> > >> On Sun, May 6, 2012 at 9:01 PM, Henrik Rydberg <rydberg@xxxxxxxxxxx> 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@xxxxxxxxx> > 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@xxxxxxxxx> > --- > 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 > ========================================================== -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
