[PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

According to Microsoft specification [1] for Precision Touchpads (and
Touchscreens) the devices use "confidence" reports to signal accidental
touches, or contacts that are "too large to be a finger". Instead of
simply marking contact inactive in this case (which causes issues if
contact was originally proper and we lost confidence in it later, as
this results in accidental clicks, drags, etc), let's report such
contacts as MT_TOOL_PALM and let userspace decide what to do.
Additionally, let's report contact size for such touches as maximum
allowed for major/minor, which should help userspace that is not yet
aware of MT_TOOL_PALM to still perform palm rejection.

An additional complication, is that some firmwares do not report
non-confident touches as active. To cope with this we delay release of
such contact (i.e. if contact was active we first report it as still
active MT+TOOL_PALM and then synthesize the release event in a separate
frame).

[1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-multitouch.c | 86 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 77 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 440b999304a5..c28defe50a10 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -114,6 +114,8 @@ struct mt_device {
 	struct timer_list release_timer;	/* to release sticky fingers */
 	struct mt_fields *fields;	/* temporary placeholder for storing the
 					   multitouch fields */
+	unsigned long *pending_palm_slots; /* slots where we reported palm
+						and need to release */
 	unsigned long mt_io_flags;	/* mt flags (MT_IO_FLAGS_*) */
 	int cc_index;	/* contact count field index in the report */
 	int cc_value_index;	/* contact count value index in the field */
@@ -543,8 +545,12 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		case HID_DG_CONFIDENCE:
 			if ((cls->name == MT_CLS_WIN_8 ||
 				cls->name == MT_CLS_WIN_8_DUAL) &&
-				field->application == HID_DG_TOUCHPAD)
+			    field->application == HID_DG_TOUCHPAD) {
 				cls->quirks |= MT_QUIRK_CONFIDENCE;
+				input_set_abs_params(hi->input,
+					ABS_MT_TOOL_TYPE,
+					MT_TOOL_FINGER, MT_TOOL_PALM, 0, 0);
+			}
 			mt_store_field(usage, td, hi);
 			return 1;
 		case HID_DG_TIPSWITCH:
@@ -657,6 +663,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 
 	if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
 		int active;
+		int tool;
 		int slotnum = mt_compute_slot(td, input);
 		struct mt_slot *s = &td->curdata;
 		struct input_mt *mt = input->mt;
@@ -671,24 +678,56 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 				return;
 		}
 
+		active = s->touch_state || s->inrange_state;
+
 		if (!(td->mtclass.quirks & MT_QUIRK_CONFIDENCE))
 			s->confidence_state = 1;
-		active = (s->touch_state || s->inrange_state) &&
-							s->confidence_state;
+
+		if (likely(s->confidence_state)) {
+			tool = MT_TOOL_FINGER;
+		} else {
+			tool = MT_TOOL_PALM;
+			if (!active &&
+			    input_mt_is_active(&mt->slots[slotnum])) {
+				/*
+				 * The non-confidence was reported for
+				 * previously valid contact that is also no
+				 * longer valid. We can't simply report
+				 * lift-off as userspace will not be aware
+				 * of non-confidence, so we need to split
+				 * it into 2 events: active MT_TOOL_PALM
+				 * and a separate liftoff.
+				 */
+				active = true;
+				set_bit(slotnum, td->pending_palm_slots);
+			}
+		}
 
 		input_mt_slot(input, slotnum);
-		input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
+		input_mt_report_slot_state(input, tool, active);
 		if (active) {
 			/* this finger is in proximity of the sensor */
 			int wide = (s->w > s->h);
 			int major = max(s->w, s->h);
 			int minor = min(s->w, s->h);
 
-			/*
-			 * divided by two to match visual scale of touch
-			 * for devices with this quirk
-			 */
-			if (td->mtclass.quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
+			if (unlikely(!s->confidence_state)) {
+				/*
+				 * When reporting palm, set contact to maximum
+				 * size to help userspace that does not
+				 * recognize MT_TOOL_PALM to reject contacts
+				 * that are too large.
+				 */
+				major = input_abs_get_max(input,
+							  ABS_MT_TOUCH_MAJOR);
+				minor = input_abs_get_max(input,
+							  ABS_MT_TOUCH_MINOR);
+			} else if (td->mtclass.quirks &
+					MT_QUIRK_TOUCH_SIZE_SCALING) {
+				/*
+				 * divided by two to match visual scale of touch
+				 * for devices with this quirk
+				 */
 				major = major >> 1;
 				minor = minor >> 1;
 			}
@@ -711,6 +750,25 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
 	td->num_received++;
 }
 
+static void mt_release_pending_palms(struct mt_device *td,
+				     struct input_dev *input)
+{
+	int slotnum;
+	bool need_sync = false;
+
+	for_each_set_bit(slotnum, td->pending_palm_slots, td->maxcontacts) {
+		clear_bit(slotnum, td->pending_palm_slots);
+
+		input_mt_slot(input, slotnum);
+		input_mt_report_slot_state(input, MT_TOOL_PALM, false);
+
+		need_sync = true;
+	}
+
+	if (need_sync)
+		input_sync(input);
+}
+
 /*
  * this function is called when a whole packet has been received and processed,
  * so that it can decide what to send to the input layer.
@@ -719,6 +777,9 @@ static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
 {
 	input_mt_sync_frame(input);
 	input_sync(input);
+
+	mt_release_pending_palms(td, input);
+
 	td->num_received = 0;
 	if (test_bit(MT_IO_FLAGS_ACTIVE_SLOTS, &td->mt_io_flags))
 		set_bit(MT_IO_FLAGS_PENDING_SLOTS, &td->mt_io_flags);
@@ -903,6 +964,13 @@ static int mt_touch_input_configured(struct hid_device *hdev,
 	if (td->is_buttonpad)
 		__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
 
+	td->pending_palm_slots = devm_kcalloc(&hi->input->dev,
+					      BITS_TO_LONGS(td->maxcontacts),
+					      sizeof(long),
+					      GFP_KERNEL);
+	if (!td->pending_palm_slots)
+		return -ENOMEM;
+
 	ret = input_mt_init_slots(input, td->maxcontacts, td->mt_flags);
 	if (ret)
 		return ret;
-- 
2.14.0.434.g98096fd7a8-goog

[PATCH 2/2] HID: multitouch: touchscreens also use confidence reports

[Dmitry Torokhov]

According to [1] the confidence is used not only by touchpad devices,
but also by touchscreens.

[1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchscreen-required-hid-top-level-collections

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-multitouch.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c28defe50a10..1cb00de4bfd1 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -545,7 +545,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		case HID_DG_CONFIDENCE:
 			if ((cls->name == MT_CLS_WIN_8 ||
 				cls->name == MT_CLS_WIN_8_DUAL) &&
-			    field->application == HID_DG_TOUCHPAD) {
+			    (field->application == HID_DG_TOUCHPAD ||
+				field->application == HID_DG_TOUCHSCREEN)) {
 				cls->quirks |= MT_QUIRK_CONFIDENCE;
 				input_set_abs_params(hi->input,
 					ABS_MT_TOOL_TYPE,
-- 
2.14.0.434.g98096fd7a8-goog

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Henrik Rydberg]

Hi Dmitry,

On 08/11/2017 02:44 AM, Dmitry Torokhov wrote:

> According to Microsoft specification [1] for Precision Touchpads (and
> Touchscreens) the devices use "confidence" reports to signal accidental
> touches, or contacts that are "too large to be a finger". Instead of
> simply marking contact inactive in this case (which causes issues if
> contact was originally proper and we lost confidence in it later, as
> this results in accidental clicks, drags, etc), let's report such
> contacts as MT_TOOL_PALM and let userspace decide what to do.
> Additionally, let's report contact size for such touches as maximum
> allowed for major/minor, which should help userspace that is not yet
> aware of MT_TOOL_PALM to still perform palm rejection.
>
> An additional complication, is that some firmwares do not report
> non-confident touches as active. To cope with this we delay release of
> such contact (i.e. if contact was active we first report it as still
> active MT+TOOL_PALM and then synthesize the release event in a separate
> frame).
Changing the tool identity to signal the tool property of low confidence 
does not seem quite right to me. Using MT_TOOL_PALM forces a semantic 
distinction between tool identity and touch state, which userland seems 
unprepared for. The additional kernel state needed to make it work 
raises the question if more considerations will turn up over with time.

Why not add a property event, like BTN_TOOL_PALM, instead? In other 
words, modifying the definition of "active" as you propose, but then use 
a BTN_TOOL_PALM property to signal "s->confidence_state"? It perhaps 
creates a different oddity for applications unaware of palm, but AFAICT, 
it would not complicate the notion of touch state. Or?

Henrik

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

Hi Henrik,

On Thu, Aug 10, 2017 at 11:14 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> Hi Dmitry,
>
> On 08/11/2017 02:44 AM, Dmitry Torokhov wrote:
>
>> According to Microsoft specification [1] for Precision Touchpads (and
>> Touchscreens) the devices use "confidence" reports to signal accidental
>> touches, or contacts that are "too large to be a finger". Instead of
>> simply marking contact inactive in this case (which causes issues if
>> contact was originally proper and we lost confidence in it later, as
>> this results in accidental clicks, drags, etc), let's report such
>> contacts as MT_TOOL_PALM and let userspace decide what to do.
>> Additionally, let's report contact size for such touches as maximum
>> allowed for major/minor, which should help userspace that is not yet
>> aware of MT_TOOL_PALM to still perform palm rejection.
>>
>> An additional complication, is that some firmwares do not report
>> non-confident touches as active. To cope with this we delay release of
>> such contact (i.e. if contact was active we first report it as still
>> active MT+TOOL_PALM and then synthesize the release event in a separate
>> frame).
>
> Changing the tool identity to signal the tool property of low confidence
> does not seem quite right to me. Using MT_TOOL_PALM forces a semantic
> distinction between tool identity and touch state, which userland seems
> unprepared for.

The meaning of confidence is literally "contact is too large to be a
finger", so it is not touch state, but really tool identity. We do
allow tool identity to change over time.

The userland either ignores tool type (and then can reject based on
contact size) or, with Peter's changes, can pay attention to
MT_TOOL_PALM. It should work reasonably well for both old and new
userspace.

> The additional kernel state needed to make it work raises
> the question if more considerations will turn up over with time.

The additional state is simply because we have never updated the tool
type on release events and userspace is not expecting it and is likely
to ignore any data in the slot that is accompanied with
ABS_TRACKING_ID == -1. So we synthesize an extra event to have
distinct tool change and release.

There might arise other considerations over time,

>
> Why not add a property event, like BTN_TOOL_PALM, instead? In other words,
> modifying the definition of "active" as you propose, but then use a
> BTN_TOOL_PALM property to signal "s->confidence_state"? It perhaps creates a
> different oddity for applications unaware of palm, but AFAICT, it would not
> complicate the notion of touch state. Or?

Mostly because with BTN_TOOL_PALM we are not able to decide which
contact is turning into palm. Also, other drivers (RMI) use
MT_TOOL_PALM and I would like to report palm state in unified way.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Henrik Rydberg]

Hi Dmitry,

> The meaning of confidence is literally "contact is too large to be a
> finger", so it is not touch state, but really tool identity. We do
> allow tool identity to change over time.
What I am arguing is rather that since "palm" is a property, just like 
contact size, there should be no need to confuse that property with the 
touch state, which is, as you state, what happens in userland when the 
tool type is modified. Using a different event for the palm property 
ought to remove that confusion.

> The additional state is simply because we have never updated the tool
> type on release events and userspace is not expecting it and is likely
> to ignore any data in the slot that is accompanied with
> ABS_TRACKING_ID == -1. So we synthesize an extra event to have
> distinct tool change and release.

We update all other properties of a contact freely at release, so logically there is no good reason to treat palm, the binary version of max contact size, differently.
  

> Mostly because with BTN_TOOL_PALM we are not able to decide which
> contact is turning into palm. Also, other drivers (RMI) use
> MT_TOOL_PALM and I would like to report palm state in unified way.
Precedent certainly matters, but in this case, I think the modification 
promises problems down the road. I would rather suggest to add a new 
binary palm property, with the precise meaning "contact size = max 
contact size", and take it from there. I dont mind writing a patch for 
it if you agree.

Henrik

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Peter Hutterer]

sorry, was at a conference/travelling and I'm slowly catching up.

On Fri, Aug 11, 2017 at 10:29:07AM +0200, Henrik Rydberg wrote:
> Hi Dmitry,
> 
> > The meaning of confidence is literally "contact is too large to be a
> > finger", so it is not touch state, but really tool identity. We do
> > allow tool identity to change over time.
> What I am arguing is rather that since "palm" is a property, just like
> contact size, there should be no need to confuse that property with the
> touch state, which is, as you state, what happens in userland when the tool
> type is modified. Using a different event for the palm property ought to
> remove that confusion.

whether it's a property or a tool type is up for interpretation, imo neither
is right or wrong. It's common enough that the tool type changes after touch
down but it's equally common for the tool type to be set at start.

> > The additional state is simply because we have never updated the tool
> > type on release events and userspace is not expecting it and is likely
> > to ignore any data in the slot that is accompanied with
> > ABS_TRACKING_ID == -1. So we synthesize an extra event to have
> > distinct tool change and release.
> 
> We update all other properties of a contact freely at release, so
> logically there is no good reason to treat palm, the binary version of max
> contact size, differently.

note that palm does not indicate max contact size, it's merely a magic
property at which point the contact is not considered a normal finger
anymore. It may be size related, but it's certainly doesn't imply that it's
the maximum touch size and I expect this to be even less true of devices in
the future.
 
> > Mostly because with BTN_TOOL_PALM we are not able to decide which
> > contact is turning into palm. Also, other drivers (RMI) use
> > MT_TOOL_PALM and I would like to report palm state in unified way.
> Precedent certainly matters, but in this case, I think the modification
> promises problems down the road. I would rather suggest to add a new binary
> palm property, with the precise meaning "contact size = max contact size",
> and take it from there. I dont mind writing a patch for it if you agree.

the closest interpretation would be "contact size/shape/location indicates
that it is not a finger". I don't think you can easily narrow this down
further, mostly because it's often driven by the firmware and who knows what
that does.

I can live with an extra property as well as long as it's per-touch, but I
don't have any problems with MT_TOOL_PALM.

Cheers,
   Peter

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Peter Hutterer]

Hi Dmitry,

On Thu, Aug 10, 2017 at 05:44:59PM -0700, Dmitry Torokhov wrote:
> According to Microsoft specification [1] for Precision Touchpads (and
> Touchscreens) the devices use "confidence" reports to signal accidental
> touches, or contacts that are "too large to be a finger". Instead of
> simply marking contact inactive in this case (which causes issues if
> contact was originally proper and we lost confidence in it later, as
> this results in accidental clicks, drags, etc), let's report such
> contacts as MT_TOOL_PALM and let userspace decide what to do.
> Additionally, let's report contact size for such touches as maximum
> allowed for major/minor, which should help userspace that is not yet
> aware of MT_TOOL_PALM to still perform palm rejection.
> 
> An additional complication, is that some firmwares do not report
> non-confident touches as active. To cope with this we delay release of
> such contact (i.e. if contact was active we first report it as still
> active MT+TOOL_PALM and then synthesize the release event in a separate
> frame).
> 
> [1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

This one popped up again in a bug report [1] and it looks like it never
got merged. fwiw, libinput does support ABS_MT_TOOL_PALM for touchpads as of
1.8.0 and just releasing the touch causes fake taps. So you have the green
light from me to merge this :)

Cheers,
   Peter

[1] https://bugs.freedesktop.org/show_bug.cgi?id=106716

> ---
>  drivers/hid/hid-multitouch.c | 86 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 77 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 440b999304a5..c28defe50a10 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -114,6 +114,8 @@ struct mt_device {
>  	struct timer_list release_timer;	/* to release sticky fingers */
>  	struct mt_fields *fields;	/* temporary placeholder for storing the
>  					   multitouch fields */
> +	unsigned long *pending_palm_slots; /* slots where we reported palm
> +						and need to release */
>  	unsigned long mt_io_flags;	/* mt flags (MT_IO_FLAGS_*) */
>  	int cc_index;	/* contact count field index in the report */
>  	int cc_value_index;	/* contact count value index in the field */
> @@ -543,8 +545,12 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  		case HID_DG_CONFIDENCE:
>  			if ((cls->name == MT_CLS_WIN_8 ||
>  				cls->name == MT_CLS_WIN_8_DUAL) &&
> -				field->application == HID_DG_TOUCHPAD)
> +			    field->application == HID_DG_TOUCHPAD) {
>  				cls->quirks |= MT_QUIRK_CONFIDENCE;
> +				input_set_abs_params(hi->input,
> +					ABS_MT_TOOL_TYPE,
> +					MT_TOOL_FINGER, MT_TOOL_PALM, 0, 0);
> +			}
>  			mt_store_field(usage, td, hi);
>  			return 1;
>  		case HID_DG_TIPSWITCH:
> @@ -657,6 +663,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  
>  	if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
>  		int active;
> +		int tool;
>  		int slotnum = mt_compute_slot(td, input);
>  		struct mt_slot *s = &td->curdata;
>  		struct input_mt *mt = input->mt;
> @@ -671,24 +678,56 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  				return;
>  		}
>  
> +		active = s->touch_state || s->inrange_state;
> +
>  		if (!(td->mtclass.quirks & MT_QUIRK_CONFIDENCE))
>  			s->confidence_state = 1;
> -		active = (s->touch_state || s->inrange_state) &&
> -							s->confidence_state;
> +
> +		if (likely(s->confidence_state)) {
> +			tool = MT_TOOL_FINGER;
> +		} else {
> +			tool = MT_TOOL_PALM;
> +			if (!active &&
> +			    input_mt_is_active(&mt->slots[slotnum])) {
> +				/*
> +				 * The non-confidence was reported for
> +				 * previously valid contact that is also no
> +				 * longer valid. We can't simply report
> +				 * lift-off as userspace will not be aware
> +				 * of non-confidence, so we need to split
> +				 * it into 2 events: active MT_TOOL_PALM
> +				 * and a separate liftoff.
> +				 */
> +				active = true;
> +				set_bit(slotnum, td->pending_palm_slots);
> +			}
> +		}
>  
>  		input_mt_slot(input, slotnum);
> -		input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
> +		input_mt_report_slot_state(input, tool, active);
>  		if (active) {
>  			/* this finger is in proximity of the sensor */
>  			int wide = (s->w > s->h);
>  			int major = max(s->w, s->h);
>  			int minor = min(s->w, s->h);
>  
> -			/*
> -			 * divided by two to match visual scale of touch
> -			 * for devices with this quirk
> -			 */
> -			if (td->mtclass.quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
> +			if (unlikely(!s->confidence_state)) {
> +				/*
> +				 * When reporting palm, set contact to maximum
> +				 * size to help userspace that does not
> +				 * recognize MT_TOOL_PALM to reject contacts
> +				 * that are too large.
> +				 */
> +				major = input_abs_get_max(input,
> +							  ABS_MT_TOUCH_MAJOR);
> +				minor = input_abs_get_max(input,
> +							  ABS_MT_TOUCH_MINOR);
> +			} else if (td->mtclass.quirks &
> +					MT_QUIRK_TOUCH_SIZE_SCALING) {
> +				/*
> +				 * divided by two to match visual scale of touch
> +				 * for devices with this quirk
> +				 */
>  				major = major >> 1;
>  				minor = minor >> 1;
>  			}
> @@ -711,6 +750,25 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>  	td->num_received++;
>  }
>  
> +static void mt_release_pending_palms(struct mt_device *td,
> +				     struct input_dev *input)
> +{
> +	int slotnum;
> +	bool need_sync = false;
> +
> +	for_each_set_bit(slotnum, td->pending_palm_slots, td->maxcontacts) {
> +		clear_bit(slotnum, td->pending_palm_slots);
> +
> +		input_mt_slot(input, slotnum);
> +		input_mt_report_slot_state(input, MT_TOOL_PALM, false);
> +
> +		need_sync = true;
> +	}
> +
> +	if (need_sync)
> +		input_sync(input);
> +}
> +
>  /*
>   * this function is called when a whole packet has been received and processed,
>   * so that it can decide what to send to the input layer.
> @@ -719,6 +777,9 @@ static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
>  {
>  	input_mt_sync_frame(input);
>  	input_sync(input);
> +
> +	mt_release_pending_palms(td, input);
> +
>  	td->num_received = 0;
>  	if (test_bit(MT_IO_FLAGS_ACTIVE_SLOTS, &td->mt_io_flags))
>  		set_bit(MT_IO_FLAGS_PENDING_SLOTS, &td->mt_io_flags);
> @@ -903,6 +964,13 @@ static int mt_touch_input_configured(struct hid_device *hdev,
>  	if (td->is_buttonpad)
>  		__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
>  
> +	td->pending_palm_slots = devm_kcalloc(&hi->input->dev,
> +					      BITS_TO_LONGS(td->maxcontacts),
> +					      sizeof(long),
> +					      GFP_KERNEL);
> +	if (!td->pending_palm_slots)
> +		return -ENOMEM;
> +
>  	ret = input_mt_init_slots(input, td->maxcontacts, td->mt_flags);
>  	if (ret)
>  		return ret;
> -- 
> 2.14.0.434.g98096fd7a8-goog
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Benjamin Tissoires]

On Thu, May 31, 2018 at 1:12 AM, Peter Hutterer
<peter.hutterer@who-t.net> wrote:
> Hi Dmitry,
>
> On Thu, Aug 10, 2017 at 05:44:59PM -0700, Dmitry Torokhov wrote:
>> According to Microsoft specification [1] for Precision Touchpads (and
>> Touchscreens) the devices use "confidence" reports to signal accidental
>> touches, or contacts that are "too large to be a finger". Instead of
>> simply marking contact inactive in this case (which causes issues if
>> contact was originally proper and we lost confidence in it later, as
>> this results in accidental clicks, drags, etc), let's report such
>> contacts as MT_TOOL_PALM and let userspace decide what to do.
>> Additionally, let's report contact size for such touches as maximum
>> allowed for major/minor, which should help userspace that is not yet
>> aware of MT_TOOL_PALM to still perform palm rejection.
>>
>> An additional complication, is that some firmwares do not report
>> non-confident touches as active. To cope with this we delay release of
>> such contact (i.e. if contact was active we first report it as still
>> active MT+TOOL_PALM and then synthesize the release event in a separate
>> frame).
>>
>> [1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection
>>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> This one popped up again in a bug report [1] and it looks like it never
> got merged. fwiw, libinput does support ABS_MT_TOOL_PALM for touchpads as of
> 1.8.0 and just releasing the touch causes fake taps. So you have the green
> light from me to merge this :)
>
> Cheers,
>    Peter
>
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=106716

I can probably integrate this one in the hid-multitouch revamp I am
making, to avoid other the pain of rebasing.

Cheers,
Benjamin

>
>> ---
>>  drivers/hid/hid-multitouch.c | 86 +++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 77 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 440b999304a5..c28defe50a10 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -114,6 +114,8 @@ struct mt_device {
>>       struct timer_list release_timer;        /* to release sticky fingers */
>>       struct mt_fields *fields;       /* temporary placeholder for storing the
>>                                          multitouch fields */
>> +     unsigned long *pending_palm_slots; /* slots where we reported palm
>> +                                             and need to release */
>>       unsigned long mt_io_flags;      /* mt flags (MT_IO_FLAGS_*) */
>>       int cc_index;   /* contact count field index in the report */
>>       int cc_value_index;     /* contact count value index in the field */
>> @@ -543,8 +545,12 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>>               case HID_DG_CONFIDENCE:
>>                       if ((cls->name == MT_CLS_WIN_8 ||
>>                               cls->name == MT_CLS_WIN_8_DUAL) &&
>> -                             field->application == HID_DG_TOUCHPAD)
>> +                         field->application == HID_DG_TOUCHPAD) {
>>                               cls->quirks |= MT_QUIRK_CONFIDENCE;
>> +                             input_set_abs_params(hi->input,
>> +                                     ABS_MT_TOOL_TYPE,
>> +                                     MT_TOOL_FINGER, MT_TOOL_PALM, 0, 0);
>> +                     }
>>                       mt_store_field(usage, td, hi);
>>                       return 1;
>>               case HID_DG_TIPSWITCH:
>> @@ -657,6 +663,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>
>>       if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
>>               int active;
>> +             int tool;
>>               int slotnum = mt_compute_slot(td, input);
>>               struct mt_slot *s = &td->curdata;
>>               struct input_mt *mt = input->mt;
>> @@ -671,24 +678,56 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>                               return;
>>               }
>>
>> +             active = s->touch_state || s->inrange_state;
>> +
>>               if (!(td->mtclass.quirks & MT_QUIRK_CONFIDENCE))
>>                       s->confidence_state = 1;
>> -             active = (s->touch_state || s->inrange_state) &&
>> -                                                     s->confidence_state;
>> +
>> +             if (likely(s->confidence_state)) {
>> +                     tool = MT_TOOL_FINGER;
>> +             } else {
>> +                     tool = MT_TOOL_PALM;
>> +                     if (!active &&
>> +                         input_mt_is_active(&mt->slots[slotnum])) {
>> +                             /*
>> +                              * The non-confidence was reported for
>> +                              * previously valid contact that is also no
>> +                              * longer valid. We can't simply report
>> +                              * lift-off as userspace will not be aware
>> +                              * of non-confidence, so we need to split
>> +                              * it into 2 events: active MT_TOOL_PALM
>> +                              * and a separate liftoff.
>> +                              */
>> +                             active = true;
>> +                             set_bit(slotnum, td->pending_palm_slots);
>> +                     }
>> +             }
>>
>>               input_mt_slot(input, slotnum);
>> -             input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
>> +             input_mt_report_slot_state(input, tool, active);
>>               if (active) {
>>                       /* this finger is in proximity of the sensor */
>>                       int wide = (s->w > s->h);
>>                       int major = max(s->w, s->h);
>>                       int minor = min(s->w, s->h);
>>
>> -                     /*
>> -                      * divided by two to match visual scale of touch
>> -                      * for devices with this quirk
>> -                      */
>> -                     if (td->mtclass.quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
>> +                     if (unlikely(!s->confidence_state)) {
>> +                             /*
>> +                              * When reporting palm, set contact to maximum
>> +                              * size to help userspace that does not
>> +                              * recognize MT_TOOL_PALM to reject contacts
>> +                              * that are too large.
>> +                              */
>> +                             major = input_abs_get_max(input,
>> +                                                       ABS_MT_TOUCH_MAJOR);
>> +                             minor = input_abs_get_max(input,
>> +                                                       ABS_MT_TOUCH_MINOR);
>> +                     } else if (td->mtclass.quirks &
>> +                                     MT_QUIRK_TOUCH_SIZE_SCALING) {
>> +                             /*
>> +                              * divided by two to match visual scale of touch
>> +                              * for devices with this quirk
>> +                              */
>>                               major = major >> 1;
>>                               minor = minor >> 1;
>>                       }
>> @@ -711,6 +750,25 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>>       td->num_received++;
>>  }
>>
>> +static void mt_release_pending_palms(struct mt_device *td,
>> +                                  struct input_dev *input)
>> +{
>> +     int slotnum;
>> +     bool need_sync = false;
>> +
>> +     for_each_set_bit(slotnum, td->pending_palm_slots, td->maxcontacts) {
>> +             clear_bit(slotnum, td->pending_palm_slots);
>> +
>> +             input_mt_slot(input, slotnum);
>> +             input_mt_report_slot_state(input, MT_TOOL_PALM, false);
>> +
>> +             need_sync = true;
>> +     }
>> +
>> +     if (need_sync)
>> +             input_sync(input);
>> +}
>> +
>>  /*
>>   * this function is called when a whole packet has been received and processed,
>>   * so that it can decide what to send to the input layer.
>> @@ -719,6 +777,9 @@ static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
>>  {
>>       input_mt_sync_frame(input);
>>       input_sync(input);
>> +
>> +     mt_release_pending_palms(td, input);
>> +
>>       td->num_received = 0;
>>       if (test_bit(MT_IO_FLAGS_ACTIVE_SLOTS, &td->mt_io_flags))
>>               set_bit(MT_IO_FLAGS_PENDING_SLOTS, &td->mt_io_flags);
>> @@ -903,6 +964,13 @@ static int mt_touch_input_configured(struct hid_device *hdev,
>>       if (td->is_buttonpad)
>>               __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
>>
>> +     td->pending_palm_slots = devm_kcalloc(&hi->input->dev,
>> +                                           BITS_TO_LONGS(td->maxcontacts),
>> +                                           sizeof(long),
>> +                                           GFP_KERNEL);
>> +     if (!td->pending_palm_slots)
>> +             return -ENOMEM;
>> +
>>       ret = input_mt_init_slots(input, td->maxcontacts, td->mt_flags);
>>       if (ret)
>>               return ret;
>> --
>> 2.14.0.434.g98096fd7a8-goog
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Benjamin Tissoires]

On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> According to Microsoft specification [1] for Precision Touchpads (and
> Touchscreens) the devices use "confidence" reports to signal accidental
> touches, or contacts that are "too large to be a finger". Instead of
> simply marking contact inactive in this case (which causes issues if
> contact was originally proper and we lost confidence in it later, as
> this results in accidental clicks, drags, etc), let's report such
> contacts as MT_TOOL_PALM and let userspace decide what to do.
> Additionally, let's report contact size for such touches as maximum
> allowed for major/minor, which should help userspace that is not yet
> aware of MT_TOOL_PALM to still perform palm rejection.
>
> An additional complication, is that some firmwares do not report
> non-confident touches as active. To cope with this we delay release of
> such contact (i.e. if contact was active we first report it as still
> active MT+TOOL_PALM and then synthesize the release event in a separate
> frame).

I am not sure I agree with this part. The spec says that "Once a
device has determined that a contact is unintentional, it should clear
the confidence bit for that contact report and all subsequent
reports."
So in theory the spec says that if a touch has been detected as a
palm, the flow of events should not stop (tested on the PTP of the
Dell XPS 9360).

However, I interpret a firmware that send (confidence 1, tip switch 1)
and then (confidence 0, tip switch 0) a simple release, and the
confidence bit should not be relayed.

Do you have any precise example of reports where you need that feature?

Cheers,
Benjamin

>
> [1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/hid/hid-multitouch.c | 86 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 77 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 440b999304a5..c28defe50a10 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -114,6 +114,8 @@ struct mt_device {
>         struct timer_list release_timer;        /* to release sticky fingers */
>         struct mt_fields *fields;       /* temporary placeholder for storing the
>                                            multitouch fields */
> +       unsigned long *pending_palm_slots; /* slots where we reported palm
> +                                               and need to release */
>         unsigned long mt_io_flags;      /* mt flags (MT_IO_FLAGS_*) */
>         int cc_index;   /* contact count field index in the report */
>         int cc_value_index;     /* contact count value index in the field */
> @@ -543,8 +545,12 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>                 case HID_DG_CONFIDENCE:
>                         if ((cls->name == MT_CLS_WIN_8 ||
>                                 cls->name == MT_CLS_WIN_8_DUAL) &&
> -                               field->application == HID_DG_TOUCHPAD)
> +                           field->application == HID_DG_TOUCHPAD) {
>                                 cls->quirks |= MT_QUIRK_CONFIDENCE;
> +                               input_set_abs_params(hi->input,
> +                                       ABS_MT_TOOL_TYPE,
> +                                       MT_TOOL_FINGER, MT_TOOL_PALM, 0, 0);
> +                       }
>                         mt_store_field(usage, td, hi);
>                         return 1;
>                 case HID_DG_TIPSWITCH:
> @@ -657,6 +663,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>
>         if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
>                 int active;
> +               int tool;
>                 int slotnum = mt_compute_slot(td, input);
>                 struct mt_slot *s = &td->curdata;
>                 struct input_mt *mt = input->mt;
> @@ -671,24 +678,56 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>                                 return;
>                 }
>
> +               active = s->touch_state || s->inrange_state;
> +
>                 if (!(td->mtclass.quirks & MT_QUIRK_CONFIDENCE))
>                         s->confidence_state = 1;
> -               active = (s->touch_state || s->inrange_state) &&
> -                                                       s->confidence_state;
> +
> +               if (likely(s->confidence_state)) {
> +                       tool = MT_TOOL_FINGER;
> +               } else {
> +                       tool = MT_TOOL_PALM;
> +                       if (!active &&
> +                           input_mt_is_active(&mt->slots[slotnum])) {
> +                               /*
> +                                * The non-confidence was reported for
> +                                * previously valid contact that is also no
> +                                * longer valid. We can't simply report
> +                                * lift-off as userspace will not be aware
> +                                * of non-confidence, so we need to split
> +                                * it into 2 events: active MT_TOOL_PALM
> +                                * and a separate liftoff.
> +                                */
> +                               active = true;
> +                               set_bit(slotnum, td->pending_palm_slots);
> +                       }
> +               }
>
>                 input_mt_slot(input, slotnum);
> -               input_mt_report_slot_state(input, MT_TOOL_FINGER, active);
> +               input_mt_report_slot_state(input, tool, active);
>                 if (active) {
>                         /* this finger is in proximity of the sensor */
>                         int wide = (s->w > s->h);
>                         int major = max(s->w, s->h);
>                         int minor = min(s->w, s->h);
>
> -                       /*
> -                        * divided by two to match visual scale of touch
> -                        * for devices with this quirk
> -                        */
> -                       if (td->mtclass.quirks & MT_QUIRK_TOUCH_SIZE_SCALING) {
> +                       if (unlikely(!s->confidence_state)) {
> +                               /*
> +                                * When reporting palm, set contact to maximum
> +                                * size to help userspace that does not
> +                                * recognize MT_TOOL_PALM to reject contacts
> +                                * that are too large.
> +                                */
> +                               major = input_abs_get_max(input,
> +                                                         ABS_MT_TOUCH_MAJOR);
> +                               minor = input_abs_get_max(input,
> +                                                         ABS_MT_TOUCH_MINOR);
> +                       } else if (td->mtclass.quirks &
> +                                       MT_QUIRK_TOUCH_SIZE_SCALING) {
> +                               /*
> +                                * divided by two to match visual scale of touch
> +                                * for devices with this quirk
> +                                */
>                                 major = major >> 1;
>                                 minor = minor >> 1;
>                         }
> @@ -711,6 +750,25 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
>         td->num_received++;
>  }
>
> +static void mt_release_pending_palms(struct mt_device *td,
> +                                    struct input_dev *input)
> +{
> +       int slotnum;
> +       bool need_sync = false;
> +
> +       for_each_set_bit(slotnum, td->pending_palm_slots, td->maxcontacts) {
> +               clear_bit(slotnum, td->pending_palm_slots);
> +
> +               input_mt_slot(input, slotnum);
> +               input_mt_report_slot_state(input, MT_TOOL_PALM, false);
> +
> +               need_sync = true;
> +       }
> +
> +       if (need_sync)
> +               input_sync(input);
> +}
> +
>  /*
>   * this function is called when a whole packet has been received and processed,
>   * so that it can decide what to send to the input layer.
> @@ -719,6 +777,9 @@ static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
>  {
>         input_mt_sync_frame(input);
>         input_sync(input);
> +
> +       mt_release_pending_palms(td, input);
> +
>         td->num_received = 0;
>         if (test_bit(MT_IO_FLAGS_ACTIVE_SLOTS, &td->mt_io_flags))
>                 set_bit(MT_IO_FLAGS_PENDING_SLOTS, &td->mt_io_flags);
> @@ -903,6 +964,13 @@ static int mt_touch_input_configured(struct hid_device *hdev,
>         if (td->is_buttonpad)
>                 __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
>
> +       td->pending_palm_slots = devm_kcalloc(&hi->input->dev,
> +                                             BITS_TO_LONGS(td->maxcontacts),
> +                                             sizeof(long),
> +                                             GFP_KERNEL);
> +       if (!td->pending_palm_slots)
> +               return -ENOMEM;
> +
>         ret = input_mt_init_slots(input, td->maxcontacts, td->mt_flags);
>         if (ret)
>                 return ret;
> --
> 2.14.0.434.g98096fd7a8-goog
>

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > According to Microsoft specification [1] for Precision Touchpads (and
> > Touchscreens) the devices use "confidence" reports to signal accidental
> > touches, or contacts that are "too large to be a finger". Instead of
> > simply marking contact inactive in this case (which causes issues if
> > contact was originally proper and we lost confidence in it later, as
> > this results in accidental clicks, drags, etc), let's report such
> > contacts as MT_TOOL_PALM and let userspace decide what to do.
> > Additionally, let's report contact size for such touches as maximum
> > allowed for major/minor, which should help userspace that is not yet
> > aware of MT_TOOL_PALM to still perform palm rejection.
> >
> > An additional complication, is that some firmwares do not report
> > non-confident touches as active. To cope with this we delay release of
> > such contact (i.e. if contact was active we first report it as still
> > active MT+TOOL_PALM and then synthesize the release event in a separate
> > frame).
> 
> I am not sure I agree with this part. The spec says that "Once a
> device has determined that a contact is unintentional, it should clear
> the confidence bit for that contact report and all subsequent
> reports."
> So in theory the spec says that if a touch has been detected as a
> palm, the flow of events should not stop (tested on the PTP of the
> Dell XPS 9360).
> 
> However, I interpret a firmware that send (confidence 1, tip switch 1)
> and then (confidence 0, tip switch 0) a simple release, and the
> confidence bit should not be relayed.

This unfortunately leads to false clicks: you start with finger, so
confidence is 1, then you transition the same touch to palm (use your
thumb and "roll" your hand until heel of it comes into contact with the
screen). The firmware reports "no-confidence" and "release" in the same
report and userspace seeing release does not pay attention to confidence
(i.e. it does exactly "simple release" logic) and this results in UI
interpreting this as a click. With splitting no-confidence
(MT_TOOL_PALM) and release event into separate frames we help userspace
to recognize that the contact should be discarded.

> 
> Do you have any precise example of reports where you need that feature?

It was observed on Pixelbooks which use Wacom digitizers IIRC.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Henrik Rydberg]

>> However, I interpret a firmware that send (confidence 1, tip switch 1)
>> and then (confidence 0, tip switch 0) a simple release, and the
>> confidence bit should not be relayed.
> This unfortunately leads to false clicks: you start with finger, so
> confidence is 1, then you transition the same touch to palm (use your
> thumb and "roll" your hand until heel of it comes into contact with the
> screen). The firmware reports "no-confidence" and "release" in the same
> report and userspace seeing release does not pay attention to confidence
> (i.e. it does exactly "simple release" logic) and this results in UI
> interpreting this as a click. With splitting no-confidence
> (MT_TOOL_PALM) and release event into separate frames we help userspace
> to recognize that the contact should be discarded.
This is in part why I objected to this patch on August 11th, 2017. 
Logically, the confidence state is a property of a contact, not a new 
type of contact. Trying to use it in any other way is bound to lead to 
confusion.

Henrik

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Benjamin Tissoires]

On Fri, Jun 1, 2018 at 9:03 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>
>>> However, I interpret a firmware that send (confidence 1, tip switch 1)
>>> and then (confidence 0, tip switch 0) a simple release, and the
>>> confidence bit should not be relayed.
>>
>> This unfortunately leads to false clicks: you start with finger, so
>> confidence is 1, then you transition the same touch to palm (use your
>> thumb and "roll" your hand until heel of it comes into contact with the
>> screen). The firmware reports "no-confidence" and "release" in the same
>> report and userspace seeing release does not pay attention to confidence
>> (i.e. it does exactly "simple release" logic) and this results in UI
>> interpreting this as a click. With splitting no-confidence
>> (MT_TOOL_PALM) and release event into separate frames we help userspace
>> to recognize that the contact should be discarded.
>
> This is in part why I objected to this patch on August 11th, 2017.
> Logically, the confidence state is a property of a contact, not a new type
> of contact. Trying to use it in any other way is bound to lead to confusion.

Problem is that MT_TOOL_PALM has been introduced in the kernel since
v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
I can't find any other users in the current upstream tree, but those
two are already making a precedent and changing the semantic is a
little bit late :/

Cheers,
Benjamin

>
> Henrik
>

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Benjamin Tissoires]

On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
>> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > According to Microsoft specification [1] for Precision Touchpads (and
>> > Touchscreens) the devices use "confidence" reports to signal accidental
>> > touches, or contacts that are "too large to be a finger". Instead of
>> > simply marking contact inactive in this case (which causes issues if
>> > contact was originally proper and we lost confidence in it later, as
>> > this results in accidental clicks, drags, etc), let's report such
>> > contacts as MT_TOOL_PALM and let userspace decide what to do.
>> > Additionally, let's report contact size for such touches as maximum
>> > allowed for major/minor, which should help userspace that is not yet
>> > aware of MT_TOOL_PALM to still perform palm rejection.
>> >
>> > An additional complication, is that some firmwares do not report
>> > non-confident touches as active. To cope with this we delay release of
>> > such contact (i.e. if contact was active we first report it as still
>> > active MT+TOOL_PALM and then synthesize the release event in a separate
>> > frame).
>>
>> I am not sure I agree with this part. The spec says that "Once a
>> device has determined that a contact is unintentional, it should clear
>> the confidence bit for that contact report and all subsequent
>> reports."
>> So in theory the spec says that if a touch has been detected as a
>> palm, the flow of events should not stop (tested on the PTP of the
>> Dell XPS 9360).
>>
>> However, I interpret a firmware that send (confidence 1, tip switch 1)
>> and then (confidence 0, tip switch 0) a simple release, and the
>> confidence bit should not be relayed.
>
> This unfortunately leads to false clicks: you start with finger, so
> confidence is 1, then you transition the same touch to palm (use your
> thumb and "roll" your hand until heel of it comes into contact with the
> screen). The firmware reports "no-confidence" and "release" in the same
> report and userspace seeing release does not pay attention to confidence
> (i.e. it does exactly "simple release" logic) and this results in UI
> interpreting this as a click. With splitting no-confidence
> (MT_TOOL_PALM) and release event into separate frames we help userspace
> to recognize that the contact should be discarded.

After further thoughts, I would consider this to be a firmware bug,
and not how the firmware is supposed to be reporting palm.
For the precision touchpads, the spec says that the device "should
clear the confidence bit for that contact report and all subsequent
reports.". And it is how the Dell device I have here reports palms.
The firmware is not supposed to cut the event stream.

There is a test for that:
https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
which tells me that I am right here for PTP.

The touchscreen spec is blurrier however.

>
>>
>> Do you have any precise example of reports where you need that feature?
>
> It was observed on Pixelbooks which use Wacom digitizers IIRC.

Pixelbooks + Wacom means that it was likely a touchscreen. I am right
guessing the device did not went through Microsoft certification
process?

I am in favor of splitting the patch in 2. One for the generic
processing of confidence bit, and one for this spurious release. For
the spurious release, I'm more in favor of explicitly quirking the
devices in need of such quirk.

If you agree, I'll rebase your patch on top of my series as rebasing
my series on top of yours will take more effort.

I am trying to be cautious in the generic path because I want to merge
the cleanest multitouch implementation in hid-core/hid-input, and
leave all the quirks in hid-multitouch for the devices in need.

Cheers,
Benjamin

>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

On Mon, Jun 04, 2018 at 02:57:54PM +0200, Benjamin Tissoires wrote:
> On Fri, Jun 1, 2018 at 9:03 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> >
> >>> However, I interpret a firmware that send (confidence 1, tip switch 1)
> >>> and then (confidence 0, tip switch 0) a simple release, and the
> >>> confidence bit should not be relayed.
> >>
> >> This unfortunately leads to false clicks: you start with finger, so
> >> confidence is 1, then you transition the same touch to palm (use your
> >> thumb and "roll" your hand until heel of it comes into contact with the
> >> screen). The firmware reports "no-confidence" and "release" in the same
> >> report and userspace seeing release does not pay attention to confidence
> >> (i.e. it does exactly "simple release" logic) and this results in UI
> >> interpreting this as a click. With splitting no-confidence
> >> (MT_TOOL_PALM) and release event into separate frames we help userspace
> >> to recognize that the contact should be discarded.
> >
> > This is in part why I objected to this patch on August 11th, 2017.
> > Logically, the confidence state is a property of a contact, not a new type
> > of contact. Trying to use it in any other way is bound to lead to confusion.
> 
> Problem is that MT_TOOL_PALM has been introduced in the kernel since
> v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> I can't find any other users in the current upstream tree, but those
> two are already making a precedent and changing the semantic is a
> little bit late :/

I am sorry I did not respond and lost track of this issue back then, but
I disagree with Henrik here. While confidence is a property of contact,
so is the type of contact and it can and will change throughout life of
a contact, especially if we will continue adding new types, such as, for
example, thumb. In this case the firmware can transition through
finger->thumb or finger->thumb->palm or finger->palm as the nature of
contact becomes better understood. Still it is the same contact and we
should not attempt to signal userspace differently.

We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
complement ABS_MT_TOOL, but that would not really solve the issue with
Wacom firmware (declaring contact non-confident and releasing it right
away) and given MS explanation of the confidence as "contact is too big"
MT_TOOL_PALM fits it perfectly.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >> > According to Microsoft specification [1] for Precision Touchpads (and
> >> > Touchscreens) the devices use "confidence" reports to signal accidental
> >> > touches, or contacts that are "too large to be a finger". Instead of
> >> > simply marking contact inactive in this case (which causes issues if
> >> > contact was originally proper and we lost confidence in it later, as
> >> > this results in accidental clicks, drags, etc), let's report such
> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
> >> > Additionally, let's report contact size for such touches as maximum
> >> > allowed for major/minor, which should help userspace that is not yet
> >> > aware of MT_TOOL_PALM to still perform palm rejection.
> >> >
> >> > An additional complication, is that some firmwares do not report
> >> > non-confident touches as active. To cope with this we delay release of
> >> > such contact (i.e. if contact was active we first report it as still
> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
> >> > frame).
> >>
> >> I am not sure I agree with this part. The spec says that "Once a
> >> device has determined that a contact is unintentional, it should clear
> >> the confidence bit for that contact report and all subsequent
> >> reports."
> >> So in theory the spec says that if a touch has been detected as a
> >> palm, the flow of events should not stop (tested on the PTP of the
> >> Dell XPS 9360).
> >>
> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
> >> and then (confidence 0, tip switch 0) a simple release, and the
> >> confidence bit should not be relayed.
> >
> > This unfortunately leads to false clicks: you start with finger, so
> > confidence is 1, then you transition the same touch to palm (use your
> > thumb and "roll" your hand until heel of it comes into contact with the
> > screen). The firmware reports "no-confidence" and "release" in the same
> > report and userspace seeing release does not pay attention to confidence
> > (i.e. it does exactly "simple release" logic) and this results in UI
> > interpreting this as a click. With splitting no-confidence
> > (MT_TOOL_PALM) and release event into separate frames we help userspace
> > to recognize that the contact should be discarded.
> 
> After further thoughts, I would consider this to be a firmware bug,
> and not how the firmware is supposed to be reporting palm.
> For the precision touchpads, the spec says that the device "should
> clear the confidence bit for that contact report and all subsequent
> reports.". And it is how the Dell device I have here reports palms.
> The firmware is not supposed to cut the event stream.
> 
> There is a test for that:
> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
> which tells me that I am right here for PTP.
> 
> The touchscreen spec is blurrier however.

OK, that is great to know.

> 
> >
> >>
> >> Do you have any precise example of reports where you need that feature?
> >
> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
> 
> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
> guessing the device did not went through Microsoft certification
> process?

That would be correct ;) At least the firmware that is shipping with
Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
their MSWin devices.

> 
> I am in favor of splitting the patch in 2. One for the generic
> processing of confidence bit, and one for this spurious release. For
> the spurious release, I'm more in favor of explicitly quirking the
> devices in need of such quirk.

Hmm, I am not sure about having specific quirk. It will be hard for
users to accurately diagnose the issue if firmware is broken in this way
so we could add a new quirk for a new device.

> 
> If you agree, I'll rebase your patch on top of my series as rebasing
> my series on top of yours will take more effort.

That would be great.

> 
> I am trying to be cautious in the generic path because I want to merge
> the cleanest multitouch implementation in hid-core/hid-input, and
> leave all the quirks in hid-multitouch for the devices in need.
> 
> Cheers,
> Benjamin
> 
> >
> > Thanks.
> >
> > --
> > Dmitry

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Henrik Rydberg]

Hi Dmitry,

>>> Logically, the confidence state is a property of a contact, not a new type
>>> of contact. Trying to use it in any other way is bound to lead to confusion.
>>>
>>> Problem is that MT_TOOL_PALM has been introduced in the kernel since
>>> v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
>>> It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
>>> I can't find any other users in the current upstream tree, but those
>>> two are already making a precedent and changing the semantic is a
>>> little bit late :/
> I am sorry I did not respond and lost track of this issue back then, but
> I disagree with Henrik here. While confidence is a property of contact,
> so is the type of contact and it can and will change throughout life of
> a contact, especially if we will continue adding new types, such as, for
> example, thumb. In this case the firmware can transition through
> finger->thumb or finger->thumb->palm or finger->palm as the nature of
> contact becomes better understood. Still it is the same contact and we
> should not attempt to signal userspace differently.
We agree that the contact should stay the same, but the fear, and I 
think somewhere along the blurry history of this thread, the problem was 
that userspace interpreted the property change as a new contact (lift 
up/double click/etc). Finger/thumb/palm is one set of hand properties, 
but what about Pen? It would be hard for an application to consider a 
switch from finger to pen as the same contact, which is where the 
natural implementation starts to diverge from the intention.

> We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> complement ABS_MT_TOOL, but that would not really solve the issue with
> Wacom firmware (declaring contact non-confident and releasing it right
> away) and given MS explanation of the confidence as "contact is too big"
> MT_TOOL_PALM fits it perfectly.
Indeed, the Wacom firmware seems to need some special handling, which 
should be fine by everyone. I do think it would make sense to add 
ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would 
apply also to a pen lying down on a touchpad, for instance.

Henrik

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
> Hi Dmitry,
> 
> > > > Logically, the confidence state is a property of a contact, not a new type
> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
> > > > 
> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> > > > I can't find any other users in the current upstream tree, but those
> > > > two are already making a precedent and changing the semantic is a
> > > > little bit late :/
> > I am sorry I did not respond and lost track of this issue back then, but
> > I disagree with Henrik here. While confidence is a property of contact,
> > so is the type of contact and it can and will change throughout life of
> > a contact, especially if we will continue adding new types, such as, for
> > example, thumb. In this case the firmware can transition through
> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
> > contact becomes better understood. Still it is the same contact and we
> > should not attempt to signal userspace differently.
> We agree that the contact should stay the same, but the fear, and I think
> somewhere along the blurry history of this thread, the problem was that
> userspace interpreted the property change as a new contact (lift up/double
> click/etc). Finger/thumb/palm is one set of hand properties, but what about
> Pen? It would be hard for an application to consider a switch from finger to
> pen as the same contact, which is where the natural implementation starts to
> diverge from the intention.

I think the userspace has to trust our tracking ID to decide whether it
is a same contact or not. The current issue is that kernel is forcing
tracking ID change on tool type change, and one of the 2 patches that I
posted fixed that, allowing us to keep the tracking ID for finger->palm
transitions.

I think it is kernel task to not signal transitions that do not make
sense, such as finger->pen or palm->pen etc.

> 
> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> > complement ABS_MT_TOOL, but that would not really solve the issue with
> > Wacom firmware (declaring contact non-confident and releasing it right
> > away) and given MS explanation of the confidence as "contact is too big"
> > MT_TOOL_PALM fits it perfectly.
> Indeed, the Wacom firmware seems to need some special handling, which should
> be fine by everyone. I do think it would make sense to add
> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
> also to a pen lying down on a touchpad, for instance.

OK, I can see that for Pens, if we have firmware that would recognize
such condition, it would be weird to report PALM. We could indeed have
ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
the hardware can recognize it as such). Maybe we'd be better off just
having userspace going by contact size for pens. Peter, any suggestions
here?

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Benjamin Tissoires]

On Mon, Jun 4, 2018 at 7:33 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
>> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
>> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
>> >> <dmitry.torokhov@gmail.com> wrote:
>> >> > According to Microsoft specification [1] for Precision Touchpads (and
>> >> > Touchscreens) the devices use "confidence" reports to signal accidental
>> >> > touches, or contacts that are "too large to be a finger". Instead of
>> >> > simply marking contact inactive in this case (which causes issues if
>> >> > contact was originally proper and we lost confidence in it later, as
>> >> > this results in accidental clicks, drags, etc), let's report such
>> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
>> >> > Additionally, let's report contact size for such touches as maximum
>> >> > allowed for major/minor, which should help userspace that is not yet
>> >> > aware of MT_TOOL_PALM to still perform palm rejection.
>> >> >
>> >> > An additional complication, is that some firmwares do not report
>> >> > non-confident touches as active. To cope with this we delay release of
>> >> > such contact (i.e. if contact was active we first report it as still
>> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
>> >> > frame).
>> >>
>> >> I am not sure I agree with this part. The spec says that "Once a
>> >> device has determined that a contact is unintentional, it should clear
>> >> the confidence bit for that contact report and all subsequent
>> >> reports."
>> >> So in theory the spec says that if a touch has been detected as a
>> >> palm, the flow of events should not stop (tested on the PTP of the
>> >> Dell XPS 9360).
>> >>
>> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
>> >> and then (confidence 0, tip switch 0) a simple release, and the
>> >> confidence bit should not be relayed.
>> >
>> > This unfortunately leads to false clicks: you start with finger, so
>> > confidence is 1, then you transition the same touch to palm (use your
>> > thumb and "roll" your hand until heel of it comes into contact with the
>> > screen). The firmware reports "no-confidence" and "release" in the same
>> > report and userspace seeing release does not pay attention to confidence
>> > (i.e. it does exactly "simple release" logic) and this results in UI
>> > interpreting this as a click. With splitting no-confidence
>> > (MT_TOOL_PALM) and release event into separate frames we help userspace
>> > to recognize that the contact should be discarded.
>>
>> After further thoughts, I would consider this to be a firmware bug,
>> and not how the firmware is supposed to be reporting palm.
>> For the precision touchpads, the spec says that the device "should
>> clear the confidence bit for that contact report and all subsequent
>> reports.". And it is how the Dell device I have here reports palms.
>> The firmware is not supposed to cut the event stream.
>>
>> There is a test for that:
>> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
>> which tells me that I am right here for PTP.
>>
>> The touchscreen spec is blurrier however.
>
> OK, that is great to know.
>
>>
>> >
>> >>
>> >> Do you have any precise example of reports where you need that feature?
>> >
>> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
>>
>> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
>> guessing the device did not went through Microsoft certification
>> process?
>
> That would be correct ;) At least the firmware that is shipping with
> Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
> their MSWin devices.
>
>>
>> I am in favor of splitting the patch in 2. One for the generic
>> processing of confidence bit, and one for this spurious release. For
>> the spurious release, I'm more in favor of explicitly quirking the
>> devices in need of such quirk.
>
> Hmm, I am not sure about having specific quirk. It will be hard for
> users to accurately diagnose the issue if firmware is broken in this way
> so we could add a new quirk for a new device.

One thing we can do is keep the quirked mechanism as default in
hid-multitouch, but remove it in hid-core. If people need the quirk,
they can just use hid-multitouch instead (talking about the long run
here).

However, I really believe this might only be required for a handful of
devices, and probably only touchscreens. So I would be tempted to not
make it default and see how many bug reports we have.

>
>>
>> If you agree, I'll rebase your patch on top of my series as rebasing
>> my series on top of yours will take more effort.
>
> That would be great.

\o/

Cheers,
Benjamin

>
>>
>> I am trying to be cautious in the generic path because I want to merge
>> the cleanest multitouch implementation in hid-core/hid-input, and
>> leave all the quirks in hid-multitouch for the devices in need.
>>
>> Cheers,
>> Benjamin
>>
>> >
>> > Thanks.
>> >
>> > --
>> > Dmitry
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Benjamin Tissoires]

On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
>> Hi Dmitry,
>>
>> > > > Logically, the confidence state is a property of a contact, not a new type
>> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
>> > > >
>> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
>> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
>> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
>> > > > I can't find any other users in the current upstream tree, but those
>> > > > two are already making a precedent and changing the semantic is a
>> > > > little bit late :/
>> > I am sorry I did not respond and lost track of this issue back then, but
>> > I disagree with Henrik here. While confidence is a property of contact,
>> > so is the type of contact and it can and will change throughout life of
>> > a contact, especially if we will continue adding new types, such as, for
>> > example, thumb. In this case the firmware can transition through
>> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
>> > contact becomes better understood. Still it is the same contact and we
>> > should not attempt to signal userspace differently.
>> We agree that the contact should stay the same, but the fear, and I think
>> somewhere along the blurry history of this thread, the problem was that
>> userspace interpreted the property change as a new contact (lift up/double
>> click/etc). Finger/thumb/palm is one set of hand properties, but what about
>> Pen? It would be hard for an application to consider a switch from finger to
>> pen as the same contact, which is where the natural implementation starts to
>> diverge from the intention.
>
> I think the userspace has to trust our tracking ID to decide whether it
> is a same contact or not. The current issue is that kernel is forcing
> tracking ID change on tool type change, and one of the 2 patches that I
> posted fixed that, allowing us to keep the tracking ID for finger->palm
> transitions.

I think I missed those 2 patches, can you point a LKML link?
Also, note that libevdev discards the tracking ID change now (it
shouts at the user in the logs). So that means that it will now be
hard to force libevdev to trust the kernel again for the tracking ID.
The current rule is:
- tracking ID >= 0 -> new touch
- any subsequent tracking ID >= 0 -> discarded
- tracking ID == -1 -> end of touch

>
> I think it is kernel task to not signal transitions that do not make
> sense, such as finger->pen or palm->pen etc.

I fully agree, though there is currently no such guard in the kernel
(maybe it's part of your series). I am worried about the RMI4 F12
driver that automatically forward the info from the firmware, so if
the firmware does something crazy, it will be exported to user space.
But I guess it might be better to treat that on a per driver basis.

>
>>
>> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
>> > complement ABS_MT_TOOL, but that would not really solve the issue with
>> > Wacom firmware (declaring contact non-confident and releasing it right
>> > away) and given MS explanation of the confidence as "contact is too big"
>> > MT_TOOL_PALM fits it perfectly.
>> Indeed, the Wacom firmware seems to need some special handling, which should
>> be fine by everyone. I do think it would make sense to add
>> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply

Except we are already running out of ABS_* axes.

>> also to a pen lying down on a touchpad, for instance.
>
> OK, I can see that for Pens, if we have firmware that would recognize
> such condition, it would be weird to report PALM. We could indeed have
> ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
> the hardware can recognize it as such). Maybe we'd be better off just
> having userspace going by contact size for pens. Peter, any suggestions
> here?

I don't think we have size handling in the tablet implementation in
libinput. I do not see it as a big issue to add such axes from a
libinput point of view. However, there is no existing hardware that
would provide such information, so I guess this will be a 'no' until
actual hardware comes in.

Also note that the MT_TOOL_PEN implementation is limited (even
non-existant if I remember correctly). Peter and I do not have access
to any device that support such multi pen, so AFAICT, there is no code
to handle this in libinput.

One last point from libinput, the pen device would need to be on its
separate kernel node for the protocol to be smoothly handled. So
basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
would not be handled properly right now. The Pen event will be treated
as a touch.

Cheers,
Benjamin

>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

On Mon, Jun 04, 2018 at 10:42:31PM +0200, Benjamin Tissoires wrote:
> On Mon, Jun 4, 2018 at 7:33 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
> >> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
> >> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
> >> >> <dmitry.torokhov@gmail.com> wrote:
> >> >> > According to Microsoft specification [1] for Precision Touchpads (and
> >> >> > Touchscreens) the devices use "confidence" reports to signal accidental
> >> >> > touches, or contacts that are "too large to be a finger". Instead of
> >> >> > simply marking contact inactive in this case (which causes issues if
> >> >> > contact was originally proper and we lost confidence in it later, as
> >> >> > this results in accidental clicks, drags, etc), let's report such
> >> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
> >> >> > Additionally, let's report contact size for such touches as maximum
> >> >> > allowed for major/minor, which should help userspace that is not yet
> >> >> > aware of MT_TOOL_PALM to still perform palm rejection.
> >> >> >
> >> >> > An additional complication, is that some firmwares do not report
> >> >> > non-confident touches as active. To cope with this we delay release of
> >> >> > such contact (i.e. if contact was active we first report it as still
> >> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
> >> >> > frame).
> >> >>
> >> >> I am not sure I agree with this part. The spec says that "Once a
> >> >> device has determined that a contact is unintentional, it should clear
> >> >> the confidence bit for that contact report and all subsequent
> >> >> reports."
> >> >> So in theory the spec says that if a touch has been detected as a
> >> >> palm, the flow of events should not stop (tested on the PTP of the
> >> >> Dell XPS 9360).
> >> >>
> >> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
> >> >> and then (confidence 0, tip switch 0) a simple release, and the
> >> >> confidence bit should not be relayed.
> >> >
> >> > This unfortunately leads to false clicks: you start with finger, so
> >> > confidence is 1, then you transition the same touch to palm (use your
> >> > thumb and "roll" your hand until heel of it comes into contact with the
> >> > screen). The firmware reports "no-confidence" and "release" in the same
> >> > report and userspace seeing release does not pay attention to confidence
> >> > (i.e. it does exactly "simple release" logic) and this results in UI
> >> > interpreting this as a click. With splitting no-confidence
> >> > (MT_TOOL_PALM) and release event into separate frames we help userspace
> >> > to recognize that the contact should be discarded.
> >>
> >> After further thoughts, I would consider this to be a firmware bug,
> >> and not how the firmware is supposed to be reporting palm.
> >> For the precision touchpads, the spec says that the device "should
> >> clear the confidence bit for that contact report and all subsequent
> >> reports.". And it is how the Dell device I have here reports palms.
> >> The firmware is not supposed to cut the event stream.
> >>
> >> There is a test for that:
> >> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
> >> which tells me that I am right here for PTP.
> >>
> >> The touchscreen spec is blurrier however.
> >
> > OK, that is great to know.
> >
> >>
> >> >
> >> >>
> >> >> Do you have any precise example of reports where you need that feature?
> >> >
> >> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
> >>
> >> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
> >> guessing the device did not went through Microsoft certification
> >> process?
> >
> > That would be correct ;) At least the firmware that is shipping with
> > Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
> > their MSWin devices.
> >
> >>
> >> I am in favor of splitting the patch in 2. One for the generic
> >> processing of confidence bit, and one for this spurious release. For
> >> the spurious release, I'm more in favor of explicitly quirking the
> >> devices in need of such quirk.
> >
> > Hmm, I am not sure about having specific quirk. It will be hard for
> > users to accurately diagnose the issue if firmware is broken in this way
> > so we could add a new quirk for a new device.
> 
> One thing we can do is keep the quirked mechanism as default in
> hid-multitouch, but remove it in hid-core. If people need the quirk,
> they can just use hid-multitouch instead (talking about the long run
> here).

Hmm, I am confused. My patch did not touch hid-core or hid-input, only
hid-multitouch... So we are already doing what you are proposing?..

> 
> However, I really believe this might only be required for a handful of
> devices, and probably only touchscreens. So I would be tempted to not
> make it default and see how many bug reports we have.

Up to you but it is hard to detect for users. If just sometimes there
are stray clicks...

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
> On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
> >> Hi Dmitry,
> >>
> >> > > > Logically, the confidence state is a property of a contact, not a new type
> >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
> >> > > >
> >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
> >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> >> > > > I can't find any other users in the current upstream tree, but those
> >> > > > two are already making a precedent and changing the semantic is a
> >> > > > little bit late :/
> >> > I am sorry I did not respond and lost track of this issue back then, but
> >> > I disagree with Henrik here. While confidence is a property of contact,
> >> > so is the type of contact and it can and will change throughout life of
> >> > a contact, especially if we will continue adding new types, such as, for
> >> > example, thumb. In this case the firmware can transition through
> >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
> >> > contact becomes better understood. Still it is the same contact and we
> >> > should not attempt to signal userspace differently.
> >> We agree that the contact should stay the same, but the fear, and I think
> >> somewhere along the blurry history of this thread, the problem was that
> >> userspace interpreted the property change as a new contact (lift up/double
> >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
> >> Pen? It would be hard for an application to consider a switch from finger to
> >> pen as the same contact, which is where the natural implementation starts to
> >> diverge from the intention.
> >
> > I think the userspace has to trust our tracking ID to decide whether it
> > is a same contact or not. The current issue is that kernel is forcing
> > tracking ID change on tool type change, and one of the 2 patches that I
> > posted fixed that, allowing us to keep the tracking ID for finger->palm
> > transitions.
> 
> I think I missed those 2 patches, can you point a LKML link?

Sorry, I thought I sent it out with the patch we are talking about here,
but I did not. See below. Note that it doe snot have any protections on
finger->pen transitions and I am not sure any are needed at the moment.
We can add them wither to MT core or to drivers if we see issues with
devices.

> Also, note that libevdev discards the tracking ID change now (it
> shouts at the user in the logs). So that means that it will now be
> hard to force libevdev to trust the kernel again for the tracking ID.
> The current rule is:
> - tracking ID >= 0 -> new touch
> - any subsequent tracking ID >= 0 -> discarded
> - tracking ID == -1 -> end of touch

Well, I guess it is like synaptics driver that used to dump core
whenever it saw tracking ID change for the same slot (not going though
-1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
us having to produce weird results when users would use fancy gestures
with 3+ fingers.

It probably does not matter with devices with 5+ slots. We should pretty
much always have free slot for new contact.

> 
> >
> > I think it is kernel task to not signal transitions that do not make
> > sense, such as finger->pen or palm->pen etc.
> 
> I fully agree, though there is currently no such guard in the kernel
> (maybe it's part of your series). I am worried about the RMI4 F12
> driver that automatically forward the info from the firmware, so if
> the firmware does something crazy, it will be exported to user space.
> But I guess it might be better to treat that on a per driver basis.

Yeah, I think so.

> 
> >
> >>
> >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> >> > complement ABS_MT_TOOL, but that would not really solve the issue with
> >> > Wacom firmware (declaring contact non-confident and releasing it right
> >> > away) and given MS explanation of the confidence as "contact is too big"
> >> > MT_TOOL_PALM fits it perfectly.
> >> Indeed, the Wacom firmware seems to need some special handling, which should
> >> be fine by everyone. I do think it would make sense to add
> >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
> 
> Except we are already running out of ABS_* axes.

Sorry, meants MT_TOOL_TO_BIG, not a new axis.

> 
> >> also to a pen lying down on a touchpad, for instance.
> >
> > OK, I can see that for Pens, if we have firmware that would recognize
> > such condition, it would be weird to report PALM. We could indeed have
> > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
> > the hardware can recognize it as such). Maybe we'd be better off just
> > having userspace going by contact size for pens. Peter, any suggestions
> > here?
> 
> I don't think we have size handling in the tablet implementation in
> libinput. I do not see it as a big issue to add such axes from a
> libinput point of view. However, there is no existing hardware that
> would provide such information, so I guess this will be a 'no' until
> actual hardware comes in.
> 
> Also note that the MT_TOOL_PEN implementation is limited (even
> non-existant if I remember correctly). Peter and I do not have access
> to any device that support such multi pen, so AFAICT, there is no code
> to handle this in libinput.
> 
> One last point from libinput, the pen device would need to be on its
> separate kernel node for the protocol to be smoothly handled. So
> basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
> would not be handled properly right now. The Pen event will be treated
> as a touch.

I think normally pen and touch a separate controllers, so we have that
going for us...

Thanks.

-- 
Dmitry


Input: do not assign new tracking ID when changing tool type

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

We allow changing tool type (from MT_TOOL_FINGER to MT_TOOL_PALM) so we
should not be forcing new tracking ID for the slot.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/input-mt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index a1bbec9cda8d4..7ca4b318ed419 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -151,7 +151,7 @@ void input_mt_report_slot_state(struct input_dev *dev,
 	}
 
 	id = input_mt_get_value(slot, ABS_MT_TRACKING_ID);
-	if (id < 0 || input_mt_get_value(slot, ABS_MT_TOOL_TYPE) != tool_type)
+	if (id < 0)
 		id = input_mt_new_trkid(mt);
 
 	input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, id);

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Benjamin Tissoires]

On Mon, Jun 4, 2018 at 11:19 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jun 04, 2018 at 10:42:31PM +0200, Benjamin Tissoires wrote:
>> On Mon, Jun 4, 2018 at 7:33 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
>> >> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
>> >> <dmitry.torokhov@gmail.com> wrote:
>> >> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
>> >> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
>> >> >> <dmitry.torokhov@gmail.com> wrote:
>> >> >> > According to Microsoft specification [1] for Precision Touchpads (and
>> >> >> > Touchscreens) the devices use "confidence" reports to signal accidental
>> >> >> > touches, or contacts that are "too large to be a finger". Instead of
>> >> >> > simply marking contact inactive in this case (which causes issues if
>> >> >> > contact was originally proper and we lost confidence in it later, as
>> >> >> > this results in accidental clicks, drags, etc), let's report such
>> >> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
>> >> >> > Additionally, let's report contact size for such touches as maximum
>> >> >> > allowed for major/minor, which should help userspace that is not yet
>> >> >> > aware of MT_TOOL_PALM to still perform palm rejection.
>> >> >> >
>> >> >> > An additional complication, is that some firmwares do not report
>> >> >> > non-confident touches as active. To cope with this we delay release of
>> >> >> > such contact (i.e. if contact was active we first report it as still
>> >> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
>> >> >> > frame).
>> >> >>
>> >> >> I am not sure I agree with this part. The spec says that "Once a
>> >> >> device has determined that a contact is unintentional, it should clear
>> >> >> the confidence bit for that contact report and all subsequent
>> >> >> reports."
>> >> >> So in theory the spec says that if a touch has been detected as a
>> >> >> palm, the flow of events should not stop (tested on the PTP of the
>> >> >> Dell XPS 9360).
>> >> >>
>> >> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
>> >> >> and then (confidence 0, tip switch 0) a simple release, and the
>> >> >> confidence bit should not be relayed.
>> >> >
>> >> > This unfortunately leads to false clicks: you start with finger, so
>> >> > confidence is 1, then you transition the same touch to palm (use your
>> >> > thumb and "roll" your hand until heel of it comes into contact with the
>> >> > screen). The firmware reports "no-confidence" and "release" in the same
>> >> > report and userspace seeing release does not pay attention to confidence
>> >> > (i.e. it does exactly "simple release" logic) and this results in UI
>> >> > interpreting this as a click. With splitting no-confidence
>> >> > (MT_TOOL_PALM) and release event into separate frames we help userspace
>> >> > to recognize that the contact should be discarded.
>> >>
>> >> After further thoughts, I would consider this to be a firmware bug,
>> >> and not how the firmware is supposed to be reporting palm.
>> >> For the precision touchpads, the spec says that the device "should
>> >> clear the confidence bit for that contact report and all subsequent
>> >> reports.". And it is how the Dell device I have here reports palms.
>> >> The firmware is not supposed to cut the event stream.
>> >>
>> >> There is a test for that:
>> >> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
>> >> which tells me that I am right here for PTP.
>> >>
>> >> The touchscreen spec is blurrier however.
>> >
>> > OK, that is great to know.
>> >
>> >>
>> >> >
>> >> >>
>> >> >> Do you have any precise example of reports where you need that feature?
>> >> >
>> >> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
>> >>
>> >> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
>> >> guessing the device did not went through Microsoft certification
>> >> process?
>> >
>> > That would be correct ;) At least the firmware that is shipping with
>> > Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
>> > their MSWin devices.
>> >
>> >>
>> >> I am in favor of splitting the patch in 2. One for the generic
>> >> processing of confidence bit, and one for this spurious release. For
>> >> the spurious release, I'm more in favor of explicitly quirking the
>> >> devices in need of such quirk.
>> >
>> > Hmm, I am not sure about having specific quirk. It will be hard for
>> > users to accurately diagnose the issue if firmware is broken in this way
>> > so we could add a new quirk for a new device.
>>
>> One thing we can do is keep the quirked mechanism as default in
>> hid-multitouch, but remove it in hid-core. If people need the quirk,
>> they can just use hid-multitouch instead (talking about the long run
>> here).
>
> Hmm, I am confused. My patch did not touch hid-core or hid-input, only
> hid-multitouch... So we are already doing what you are proposing?..

It's the long run solution. I am trying in my last series to
streamline hid-multitouch so I can merge it in hid-core. I am planning
on having a few revision with hid-multitouch as an external module,
and then switch the win 8 (touchscreens and PTPs) devices to use a
generic processing in hid-input.c. So that's why I'd rather keep the
quirked devices separately and iron out the generic ones. It's easier
to override hid-multiouch for users than it is to override hid.ko.

>
>>
>> However, I really believe this might only be required for a handful of
>> devices, and probably only touchscreens. So I would be tempted to not
>> make it default and see how many bug reports we have.
>
> Up to you but it is hard to detect for users. If just sometimes there
> are stray clicks...

Users might not directly target the bugs toward me, but will likely
complain to Peter first :)
We can also add some automatic sanity check in libinput to relay the
information that there is a FW/kernel bug for such devices.

Cheers,
Benjamin

>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Benjamin Tissoires]

On Mon, Jun 4, 2018 at 11:32 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
>> On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
>> >> Hi Dmitry,
>> >>
>> >> > > > Logically, the confidence state is a property of a contact, not a new type
>> >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
>> >> > > >
>> >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
>> >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
>> >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
>> >> > > > I can't find any other users in the current upstream tree, but those
>> >> > > > two are already making a precedent and changing the semantic is a
>> >> > > > little bit late :/
>> >> > I am sorry I did not respond and lost track of this issue back then, but
>> >> > I disagree with Henrik here. While confidence is a property of contact,
>> >> > so is the type of contact and it can and will change throughout life of
>> >> > a contact, especially if we will continue adding new types, such as, for
>> >> > example, thumb. In this case the firmware can transition through
>> >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
>> >> > contact becomes better understood. Still it is the same contact and we
>> >> > should not attempt to signal userspace differently.
>> >> We agree that the contact should stay the same, but the fear, and I think
>> >> somewhere along the blurry history of this thread, the problem was that
>> >> userspace interpreted the property change as a new contact (lift up/double
>> >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
>> >> Pen? It would be hard for an application to consider a switch from finger to
>> >> pen as the same contact, which is where the natural implementation starts to
>> >> diverge from the intention.
>> >
>> > I think the userspace has to trust our tracking ID to decide whether it
>> > is a same contact or not. The current issue is that kernel is forcing
>> > tracking ID change on tool type change, and one of the 2 patches that I
>> > posted fixed that, allowing us to keep the tracking ID for finger->palm
>> > transitions.
>>
>> I think I missed those 2 patches, can you point a LKML link?
>
> Sorry, I thought I sent it out with the patch we are talking about here,
> but I did not. See below. Note that it doe snot have any protections on
> finger->pen transitions and I am not sure any are needed at the moment.
> We can add them wither to MT core or to drivers if we see issues with
> devices.

For what it worth, after a quick reading of the patch, it would be:
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

I can't remember exactly why we checked for the tool in the first place.

>
>> Also, note that libevdev discards the tracking ID change now (it
>> shouts at the user in the logs). So that means that it will now be
>> hard to force libevdev to trust the kernel again for the tracking ID.
>> The current rule is:
>> - tracking ID >= 0 -> new touch
>> - any subsequent tracking ID >= 0 -> discarded
>> - tracking ID == -1 -> end of touch
>
> Well, I guess it is like synaptics driver that used to dump core
> whenever it saw tracking ID change for the same slot (not going though
> -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> us having to produce weird results when users would use fancy gestures
> with 3+ fingers.
>
> It probably does not matter with devices with 5+ slots. We should pretty
> much always have free slot for new contact.

On Win 8 devices, we can safely expect the firmware to have a correct
maximum contact number information (there is no point reporting 5
fingers and saying you can support only 2). So if we try to add a new
slot and we are out of them, it's pretty much down to a firmware bug
or a touch we missed to release.

>
>>
>> >
>> > I think it is kernel task to not signal transitions that do not make
>> > sense, such as finger->pen or palm->pen etc.
>>
>> I fully agree, though there is currently no such guard in the kernel
>> (maybe it's part of your series). I am worried about the RMI4 F12
>> driver that automatically forward the info from the firmware, so if
>> the firmware does something crazy, it will be exported to user space.
>> But I guess it might be better to treat that on a per driver basis.
>
> Yeah, I think so.
>
>>
>> >
>> >>
>> >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
>> >> > complement ABS_MT_TOOL, but that would not really solve the issue with
>> >> > Wacom firmware (declaring contact non-confident and releasing it right
>> >> > away) and given MS explanation of the confidence as "contact is too big"
>> >> > MT_TOOL_PALM fits it perfectly.
>> >> Indeed, the Wacom firmware seems to need some special handling, which should
>> >> be fine by everyone. I do think it would make sense to add
>> >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
>>
>> Except we are already running out of ABS_* axes.
>
> Sorry, meants MT_TOOL_TO_BIG, not a new axis.
>
>>
>> >> also to a pen lying down on a touchpad, for instance.
>> >
>> > OK, I can see that for Pens, if we have firmware that would recognize
>> > such condition, it would be weird to report PALM. We could indeed have
>> > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
>> > the hardware can recognize it as such). Maybe we'd be better off just
>> > having userspace going by contact size for pens. Peter, any suggestions
>> > here?
>>
>> I don't think we have size handling in the tablet implementation in
>> libinput. I do not see it as a big issue to add such axes from a
>> libinput point of view. However, there is no existing hardware that
>> would provide such information, so I guess this will be a 'no' until
>> actual hardware comes in.
>>
>> Also note that the MT_TOOL_PEN implementation is limited (even
>> non-existant if I remember correctly). Peter and I do not have access
>> to any device that support such multi pen, so AFAICT, there is no code
>> to handle this in libinput.
>>
>> One last point from libinput, the pen device would need to be on its
>> separate kernel node for the protocol to be smoothly handled. So
>> basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
>> would not be handled properly right now. The Pen event will be treated
>> as a touch.
>
> I think normally pen and touch a separate controllers, so we have that
> going for us...

The problem is for devices like the N-trig DualSense or the Wacom AES
where only one sensor handles both pen and touch. For now, given that
MS says it supports only one pen we are on the safe side, but things
might gets scarier in the future :)

Cheers,
Benjamin

>
> Thanks.
>
> --
> Dmitry
>
>
> Input: do not assign new tracking ID when changing tool type
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> We allow changing tool type (from MT_TOOL_FINGER to MT_TOOL_PALM) so we
> should not be forcing new tracking ID for the slot.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/input-mt.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index a1bbec9cda8d4..7ca4b318ed419 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -151,7 +151,7 @@ void input_mt_report_slot_state(struct input_dev *dev,
>         }
>
>         id = input_mt_get_value(slot, ABS_MT_TRACKING_ID);
> -       if (id < 0 || input_mt_get_value(slot, ABS_MT_TOOL_TYPE) != tool_type)
> +       if (id < 0)
>                 id = input_mt_new_trkid(mt);
>
>         input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, id);
>

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Peter Hutterer]

On Mon, Jun 04, 2018 at 02:19:44PM -0700, Dmitry Torokhov wrote:
> On Mon, Jun 04, 2018 at 10:42:31PM +0200, Benjamin Tissoires wrote:
> > On Mon, Jun 4, 2018 at 7:33 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
> > >> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
> > >> <dmitry.torokhov@gmail.com> wrote:
> > >> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
> > >> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
> > >> >> <dmitry.torokhov@gmail.com> wrote:
> > >> >> > According to Microsoft specification [1] for Precision Touchpads (and
> > >> >> > Touchscreens) the devices use "confidence" reports to signal accidental
> > >> >> > touches, or contacts that are "too large to be a finger". Instead of
> > >> >> > simply marking contact inactive in this case (which causes issues if
> > >> >> > contact was originally proper and we lost confidence in it later, as
> > >> >> > this results in accidental clicks, drags, etc), let's report such
> > >> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
> > >> >> > Additionally, let's report contact size for such touches as maximum
> > >> >> > allowed for major/minor, which should help userspace that is not yet
> > >> >> > aware of MT_TOOL_PALM to still perform palm rejection.
> > >> >> >
> > >> >> > An additional complication, is that some firmwares do not report
> > >> >> > non-confident touches as active. To cope with this we delay release of
> > >> >> > such contact (i.e. if contact was active we first report it as still
> > >> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
> > >> >> > frame).
> > >> >>
> > >> >> I am not sure I agree with this part. The spec says that "Once a
> > >> >> device has determined that a contact is unintentional, it should clear
> > >> >> the confidence bit for that contact report and all subsequent
> > >> >> reports."
> > >> >> So in theory the spec says that if a touch has been detected as a
> > >> >> palm, the flow of events should not stop (tested on the PTP of the
> > >> >> Dell XPS 9360).
> > >> >>
> > >> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
> > >> >> and then (confidence 0, tip switch 0) a simple release, and the
> > >> >> confidence bit should not be relayed.
> > >> >
> > >> > This unfortunately leads to false clicks: you start with finger, so
> > >> > confidence is 1, then you transition the same touch to palm (use your
> > >> > thumb and "roll" your hand until heel of it comes into contact with the
> > >> > screen). The firmware reports "no-confidence" and "release" in the same
> > >> > report and userspace seeing release does not pay attention to confidence
> > >> > (i.e. it does exactly "simple release" logic) and this results in UI
> > >> > interpreting this as a click. With splitting no-confidence
> > >> > (MT_TOOL_PALM) and release event into separate frames we help userspace
> > >> > to recognize that the contact should be discarded.
> > >>
> > >> After further thoughts, I would consider this to be a firmware bug,
> > >> and not how the firmware is supposed to be reporting palm.
> > >> For the precision touchpads, the spec says that the device "should
> > >> clear the confidence bit for that contact report and all subsequent
> > >> reports.". And it is how the Dell device I have here reports palms.
> > >> The firmware is not supposed to cut the event stream.
> > >>
> > >> There is a test for that:
> > >> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
> > >> which tells me that I am right here for PTP.
> > >>
> > >> The touchscreen spec is blurrier however.
> > >
> > > OK, that is great to know.
> > >
> > >>
> > >> >
> > >> >>
> > >> >> Do you have any precise example of reports where you need that feature?
> > >> >
> > >> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
> > >>
> > >> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
> > >> guessing the device did not went through Microsoft certification
> > >> process?
> > >
> > > That would be correct ;) At least the firmware that is shipping with
> > > Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
> > > their MSWin devices.
> > >
> > >>
> > >> I am in favor of splitting the patch in 2. One for the generic
> > >> processing of confidence bit, and one for this spurious release. For
> > >> the spurious release, I'm more in favor of explicitly quirking the
> > >> devices in need of such quirk.
> > >
> > > Hmm, I am not sure about having specific quirk. It will be hard for
> > > users to accurately diagnose the issue if firmware is broken in this way
> > > so we could add a new quirk for a new device.
> > 
> > One thing we can do is keep the quirked mechanism as default in
> > hid-multitouch, but remove it in hid-core. If people need the quirk,
> > they can just use hid-multitouch instead (talking about the long run
> > here).
> 
> Hmm, I am confused. My patch did not touch hid-core or hid-input, only
> hid-multitouch... So we are already doing what you are proposing?..
> 
> > 
> > However, I really believe this might only be required for a handful of
> > devices, and probably only touchscreens. So I would be tempted to not
> > make it default and see how many bug reports we have.
> 
> Up to you but it is hard to detect for users. If just sometimes there
> are stray clicks...

fwiw, from my POV, if you give me MT_TOOL_PALM in the same frame as the
ABS_MT_TRACKING_ID -1 I can work that into libinput to do the right thing.
Not 100% whether that already works anyway but probably not. I'd prefer it
being fixed in the kernel though, less work for me :)

Cheers,
   Peter

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Peter Hutterer]

On Mon, Jun 04, 2018 at 02:32:55PM -0700, Dmitry Torokhov wrote:
> On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
> > On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
> > >> Hi Dmitry,
> > >>
> > >> > > > Logically, the confidence state is a property of a contact, not a new type
> > >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
> > >> > > >
> > >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
> > >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> > >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> > >> > > > I can't find any other users in the current upstream tree, but those
> > >> > > > two are already making a precedent and changing the semantic is a
> > >> > > > little bit late :/
> > >> > I am sorry I did not respond and lost track of this issue back then, but
> > >> > I disagree with Henrik here. While confidence is a property of contact,
> > >> > so is the type of contact and it can and will change throughout life of
> > >> > a contact, especially if we will continue adding new types, such as, for
> > >> > example, thumb. In this case the firmware can transition through
> > >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
> > >> > contact becomes better understood. Still it is the same contact and we
> > >> > should not attempt to signal userspace differently.
> > >> We agree that the contact should stay the same, but the fear, and I think
> > >> somewhere along the blurry history of this thread, the problem was that
> > >> userspace interpreted the property change as a new contact (lift up/double
> > >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
> > >> Pen? It would be hard for an application to consider a switch from finger to
> > >> pen as the same contact, which is where the natural implementation starts to
> > >> diverge from the intention.
> > >
> > > I think the userspace has to trust our tracking ID to decide whether it
> > > is a same contact or not. The current issue is that kernel is forcing
> > > tracking ID change on tool type change, and one of the 2 patches that I
> > > posted fixed that, allowing us to keep the tracking ID for finger->palm
> > > transitions.
> > 
> > I think I missed those 2 patches, can you point a LKML link?
> 
> Sorry, I thought I sent it out with the patch we are talking about here,
> but I did not. See below. Note that it doe snot have any protections on
> finger->pen transitions and I am not sure any are needed at the moment.
> We can add them wither to MT core or to drivers if we see issues with
> devices.
> 
> > Also, note that libevdev discards the tracking ID change now (it
> > shouts at the user in the logs). So that means that it will now be
> > hard to force libevdev to trust the kernel again for the tracking ID.
> > The current rule is:
> > - tracking ID >= 0 -> new touch
> > - any subsequent tracking ID >= 0 -> discarded
> > - tracking ID == -1 -> end of touch
> 
> Well, I guess it is like synaptics driver that used to dump core
> whenever it saw tracking ID change for the same slot (not going though
> -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> us having to produce weird results when users would use fancy gestures
> with 3+ fingers.

yeah, my mistake, sorry. I always assumed a transition from M to -1 to N,
never M to N. This assumption made its way into libevdev (where the tracking
ID is transparently discarded, albeit with a warning). There are libevdev
patches to get rid of that but whatever device needed it got fixed in some
other way, so the patch didn't get pushed.

fwiw, dump core was just "print the backtrace to the log" here, there was no
actual core dump.

> It probably does not matter with devices with 5+ slots. We should pretty
> much always have free slot for new contact.
> 
> > 
> > >
> > > I think it is kernel task to not signal transitions that do not make
> > > sense, such as finger->pen or palm->pen etc.
> > 
> > I fully agree, though there is currently no such guard in the kernel
> > (maybe it's part of your series). I am worried about the RMI4 F12
> > driver that automatically forward the info from the firmware, so if
> > the firmware does something crazy, it will be exported to user space.
> > But I guess it might be better to treat that on a per driver basis.
> 
> Yeah, I think so.
> 
> > 
> > >
> > >>
> > >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> > >> > complement ABS_MT_TOOL, but that would not really solve the issue with
> > >> > Wacom firmware (declaring contact non-confident and releasing it right
> > >> > away) and given MS explanation of the confidence as "contact is too big"
> > >> > MT_TOOL_PALM fits it perfectly.
> > >> Indeed, the Wacom firmware seems to need some special handling, which should
> > >> be fine by everyone. I do think it would make sense to add
> > >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
> > 
> > Except we are already running out of ABS_* axes.
> 
> Sorry, meants MT_TOOL_TO_BIG, not a new axis.

bikeshed: MT_TOOL_IGNORE is a more generic name and does not imply size. A
pen that's lying on its side doesn't have a size but should still be
ignored.

> > 
> > >> also to a pen lying down on a touchpad, for instance.
> > >
> > > OK, I can see that for Pens, if we have firmware that would recognize
> > > such condition, it would be weird to report PALM. We could indeed have
> > > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
> > > the hardware can recognize it as such). Maybe we'd be better off just
> > > having userspace going by contact size for pens. Peter, any suggestions
> > > here?
> > 
> > I don't think we have size handling in the tablet implementation in
> > libinput. I do not see it as a big issue to add such axes from a
> > libinput point of view. However, there is no existing hardware that
> > would provide such information, so I guess this will be a 'no' until
> > actual hardware comes in.

correct on all counts :)


> > Also note that the MT_TOOL_PEN implementation is limited (even
> > non-existant if I remember correctly). Peter and I do not have access
> > to any device that support such multi pen, so AFAICT, there is no code
> > to handle this in libinput.

Yep, correct. On this note: libinput very much follows a "no hardware, no
implementation" rule. I played the game of trying to support everything in a
generic manner with the X drivers and it's a nightmare to maintain. libinput
instead takes a use case and tries to make it sensible - but for that to
work we need to know both the hardware and the use-cases. That's why tablet
handling coming out of libinput is very different to the handling we have in
X but, afaict, everyone is better off for it so far.

This means that if you give me a MT_TOOL_FINGER → MT_TOOL_PEN transition,
I'll handle it, but only after you also give me the use-case for it and the
promise of real devices that need it. 

> > One last point from libinput, the pen device would need to be on its
> > separate kernel node for the protocol to be smoothly handled. So
> > basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
> > would not be handled properly right now. The Pen event will be treated
> > as a touch.
> 
> I think normally pen and touch a separate controllers, so we have that
> going for us...

Side-effect of this is: the tablet interface doesn't handle touch at all
because it didn't need to yet. So while technically possible, it requires a
fair bit of re-arranging.

> Input: do not assign new tracking ID when changing tool type
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> We allow changing tool type (from MT_TOOL_FINGER to MT_TOOL_PALM) so we
> should not be forcing new tracking ID for the slot.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>

Cheers,
   Peter

> ---
>  drivers/input/input-mt.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index a1bbec9cda8d4..7ca4b318ed419 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -151,7 +151,7 @@ void input_mt_report_slot_state(struct input_dev *dev,
>  	}
>  
>  	id = input_mt_get_value(slot, ABS_MT_TRACKING_ID);
> -	if (id < 0 || input_mt_get_value(slot, ABS_MT_TOOL_TYPE) != tool_type)
> +	if (id < 0)
>  		id = input_mt_new_trkid(mt);
>  
>  	input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, id);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

On Tue, Jun 05, 2018 at 09:06:24AM +1000, Peter Hutterer wrote:
> On Mon, Jun 04, 2018 at 02:32:55PM -0700, Dmitry Torokhov wrote:
> > On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
> > > On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
> > > >> Hi Dmitry,
> > > >>
> > > >> > > > Logically, the confidence state is a property of a contact, not a new type
> > > >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
> > > >> > > >
> > > >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
> > > >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> > > >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> > > >> > > > I can't find any other users in the current upstream tree, but those
> > > >> > > > two are already making a precedent and changing the semantic is a
> > > >> > > > little bit late :/
> > > >> > I am sorry I did not respond and lost track of this issue back then, but
> > > >> > I disagree with Henrik here. While confidence is a property of contact,
> > > >> > so is the type of contact and it can and will change throughout life of
> > > >> > a contact, especially if we will continue adding new types, such as, for
> > > >> > example, thumb. In this case the firmware can transition through
> > > >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
> > > >> > contact becomes better understood. Still it is the same contact and we
> > > >> > should not attempt to signal userspace differently.
> > > >> We agree that the contact should stay the same, but the fear, and I think
> > > >> somewhere along the blurry history of this thread, the problem was that
> > > >> userspace interpreted the property change as a new contact (lift up/double
> > > >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
> > > >> Pen? It would be hard for an application to consider a switch from finger to
> > > >> pen as the same contact, which is where the natural implementation starts to
> > > >> diverge from the intention.
> > > >
> > > > I think the userspace has to trust our tracking ID to decide whether it
> > > > is a same contact or not. The current issue is that kernel is forcing
> > > > tracking ID change on tool type change, and one of the 2 patches that I
> > > > posted fixed that, allowing us to keep the tracking ID for finger->palm
> > > > transitions.
> > > 
> > > I think I missed those 2 patches, can you point a LKML link?
> > 
> > Sorry, I thought I sent it out with the patch we are talking about here,
> > but I did not. See below. Note that it doe snot have any protections on
> > finger->pen transitions and I am not sure any are needed at the moment.
> > We can add them wither to MT core or to drivers if we see issues with
> > devices.
> > 
> > > Also, note that libevdev discards the tracking ID change now (it
> > > shouts at the user in the logs). So that means that it will now be
> > > hard to force libevdev to trust the kernel again for the tracking ID.
> > > The current rule is:
> > > - tracking ID >= 0 -> new touch
> > > - any subsequent tracking ID >= 0 -> discarded
> > > - tracking ID == -1 -> end of touch
> > 
> > Well, I guess it is like synaptics driver that used to dump core
> > whenever it saw tracking ID change for the same slot (not going though
> > -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> > us having to produce weird results when users would use fancy gestures
> > with 3+ fingers.
> 
> yeah, my mistake, sorry. I always assumed a transition from M to -1 to N,
> never M to N. This assumption made its way into libevdev (where the tracking
> ID is transparently discarded, albeit with a warning). There are libevdev
> patches to get rid of that but whatever device needed it got fixed in some
> other way, so the patch didn't get pushed.
> 
> fwiw, dump core was just "print the backtrace to the log" here, there was no
> actual core dump.

Hmm, I do not recall what version I was playing with, but I tried
changing Synaptics kernel driver to not insert the fake -1 tracking ID
for a slot when rolling 3 fingers on a 2-slot device (so removing finger
1 while holding finger 2 and adding finger 3 does not appear to
userspace as 2 - 1 - 2 fingers on the surface, but 2 - 2 - 2 instead)
and xf86-input-synaptics-1.7.8 would scream about too many slots and
stop working.

That was a while ago though, before libinput I think.

> 
> > It probably does not matter with devices with 5+ slots. We should pretty
> > much always have free slot for new contact.
> > 
> > > 
> > > >
> > > > I think it is kernel task to not signal transitions that do not make
> > > > sense, such as finger->pen or palm->pen etc.
> > > 
> > > I fully agree, though there is currently no such guard in the kernel
> > > (maybe it's part of your series). I am worried about the RMI4 F12
> > > driver that automatically forward the info from the firmware, so if
> > > the firmware does something crazy, it will be exported to user space.
> > > But I guess it might be better to treat that on a per driver basis.
> > 
> > Yeah, I think so.
> > 
> > > 
> > > >
> > > >>
> > > >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> > > >> > complement ABS_MT_TOOL, but that would not really solve the issue with
> > > >> > Wacom firmware (declaring contact non-confident and releasing it right
> > > >> > away) and given MS explanation of the confidence as "contact is too big"
> > > >> > MT_TOOL_PALM fits it perfectly.
> > > >> Indeed, the Wacom firmware seems to need some special handling, which should
> > > >> be fine by everyone. I do think it would make sense to add
> > > >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
> > > 
> > > Except we are already running out of ABS_* axes.
> > 
> > Sorry, meants MT_TOOL_TO_BIG, not a new axis.
> 
> bikeshed: MT_TOOL_IGNORE is a more generic name and does not imply size. A
> pen that's lying on its side doesn't have a size but should still be
> ignored.

OK, when we start seeing this for non finger/thumb/palm objects we can
add this tool type. For current devices we are dealing with palms.

> 
> > > 
> > > >> also to a pen lying down on a touchpad, for instance.
> > > >
> > > > OK, I can see that for Pens, if we have firmware that would recognize
> > > > such condition, it would be weird to report PALM. We could indeed have
> > > > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
> > > > the hardware can recognize it as such). Maybe we'd be better off just
> > > > having userspace going by contact size for pens. Peter, any suggestions
> > > > here?
> > > 
> > > I don't think we have size handling in the tablet implementation in
> > > libinput. I do not see it as a big issue to add such axes from a
> > > libinput point of view. However, there is no existing hardware that
> > > would provide such information, so I guess this will be a 'no' until
> > > actual hardware comes in.
> 
> correct on all counts :)
> 
> 
> > > Also note that the MT_TOOL_PEN implementation is limited (even
> > > non-existant if I remember correctly). Peter and I do not have access
> > > to any device that support such multi pen, so AFAICT, there is no code
> > > to handle this in libinput.
> 
> Yep, correct. On this note: libinput very much follows a "no hardware, no
> implementation" rule. I played the game of trying to support everything in a
> generic manner with the X drivers and it's a nightmare to maintain. libinput
> instead takes a use case and tries to make it sensible - but for that to
> work we need to know both the hardware and the use-cases. That's why tablet
> handling coming out of libinput is very different to the handling we have in
> X but, afaict, everyone is better off for it so far.
> 
> This means that if you give me a MT_TOOL_FINGER → MT_TOOL_PEN transition,
> I'll handle it, but only after you also give me the use-case for it and the
> promise of real devices that need it. 
> 
> > > One last point from libinput, the pen device would need to be on its
> > > separate kernel node for the protocol to be smoothly handled. So
> > > basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
> > > would not be handled properly right now. The Pen event will be treated
> > > as a touch.
> > 
> > I think normally pen and touch a separate controllers, so we have that
> > going for us...
> 
> Side-effect of this is: the tablet interface doesn't handle touch at all
> because it didn't need to yet. So while technically possible, it requires a
> fair bit of re-arranging.

What about things like Bamboo touch? It is a Wacom tablet with both
multitouch finger and stylus.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Peter Hutterer]

On Mon, Jun 04, 2018 at 04:28:01PM -0700, Dmitry Torokhov wrote:
> On Tue, Jun 05, 2018 at 09:06:24AM +1000, Peter Hutterer wrote:
> > On Mon, Jun 04, 2018 at 02:32:55PM -0700, Dmitry Torokhov wrote:
> > > On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
> > > > On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
> > > > >> Hi Dmitry,
> > > > >>
> > > > >> > > > Logically, the confidence state is a property of a contact, not a new type
> > > > >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
> > > > >> > > >
> > > > >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
> > > > >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> > > > >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> > > > >> > > > I can't find any other users in the current upstream tree, but those
> > > > >> > > > two are already making a precedent and changing the semantic is a
> > > > >> > > > little bit late :/
> > > > >> > I am sorry I did not respond and lost track of this issue back then, but
> > > > >> > I disagree with Henrik here. While confidence is a property of contact,
> > > > >> > so is the type of contact and it can and will change throughout life of
> > > > >> > a contact, especially if we will continue adding new types, such as, for
> > > > >> > example, thumb. In this case the firmware can transition through
> > > > >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
> > > > >> > contact becomes better understood. Still it is the same contact and we
> > > > >> > should not attempt to signal userspace differently.
> > > > >> We agree that the contact should stay the same, but the fear, and I think
> > > > >> somewhere along the blurry history of this thread, the problem was that
> > > > >> userspace interpreted the property change as a new contact (lift up/double
> > > > >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
> > > > >> Pen? It would be hard for an application to consider a switch from finger to
> > > > >> pen as the same contact, which is where the natural implementation starts to
> > > > >> diverge from the intention.
> > > > >
> > > > > I think the userspace has to trust our tracking ID to decide whether it
> > > > > is a same contact or not. The current issue is that kernel is forcing
> > > > > tracking ID change on tool type change, and one of the 2 patches that I
> > > > > posted fixed that, allowing us to keep the tracking ID for finger->palm
> > > > > transitions.
> > > > 
> > > > I think I missed those 2 patches, can you point a LKML link?
> > > 
> > > Sorry, I thought I sent it out with the patch we are talking about here,
> > > but I did not. See below. Note that it doe snot have any protections on
> > > finger->pen transitions and I am not sure any are needed at the moment.
> > > We can add them wither to MT core or to drivers if we see issues with
> > > devices.
> > > 
> > > > Also, note that libevdev discards the tracking ID change now (it
> > > > shouts at the user in the logs). So that means that it will now be
> > > > hard to force libevdev to trust the kernel again for the tracking ID.
> > > > The current rule is:
> > > > - tracking ID >= 0 -> new touch
> > > > - any subsequent tracking ID >= 0 -> discarded
> > > > - tracking ID == -1 -> end of touch
> > > 
> > > Well, I guess it is like synaptics driver that used to dump core
> > > whenever it saw tracking ID change for the same slot (not going though
> > > -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> > > us having to produce weird results when users would use fancy gestures
> > > with 3+ fingers.
> > 
> > yeah, my mistake, sorry. I always assumed a transition from M to -1 to N,
> > never M to N. This assumption made its way into libevdev (where the tracking
> > ID is transparently discarded, albeit with a warning). There are libevdev
> > patches to get rid of that but whatever device needed it got fixed in some
> > other way, so the patch didn't get pushed.
> > 
> > fwiw, dump core was just "print the backtrace to the log" here, there was no
> > actual core dump.
> 
> Hmm, I do not recall what version I was playing with, but I tried
> changing Synaptics kernel driver to not insert the fake -1 tracking ID
> for a slot when rolling 3 fingers on a 2-slot device (so removing finger
> 1 while holding finger 2 and adding finger 3 does not appear to
> userspace as 2 - 1 - 2 fingers on the surface, but 2 - 2 - 2 instead)
> and xf86-input-synaptics-1.7.8 would scream about too many slots and
> stop working.
> 
> That was a while ago though, before libinput I think.
> 
> > 
> > > It probably does not matter with devices with 5+ slots. We should pretty
> > > much always have free slot for new contact.
> > > 
> > > > 
> > > > >
> > > > > I think it is kernel task to not signal transitions that do not make
> > > > > sense, such as finger->pen or palm->pen etc.
> > > > 
> > > > I fully agree, though there is currently no such guard in the kernel
> > > > (maybe it's part of your series). I am worried about the RMI4 F12
> > > > driver that automatically forward the info from the firmware, so if
> > > > the firmware does something crazy, it will be exported to user space.
> > > > But I guess it might be better to treat that on a per driver basis.
> > > 
> > > Yeah, I think so.
> > > 
> > > > 
> > > > >
> > > > >>
> > > > >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> > > > >> > complement ABS_MT_TOOL, but that would not really solve the issue with
> > > > >> > Wacom firmware (declaring contact non-confident and releasing it right
> > > > >> > away) and given MS explanation of the confidence as "contact is too big"
> > > > >> > MT_TOOL_PALM fits it perfectly.
> > > > >> Indeed, the Wacom firmware seems to need some special handling, which should
> > > > >> be fine by everyone. I do think it would make sense to add
> > > > >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
> > > > 
> > > > Except we are already running out of ABS_* axes.
> > > 
> > > Sorry, meants MT_TOOL_TO_BIG, not a new axis.
> > 
> > bikeshed: MT_TOOL_IGNORE is a more generic name and does not imply size. A
> > pen that's lying on its side doesn't have a size but should still be
> > ignored.
> 
> OK, when we start seeing this for non finger/thumb/palm objects we can
> add this tool type. For current devices we are dealing with palms.
> 
> > 
> > > > 
> > > > >> also to a pen lying down on a touchpad, for instance.
> > > > >
> > > > > OK, I can see that for Pens, if we have firmware that would recognize
> > > > > such condition, it would be weird to report PALM. We could indeed have
> > > > > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
> > > > > the hardware can recognize it as such). Maybe we'd be better off just
> > > > > having userspace going by contact size for pens. Peter, any suggestions
> > > > > here?
> > > > 
> > > > I don't think we have size handling in the tablet implementation in
> > > > libinput. I do not see it as a big issue to add such axes from a
> > > > libinput point of view. However, there is no existing hardware that
> > > > would provide such information, so I guess this will be a 'no' until
> > > > actual hardware comes in.
> > 
> > correct on all counts :)
> > 
> > 
> > > > Also note that the MT_TOOL_PEN implementation is limited (even
> > > > non-existant if I remember correctly). Peter and I do not have access
> > > > to any device that support such multi pen, so AFAICT, there is no code
> > > > to handle this in libinput.
> > 
> > Yep, correct. On this note: libinput very much follows a "no hardware, no
> > implementation" rule. I played the game of trying to support everything in a
> > generic manner with the X drivers and it's a nightmare to maintain. libinput
> > instead takes a use case and tries to make it sensible - but for that to
> > work we need to know both the hardware and the use-cases. That's why tablet
> > handling coming out of libinput is very different to the handling we have in
> > X but, afaict, everyone is better off for it so far.
> > 
> > This means that if you give me a MT_TOOL_FINGER → MT_TOOL_PEN transition,
> > I'll handle it, but only after you also give me the use-case for it and the
> > promise of real devices that need it. 
> > 
> > > > One last point from libinput, the pen device would need to be on its
> > > > separate kernel node for the protocol to be smoothly handled. So
> > > > basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
> > > > would not be handled properly right now. The Pen event will be treated
> > > > as a touch.
> > > 
> > > I think normally pen and touch a separate controllers, so we have that
> > > going for us...
> > 
> > Side-effect of this is: the tablet interface doesn't handle touch at all
> > because it didn't need to yet. So while technically possible, it requires a
> > fair bit of re-arranging.
> 
> What about things like Bamboo touch? It is a Wacom tablet with both
> multitouch finger and stylus.

these are on two different event nodes though, isn't it? If not, then no-one
has tested them with libinput so far...

Cheers,
   Peter

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

On Tue, Jun 05, 2018 at 09:51:14AM +1000, Peter Hutterer wrote:
> On Mon, Jun 04, 2018 at 04:28:01PM -0700, Dmitry Torokhov wrote:
> > On Tue, Jun 05, 2018 at 09:06:24AM +1000, Peter Hutterer wrote:
> > > On Mon, Jun 04, 2018 at 02:32:55PM -0700, Dmitry Torokhov wrote:
> > > > On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
> > > > > On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
> > > > > >> Hi Dmitry,
> > > > > >>
> > > > > >> > > > Logically, the confidence state is a property of a contact, not a new type
> > > > > >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
> > > > > >> > > >
> > > > > >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
> > > > > >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> > > > > >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> > > > > >> > > > I can't find any other users in the current upstream tree, but those
> > > > > >> > > > two are already making a precedent and changing the semantic is a
> > > > > >> > > > little bit late :/
> > > > > >> > I am sorry I did not respond and lost track of this issue back then, but
> > > > > >> > I disagree with Henrik here. While confidence is a property of contact,
> > > > > >> > so is the type of contact and it can and will change throughout life of
> > > > > >> > a contact, especially if we will continue adding new types, such as, for
> > > > > >> > example, thumb. In this case the firmware can transition through
> > > > > >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
> > > > > >> > contact becomes better understood. Still it is the same contact and we
> > > > > >> > should not attempt to signal userspace differently.
> > > > > >> We agree that the contact should stay the same, but the fear, and I think
> > > > > >> somewhere along the blurry history of this thread, the problem was that
> > > > > >> userspace interpreted the property change as a new contact (lift up/double
> > > > > >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
> > > > > >> Pen? It would be hard for an application to consider a switch from finger to
> > > > > >> pen as the same contact, which is where the natural implementation starts to
> > > > > >> diverge from the intention.
> > > > > >
> > > > > > I think the userspace has to trust our tracking ID to decide whether it
> > > > > > is a same contact or not. The current issue is that kernel is forcing
> > > > > > tracking ID change on tool type change, and one of the 2 patches that I
> > > > > > posted fixed that, allowing us to keep the tracking ID for finger->palm
> > > > > > transitions.
> > > > > 
> > > > > I think I missed those 2 patches, can you point a LKML link?
> > > > 
> > > > Sorry, I thought I sent it out with the patch we are talking about here,
> > > > but I did not. See below. Note that it doe snot have any protections on
> > > > finger->pen transitions and I am not sure any are needed at the moment.
> > > > We can add them wither to MT core or to drivers if we see issues with
> > > > devices.
> > > > 
> > > > > Also, note that libevdev discards the tracking ID change now (it
> > > > > shouts at the user in the logs). So that means that it will now be
> > > > > hard to force libevdev to trust the kernel again for the tracking ID.
> > > > > The current rule is:
> > > > > - tracking ID >= 0 -> new touch
> > > > > - any subsequent tracking ID >= 0 -> discarded
> > > > > - tracking ID == -1 -> end of touch
> > > > 
> > > > Well, I guess it is like synaptics driver that used to dump core
> > > > whenever it saw tracking ID change for the same slot (not going though
> > > > -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> > > > us having to produce weird results when users would use fancy gestures
> > > > with 3+ fingers.
> > > 
> > > yeah, my mistake, sorry. I always assumed a transition from M to -1 to N,
> > > never M to N. This assumption made its way into libevdev (where the tracking
> > > ID is transparently discarded, albeit with a warning). There are libevdev
> > > patches to get rid of that but whatever device needed it got fixed in some
> > > other way, so the patch didn't get pushed.
> > > 
> > > fwiw, dump core was just "print the backtrace to the log" here, there was no
> > > actual core dump.
> > 
> > Hmm, I do not recall what version I was playing with, but I tried
> > changing Synaptics kernel driver to not insert the fake -1 tracking ID
> > for a slot when rolling 3 fingers on a 2-slot device (so removing finger
> > 1 while holding finger 2 and adding finger 3 does not appear to
> > userspace as 2 - 1 - 2 fingers on the surface, but 2 - 2 - 2 instead)
> > and xf86-input-synaptics-1.7.8 would scream about too many slots and
> > stop working.
> > 
> > That was a while ago though, before libinput I think.
> > 
> > > 
> > > > It probably does not matter with devices with 5+ slots. We should pretty
> > > > much always have free slot for new contact.
> > > > 
> > > > > 
> > > > > >
> > > > > > I think it is kernel task to not signal transitions that do not make
> > > > > > sense, such as finger->pen or palm->pen etc.
> > > > > 
> > > > > I fully agree, though there is currently no such guard in the kernel
> > > > > (maybe it's part of your series). I am worried about the RMI4 F12
> > > > > driver that automatically forward the info from the firmware, so if
> > > > > the firmware does something crazy, it will be exported to user space.
> > > > > But I guess it might be better to treat that on a per driver basis.
> > > > 
> > > > Yeah, I think so.
> > > > 
> > > > > 
> > > > > >
> > > > > >>
> > > > > >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> > > > > >> > complement ABS_MT_TOOL, but that would not really solve the issue with
> > > > > >> > Wacom firmware (declaring contact non-confident and releasing it right
> > > > > >> > away) and given MS explanation of the confidence as "contact is too big"
> > > > > >> > MT_TOOL_PALM fits it perfectly.
> > > > > >> Indeed, the Wacom firmware seems to need some special handling, which should
> > > > > >> be fine by everyone. I do think it would make sense to add
> > > > > >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
> > > > > 
> > > > > Except we are already running out of ABS_* axes.
> > > > 
> > > > Sorry, meants MT_TOOL_TO_BIG, not a new axis.
> > > 
> > > bikeshed: MT_TOOL_IGNORE is a more generic name and does not imply size. A
> > > pen that's lying on its side doesn't have a size but should still be
> > > ignored.
> > 
> > OK, when we start seeing this for non finger/thumb/palm objects we can
> > add this tool type. For current devices we are dealing with palms.
> > 
> > > 
> > > > > 
> > > > > >> also to a pen lying down on a touchpad, for instance.
> > > > > >
> > > > > > OK, I can see that for Pens, if we have firmware that would recognize
> > > > > > such condition, it would be weird to report PALM. We could indeed have
> > > > > > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
> > > > > > the hardware can recognize it as such). Maybe we'd be better off just
> > > > > > having userspace going by contact size for pens. Peter, any suggestions
> > > > > > here?
> > > > > 
> > > > > I don't think we have size handling in the tablet implementation in
> > > > > libinput. I do not see it as a big issue to add such axes from a
> > > > > libinput point of view. However, there is no existing hardware that
> > > > > would provide such information, so I guess this will be a 'no' until
> > > > > actual hardware comes in.
> > > 
> > > correct on all counts :)
> > > 
> > > 
> > > > > Also note that the MT_TOOL_PEN implementation is limited (even
> > > > > non-existant if I remember correctly). Peter and I do not have access
> > > > > to any device that support such multi pen, so AFAICT, there is no code
> > > > > to handle this in libinput.
> > > 
> > > Yep, correct. On this note: libinput very much follows a "no hardware, no
> > > implementation" rule. I played the game of trying to support everything in a
> > > generic manner with the X drivers and it's a nightmare to maintain. libinput
> > > instead takes a use case and tries to make it sensible - but for that to
> > > work we need to know both the hardware and the use-cases. That's why tablet
> > > handling coming out of libinput is very different to the handling we have in
> > > X but, afaict, everyone is better off for it so far.
> > > 
> > > This means that if you give me a MT_TOOL_FINGER → MT_TOOL_PEN transition,
> > > I'll handle it, but only after you also give me the use-case for it and the
> > > promise of real devices that need it. 
> > > 
> > > > > One last point from libinput, the pen device would need to be on its
> > > > > separate kernel node for the protocol to be smoothly handled. So
> > > > > basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
> > > > > would not be handled properly right now. The Pen event will be treated
> > > > > as a touch.
> > > > 
> > > > I think normally pen and touch a separate controllers, so we have that
> > > > going for us...
> > > 
> > > Side-effect of this is: the tablet interface doesn't handle touch at all
> > > because it didn't need to yet. So while technically possible, it requires a
> > > fair bit of re-arranging.
> > 
> > What about things like Bamboo touch? It is a Wacom tablet with both
> > multitouch finger and stylus.
> 
> these are on two different event nodes though, isn't it? If not, then no-one
> has tested them with libinput so far...

I'll have to plug it in and see... It was a while since I used it.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Benjamin Tissoires]

On Tue, Jun 5, 2018 at 12:55 AM, Peter Hutterer
<peter.hutterer@who-t.net> wrote:
> On Mon, Jun 04, 2018 at 02:19:44PM -0700, Dmitry Torokhov wrote:
>> On Mon, Jun 04, 2018 at 10:42:31PM +0200, Benjamin Tissoires wrote:
>> > On Mon, Jun 4, 2018 at 7:33 PM, Dmitry Torokhov
>> > <dmitry.torokhov@gmail.com> wrote:
>> > > On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
>> > >> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
>> > >> <dmitry.torokhov@gmail.com> wrote:
>> > >> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
>> > >> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
>> > >> >> <dmitry.torokhov@gmail.com> wrote:
>> > >> >> > According to Microsoft specification [1] for Precision Touchpads (and
>> > >> >> > Touchscreens) the devices use "confidence" reports to signal accidental
>> > >> >> > touches, or contacts that are "too large to be a finger". Instead of
>> > >> >> > simply marking contact inactive in this case (which causes issues if
>> > >> >> > contact was originally proper and we lost confidence in it later, as
>> > >> >> > this results in accidental clicks, drags, etc), let's report such
>> > >> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
>> > >> >> > Additionally, let's report contact size for such touches as maximum
>> > >> >> > allowed for major/minor, which should help userspace that is not yet
>> > >> >> > aware of MT_TOOL_PALM to still perform palm rejection.
>> > >> >> >
>> > >> >> > An additional complication, is that some firmwares do not report
>> > >> >> > non-confident touches as active. To cope with this we delay release of
>> > >> >> > such contact (i.e. if contact was active we first report it as still
>> > >> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
>> > >> >> > frame).
>> > >> >>
>> > >> >> I am not sure I agree with this part. The spec says that "Once a
>> > >> >> device has determined that a contact is unintentional, it should clear
>> > >> >> the confidence bit for that contact report and all subsequent
>> > >> >> reports."
>> > >> >> So in theory the spec says that if a touch has been detected as a
>> > >> >> palm, the flow of events should not stop (tested on the PTP of the
>> > >> >> Dell XPS 9360).
>> > >> >>
>> > >> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
>> > >> >> and then (confidence 0, tip switch 0) a simple release, and the
>> > >> >> confidence bit should not be relayed.
>> > >> >
>> > >> > This unfortunately leads to false clicks: you start with finger, so
>> > >> > confidence is 1, then you transition the same touch to palm (use your
>> > >> > thumb and "roll" your hand until heel of it comes into contact with the
>> > >> > screen). The firmware reports "no-confidence" and "release" in the same
>> > >> > report and userspace seeing release does not pay attention to confidence
>> > >> > (i.e. it does exactly "simple release" logic) and this results in UI
>> > >> > interpreting this as a click. With splitting no-confidence
>> > >> > (MT_TOOL_PALM) and release event into separate frames we help userspace
>> > >> > to recognize that the contact should be discarded.
>> > >>
>> > >> After further thoughts, I would consider this to be a firmware bug,
>> > >> and not how the firmware is supposed to be reporting palm.
>> > >> For the precision touchpads, the spec says that the device "should
>> > >> clear the confidence bit for that contact report and all subsequent
>> > >> reports.". And it is how the Dell device I have here reports palms.
>> > >> The firmware is not supposed to cut the event stream.
>> > >>
>> > >> There is a test for that:
>> > >> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
>> > >> which tells me that I am right here for PTP.
>> > >>
>> > >> The touchscreen spec is blurrier however.
>> > >
>> > > OK, that is great to know.
>> > >
>> > >>
>> > >> >
>> > >> >>
>> > >> >> Do you have any precise example of reports where you need that feature?
>> > >> >
>> > >> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
>> > >>
>> > >> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
>> > >> guessing the device did not went through Microsoft certification
>> > >> process?
>> > >
>> > > That would be correct ;) At least the firmware that is shipping with
>> > > Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
>> > > their MSWin devices.
>> > >
>> > >>
>> > >> I am in favor of splitting the patch in 2. One for the generic
>> > >> processing of confidence bit, and one for this spurious release. For
>> > >> the spurious release, I'm more in favor of explicitly quirking the
>> > >> devices in need of such quirk.
>> > >
>> > > Hmm, I am not sure about having specific quirk. It will be hard for
>> > > users to accurately diagnose the issue if firmware is broken in this way
>> > > so we could add a new quirk for a new device.
>> >
>> > One thing we can do is keep the quirked mechanism as default in
>> > hid-multitouch, but remove it in hid-core. If people need the quirk,
>> > they can just use hid-multitouch instead (talking about the long run
>> > here).
>>
>> Hmm, I am confused. My patch did not touch hid-core or hid-input, only
>> hid-multitouch... So we are already doing what you are proposing?..
>>
>> >
>> > However, I really believe this might only be required for a handful of
>> > devices, and probably only touchscreens. So I would be tempted to not
>> > make it default and see how many bug reports we have.
>>
>> Up to you but it is hard to detect for users. If just sometimes there
>> are stray clicks...
>
> fwiw, from my POV, if you give me MT_TOOL_PALM in the same frame as the
> ABS_MT_TRACKING_ID -1 I can work that into libinput to do the right thing.

This would be a one line change in the kernel, so you got my attention :)

> Not 100% whether that already works anyway but probably not. I'd prefer it
> being fixed in the kernel though, less work for me :)

What do you mean by "fixed"?
Is it incorrect to send a tool while tracking ID is set to -1?
>From what I read on multi-touch-protocol.rst this shouldn't be
violating the protocol, and this would save quite a mess in the kernel
in which we need to add an artificial event in the queue for the
release.

Cheers,
Benjamin

Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches

[Dmitry Torokhov]

On Tue, Jun 05, 2018 at 03:50:15PM +0200, Benjamin Tissoires wrote:
> On Tue, Jun 5, 2018 at 12:55 AM, Peter Hutterer
> <peter.hutterer@who-t.net> wrote:
> > On Mon, Jun 04, 2018 at 02:19:44PM -0700, Dmitry Torokhov wrote:
> >> On Mon, Jun 04, 2018 at 10:42:31PM +0200, Benjamin Tissoires wrote:
> >> > On Mon, Jun 4, 2018 at 7:33 PM, Dmitry Torokhov
> >> > <dmitry.torokhov@gmail.com> wrote:
> >> > > On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
> >> > >> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
> >> > >> <dmitry.torokhov@gmail.com> wrote:
> >> > >> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
> >> > >> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
> >> > >> >> <dmitry.torokhov@gmail.com> wrote:
> >> > >> >> > According to Microsoft specification [1] for Precision Touchpads (and
> >> > >> >> > Touchscreens) the devices use "confidence" reports to signal accidental
> >> > >> >> > touches, or contacts that are "too large to be a finger". Instead of
> >> > >> >> > simply marking contact inactive in this case (which causes issues if
> >> > >> >> > contact was originally proper and we lost confidence in it later, as
> >> > >> >> > this results in accidental clicks, drags, etc), let's report such
> >> > >> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
> >> > >> >> > Additionally, let's report contact size for such touches as maximum
> >> > >> >> > allowed for major/minor, which should help userspace that is not yet
> >> > >> >> > aware of MT_TOOL_PALM to still perform palm rejection.
> >> > >> >> >
> >> > >> >> > An additional complication, is that some firmwares do not report
> >> > >> >> > non-confident touches as active. To cope with this we delay release of
> >> > >> >> > such contact (i.e. if contact was active we first report it as still
> >> > >> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
> >> > >> >> > frame).
> >> > >> >>
> >> > >> >> I am not sure I agree with this part. The spec says that "Once a
> >> > >> >> device has determined that a contact is unintentional, it should clear
> >> > >> >> the confidence bit for that contact report and all subsequent
> >> > >> >> reports."
> >> > >> >> So in theory the spec says that if a touch has been detected as a
> >> > >> >> palm, the flow of events should not stop (tested on the PTP of the
> >> > >> >> Dell XPS 9360).
> >> > >> >>
> >> > >> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
> >> > >> >> and then (confidence 0, tip switch 0) a simple release, and the
> >> > >> >> confidence bit should not be relayed.
> >> > >> >
> >> > >> > This unfortunately leads to false clicks: you start with finger, so
> >> > >> > confidence is 1, then you transition the same touch to palm (use your
> >> > >> > thumb and "roll" your hand until heel of it comes into contact with the
> >> > >> > screen). The firmware reports "no-confidence" and "release" in the same
> >> > >> > report and userspace seeing release does not pay attention to confidence
> >> > >> > (i.e. it does exactly "simple release" logic) and this results in UI
> >> > >> > interpreting this as a click. With splitting no-confidence
> >> > >> > (MT_TOOL_PALM) and release event into separate frames we help userspace
> >> > >> > to recognize that the contact should be discarded.
> >> > >>
> >> > >> After further thoughts, I would consider this to be a firmware bug,
> >> > >> and not how the firmware is supposed to be reporting palm.
> >> > >> For the precision touchpads, the spec says that the device "should
> >> > >> clear the confidence bit for that contact report and all subsequent
> >> > >> reports.". And it is how the Dell device I have here reports palms.
> >> > >> The firmware is not supposed to cut the event stream.
> >> > >>
> >> > >> There is a test for that:
> >> > >> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
> >> > >> which tells me that I am right here for PTP.
> >> > >>
> >> > >> The touchscreen spec is blurrier however.
> >> > >
> >> > > OK, that is great to know.
> >> > >
> >> > >>
> >> > >> >
> >> > >> >>
> >> > >> >> Do you have any precise example of reports where you need that feature?
> >> > >> >
> >> > >> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
> >> > >>
> >> > >> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
> >> > >> guessing the device did not went through Microsoft certification
> >> > >> process?
> >> > >
> >> > > That would be correct ;) At least the firmware that is shipping with
> >> > > Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
> >> > > their MSWin devices.
> >> > >
> >> > >>
> >> > >> I am in favor of splitting the patch in 2. One for the generic
> >> > >> processing of confidence bit, and one for this spurious release. For
> >> > >> the spurious release, I'm more in favor of explicitly quirking the
> >> > >> devices in need of such quirk.
> >> > >
> >> > > Hmm, I am not sure about having specific quirk. It will be hard for
> >> > > users to accurately diagnose the issue if firmware is broken in this way
> >> > > so we could add a new quirk for a new device.
> >> >
> >> > One thing we can do is keep the quirked mechanism as default in
> >> > hid-multitouch, but remove it in hid-core. If people need the quirk,
> >> > they can just use hid-multitouch instead (talking about the long run
> >> > here).
> >>
> >> Hmm, I am confused. My patch did not touch hid-core or hid-input, only
> >> hid-multitouch... So we are already doing what you are proposing?..
> >>
> >> >
> >> > However, I really believe this might only be required for a handful of
> >> > devices, and probably only touchscreens. So I would be tempted to not
> >> > make it default and see how many bug reports we have.
> >>
> >> Up to you but it is hard to detect for users. If just sometimes there
> >> are stray clicks...
> >
> > fwiw, from my POV, if you give me MT_TOOL_PALM in the same frame as the
> > ABS_MT_TRACKING_ID -1 I can work that into libinput to do the right thing.
> 
> This would be a one line change in the kernel, so you got my attention :)

Umm, there are other input stacks beyond libinput.

> 
> > Not 100% whether that already works anyway but probably not. I'd prefer it
> > being fixed in the kernel though, less work for me :)
> 
> What do you mean by "fixed"?
> Is it incorrect to send a tool while tracking ID is set to -1?
> From what I read on multi-touch-protocol.rst this shouldn't be
> violating the protocol, and this would save quite a mess in the kernel
> in which we need to add an artificial event in the queue for the
> release.

Well, we say "A non-negative tracking id is interpreted as a contact,
and the value -1 denotes an unused slot." Unless you are a protocol
lawyer, the most sensible way of interpreting it is to ignore whatever
is transmitted for the slot once receiving tracking ID of -1.

Given that this is particular firmware quirk that sends confidence and
release in the same report, I'd prefer if we had a quirk in driver
rather than pushing the responsibility to userspace.

Thanks.

-- 
Dmitry