linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] reduce Surface Pro 3 multitouch jitter
@ 2018-07-01  0:19 Joey Pabalinas
  2018-07-01  0:19 ` [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks Joey Pabalinas
                   ` (4 more replies)
  0 siblings, 5 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-07-01  0:19 UTC (permalink / raw)
  To: linux-input; +Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Daniel Martin

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks
  2018-07-01  0:19 [PATCH 0/4] reduce Surface Pro 3 multitouch jitter Joey Pabalinas
@ 2018-07-01  0:19 ` Joey Pabalinas
  2018-07-03  7:53   ` Benjamin Tissoires
                     ` (2 more replies)
  2018-07-01  0:19 ` [PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol Joey Pabalinas
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-07-01  0:19 UTC (permalink / raw)
  To: linux-input; +Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Daniel Martin

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 <joeypabalinas@gmail.com>

 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol
  2018-07-01  0:19 [PATCH 0/4] reduce Surface Pro 3 multitouch jitter Joey Pabalinas
  2018-07-01  0:19 ` [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks Joey Pabalinas
@ 2018-07-01  0:19 ` Joey Pabalinas
  2018-07-03  8:16   ` Benjamin Tissoires
  2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
  2018-07-01  0:19 ` [PATCH 3/4] HID: multitouch: drop reports containing invalid values Joey Pabalinas
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-07-01  0:19 UTC (permalink / raw)
  To: linux-input; +Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Daniel Martin

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 <joeypabalinas@gmail.com>

 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/4] HID: multitouch: drop reports containing invalid values
  2018-07-01  0:19 [PATCH 0/4] reduce Surface Pro 3 multitouch jitter Joey Pabalinas
  2018-07-01  0:19 ` [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks Joey Pabalinas
  2018-07-01  0:19 ` [PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol Joey Pabalinas
@ 2018-07-01  0:19 ` Joey Pabalinas
  2018-07-03  8:13   ` Benjamin Tissoires
  2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
  2018-07-01  0:19 ` [PATCH 4/4] HID: multitouch: remove unneeded else conditional cases Joey Pabalinas
  2018-08-09 23:39 ` [PATCH RESEND 0/4] reduce Surface Pro 3 multitouch jitter Joey Pabalinas
  4 siblings, 2 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-07-01  0:19 UTC (permalink / raw)
  To: linux-input; +Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Daniel Martin

Avoid processing reports containing invalid values to reduce
multitouch input stutter.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/4] HID: multitouch: remove unneeded else conditional cases
  2018-07-01  0:19 [PATCH 0/4] reduce Surface Pro 3 multitouch jitter Joey Pabalinas
                   ` (2 preceding siblings ...)
  2018-07-01  0:19 ` [PATCH 3/4] HID: multitouch: drop reports containing invalid values Joey Pabalinas
@ 2018-07-01  0:19 ` Joey Pabalinas
  2018-07-03  8:17   ` Benjamin Tissoires
  2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
  2018-08-09 23:39 ` [PATCH RESEND 0/4] reduce Surface Pro 3 multitouch jitter Joey Pabalinas
  4 siblings, 2 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-07-01  0:19 UTC (permalink / raw)
  To: linux-input; +Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Daniel Martin

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 <joeypabalinas@gmail.com>

 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks
  2018-07-01  0:19 ` [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks Joey Pabalinas
@ 2018-07-03  7:53   ` Benjamin Tissoires
  2018-08-09 23:57     ` Joey Pabalinas
  2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
  2018-08-10  3:54   ` Joey Pabalinas
  2 siblings, 1 reply; 19+ messages in thread
From: Benjamin Tissoires @ 2018-07-03  7:53 UTC (permalink / raw)
  To: Joey Pabalinas; +Cc: open list:HID CORE LAYER, lkml, Jiri Kosina, Daniel Martin

On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <joeypabalinas@gmail.com> 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 <joeypabalinas@gmail.com>
>
>  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
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/4] HID: multitouch: drop reports containing invalid values
  2018-07-01  0:19 ` [PATCH 3/4] HID: multitouch: drop reports containing invalid values Joey Pabalinas
@ 2018-07-03  8:13   ` Benjamin Tissoires
  2018-08-10  3:27     ` Joey Pabalinas
  2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
  1 sibling, 1 reply; 19+ messages in thread
From: Benjamin Tissoires @ 2018-07-03  8:13 UTC (permalink / raw)
  To: Joey Pabalinas; +Cc: open list:HID CORE LAYER, lkml, Jiri Kosina, Daniel Martin

Hi Joey,

On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <joeypabalinas@gmail.com> wrote:
> Avoid processing reports containing invalid values to reduce
> multitouch input stutter.
>
> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
>
>  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
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol
  2018-07-01  0:19 ` [PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol Joey Pabalinas
@ 2018-07-03  8:16   ` Benjamin Tissoires
  2018-08-10  3:50     ` Joey Pabalinas
  2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
  1 sibling, 1 reply; 19+ messages in thread
From: Benjamin Tissoires @ 2018-07-03  8:16 UTC (permalink / raw)
  To: Joey Pabalinas; +Cc: open list:HID CORE LAYER, lkml, Jiri Kosina, Daniel Martin

On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <joeypabalinas@gmail.com> 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 <joeypabalinas@gmail.com>
>
>  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
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] HID: multitouch: remove unneeded else conditional cases
  2018-07-01  0:19 ` [PATCH 4/4] HID: multitouch: remove unneeded else conditional cases Joey Pabalinas
@ 2018-07-03  8:17   ` Benjamin Tissoires
  2018-08-10  0:03     ` Joey Pabalinas
  2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
  1 sibling, 1 reply; 19+ messages in thread
From: Benjamin Tissoires @ 2018-07-03  8:17 UTC (permalink / raw)
  To: Joey Pabalinas; +Cc: open list:HID CORE LAYER, lkml, Jiri Kosina, Daniel Martin

On Sun, Jul 1, 2018 at 2:19 AM, Joey Pabalinas <joeypabalinas@gmail.com> 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 <joeypabalinas@gmail.com>
>

This one looks good.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

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
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH RESEND 0/4] reduce Surface Pro 3 multitouch jitter
  2018-07-01  0:19 [PATCH 0/4] reduce Surface Pro 3 multitouch jitter Joey Pabalinas
                   ` (3 preceding siblings ...)
  2018-07-01  0:19 ` [PATCH 4/4] HID: multitouch: remove unneeded else conditional cases Joey Pabalinas
@ 2018-08-09 23:39 ` Joey Pabalinas
  4 siblings, 0 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-08-09 23:39 UTC (permalink / raw)
  To: linux-input; +Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Daniel Martin

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

Patches have been tested on my personal Surface Pro 3 for a
couple months without any problems, as well as being run in my
Arch Linux AUR kernel package [1] without a single complaint
so far.

[1] https://aur.archlinux.org/packages/linux-surfacepro3-git

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH RESEND 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks
  2018-07-01  0:19 ` [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks Joey Pabalinas
  2018-07-03  7:53   ` Benjamin Tissoires
@ 2018-08-09 23:39   ` Joey Pabalinas
  2018-08-10  3:54   ` Joey Pabalinas
  2 siblings, 0 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-08-09 23:39 UTC (permalink / raw)
  To: linux-input; +Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Daniel Martin

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 <joeypabalinas@gmail.com>

 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH RESEND 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol
  2018-07-01  0:19 ` [PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol Joey Pabalinas
  2018-07-03  8:16   ` Benjamin Tissoires
@ 2018-08-09 23:39   ` Joey Pabalinas
  1 sibling, 0 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-08-09 23:39 UTC (permalink / raw)
  To: linux-input; +Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Daniel Martin

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 <joeypabalinas@gmail.com>

 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH RESEND 3/4] HID: multitouch: drop reports containing invalid values
  2018-07-01  0:19 ` [PATCH 3/4] HID: multitouch: drop reports containing invalid values Joey Pabalinas
  2018-07-03  8:13   ` Benjamin Tissoires
@ 2018-08-09 23:39   ` Joey Pabalinas
  1 sibling, 0 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-08-09 23:39 UTC (permalink / raw)
  To: linux-input; +Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Daniel Martin

Avoid processing reports containing invalid values to reduce
multitouch input stutter.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH RESEND 4/4] HID: multitouch: remove unneeded else conditional cases
  2018-07-01  0:19 ` [PATCH 4/4] HID: multitouch: remove unneeded else conditional cases Joey Pabalinas
  2018-07-03  8:17   ` Benjamin Tissoires
@ 2018-08-09 23:39   ` Joey Pabalinas
  1 sibling, 0 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-08-09 23:39 UTC (permalink / raw)
  To: linux-input; +Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires, Daniel Martin

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 <joeypabalinas@gmail.com>

 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks
  2018-07-03  7:53   ` Benjamin Tissoires
@ 2018-08-09 23:57     ` Joey Pabalinas
  0 siblings, 0 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-08-09 23:57 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Joey Pabalinas, open list:HID CORE LAYER, lkml, Jiri Kosina,
	Daniel Martin

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] HID: multitouch: remove unneeded else conditional cases
  2018-07-03  8:17   ` Benjamin Tissoires
@ 2018-08-10  0:03     ` Joey Pabalinas
  0 siblings, 0 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-08-10  0:03 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Joey Pabalinas, open list:HID CORE LAYER, lkml, Jiri Kosina,
	Daniel Martin

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On Tue, Jul 03, 2018 at 10:17:58AM +0200, Benjamin Tissoires wrote:
> This one looks good.
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/4] HID: multitouch: drop reports containing invalid values
  2018-07-03  8:13   ` Benjamin Tissoires
@ 2018-08-10  3:27     ` Joey Pabalinas
  0 siblings, 0 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-08-10  3:27 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Joey Pabalinas, open list:HID CORE LAYER, lkml, Jiri Kosina,
	Daniel Martin

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol
  2018-07-03  8:16   ` Benjamin Tissoires
@ 2018-08-10  3:50     ` Joey Pabalinas
  0 siblings, 0 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-08-10  3:50 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Joey Pabalinas, open list:HID CORE LAYER, lkml, Jiri Kosina,
	Daniel Martin

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH RESEND 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks
  2018-07-01  0:19 ` [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks Joey Pabalinas
  2018-07-03  7:53   ` Benjamin Tissoires
  2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
@ 2018-08-10  3:54   ` Joey Pabalinas
  2 siblings, 0 replies; 19+ messages in thread
From: Joey Pabalinas @ 2018-08-10  3:54 UTC (permalink / raw)
  To: Joey Pabalinas
  Cc: linux-input, linux-kernel, Jiri Kosina, Benjamin Tissoires,
	Daniel Martin

[-- Attachment #1: Type: text/plain, Size: 604 bytes --]

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 <joeypabalinas@gmail.com>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2018-08-10  3:54 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-01  0:19 [PATCH 0/4] reduce Surface Pro 3 multitouch jitter Joey Pabalinas
2018-07-01  0:19 ` [PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks Joey Pabalinas
2018-07-03  7:53   ` Benjamin Tissoires
2018-08-09 23:57     ` Joey Pabalinas
2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
2018-08-10  3:54   ` Joey Pabalinas
2018-07-01  0:19 ` [PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol Joey Pabalinas
2018-07-03  8:16   ` Benjamin Tissoires
2018-08-10  3:50     ` Joey Pabalinas
2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
2018-07-01  0:19 ` [PATCH 3/4] HID: multitouch: drop reports containing invalid values Joey Pabalinas
2018-07-03  8:13   ` Benjamin Tissoires
2018-08-10  3:27     ` Joey Pabalinas
2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
2018-07-01  0:19 ` [PATCH 4/4] HID: multitouch: remove unneeded else conditional cases Joey Pabalinas
2018-07-03  8:17   ` Benjamin Tissoires
2018-08-10  0:03     ` Joey Pabalinas
2018-08-09 23:39   ` [PATCH RESEND " Joey Pabalinas
2018-08-09 23:39 ` [PATCH RESEND 0/4] reduce Surface Pro 3 multitouch jitter Joey Pabalinas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).