[RFC/PATCH] Input: release MT fingers on device reset/disconnect

[RFC/PATCH] Input: release MT fingers on device reset/disconnect

[Yufeng Shen]

We are already releasing all pressed keys on device reset and
disconnect. This patch adds the finger release for multi-touch
device.

Different than the key release logic that only the pressed keys
are released, here all the possible fingers are released even
it is not recorded as touched down in the input_dev's internal
state. The motivation is that while releasing an alreay released
finger won't do harm to downstream, it is helpful in the case
when the downstream is out of sync with input_dev's internal
state, we are bringing them back to in sync by cleaning up both
input_dev and downstream's finger states.

Signed-off-by: Yufeng Shen <miletus@chromium.org>
---
 drivers/input/input.c |   34 ++++++++++++++++++++++++++++++++--
 1 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 768e46b..ebdc450 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -584,6 +584,34 @@ void input_close_device(struct input_handle *handle)
 EXPORT_SYMBOL(input_close_device);
 
 /*
+ * Simulate finger release events for all possible fingers. It has no harm
+ * to release an alreay released finger, but has the benefit that if the
+ * downstream finger states are out of sync with input_dev's finger states,
+ * we are helping to clean them up.
+ * The function must be called with dev->event_lock held.
+ */
+static void input_dev_release_mt(struct input_dev *dev)
+{
+	int i;
+
+	if (!dev->mt)
+		return;
+
+	if (!is_event_supported(EV_ABS, dev->evbit, EV_MAX) ||
+	    !is_event_supported(ABS_MT_SLOT, dev->absbit, ABS_MAX) ||
+	    !is_event_supported(ABS_MT_TRACKING_ID, dev->absbit, ABS_MAX))
+		return;
+
+	for (i = 0; i < dev->mtsize; i++) {
+		input_mt_set_value(&dev->mt[i], ABS_MT_TRACKING_ID, -1);
+		input_pass_event(dev, EV_ABS, ABS_MT_SLOT, i);
+		input_pass_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
+	}
+
+	input_pass_event(dev, EV_SYN, SYN_REPORT, 1);
+}
+
+/*
  * Simulate keyup events for all keys that are marked as pressed.
  * The function must be called with dev->event_lock held.
  */
@@ -627,6 +655,7 @@ static void input_disconnect_device(struct input_dev *dev)
 	 * reach any handlers.
 	 */
 	input_dev_release_keys(dev);
+	input_dev_release_mt(dev);
 
 	list_for_each_entry(handle, &dev->h_list, d_node)
 		handle->open = 0;
@@ -1569,7 +1598,7 @@ static void input_dev_toggle(struct input_dev *dev, bool activate)
  * @dev: input device whose state needs to be reset
  *
  * This function tries to reset the state of an opened input device and
- * bring internal state and state if the hardware in sync with each other.
+ * bring internal state and state of the hardware in sync with each other.
  * We mark all keys as released, restore LED state, repeat rate, etc.
  */
 void input_reset_device(struct input_dev *dev)
@@ -1581,10 +1610,11 @@ void input_reset_device(struct input_dev *dev)
 
 		/*
 		 * Keys that have been pressed at suspend time are unlikely
-		 * to be still pressed when we resume.
+		 * to be still pressed when we resume. Same logic for MT.
 		 */
 		spin_lock_irq(&dev->event_lock);
 		input_dev_release_keys(dev);
+		input_dev_release_mt(dev);
 		spin_unlock_irq(&dev->event_lock);
 	}
 
-- 
1.7.7.3

Re: [RFC/PATCH] Input: release MT fingers on device reset/disconnect

[Henrik Rydberg]

Hi Yufeng,

> We are already releasing all pressed keys on device reset and
> disconnect. This patch adds the finger release for multi-touch
> device.
> 
> Different than the key release logic that only the pressed keys
> are released, here all the possible fingers are released even
> it is not recorded as touched down in the input_dev's internal
> state. The motivation is that while releasing an alreay released
> finger won't do harm to downstream, it is helpful in the case
> when the downstream is out of sync with input_dev's internal
> state, we are bringing them back to in sync by cleaning up both
> input_dev and downstream's finger states.
> 
> Signed-off-by: Yufeng Shen <miletus@chromium.org>
> ---

To reset the input state centrally upon disconnect makes some sense;
upon suspend/resume, less so. It is really up to the driver to
determine what part of the input state survives a suspend/resume
cycle

>  drivers/input/input.c |   34 ++++++++++++++++++++++++++++++++--
>  1 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 768e46b..ebdc450 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -584,6 +584,34 @@ void input_close_device(struct input_handle *handle)
>  EXPORT_SYMBOL(input_close_device);
>  
>  /*
> + * Simulate finger release events for all possible fingers. It has no harm
> + * to release an alreay released finger, but has the benefit that if the
> + * downstream finger states are out of sync with input_dev's finger states,
> + * we are helping to clean them up.
> + * The function must be called with dev->event_lock held.
> + */
> +static void input_dev_release_mt(struct input_dev *dev)

This should be moved to input-mt.c instead.

> +{
> +	int i;
> +
> +	if (!dev->mt)
> +		return;
> +
> +	if (!is_event_supported(EV_ABS, dev->evbit, EV_MAX) ||
> +	    !is_event_supported(ABS_MT_SLOT, dev->absbit, ABS_MAX) ||
> +	    !is_event_supported(ABS_MT_TRACKING_ID, dev->absbit, ABS_MAX))
> +		return;

No need to check those; implied by mtsize != 0.

> +
> +	for (i = 0; i < dev->mtsize; i++) {
> +		input_mt_set_value(&dev->mt[i], ABS_MT_TRACKING_ID, -1);
> +		input_pass_event(dev, EV_ABS, ABS_MT_SLOT, i);
> +		input_pass_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
> +	}

Please use the proper api here (see any mt driver for hints).

> +
> +	input_pass_event(dev, EV_SYN, SYN_REPORT, 1);

input_sync()

> +}
> +
> +/*
>   * Simulate keyup events for all keys that are marked as pressed.
>   * The function must be called with dev->event_lock held.
>   */
> @@ -627,6 +655,7 @@ static void input_disconnect_device(struct input_dev *dev)
>  	 * reach any handlers.
>  	 */
>  	input_dev_release_keys(dev);
> +	input_dev_release_mt(dev);
>  
>  	list_for_each_entry(handle, &dev->h_list, d_node)
>  		handle->open = 0;
> @@ -1569,7 +1598,7 @@ static void input_dev_toggle(struct input_dev *dev, bool activate)
>   * @dev: input device whose state needs to be reset
>   *
>   * This function tries to reset the state of an opened input device and
> - * bring internal state and state if the hardware in sync with each other.
> + * bring internal state and state of the hardware in sync with each other.
>   * We mark all keys as released, restore LED state, repeat rate, etc.
>   */
>  void input_reset_device(struct input_dev *dev)
> @@ -1581,10 +1610,11 @@ void input_reset_device(struct input_dev *dev)
>  
>  		/*
>  		 * Keys that have been pressed at suspend time are unlikely
> -		 * to be still pressed when we resume.
> +		 * to be still pressed when we resume. Same logic for MT.
>  		 */
>  		spin_lock_irq(&dev->event_lock);
>  		input_dev_release_keys(dev);
> +		input_dev_release_mt(dev);

Since this is called from input_dev_resume(), I am not so sure this is
a good idea. Rather, we should be able to rely on the driver's
suspend/resume implementation here.

>  		spin_unlock_irq(&dev->event_lock);
>  	}
>  
> -- 
> 1.7.7.3
> 

Thanks,
Henrik