Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection
From: Benjamin Tissoires
Date: Mon May 21 2012 - 12:43:28 EST
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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/