The Surface Pro 3 firmware doesn't reliably send contact lift off
reports nor handle invalid report values gracefully.
To reduce touchscreen input jitter:
- add MT_QUIRK_NOT_SEEN_MEANS_UP to the MT_CLS_WIN_8
- drop invalid report values
Joey Pabalinas (4):
HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks
HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol
HID: multitouch: drop reports containing invalid values
HID: multitouch: remove unneeded else conditional cases
drivers/hid/hid-multitouch.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
--
2.18.0
Avoid processing reports containing invalid values to reduce
multitouch input stutter.
Signed-off-by: Joey Pabalinas <[email protected]>
1 file changed, 9 insertions(+)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c0654db0b736543ca0..08b50e5908cecdda66 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -694,10 +694,19 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
{
if ((td->mtclass.quirks & MT_QUIRK_CONTACT_CNT_ACCURATE) &&
td->num_received >= td->num_expected)
return;
+ /* drop invalid values after counting them */
+ if (td->curdata.x == 0xffff &&
+ td->curdata.y == 0xffff &&
+ td->curdata.w == 0xffff &&
+ td->curdata.h == 0xffff) {
+ td->num_received++;
+ return;
+ }
+
if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
int active;
int slotnum = mt_compute_slot(td, input);
struct mt_slot *s = &td->curdata;
struct input_mt *mt = input->mt;
--
2.18.0
Elide lone `else` cases and replace `else if` clauses
with plain `if` conditionals when they occur immediately
after return statements.
Signed-off-by: Joey Pabalinas <[email protected]>
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 08b50e5908cecdda66..30b1a2c67f39a41325 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -205,12 +205,12 @@ static void mt_post_parse(struct mt_device *td);
static int cypress_compute_slot(struct mt_device *td)
{
if (td->curdata.contactid != 0 || td->num_received == 0)
return td->curdata.contactid;
- else
- return -1;
+
+ return -1;
}
static struct mt_class mt_classes[] = {
{ .name = MT_CLS_DEFAULT,
.quirks = MT_QUIRK_ALWAYS_VALID |
@@ -803,12 +803,12 @@ static int mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
delta *= 100;
if (jdelta > MAX_TIMESTAMP_INTERVAL)
/* No data received for a while, resync the timestamp. */
return 0;
- else
- return td->timestamp + delta;
+
+ return td->timestamp + delta;
}
static int mt_touch_event(struct hid_device *hid, struct hid_field *field,
struct hid_usage *usage, __s32 value)
{
@@ -1110,11 +1110,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
* HID_DG_CONTACTCOUNT from the pen report as it is outside the physical
* collection, but within the report ID.
*/
if (field->physical == HID_DG_STYLUS)
return 0;
- else if ((field->physical == 0) &&
+
+ if ((field->physical == 0) &&
(field->report->id != td->mt_report_id) &&
(td->mt_report_id != -1))
return 0;
if (field->application == HID_DG_TOUCHSCREEN ||
--
2.18.0
The HID_GROUP_MULTITOUCH_WIN_8 group never needs to check for the serial
protocol, so avoid setting `td->serial_maybe = true;` in order to avoid
an unnecessary mt_post_parse_default_settings() call
Signed-off-by: Joey Pabalinas <[email protected]>
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index a793076139d7d0db9b..c0654db0b736543ca0 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1460,11 +1460,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (!td->fields) {
dev_err(&hdev->dev, "cannot allocate multitouch fields data\n");
return -ENOMEM;
}
- if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
+ if (id->vendor == HID_ANY_ID
+ && id->product == HID_ANY_ID
+ && id->group != HID_GROUP_MULTITOUCH_WIN_8)
td->serial_maybe = true;
/* This allows the driver to correctly support devices
* that emit events over several HID messages.
*/
--
2.18.0
The firmware found in the touch screen of the Surface Pro 3 is slightly
buggy and occasionally doesn't send lift off reports for contacts; add
MT_QUIRK_NOT_SEEN_MEANS_UP to .quirks to compensate for the missed
reports.
Signed-off-by: Joey Pabalinas <[email protected]>
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 45968f7970f87775fa..a793076139d7d0db9b 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -242,11 +242,12 @@ static struct mt_class mt_classes[] = {
.quirks = MT_QUIRK_ALWAYS_VALID |
MT_QUIRK_IGNORE_DUPLICATES |
MT_QUIRK_HOVERING |
MT_QUIRK_CONTACT_CNT_ACCURATE |
MT_QUIRK_STICKY_FINGERS |
- MT_QUIRK_WIN8_PTP_BUTTONS },
+ MT_QUIRK_WIN8_PTP_BUTTONS |
+ MT_QUIRK_NOT_SEEN_MEANS_UP },
{ .name = MT_CLS_EXPORT_ALL_INPUTS,
.quirks = MT_QUIRK_ALWAYS_VALID |
MT_QUIRK_CONTACT_CNT_ACCURATE,
.export_all_inputs = true },
{ .name = MT_CLS_WIN_8_DUAL,
--
2.18.0
On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <[email protected]> wrote:
> The firmware found in the touch screen of the Surface Pro 3 is slightly
> buggy and occasionally doesn't send lift off reports for contacts; add
> MT_QUIRK_NOT_SEEN_MEANS_UP to .quirks to compensate for the missed
> reports.
>
> Signed-off-by: Joey Pabalinas <[email protected]>
>
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 45968f7970f87775fa..a793076139d7d0db9b 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -242,11 +242,12 @@ static struct mt_class mt_classes[] = {
> .quirks = MT_QUIRK_ALWAYS_VALID |
> MT_QUIRK_IGNORE_DUPLICATES |
> MT_QUIRK_HOVERING |
> MT_QUIRK_CONTACT_CNT_ACCURATE |
> MT_QUIRK_STICKY_FINGERS |
> - MT_QUIRK_WIN8_PTP_BUTTONS },
> + MT_QUIRK_WIN8_PTP_BUTTONS |
> + MT_QUIRK_NOT_SEEN_MEANS_UP },
NACK on this.
If the Surface has a buggy firmware, we should handle it as a special
case, not as a common failure.
Also, I am not sure this quirk is compatible with Win 8 specification.
Last, we now have a timeout for unreleased touches, so I really don't
think you need that in recent kernels.
Cheers,
Benjamin
> { .name = MT_CLS_EXPORT_ALL_INPUTS,
> .quirks = MT_QUIRK_ALWAYS_VALID |
> MT_QUIRK_CONTACT_CNT_ACCURATE,
> .export_all_inputs = true },
> { .name = MT_CLS_WIN_8_DUAL,
> --
> 2.18.0
>
Hi Joey,
On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <[email protected]> wrote:
> Avoid processing reports containing invalid values to reduce
> multitouch input stutter.
>
> Signed-off-by: Joey Pabalinas <[email protected]>
>
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index c0654db0b736543ca0..08b50e5908cecdda66 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -694,10 +694,19 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
> {
> if ((td->mtclass.quirks & MT_QUIRK_CONTACT_CNT_ACCURATE) &&
> td->num_received >= td->num_expected)
> return;
>
> + /* drop invalid values after counting them */
> + if (td->curdata.x == 0xffff &&
> + td->curdata.y == 0xffff &&
> + td->curdata.w == 0xffff &&
> + td->curdata.h == 0xffff) {
You can't really use plain values like that. There is a tiny chance
these values are valid on an other device.
IIRC, MS spec says that we should ignore out of band values if they
are tagged as such. Such input are tagged with NULL values
(http://www.usb.org/developers/hidpage/HID1_11.pdf page 31) and MS
spec mentioned this.
All in all, if you have this bit set, you need to compare the value
with the logical_max/min for each field.
I never encountered a device that required this, so you are probably
the lucky one :)
Cheers,
Benjamin
> + td->num_received++;
> + return;
> + }
> +
> if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
> int active;
> int slotnum = mt_compute_slot(td, input);
> struct mt_slot *s = &td->curdata;
> struct input_mt *mt = input->mt;
> --
> 2.18.0
>
On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <[email protected]> wrote:
> The HID_GROUP_MULTITOUCH_WIN_8 group never needs to check for the serial
> protocol, so avoid setting `td->serial_maybe = true;` in order to avoid
> an unnecessary mt_post_parse_default_settings() call
>
> Signed-off-by: Joey Pabalinas <[email protected]>
>
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index a793076139d7d0db9b..c0654db0b736543ca0 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -1460,11 +1460,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (!td->fields) {
> dev_err(&hdev->dev, "cannot allocate multitouch fields data\n");
> return -ENOMEM;
> }
>
> - if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
> + if (id->vendor == HID_ANY_ID
> + && id->product == HID_ANY_ID
> + && id->group != HID_GROUP_MULTITOUCH_WIN_8)
There is a tiny difference between the HID group (this device looks
like it is used as a Win 8 device) and the device class (this is
effectively a Win8 device)
It makes sense to remove this check for Win8 devices, but I don't
think it should be done for all devices.
All in all, it won't change much.
Cheers,
Benjamin
> td->serial_maybe = true;
>
> /* This allows the driver to correctly support devices
> * that emit events over several HID messages.
> */
> --
> 2.18.0
>
On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <[email protected]> wrote:
> Elide lone `else` cases and replace `else if` clauses
> with plain `if` conditionals when they occur immediately
> after return statements.
>
> Signed-off-by: Joey Pabalinas <[email protected]>
>
This one looks good.
Reviewed-by: Benjamin Tissoires <[email protected]>
Just FYI, I sent out a big refactor of the hid-multitouch code. Jiri
should be still reviewing it, so I am not so sure who will have to
rebase which series on top of the other, but someone between us will
have to do it :)
Cheers,
Benjamin
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 08b50e5908cecdda66..30b1a2c67f39a41325 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -205,12 +205,12 @@ static void mt_post_parse(struct mt_device *td);
>
> static int cypress_compute_slot(struct mt_device *td)
> {
> if (td->curdata.contactid != 0 || td->num_received == 0)
> return td->curdata.contactid;
> - else
> - return -1;
> +
> + return -1;
> }
>
> static struct mt_class mt_classes[] = {
> { .name = MT_CLS_DEFAULT,
> .quirks = MT_QUIRK_ALWAYS_VALID |
> @@ -803,12 +803,12 @@ static int mt_compute_timestamp(struct mt_device *td, struct hid_field *field,
> delta *= 100;
>
> if (jdelta > MAX_TIMESTAMP_INTERVAL)
> /* No data received for a while, resync the timestamp. */
> return 0;
> - else
> - return td->timestamp + delta;
> +
> + return td->timestamp + delta;
> }
>
> static int mt_touch_event(struct hid_device *hid, struct hid_field *field,
> struct hid_usage *usage, __s32 value)
> {
> @@ -1110,11 +1110,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> * HID_DG_CONTACTCOUNT from the pen report as it is outside the physical
> * collection, but within the report ID.
> */
> if (field->physical == HID_DG_STYLUS)
> return 0;
> - else if ((field->physical == 0) &&
> +
> + if ((field->physical == 0) &&
> (field->report->id != td->mt_report_id) &&
> (td->mt_report_id != -1))
> return 0;
>
> if (field->application == HID_DG_TOUCHSCREEN ||
> --
> 2.18.0
>
On Tue, Jul 03, 2018 at 10:17:58AM +0200, Benjamin Tissoires wrote:
> This one looks good.
> Reviewed-by: Benjamin Tissoires <[email protected]>
>
> Just FYI, I sent out a big refactor of the hid-multitouch code. Jiri
> should be still reviewing it, so I am not so sure who will have to
> rebase which series on top of the other, but someone between us will
> have to do it :)
Somehow also missed this reply... I guess I messed up my LKML
filters somewhere. Noted, thanks for the review; I'll check
your trees for the refactor when (if) I do my v2.
--
Cheers,
Joey Pabalinas
On Tue, Jul 03, 2018 at 09:53:19AM +0200, Benjamin Tissoires wrote:
> NACK on this.
> If the Surface has a buggy firmware, we should handle it as a special
> case, not as a common failure.
> Also, I am not sure this quirk is compatible with Win 8 specification.
> Last, we now have a timeout for unreleased touches, so I really don't
> think you need that in recent kernels.
>
> Cheers,
> Benjamin
>
> > { .name = MT_CLS_EXPORT_ALL_INPUTS,
> > .quirks = MT_QUIRK_ALWAYS_VALID |
> > MT_QUIRK_CONTACT_CNT_ACCURATE,
> > .export_all_inputs = true },
> > { .name = MT_CLS_WIN_8_DUAL,
> > --
> > 2.18.0
> >
Ah shoot, I had completely missed this reply to my earlier patchset, I
apologize. Now that you mention it, I think handling it as a special
case would be a better way to handle it, so I'll do a bit more research
on the quirk thing and then revise my patches (assuming they end up
as something still worth sending). Appreciate the comments, thanks.
--
Cheers,
Joey Pabalinas
On Tue, Jul 03, 2018 at 10:13:54AM +0200, Benjamin Tissoires wrote:
> Hi Joey,
> You can't really use plain values like that. There is a tiny chance
> these values are valid on an other device.
> IIRC, MS spec says that we should ignore out of band values if they
> are tagged as such. Such input are tagged with NULL values
> (http://www.usb.org/developers/hidpage/HID1_11.pdf page 31) and MS
> spec mentioned this.
>
> All in all, if you have this bit set, you need to compare the value
> with the logical_max/min for each field.
>
> I never encountered a device that required this, so you are probably
> the lucky one :)
Ah, you are completely right. After giving that pdf a read over
I will definitely be dropping this patch from the v2.
--
Cheers,
Joey Pabalinas
On Tue, Jul 03, 2018 at 10:16:01AM +0200, Benjamin Tissoires wrote:
> There is a tiny difference between the HID group (this device looks
> like it is used as a Win 8 device) and the device class (this is
> effectively a Win8 device)
>
> It makes sense to remove this check for Win8 devices, but I don't
> think it should be done for all devices.
>
> All in all, it won't change much.
Hm, that sounds sane. I'll do a bit more research on this and see if
restricting it to just Win8 devices is simple enough to be worthwhile.
--
Cheers,
Joey Pabalinas
On Thu, Aug 09, 2018 at 01:39:16PM -1000, Joey Pabalinas wrote:
> The firmware found in the touch screen of the Surface Pro 3 is slightly
> buggy and occasionally doesn't send lift off reports for contacts; add
> MT_QUIRK_NOT_SEEN_MEANS_UP to .quirks to compensate for the missed
> reports.
>
> Signed-off-by: Joey Pabalinas <[email protected]>
Sorry, my mail filters are in need of some adjusting; I completely missed
the review of the first send of this patchset :(
Please disregard this resend, as I am going to revise it and submit a
v2, thanks.
--
Cheers,
Joey Pabalinas