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

[Peter Hutterer <peter.hutterer@who-t.net>]

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
>