Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1709143 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-08-11 02:50 +0200
    [PATCH 2/2] HID: multitouch: touchscreens also use confidence reports Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-08-11 02:50 +0200
    Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for  non-confident touches Henrik Rydberg <rydberg@bitmath.org> - 2017-08-11 08:30 +0200
      Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for  non-confident touches Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-08-11 09:00 +0200
        Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for  non-confident touches Henrik Rydberg <rydberg@bitmath.org> - 2017-08-11 10:30 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web