[PATCH 0/3] HID: wacom: Fix generic multitouch device handling

[PATCH 0/3] HID: wacom: Fix generic multitouch device handling

[Jason Gerecke]

This set of patches addresses the problems that I had getting Benjamin's
"HID generic handling" patches. Each of the patches addresses a seprate
issue I encountered:

 * Patch 1/3: The devices that I'm using have some usages outside of
   a logical or physical collection container which prevents the
   reports from being sent. I'm not sure if keying on the application
   collection is ideal, but it does do the job.

 * Patch 2/3: The devices that I'm using put their X and Y usages after
   the finger count, which prevents the (compatibility) ABS_X and
   ABS_Y axes from being initialized. This patch just guarantees that
   they'll be initialized.

 * Patch 3/3: Benjamin's original code seemed to only report data about
   the last finger in a multitouch report since information about "earlier"
   fingers would be overwritten. This patch changes the logic so that
   we immediately report field values whenever we can (there are some
   cases that we can't -- the tip/range bits appear before the contact
   identifier, so we have to store their state for a short time).

[PATCH 1/3] HID: wacom: Consult the application usage when determining field type

[Jason Gerecke]

It is not necessarily sufficient to look only at the physical and logical
usages when determining if a field is for the pen or touch. Some fields
are not contained in a sub-collection and thus only have an application
usage. Not checking the application usage in such cases causes us to
ignore the field entirely, which may lead to incorrect behavior.

Signed-off-by: Jason Gerecke <killertofu@gmail.com>
---
 drivers/hid/wacom_wac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 9565d31..1468f00 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1484,9 +1484,11 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
 }
 
 #define WACOM_PEN_FIELD(f)	(((f)->logical == HID_DG_STYLUS) || \
-				 ((f)->physical == HID_DG_STYLUS))
+				 ((f)->physical == HID_DG_STYLUS) || \
+				 ((f)->application == HID_DG_PEN))
 #define WACOM_FINGER_FIELD(f)	(((f)->logical == HID_DG_FINGER) || \
-				 ((f)->physical == HID_DG_FINGER))
+				 ((f)->physical == HID_DG_FINGER) || \
+				 ((f)->application == HID_DG_TOUCHSCREEN))
 
 void wacom_wac_usage_mapping(struct hid_device *hdev,
 		struct hid_field *field, struct hid_usage *usage)
-- 
2.1.3

[PATCH 2/3] HID: wacom: Manually declare ABS_{X,Y} for pointer emulation

[Jason Gerecke]

If a HID descriptor places HID_DG_CONTACTID before HID_DG_X and HID_DG_Y then
the ABS_X and ABS_Y will not be automatically initialized by the call to
input_mt_init_slots. Here we move the setup of those axes outside of the
'if' statement so that they're always present if the associated HID field
is.

Signed-off-by: Jason Gerecke <killertofu@gmail.com>
---
 drivers/hid/wacom_wac.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 1468f00..a55e0ed 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1382,16 +1382,14 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
 
 	switch (usage->hid) {
 	case HID_GD_X:
-		if (touch_max == 1)
-			wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
-		else
+		wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
+		if (touch_max > 1)
 			wacom_map_usage(wacom, usage, field, EV_ABS,
 					ABS_MT_POSITION_X, 4);
 		break;
 	case HID_GD_Y:
-		if (touch_max == 1)
-			wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
-		else
+		wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
+		if (touch_max > 1)
 			wacom_map_usage(wacom, usage, field, EV_ABS,
 					ABS_MT_POSITION_Y, 4);
 		break;
-- 
2.1.3

[PATCH 3/3] HID: wacom: Report input events immediately upon receipt

[Jason Gerecke]

Multitouch tablets cannot work properly if wacom_wac_finger_event simply
stores the event details, since details about former fingers will be
overwritten by later ones (causing wacom_wac_finger_report to effectively
only report the state of the *last* finger in the packet).

This patch modifies the logic so that events are generated as soon as
possible in response to events. We do temporarily store HID_DG_TIPSWITCH
value since the value of HID_DG_CONTACTID is (at least in for the tablets
I've tested) not known until shortly afterwards.

Signed-off-by: Jason Gerecke <killertofu@gmail.com>
---
 drivers/hid/wacom_wac.c | 77 ++++++++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 33 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index a55e0ed..3eaa98a 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1412,19 +1412,40 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct hid_data *data = &wacom_wac->hid_data;
+	unsigned mt = wacom_wac->features.touch_max > 1;
+	struct input_dev *input = wacom_wac->input;
+	bool prox = data->tipswitch && !wacom_wac->shared->stylus_in_proximity;
+	int slot;
 
 	switch (usage->hid) {
 	case HID_GD_X:
-		wacom_wac->hid_data.x = value;
+		if (prox) {
+			input_report_abs(input,
+					 mt ? ABS_MT_POSITION_X : ABS_X,
+					 value);
+		}
 		break;
 	case HID_GD_Y:
-		wacom_wac->hid_data.y = value;
+		if (prox) {
+			input_report_abs(input,
+					 mt ? ABS_MT_POSITION_Y : ABS_Y,
+					 value);
+		}
 		break;
 	case HID_DG_CONTACTID:
-		wacom_wac->hid_data.id = value;
+		slot = input_mt_get_slot_by_key(input, value);
+
+		input_mt_slot(input, slot);
+		input_mt_report_slot_state(input, MT_TOOL_FINGER, prox);
 		break;
 	case HID_DG_TIPSWITCH:
-		wacom_wac->hid_data.tipswitch = value;
+		data->tipswitch = value;
+		if (!mt) {
+			prox = data->tipswitch &&
+			      !wacom_wac->shared->stylus_in_proximity;
+			input_report_key(input, BTN_TOUCH, prox);
+		}
 		break;
 	}
 
@@ -1432,33 +1453,26 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
 	return 0;
 }
 
-static void wacom_wac_finger_mt_report(struct wacom_wac *wacom_wac,
-		struct input_dev *input, bool touch)
+static int wacom_wac_finger_touches(struct hid_device *hdev)
 {
-	int slot;
-	struct hid_data *hid_data = &wacom_wac->hid_data;
-
-	slot = input_mt_get_slot_by_key(input, hid_data->id);
-
-	input_mt_slot(input, slot);
-	input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
-	if (touch) {
-		input_report_abs(input, ABS_MT_POSITION_X, hid_data->x);
-		input_report_abs(input, ABS_MT_POSITION_Y, hid_data->y);
-	}
-	input_mt_sync_frame(input);
-}
+	struct wacom *wacom = hid_get_drvdata(hdev);
+	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct input_dev *input = wacom_wac->input;
+	unsigned touch_max = wacom_wac->features.touch_max;
+	int count = 0;
+	int i;
 
-static void wacom_wac_finger_single_touch_report(struct wacom_wac *wacom_wac,
-		struct input_dev *input, bool touch)
-{
-	struct hid_data *hid_data = &wacom_wac->hid_data;
+	if (touch_max == 1)
+		return (wacom_wac->hid_data.tipswitch &&
+		       !wacom_wac->shared->stylus_in_proximity);
 
-	if (touch) {
-		input_report_abs(input, ABS_X, hid_data->x);
-		input_report_abs(input, ABS_Y, hid_data->y);
+	for (i = 0; i < input->mt->num_slots; i++) {
+		struct input_mt_slot *ps = &input->mt->slots[i];
+		int id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
+		if (id >= 0)
+			count++;
 	}
-	input_report_key(input, BTN_TOUCH, touch);
+	return count;
 }
 
 static void wacom_wac_finger_report(struct hid_device *hdev,
@@ -1467,18 +1481,15 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
 	struct input_dev *input = wacom_wac->input;
-	bool touch = wacom_wac->hid_data.tipswitch &&
-		     !wacom_wac->shared->stylus_in_proximity;
 	unsigned touch_max = wacom_wac->features.touch_max;
 
 	if (touch_max > 1)
-		wacom_wac_finger_mt_report(wacom_wac, input, touch);
-	else
-		wacom_wac_finger_single_touch_report(wacom_wac, input, touch);
+		input_mt_sync_frame(input);
+
 	input_sync(input);
 
 	/* keep touch state for pen event */
-	wacom_wac->shared->touch_down = touch;
+	wacom_wac->shared->touch_down = wacom_wac_finger_touches(hdev);
 }
 
 #define WACOM_PEN_FIELD(f)	(((f)->logical == HID_DG_STYLUS) || \
-- 
2.1.3

Re: [PATCH 1/3] HID: wacom: Consult the application usage when determining field type

[Benjamin Tissoires]

On Mon, Nov 24, 2014 at 6:32 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> It is not necessarily sufficient to look only at the physical and logical
> usages when determining if a field is for the pen or touch. Some fields
> are not contained in a sub-collection and thus only have an application
> usage. Not checking the application usage in such cases causes us to
> ignore the field entirely, which may lead to incorrect behavior.
>
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---

This one is Rev-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  drivers/hid/wacom_wac.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 9565d31..1468f00 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1484,9 +1484,11 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
>  }
>
>  #define WACOM_PEN_FIELD(f)     (((f)->logical == HID_DG_STYLUS) || \
> -                                ((f)->physical == HID_DG_STYLUS))
> +                                ((f)->physical == HID_DG_STYLUS) || \
> +                                ((f)->application == HID_DG_PEN))
>  #define WACOM_FINGER_FIELD(f)  (((f)->logical == HID_DG_FINGER) || \
> -                                ((f)->physical == HID_DG_FINGER))
> +                                ((f)->physical == HID_DG_FINGER) || \
> +                                ((f)->application == HID_DG_TOUCHSCREEN))
>
>  void wacom_wac_usage_mapping(struct hid_device *hdev,
>                 struct hid_field *field, struct hid_usage *usage)
> --
> 2.1.3
>

Re: [PATCH 2/3] HID: wacom: Manually declare ABS_{X,Y} for pointer emulation

[Benjamin Tissoires]

On Mon, Nov 24, 2014 at 6:32 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> If a HID descriptor places HID_DG_CONTACTID before HID_DG_X and HID_DG_Y then
> the ABS_X and ABS_Y will not be automatically initialized by the call to
> input_mt_init_slots. Here we move the setup of those axes outside of the
> 'if' statement so that they're always present if the associated HID field
> is.

No, I am afraid, that this does not work. The solution is to call
input_mt_init_slot once the mapping has been done.
I am complaining here because input_mt_init_slot will set the fuzz to
0 for ABS_X and 4 for ABS_MT_POSITION_X. If both have a fuzz of 4,
there will be a double de-fuzzing while sending out the single touch
emulation.
Likewise, the pressure should be initialized with ABS_MT_PRESSURE only
(not implemented right now, I know).

At the end of wacom_parse_hid in wacom_sys, there should be a call to
a function which would finalize the initialization. And then,
input_mt_init_slot will be called after ABS_MT_*, no matter what the
order of the fields is.

Cheers,
Benjamin

>
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---
>  drivers/hid/wacom_wac.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 1468f00..a55e0ed 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1382,16 +1382,14 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
>
>         switch (usage->hid) {
>         case HID_GD_X:
> -               if (touch_max == 1)
> -                       wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
> -               else
> +               wacom_map_usage(wacom, usage, field, EV_ABS, ABS_X, 4);
> +               if (touch_max > 1)
>                         wacom_map_usage(wacom, usage, field, EV_ABS,
>                                         ABS_MT_POSITION_X, 4);
>                 break;
>         case HID_GD_Y:
> -               if (touch_max == 1)
> -                       wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
> -               else
> +               wacom_map_usage(wacom, usage, field, EV_ABS, ABS_Y, 4);
> +               if (touch_max > 1)
>                         wacom_map_usage(wacom, usage, field, EV_ABS,
>                                         ABS_MT_POSITION_Y, 4);
>                 break;
> --
> 2.1.3
>

Re: [PATCH 3/3] HID: wacom: Report input events immediately upon receipt

[Benjamin Tissoires]

Hi Jason,

On Mon, Nov 24, 2014 at 6:32 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> Multitouch tablets cannot work properly if wacom_wac_finger_event simply
> stores the event details, since details about former fingers will be
> overwritten by later ones (causing wacom_wac_finger_report to effectively
> only report the state of the *last* finger in the packet).
>
> This patch modifies the logic so that events are generated as soon as
> possible in response to events. We do temporarily store HID_DG_TIPSWITCH
> value since the value of HID_DG_CONTACTID is (at least in for the tablets
> I've tested) not known until shortly afterwards.
>

OK, so, I don't like this one either :-(

I perfectly understand that the previous code worked in the only case
where there is only one contact per report. But this patch assumes
that the contact ID will come right after the tip switch if I
understood correctly. This assumption may strike back in the future
(like mine did), so we should be smarter.

In hid-multitouch, we count the total number of mt fields, then divide
by the number of time we see ContactID. This gives us the number of
fields per touch, and we then have a hook called when we reach this
number of fields while processing the report.
It's not perfect, but it's generic.

If you have a better idea, I am all ears, but unless the FW guys can
give us some guarantees regarding the HID descriptor*, I think we
should go for the most generic possible solution.

BTW, what I dislike here is that I am pretty sure that if we take this
one, we will have to revert part of it to have a 2-steps processing.
Relying on the order of the hid fields is simply not possible in the
long term.

Cheers,
Benjamin

* even if they give some guarantees, I am not sure I will trust them :)

> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---
>  drivers/hid/wacom_wac.c | 77 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index a55e0ed..3eaa98a 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1412,19 +1412,40 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
>  {
>         struct wacom *wacom = hid_get_drvdata(hdev);
>         struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> +       struct hid_data *data = &wacom_wac->hid_data;
> +       unsigned mt = wacom_wac->features.touch_max > 1;
> +       struct input_dev *input = wacom_wac->input;
> +       bool prox = data->tipswitch && !wacom_wac->shared->stylus_in_proximity;
> +       int slot;
>
>         switch (usage->hid) {
>         case HID_GD_X:
> -               wacom_wac->hid_data.x = value;
> +               if (prox) {
> +                       input_report_abs(input,
> +                                        mt ? ABS_MT_POSITION_X : ABS_X,
> +                                        value);
> +               }
>                 break;
>         case HID_GD_Y:
> -               wacom_wac->hid_data.y = value;
> +               if (prox) {
> +                       input_report_abs(input,
> +                                        mt ? ABS_MT_POSITION_Y : ABS_Y,
> +                                        value);
> +               }
>                 break;
>         case HID_DG_CONTACTID:
> -               wacom_wac->hid_data.id = value;
> +               slot = input_mt_get_slot_by_key(input, value);
> +
> +               input_mt_slot(input, slot);
> +               input_mt_report_slot_state(input, MT_TOOL_FINGER, prox);
>                 break;
>         case HID_DG_TIPSWITCH:
> -               wacom_wac->hid_data.tipswitch = value;
> +               data->tipswitch = value;
> +               if (!mt) {
> +                       prox = data->tipswitch &&
> +                             !wacom_wac->shared->stylus_in_proximity;
> +                       input_report_key(input, BTN_TOUCH, prox);
> +               }
>                 break;
>         }
>
> @@ -1432,33 +1453,26 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
>         return 0;
>  }
>
> -static void wacom_wac_finger_mt_report(struct wacom_wac *wacom_wac,
> -               struct input_dev *input, bool touch)
> +static int wacom_wac_finger_touches(struct hid_device *hdev)
>  {
> -       int slot;
> -       struct hid_data *hid_data = &wacom_wac->hid_data;
> -
> -       slot = input_mt_get_slot_by_key(input, hid_data->id);
> -
> -       input_mt_slot(input, slot);
> -       input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
> -       if (touch) {
> -               input_report_abs(input, ABS_MT_POSITION_X, hid_data->x);
> -               input_report_abs(input, ABS_MT_POSITION_Y, hid_data->y);
> -       }
> -       input_mt_sync_frame(input);
> -}
> +       struct wacom *wacom = hid_get_drvdata(hdev);
> +       struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> +       struct input_dev *input = wacom_wac->input;
> +       unsigned touch_max = wacom_wac->features.touch_max;
> +       int count = 0;
> +       int i;
>
> -static void wacom_wac_finger_single_touch_report(struct wacom_wac *wacom_wac,
> -               struct input_dev *input, bool touch)
> -{
> -       struct hid_data *hid_data = &wacom_wac->hid_data;
> +       if (touch_max == 1)
> +               return (wacom_wac->hid_data.tipswitch &&
> +                      !wacom_wac->shared->stylus_in_proximity);
>
> -       if (touch) {
> -               input_report_abs(input, ABS_X, hid_data->x);
> -               input_report_abs(input, ABS_Y, hid_data->y);
> +       for (i = 0; i < input->mt->num_slots; i++) {
> +               struct input_mt_slot *ps = &input->mt->slots[i];
> +               int id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
> +               if (id >= 0)
> +                       count++;
>         }
> -       input_report_key(input, BTN_TOUCH, touch);
> +       return count;
>  }
>
>  static void wacom_wac_finger_report(struct hid_device *hdev,
> @@ -1467,18 +1481,15 @@ static void wacom_wac_finger_report(struct hid_device *hdev,
>         struct wacom *wacom = hid_get_drvdata(hdev);
>         struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>         struct input_dev *input = wacom_wac->input;
> -       bool touch = wacom_wac->hid_data.tipswitch &&
> -                    !wacom_wac->shared->stylus_in_proximity;
>         unsigned touch_max = wacom_wac->features.touch_max;
>
>         if (touch_max > 1)
> -               wacom_wac_finger_mt_report(wacom_wac, input, touch);
> -       else
> -               wacom_wac_finger_single_touch_report(wacom_wac, input, touch);
> +               input_mt_sync_frame(input);
> +
>         input_sync(input);
>
>         /* keep touch state for pen event */
> -       wacom_wac->shared->touch_down = touch;
> +       wacom_wac->shared->touch_down = wacom_wac_finger_touches(hdev);
>  }
>
>  #define WACOM_PEN_FIELD(f)     (((f)->logical == HID_DG_STYLUS) || \
> --
> 2.1.3
>

Re: [PATCH 1/3] HID: wacom: Consult the application usage when determining field type

[Jiri Kosina]

On Mon, 24 Nov 2014, Jason Gerecke wrote:

> It is not necessarily sufficient to look only at the physical and logical
> usages when determining if a field is for the pen or touch. Some fields
> are not contained in a sub-collection and thus only have an application
> usage. Not checking the application usage in such cases causes us to
> ignore the field entirely, which may lead to incorrect behavior.
> 
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>

Applied.

-- 
Jiri Kosina
SUSE Labs