[PATCH 0/3] Fixes for multitouch / generic gamepads interaction

[PATCH 0/3] Fixes for multitouch / generic gamepads interaction

[Benjamin Tissoires]

Hi guys,

I have been reported recently a problem with the Sixaxis controller
(http://www.spinics.net/lists/linux-input/msg28098.html).
The root of the problem comes from hid-input, which maps unknown axis to
ABS_MISC. However, when an event code is already in use, hid-input uses the one
after, leading to uses of ABS_MISC + N, where N is the number of unknown axis.

We are encountering a problem with the multitouch protocol here because if a
device has more than 7 unknown axis (which is the case for the PS3 Sixaxis
controller), then the unknown axis get maps to ABS_MT_SLOT and beyond.

This infers two problems:
- the axis currently mapped on ABS_MT_SLOT is a special case in the kernel,
and it is not updated
- the axis after ABS_MT_SLOT are not filtered anymore, as the kernel things
the device is using a multitouch protocol A.

The patch 0001 solves the first problem, whereas the patches 0002 and 0003
fix the second. Bonus point: the userspace is now correctly notified that one of
the multitouch protocols is in used, so it does not have to rely on bad designed
heuristics.

Cheers,
Benjamin

Antonio Ospite (1):
  Only process ABS_MT_SLOT where there are slots available

Benjamin Tissoires (2):
  input: Introduce INPUT_PROP_MT
  input: reintroduce filtering of mt axis when the device is not
    multitouch

 Documentation/input/event-codes.txt          | 7 +++++++
 Documentation/input/multi-touch-protocol.txt | 6 ++++++
 drivers/hid/hid-ntrig.c                      | 1 +
 drivers/input/input-mt.c                     | 2 ++
 drivers/input/input.c                        | 7 ++++---
 drivers/input/touchscreen/auo-pixcir-ts.c    | 1 +
 drivers/input/touchscreen/bu21013_ts.c       | 1 +
 drivers/input/touchscreen/pixcir_i2c_ts.c    | 1 +
 drivers/input/touchscreen/st1232.c           | 1 +
 include/uapi/linux/input.h                   | 1 +
 10 files changed, 25 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] Only process ABS_MT_SLOT where there are slots available

[Benjamin Tissoires]

From: Antonio Ospite <ospite@studenti.unina.it>

This fixes the case when a non-multitouch device happens to have a HID
code equal to ABS_MT_SLOT, like the Sony Sixaxis has for the left dpad
analog control.

Updated to latest tree by Benjamin Tissoires.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

This patch was sent back in 2011 by Antonio and it was forgotten in the depth
of the LKML:
http://www.spinics.net/lists/linux-input/msg16881.html
Resurecting it now, as the bug is still there.

Antonio, could you please tell me if you still add your Signed-of-by line and
if I can keep your from?

Cheers,
Benjamin
drivers/input/input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index fb513da..1198785 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -213,12 +213,12 @@ static int input_handle_abs_event(struct input_dev *dev,
 	bool is_mt_event;
 	int *pold;
 
-	if (code == ABS_MT_SLOT) {
+	if (code == ABS_MT_SLOT && mt) {
 		/*
 		 * "Stage" the event; we'll flush it later, when we
 		 * get actual touch data.
 		 */
-		if (mt && *pval >= 0 && *pval < mt->num_slots)
+		if (*pval >= 0 && *pval < mt->num_slots)
 			mt->slot = *pval;
 
 		return INPUT_IGNORE_EVENT;
-- 
1.8.3.1

Re: [PATCH 1/3] Only process ABS_MT_SLOT where there are slots available

[simon]

> From: Antonio Ospite <ospite@studenti.unina.it>
>
> This fixes the case when a non-multitouch device happens to have a HID
> code equal to ABS_MT_SLOT, like the Sony Sixaxis has for the left dpad
> analog control.
>
> Updated to latest tree by Benjamin Tissoires.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Wow, its been a great week for the DualShock - got rumble and LED control,
and now I can confirm that the 'left D pad' analogue axis works.

Thanks guys,
Simon

tested-by: Simon Wood <simon@mungewell.org>

Re: [PATCH 1/3] Only process ABS_MT_SLOT where there are slots available

[Dmitry Torokhov]

Hi Benjamin, Antonio,

On Wed, Nov 20, 2013 at 04:32:19PM -0500, Benjamin Tissoires wrote:
> From: Antonio Ospite <ospite@studenti.unina.it>
> 
> This fixes the case when a non-multitouch device happens to have a HID
> code equal to ABS_MT_SLOT, like the Sony Sixaxis has for the left dpad
> analog control.
> 
> Updated to latest tree by Benjamin Tissoires.

I do not think this is a proper way to address the issue. Generic HID
driver should not encroach onto multitouch ABS range and either stop
mapping absolute axis or map them properly.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] Only process ABS_MT_SLOT where there are slots available

[Antonio Ospite]

On Wed, 20 Nov 2013 20:56:21 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Hi Benjamin, Antonio,
> 
> On Wed, Nov 20, 2013 at 04:32:19PM -0500, Benjamin Tissoires wrote:
> > From: Antonio Ospite <ospite@studenti.unina.it>
> > 
> > This fixes the case when a non-multitouch device happens to have a HID
> > code equal to ABS_MT_SLOT, like the Sony Sixaxis has for the left dpad
> > analog control.
> > 
> > Updated to latest tree by Benjamin Tissoires.
> 
> I do not think this is a proper way to address the issue. Generic HID
> driver should not encroach onto multitouch ABS range and either stop
> mapping absolute axis or map them properly.
> 
> Thanks.

Dmitry I'd agree with you in principle, and I trusted your judgment on
that in 2011 too when I first came up with this "hack", and in fact I
didn't insist to have it merged, but then I failed to properly address
the issue and users were left with a non functional device for a long
time.

Now I've got some more experience, I know how to do it, and if you
decide to drop Benjamin's _defensive_ solution I will be more than
happy to remap the axes of the joypad to fix the issue the way you like
it, the way _we_ like it.

But there were some concerns about backward compatibility:
http://www.spinics.net/lists/linux-input/msg28280.html and following.

My position is egoistical here:

  - if you will accept a patch which remaps the keys to match the
    gamepad API, even breaking the current interface I'd say we go for
    that (the current keymap is not fully working anyway because of
    the ABS_MT issue).

  - if really "we do not break user space" no matter what, then
    Benjamin's patch is OK by me.

I think I see what your fear is here: if there is such defensive
fallback mechanism in place there will be no motivation to enforce
proper fixes by driver authors at the HID level, am I right?

So Benjamin, an alternative could be to spit out warnings when a non MT
device uses MT codes and curse at lazy developers like me in the logs.
What about that?

Ciao,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 1/3] Only process ABS_MT_SLOT where there are slots available

[Benjamin Tissoires]

On 20/11/13 23:56, Dmitry Torokhov wrote:
> Hi Benjamin, Antonio,
> 
> On Wed, Nov 20, 2013 at 04:32:19PM -0500, Benjamin Tissoires wrote:
>> From: Antonio Ospite <ospite@studenti.unina.it>
>>
>> This fixes the case when a non-multitouch device happens to have a HID
>> code equal to ABS_MT_SLOT, like the Sony Sixaxis has for the left dpad
>> analog control.
>>
>> Updated to latest tree by Benjamin Tissoires.
> 
> I do not think this is a proper way to address the issue. Generic HID
> driver should not encroach onto multitouch ABS range and either stop
> mapping absolute axis or map them properly.
> 

Ok, I'm a little bit lost here. Back in May, you told us not to change
the previous mapping for legacy devices:
http://www.spinics.net/lists/linux-input/msg25651.html

And the concern I have is the history made all this things a mess:
- the hid-input.c bad mapping (line 918) is there since at least 2006
(2.6.20) -> dde5845a529ff753364a6d1aea61180946270bfa
- the PS3 Sixasis has been introduced in 2008-10-14 (2.6.28) ->
bd28ce008bdc68ef5902f68d2d62cbb7fa78c415
- the mt protocol A has been committed in 2010-04-28 (2.6.30) ->
5e5ee686e3c0f8a3cbe9b75c2690326bf91af10d
- the mt protocol B has been committed in 2010-07-15 (2.6.36) ->
40d007e7df1dab17bf1ecf91e718218354d963d7

So basically, the PS3 controller existed before we changed the semantic
of its axis. We already had users at that time, and we missed the
overlapping when we introduced the mt protocol.
Then the second mt protocol broke even more the PS3 controller.

What should be fixed now? Because I am sure that there may be other
existing controllers, which were produced and used before 2010 that have
more than 7 unmapped axis.

Cheers,
Benjamin

[PATCH 2/3] input: Introduce INPUT_PROP_MT

[Benjamin Tissoires]

There may be a clash with devices presenting a lot of absolute axis
(like the PS3 Sixaxis) and true multitouch devices. Both those kinds
of devices may present some ABS_MT_* capabilities, so setting this
property ensures user-space knows what to do with the input device.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 Documentation/input/event-codes.txt          | 7 +++++++
 Documentation/input/multi-touch-protocol.txt | 5 +++++
 drivers/hid/hid-ntrig.c                      | 1 +
 drivers/input/input-mt.c                     | 2 ++
 drivers/input/touchscreen/auo-pixcir-ts.c    | 1 +
 drivers/input/touchscreen/bu21013_ts.c       | 1 +
 drivers/input/touchscreen/pixcir_i2c_ts.c    | 1 +
 drivers/input/touchscreen/st1232.c           | 1 +
 include/uapi/linux/input.h                   | 1 +
 9 files changed, 20 insertions(+)

diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
index f1ea2c6..b54feb3 100644
--- a/Documentation/input/event-codes.txt
+++ b/Documentation/input/event-codes.txt
@@ -281,6 +281,13 @@ gestures can normally be extracted from it.
 If INPUT_PROP_SEMI_MT is not set, the device is assumed to be a true MT
 device.
 
+INPUT_PROP_MT:
+-------------
+There may be a clash with devices presenting a lot of absolute axis (like the
+PS3 Sixaxis) and true multitouch devices. Both those kinds of devices may
+present some ABS_MT_* capabilities, so setting this property ensures user-space
+knows what to do with the input device.
+
 Guidelines:
 ==========
 The guidelines below ensure proper single-touch and multi-finger functionality.
diff --git a/Documentation/input/multi-touch-protocol.txt b/Documentation/input/multi-touch-protocol.txt
index de139b1..420444f 100644
--- a/Documentation/input/multi-touch-protocol.txt
+++ b/Documentation/input/multi-touch-protocol.txt
@@ -54,6 +54,11 @@ enumeration of the full set of anonymous contacts currently on the
 surface. The order in which the packets appear in the event stream is not
 important.  Event filtering and finger tracking is left to user space [3].
 
+Note that type A device are requested to manually set the property
+INPUT_PROP_MT.
+Type B devices does not need to set INPUT_PROP_MT manually, as this is done
+by calling input_mt_init_slot().
+
 For type B devices, the kernel driver should associate a slot with each
 identified contact, and use that slot to propagate changes for the contact.
 Creation, replacement and destruction of contacts is achieved by modifying
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 600f207..6dedc64 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -879,6 +879,7 @@ static void ntrig_input_configured(struct hid_device *hid,
 		__clear_bit(BTN_TOOL_FINGER, input->keybit);
 		__clear_bit(BTN_0, input->keybit);
 		__set_bit(BTN_TOOL_DOUBLETAP, input->keybit);
+		__set_bit(INPUT_PROP_MT, input->propbit);
 		/*
 		 * The physical touchscreen (single touch)
 		 * input has a value for physical, whereas
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index d398f13..f9b70e7 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -58,6 +58,8 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
 	input_set_abs_params(dev, ABS_MT_SLOT, 0, num_slots - 1, 0, 0);
 	input_set_abs_params(dev, ABS_MT_TRACKING_ID, 0, TRKID_MAX, 0, 0);
 
+	__set_bit(INPUT_PROP_MT, dev->propbit);
+
 	if (flags & (INPUT_MT_POINTER | INPUT_MT_DIRECT)) {
 		__set_bit(EV_KEY, dev->evbit);
 		__set_bit(BTN_TOUCH, dev->keybit);
diff --git a/drivers/input/touchscreen/auo-pixcir-ts.c b/drivers/input/touchscreen/auo-pixcir-ts.c
index d3f9f6b..70836cd 100644
--- a/drivers/input/touchscreen/auo-pixcir-ts.c
+++ b/drivers/input/touchscreen/auo-pixcir-ts.c
@@ -594,6 +594,7 @@ static int auo_pixcir_probe(struct i2c_client *client,
 	input_set_abs_params(input_dev, ABS_Y, 0, pdata->y_max, 0, 0);
 
 	/* For multi touch */
+	__set_bit(INPUT_PROP_MT, input_dev->propbit);
 	input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0,
 			     pdata->x_max, 0, 0);
 	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0,
diff --git a/drivers/input/touchscreen/bu21013_ts.c b/drivers/input/touchscreen/bu21013_ts.c
index b9b5dda..4ce6220 100644
--- a/drivers/input/touchscreen/bu21013_ts.c
+++ b/drivers/input/touchscreen/bu21013_ts.c
@@ -573,6 +573,7 @@ static int bu21013_probe(struct i2c_client *client,
 	__set_bit(EV_SYN, in_dev->evbit);
 	__set_bit(EV_KEY, in_dev->evbit);
 	__set_bit(EV_ABS, in_dev->evbit);
+	__set_bit(INPUT_PROP_MT, in_dev->propbit);
 
 	input_set_abs_params(in_dev, ABS_MT_POSITION_X, 0,
 						pdata->touch_x_max, 0, 0);
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 6cc6b36..72154e4 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -157,6 +157,7 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
 	__set_bit(EV_KEY, input->evbit);
 	__set_bit(EV_ABS, input->evbit);
 	__set_bit(BTN_TOUCH, input->keybit);
+	__set_bit(INPUT_PROP_MT, input->propbit);
 	input_set_abs_params(input, ABS_X, 0, pdata->x_max, 0, 0);
 	input_set_abs_params(input, ABS_Y, 0, pdata->y_max, 0, 0);
 	input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 2f03b2f..2f17045 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -205,6 +205,7 @@ static int st1232_ts_probe(struct i2c_client *client,
 	__set_bit(EV_SYN, input_dev->evbit);
 	__set_bit(EV_KEY, input_dev->evbit);
 	__set_bit(EV_ABS, input_dev->evbit);
+	__set_bit(INPUT_PROP_MT, input_dev->propbit);
 
 	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0);
 	input_set_abs_params(input_dev, ABS_MT_POSITION_X, MIN_X, MAX_X, 0, 0);
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index a372627..b469cae 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -164,6 +164,7 @@ struct input_keymap_entry {
 #define INPUT_PROP_DIRECT		0x01	/* direct input devices */
 #define INPUT_PROP_BUTTONPAD		0x02	/* has button(s) under pad */
 #define INPUT_PROP_SEMI_MT		0x03	/* touch rectangle only */
+#define INPUT_PROP_MT			0x04	/* multitouch devices */
 
 #define INPUT_PROP_MAX			0x1f
 #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
-- 
1.8.3.1

Re: [PATCH 2/3] input: Introduce INPUT_PROP_MT

[Dmitry Torokhov]

On Wed, Nov 20, 2013 at 04:32:20PM -0500, Benjamin Tissoires wrote:
> There may be a clash with devices presenting a lot of absolute axis
> (like the PS3 Sixaxis) and true multitouch devices. Both those kinds
> of devices may present some ABS_MT_* capabilities, so setting this
> property ensures user-space knows what to do with the input device.
> 

No, let's not use the same ABS_* values to mean different things and
define proper values instead.

Thanks.

-- 
Dmitry

[PATCH 3/3] input: reintroduce filtering of mt axis when the device is not multitouch

[Benjamin Tissoires]

Some devices (like the PS3 Sixaxis) have a lot of absolute axis which
are mapped by hid-input on ABS_MISC and beyond. These axis interfere
with the multitouch protocol.
The kernel considers those device to be following the multitouch protocol
A, and removes the filtering of such events.

We can rely on INPUT_PROP_MT to know if the device is multitouch or not,
and then rely on mt to know if the device uses the protocol B.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 Documentation/input/multi-touch-protocol.txt | 3 ++-
 drivers/input/input.c                        | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/input/multi-touch-protocol.txt b/Documentation/input/multi-touch-protocol.txt
index 420444f..b476845 100644
--- a/Documentation/input/multi-touch-protocol.txt
+++ b/Documentation/input/multi-touch-protocol.txt
@@ -55,7 +55,8 @@ surface. The order in which the packets appear in the event stream is not
 important.  Event filtering and finger tracking is left to user space [3].
 
 Note that type A device are requested to manually set the property
-INPUT_PROP_MT.
+INPUT_PROP_MT. If this property is not set, the absolute multitouch axis
+will be statefull, so this step is now mandatory.
 Type B devices does not need to set INPUT_PROP_MT manually, as this is done
 by calling input_mt_init_slot().
 
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 1198785..239639e 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -224,7 +224,8 @@ static int input_handle_abs_event(struct input_dev *dev,
 		return INPUT_IGNORE_EVENT;
 	}
 
-	is_mt_event = input_is_mt_value(code);
+	is_mt_event = test_bit(INPUT_PROP_MT, dev->propbit) &&
+			input_is_mt_value(code);
 
 	if (!is_mt_event) {
 		pold = &dev->absinfo[code].value;
-- 
1.8.3.1

Re: [PATCH 0/3] Fixes for multitouch / generic gamepads interaction

[Henrik Rydberg]

Hi Benjamin,

> I have been reported recently a problem with the Sixaxis controller
> (http://www.spinics.net/lists/linux-input/msg28098.html).
> The root of the problem comes from hid-input, which maps unknown axis to
> ABS_MISC. However, when an event code is already in use, hid-input uses the one
> after, leading to uses of ABS_MISC + N, where N is the number of unknown axis.

Yes, and clearly we should not do that. :-)

> We are encountering a problem with the multitouch protocol here because if a
> device has more than 7 unknown axis (which is the case for the PS3 Sixaxis
> controller), then the unknown axis get maps to ABS_MT_SLOT and beyond.
> 
> This infers two problems:
> - the axis currently mapped on ABS_MT_SLOT is a special case in the kernel,
> and it is not updated
> - the axis after ABS_MT_SLOT are not filtered anymore, as the kernel things
> the device is using a multitouch protocol A.

The problem is generic, and ABS_MT_SLOT just happens to be the first value that
the hid logic hits. When sevenaxis comes out, we have to do this all over again.

> The patch 0001 solves the first problem, whereas the patches 0002 and 0003
> fix the second. Bonus point: the userspace is now correctly notified that one of
> the multitouch protocols is in used, so it does not have to rely on bad designed
> heuristics.

The bad heuristics are lines 918 and onwards in hid-input.c. I am sure we can
come up with a way to allocate those axes in a much less fragile way.

Thanks,
Henrik