[PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

The input-mt driver pointer emulation from 'input_mt_sync_frame' regardless
of the flags passed in to 'input_mt_init_slots' by device drivers.

Pointer emulation is undesired on drivers, which didn't request this
capability like the hid-sony driver for the Dualshock 4. This gamepad already
reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
inject touchpad values into these sticks, which is undesired.

This patch checks the flags INPUT_MT_POINTER / INPUT_MT_DIRECT from within
input_mt_sync_frame to only allow pointer emulation when the feature was
requested by the driver as the flags were set in input_mt_init_slots.
---
 drivers/input/input-mt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index a1bbec9..30c8128 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -305,7 +305,8 @@ void input_mt_sync_frame(struct input_dev *dev)
 	if ((mt->flags & INPUT_MT_POINTER) && !(mt->flags & INPUT_MT_SEMI_MT))
 		use_count = true;
 
-	input_mt_report_pointer_emulation(dev, use_count);
+	if (mt->flags & (INPUT_MT_POINTER | INPUT_MT_DIRECT))
+		input_mt_report_pointer_emulation(dev, use_count);
 
 	mt->frame++;
 }
-- 
2.7.4

RE: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Bird, Timothy]

> -----Original Message-----
> From: Roderick Colenbrander  on Thursday, October 27, 2016 2:31 PM


> The input-mt driver pointer emulation from 'input_mt_sync_frame'
> regardless
> of the flags passed in to 'input_mt_init_slots' by device drivers.

There seems to be a verb missing from this sentence.
 
> Pointer emulation is undesired on drivers, which didn't request this
> capability like the hid-sony driver for the Dualshock 4. This gamepad already
> reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
> inject touchpad values into these sticks, which is undesired.
> 
> This patch checks the flags INPUT_MT_POINTER / INPUT_MT_DIRECT from
> within
> input_mt_sync_frame to only allow pointer emulation when the feature was
> requested by the driver as the flags were set in input_mt_init_slots.
> ---
>  drivers/input/input-mt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index a1bbec9..30c8128 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -305,7 +305,8 @@ void input_mt_sync_frame(struct input_dev *dev)
>  	if ((mt->flags & INPUT_MT_POINTER) && !(mt->flags &
> INPUT_MT_SEMI_MT))
>  		use_count = true;
> 
> -	input_mt_report_pointer_emulation(dev, use_count);
> +	if (mt->flags & (INPUT_MT_POINTER | INPUT_MT_DIRECT))
> +		input_mt_report_pointer_emulation(dev, use_count);
> 
>  	mt->frame++;
>  }
> --
> 2.7.4
> 

Re: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Henrik Rydberg]

Hi Roderick,

> Pointer emulation is undesired on drivers, which didn't request this
> capability like the hid-sony driver for the Dualshock 4. This gamepad already
> reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
> inject touchpad values into these sticks, which is undesired.
> 
> This patch checks the flags INPUT_MT_POINTER / INPUT_MT_DIRECT from within
> input_mt_sync_frame to only allow pointer emulation when the feature was
> requested by the driver as the flags were set in input_mt_init_slots.

So why call input_mt_sync_frame() in the first place?

The existing drivers that use ABS_X / ABS_Y independently do not call that
function. It seems your recent hid-sony patches adds it, which leads to broken
behavior.

> ---
>  drivers/input/input-mt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index a1bbec9..30c8128 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -305,7 +305,8 @@ void input_mt_sync_frame(struct input_dev *dev)
>  	if ((mt->flags & INPUT_MT_POINTER) && !(mt->flags & INPUT_MT_SEMI_MT))
>  		use_count = true;
>  
> -	input_mt_report_pointer_emulation(dev, use_count);
> +	if (mt->flags & (INPUT_MT_POINTER | INPUT_MT_DIRECT))
> +		input_mt_report_pointer_emulation(dev, use_count);
>  
>  	mt->frame++;
>  }
> 

That said, I am not against this patch at all, I think it is good, but the
commit message should address the effect on the large number of existing drivers
that use this functionality. Is any of them affected by this patch? As you
probably know, we do not like to break userspace.

>From what I can see, only hid-multitouch (Benjamin?) and hid-sony use
input_mt_init_slots() in such a way that MT_POINTER or MT_DIRECT cannot be
directly inferred. So if Benjamin sees no issues with this, I am ok with the change.

Thanks,
Henrik

Re: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Dmitry Torokhov]

On Thu, Oct 27, 2016 at 02:31:13PM -0700, Roderick Colenbrander wrote:
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> The input-mt driver pointer emulation from 'input_mt_sync_frame' regardless
> of the flags passed in to 'input_mt_init_slots' by device drivers.

Right, because needing single-touch (pointer) emulation is not property
of device or driver but rather consumer. If we get to the point where
everything is ready to accept multi-touch (are we there yet) then we can
stop doing pointer emulation altogether.

> 
> Pointer emulation is undesired on drivers, which didn't request this
> capability like the hid-sony driver for the Dualshock 4. This gamepad already
> reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
> inject touchpad values into these sticks, which is undesired.

The driver should not be re-purposing events like that.
ABS_MT_POSITION_X and ABS_X should match.

Why doesn't driver follow Documentation/input/gamepad.txt?

> 
> This patch checks the flags INPUT_MT_POINTER / INPUT_MT_DIRECT from within
> input_mt_sync_frame to only allow pointer emulation when the feature was
> requested by the driver as the flags were set in input_mt_init_slots.
> ---
>  drivers/input/input-mt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index a1bbec9..30c8128 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -305,7 +305,8 @@ void input_mt_sync_frame(struct input_dev *dev)
>  	if ((mt->flags & INPUT_MT_POINTER) && !(mt->flags & INPUT_MT_SEMI_MT))
>  		use_count = true;
>  
> -	input_mt_report_pointer_emulation(dev, use_count);
> +	if (mt->flags & (INPUT_MT_POINTER | INPUT_MT_DIRECT))
> +		input_mt_report_pointer_emulation(dev, use_count);
>  
>  	mt->frame++;
>  }
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry

Re: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Roderick Colenbrander]

Hi Henrik,

Please see my comments inline.

Thanks,
Roderick

On Thu, Oct 27, 2016 at 3:46 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> Hi Roderick,
>
>> Pointer emulation is undesired on drivers, which didn't request this
>> capability like the hid-sony driver for the Dualshock 4. This gamepad already
>> reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
>> inject touchpad values into these sticks, which is undesired.
>>
>> This patch checks the flags INPUT_MT_POINTER / INPUT_MT_DIRECT from within
>> input_mt_sync_frame to only allow pointer emulation when the feature was
>> requested by the driver as the flags were set in input_mt_init_slots.
>
> So why call input_mt_sync_frame() in the first place?
>
> The existing drivers that use ABS_X / ABS_Y independently do not call that
> function. It seems your recent hid-sony patches adds it, which leads to broken
> behavior.

As part of the review, it was recommended to call input_mt_sync_frame
to make sure we don't lose certain events.
Later on after more validation, we noticed this issue which we weren't aware of.

>> ---
>>  drivers/input/input-mt.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
>> index a1bbec9..30c8128 100644
>> --- a/drivers/input/input-mt.c
>> +++ b/drivers/input/input-mt.c
>> @@ -305,7 +305,8 @@ void input_mt_sync_frame(struct input_dev *dev)
>>       if ((mt->flags & INPUT_MT_POINTER) && !(mt->flags & INPUT_MT_SEMI_MT))
>>               use_count = true;
>>
>> -     input_mt_report_pointer_emulation(dev, use_count);
>> +     if (mt->flags & (INPUT_MT_POINTER | INPUT_MT_DIRECT))
>> +             input_mt_report_pointer_emulation(dev, use_count);
>>
>>       mt->frame++;
>>  }
>>
>
> That said, I am not against this patch at all, I think it is good, but the
> commit message should address the effect on the large number of existing drivers
> that use this functionality. Is any of them affected by this patch? As you
> probably know, we do not like to break userspace.
>
> From what I can see, only hid-multitouch (Benjamin?) and hid-sony use
> input_mt_init_slots() in such a way that MT_POINTER or MT_DIRECT cannot be
> directly inferred. So if Benjamin sees no issues with this, I am ok with the change.
>
> Thanks,
> Henrik
>

Re: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Roderick Colenbrander]

On Thu, Oct 27, 2016 at 4:19 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Oct 27, 2016 at 02:31:13PM -0700, Roderick Colenbrander wrote:
>> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>>
>> The input-mt driver pointer emulation from 'input_mt_sync_frame' regardless
>> of the flags passed in to 'input_mt_init_slots' by device drivers.
>
> Right, because needing single-touch (pointer) emulation is not property
> of device or driver but rather consumer. If we get to the point where
> everything is ready to accept multi-touch (are we there yet) then we can
> stop doing pointer emulation altogether.
>
>>
>> Pointer emulation is undesired on drivers, which didn't request this
>> capability like the hid-sony driver for the Dualshock 4. This gamepad already
>> reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
>> inject touchpad values into these sticks, which is undesired.
>
> The driver should not be re-purposing events like that.
> ABS_MT_POSITION_X and ABS_X should match.
>
> Why doesn't driver follow Documentation/input/gamepad.txt?
>

The original hid-sony code and the multitouch handling code for the Dualshock 4
were written by the community. We are stepping in to make the hid-sony driver
properly handle the Dualshock 4.
The driver doesn't fully follow the gamepad spec yet, which we will
try to improve. At
least the left analog stick is reported as ABS_X and ABS_Y already,
but the buttons
are inconsistent and right stick is wrong as well.
While it would be great to conform to the spec, the conflict with
multitouch would still
exist as the Dualshock 4 has both analog sticks and a multitouch touchpad.

Disabling the pointer emulation feature avoids the conflict. This
issue was only exposed
recently by adding of the input_mt_sync_frame call as recommended by Benjamin as
part of some improvements we did for touchpad handling in hid-sony.

>>
>> This patch checks the flags INPUT_MT_POINTER / INPUT_MT_DIRECT from within
>> input_mt_sync_frame to only allow pointer emulation when the feature was
>> requested by the driver as the flags were set in input_mt_init_slots.
>> ---
>>  drivers/input/input-mt.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
>> index a1bbec9..30c8128 100644
>> --- a/drivers/input/input-mt.c
>> +++ b/drivers/input/input-mt.c
>> @@ -305,7 +305,8 @@ void input_mt_sync_frame(struct input_dev *dev)
>>       if ((mt->flags & INPUT_MT_POINTER) && !(mt->flags & INPUT_MT_SEMI_MT))
>>               use_count = true;
>>
>> -     input_mt_report_pointer_emulation(dev, use_count);
>> +     if (mt->flags & (INPUT_MT_POINTER | INPUT_MT_DIRECT))
>> +             input_mt_report_pointer_emulation(dev, use_count);
>>
>>       mt->frame++;
>>  }
>> --
>> 2.7.4
>>
>
> Thanks.
>
> --
> Dmitry

Thanks,

-- 
Roderick Colenbrander
Senior Manager of Software Engineering
Gaikai, a Sony Interactive Entertainment Company
roderick@gaikai.com

Re: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Dmitry Torokhov]

On Thu, Oct 27, 2016 at 04:46:01PM -0700, Roderick Colenbrander wrote:
> On Thu, Oct 27, 2016 at 4:19 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Thu, Oct 27, 2016 at 02:31:13PM -0700, Roderick Colenbrander wrote:
> >> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> >>
> >> The input-mt driver pointer emulation from 'input_mt_sync_frame' regardless
> >> of the flags passed in to 'input_mt_init_slots' by device drivers.
> >
> > Right, because needing single-touch (pointer) emulation is not property
> > of device or driver but rather consumer. If we get to the point where
> > everything is ready to accept multi-touch (are we there yet) then we can
> > stop doing pointer emulation altogether.
> >
> >>
> >> Pointer emulation is undesired on drivers, which didn't request this
> >> capability like the hid-sony driver for the Dualshock 4. This gamepad already
> >> reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
> >> inject touchpad values into these sticks, which is undesired.
> >
> > The driver should not be re-purposing events like that.
> > ABS_MT_POSITION_X and ABS_X should match.
> >
> > Why doesn't driver follow Documentation/input/gamepad.txt?
> >
> 
> The original hid-sony code and the multitouch handling code for the Dualshock 4
> were written by the community. We are stepping in to make the hid-sony driver
> properly handle the Dualshock 4.
> The driver doesn't fully follow the gamepad spec yet, which we will
> try to improve. At
> least the left analog stick is reported as ABS_X and ABS_Y already,
> but the buttons
> are inconsistent and right stick is wrong as well.
> While it would be great to conform to the spec, the conflict with
> multitouch would still
> exist as the Dualshock 4 has both analog sticks and a multitouch touchpad.
> 
> Disabling the pointer emulation feature avoids the conflict. This
> issue was only exposed
> recently by adding of the input_mt_sync_frame call as recommended by Benjamin as
> part of some improvements we did for touchpad handling in hid-sony.

As I mentioned, ABS_MT_POSITION_X and ABS_X should represent the same
data, so the driver's use of them for different sub-devices is not
correct. Maybe the multitouch "touchpad" should be reported as
completely separate input device and userspace should "stitch" it back
into composite device.

In any case, as I mentioned above, the pointer emulation is requirement
of client and thus this patch is not appropriate.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Roderick Colenbrander]

On Thu, Oct 27, 2016 at 4:54 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Oct 27, 2016 at 04:46:01PM -0700, Roderick Colenbrander wrote:
>> On Thu, Oct 27, 2016 at 4:19 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Thu, Oct 27, 2016 at 02:31:13PM -0700, Roderick Colenbrander wrote:
>> >> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>> >>
>> >> The input-mt driver pointer emulation from 'input_mt_sync_frame' regardless
>> >> of the flags passed in to 'input_mt_init_slots' by device drivers.
>> >
>> > Right, because needing single-touch (pointer) emulation is not property
>> > of device or driver but rather consumer. If we get to the point where
>> > everything is ready to accept multi-touch (are we there yet) then we can
>> > stop doing pointer emulation altogether.
>> >
>> >>
>> >> Pointer emulation is undesired on drivers, which didn't request this
>> >> capability like the hid-sony driver for the Dualshock 4. This gamepad already
>> >> reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
>> >> inject touchpad values into these sticks, which is undesired.
>> >
>> > The driver should not be re-purposing events like that.
>> > ABS_MT_POSITION_X and ABS_X should match.
>> >
>> > Why doesn't driver follow Documentation/input/gamepad.txt?
>> >
>>
>> The original hid-sony code and the multitouch handling code for the Dualshock 4
>> were written by the community. We are stepping in to make the hid-sony driver
>> properly handle the Dualshock 4.
>> The driver doesn't fully follow the gamepad spec yet, which we will
>> try to improve. At
>> least the left analog stick is reported as ABS_X and ABS_Y already,
>> but the buttons
>> are inconsistent and right stick is wrong as well.
>> While it would be great to conform to the spec, the conflict with
>> multitouch would still
>> exist as the Dualshock 4 has both analog sticks and a multitouch touchpad.
>>
>> Disabling the pointer emulation feature avoids the conflict. This
>> issue was only exposed
>> recently by adding of the input_mt_sync_frame call as recommended by Benjamin as
>> part of some improvements we did for touchpad handling in hid-sony.
>
> As I mentioned, ABS_MT_POSITION_X and ABS_X should represent the same
> data, so the driver's use of them for different sub-devices is not
> correct. Maybe the multitouch "touchpad" should be reported as
> completely separate input device and userspace should "stitch" it back
> into composite device.
>
> In any case, as I mentioned above, the pointer emulation is requirement
> of client and thus this patch is not appropriate.
>
> Thanks.
>
> --
> Dmitry

Thanks for your reply. I guess the only option then is to create an
actual sub device. I suppose
we are not the first input driver exposing sub devices. Are there good
example drivers you can
recommend (or drivers to avoid glancing at)?

The only concern I have is the stitching together in user space, which
not kinds of platforms
or userland libraries are handling. The only way is to inspect sysfs
and try to tie things together.
It would be nice if there was some better way. From memory Video4Linux
exposed various
APIs to locate subdevices from the 'master' device.

Thanks,
Roderick

Re: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Frank Praznik]

> On Thu, Oct 27, 2016 at 4:19 PM, Dmitry Torokhov
> <dmitry.torokhov@xxxxxxxxx> wrote:
> > On Thu, Oct 27, 2016 at 02:31:13PM -0700, Roderick Colenbrander wrote:
> >> From: Roderick Colenbrander <roderick.colenbrander@xxxxxxxx>
> >>
> >> The input-mt driver pointer emulation from 'input_mt_sync_frame' regardless
> >> of the flags passed in to 'input_mt_init_slots' by device drivers.
> >
> > Right, because needing single-touch (pointer) emulation is not property
> > of device or driver but rather consumer. If we get to the point where
> > everything is ready to accept multi-touch (are we there yet) then we can
> > stop doing pointer emulation altogether.
> >
> >>
> >> Pointer emulation is undesired on drivers, which didn't request this
> >> capability like the hid-sony driver for the Dualshock 4. This gamepad already
> >> reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
> >> inject touchpad values into these sticks, which is undesired.
> >
> > The driver should not be re-purposing events like that.
> > ABS_MT_POSITION_X and ABS_X should match.
> >
> > Why doesn't driver follow Documentation/input/gamepad.txt?
> >
> 
> The original hid-sony code and the multitouch handling code for the Dualshock 4
> were written by the community. We are stepping in to make the hid-sony driver
> properly handle the Dualshock 4.
> The driver doesn't fully follow the gamepad spec yet, which we will
> try to improve. At
> least the left analog stick is reported as ABS_X and ABS_Y already,
> but the buttons
> are inconsistent and right stick is wrong as well.
> While it would be great to conform to the spec, the conflict with
> multitouch would still
> exist as the Dualshock 4 has both analog sticks and a multitouch touchpad.

The button and axis mappings are what the controller’s default HID descriptor provides.  The descriptors in the driver just extend the ones sent by the controller to allow additional functionality(i.e. gyro/accel and full Bluetooth mode), but the button and axis mappings were unchanged from the defaults so as not to introduce mapping discrepencies with older kernels which used the generic HID driver.  Note that changing them at this point *will* break a lot of software (SDL has had these mappings hardcoded into it’s controller database header for years now).

> 
> Disabling the pointer emulation feature avoids the conflict. This
> issue was only exposed
> recently by adding of the input_mt_sync_frame call as recommended by Benjamin as
> part of some improvements we did for touchpad handling in hid-sony.
> 

Is input_mt_sync_frame even necessary?  The original implementation followed the protocol for a “Type B” device as defined in Documentation/input/multi-touch-protocol.txt which doesn’t require SYN_MT_REPORT messages.

RE: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Colenbrander, Roelof]

_______________________________________
From: Frank Praznik [frank.praznik@gmail.com]
Sent: Sunday, October 30, 2016 8:04 AM
To: linux-input@vger.kernel.org
Cc: Dmitry Torokhov; Henrik Rydberg; Benjamin Tissoires; Bird, Timothy; Colenbrander, Roelof
Subject: Re: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

>> On Thu, Oct 27, 2016 at 4:19 PM, Dmitry Torokhov
>> <dmitry.torokhov@xxxxxxxxx> wrote:
>> > On Thu, Oct 27, 2016 at 02:31:13PM -0700, Roderick Colenbrander wrote:
>> >> From: Roderick Colenbrander <roderick.colenbrander@xxxxxxxx>
>> >>
>> >> The input-mt driver pointer emulation from 'input_mt_sync_frame' regardless
>> >> of the flags passed in to 'input_mt_init_slots' by device drivers.
>> >
>> > Right, because needing single-touch (pointer) emulation is not property
>> > of device or driver but rather consumer. If we get to the point where
>> > everything is ready to accept multi-touch (are we there yet) then we can
>> > stop doing pointer emulation altogether.
>> >
>> >>
>> >> Pointer emulation is undesired on drivers, which didn't request this
>> >> capability like the hid-sony driver for the Dualshock 4. This gamepad already
>> >> reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
>> >> inject touchpad values into these sticks, which is undesired.
>> >
>> > The driver should not be re-purposing events like that.
>> > ABS_MT_POSITION_X and ABS_X should match.
>> >
>> > Why doesn't driver follow Documentation/input/gamepad.txt?
>> >
>>
>> The original hid-sony code and the multitouch handling code for the Dualshock 4
>> were written by the community. We are stepping in to make the hid-sony driver
>> properly handle the Dualshock 4.
>> The driver doesn't fully follow the gamepad spec yet, which we will
>> try to improve. At
>> least the left analog stick is reported as ABS_X and ABS_Y already,
>> but the buttons
>> are inconsistent and right stick is wrong as well.
>> While it would be great to conform to the spec, the conflict with
>> multitouch would still
>> exist as the Dualshock 4 has both analog sticks and a multitouch touchpad.

> The button and axis mappings are what the controller’s default HID descriptor provides.  The descriptors in the driver just extend the ones sent by the controller to allow additional functionality(i.e. gyro/accel and full Bluetooth mode), but the button and axis mappings were unchanged from the defaults so as not to introduce mapping discrepencies with older kernels which used the generic HID driver.  Note that changing them at this point *will* break a lot of software (SDL has had these mappings hardcoded into it’s controller database header for years now).

Ideally I would like to start following the spec (the current mapping is so awful), but SDL is the issue. Its database can be updated as the table uses a guid composed of bus_type | vendor_id | product_id | version. I don't know if it is allowed, but the version field could be utilized to negotiate a different button mapping... We wouldn't mind sending such patches.

>>>
>>> Disabling the pointer emulation feature avoids the conflict. This
>>> issue was only exposed
>>> recently by adding of the input_mt_sync_frame call as recommended by Benjamin as
>>> part of some improvements we did for touchpad handling in hid-sony.
>>>

> Is input_mt_sync_frame even necessary?  The original implementation followed the protocol for a “Type B” device as defined in Documentation/input/multi-touch-protocol.txt which doesn’t require SYN_MT_REPORT messages.

The call was added on recommendation from Benjamin Tissoires when we added support for touch history support. He said that if we didn't add this call the values would gt mangled by the kernel in evdev and data would get lost. For us the history was important for gesture reasons. 

Re: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality

[Benjamin Tissoires]

On Oct 30 2016 or thereabouts, Colenbrander, Roelof wrote:
> _______________________________________
> From: Frank Praznik [frank.praznik@gmail.com]
> Sent: Sunday, October 30, 2016 8:04 AM
> To: linux-input@vger.kernel.org
> Cc: Dmitry Torokhov; Henrik Rydberg; Benjamin Tissoires; Bird, Timothy; Colenbrander, Roelof
> Subject: Re: [PATCH] Input: mt: only perform pointer emulation on drivers desiring this functionality
> 
> >> On Thu, Oct 27, 2016 at 4:19 PM, Dmitry Torokhov
> >> <dmitry.torokhov@xxxxxxxxx> wrote:
> >> > On Thu, Oct 27, 2016 at 02:31:13PM -0700, Roderick Colenbrander wrote:
> >> >> From: Roderick Colenbrander <roderick.colenbrander@xxxxxxxx>
> >> >>
> >> >> The input-mt driver pointer emulation from 'input_mt_sync_frame' regardless
> >> >> of the flags passed in to 'input_mt_init_slots' by device drivers.
> >> >
> >> > Right, because needing single-touch (pointer) emulation is not property
> >> > of device or driver but rather consumer. If we get to the point where
> >> > everything is ready to accept multi-touch (are we there yet) then we can
> >> > stop doing pointer emulation altogether.
> >> >
> >> >>
> >> >> Pointer emulation is undesired on drivers, which didn't request this
> >> >> capability like the hid-sony driver for the Dualshock 4. This gamepad already
> >> >> reports ABS_X / ABS_Y for gamepad stick purposes. Pointer emulation would
> >> >> inject touchpad values into these sticks, which is undesired.
> >> >
> >> > The driver should not be re-purposing events like that.
> >> > ABS_MT_POSITION_X and ABS_X should match.
> >> >
> >> > Why doesn't driver follow Documentation/input/gamepad.txt?
> >> >
> >>
> >> The original hid-sony code and the multitouch handling code for the Dualshock 4
> >> were written by the community. We are stepping in to make the hid-sony driver
> >> properly handle the Dualshock 4.
> >> The driver doesn't fully follow the gamepad spec yet, which we will
> >> try to improve. At
> >> least the left analog stick is reported as ABS_X and ABS_Y already,
> >> but the buttons
> >> are inconsistent and right stick is wrong as well.
> >> While it would be great to conform to the spec, the conflict with
> >> multitouch would still
> >> exist as the Dualshock 4 has both analog sticks and a multitouch touchpad.
> 
> > The button and axis mappings are what the controller’s default HID descriptor provides.  The descriptors in the driver just extend the ones sent by the controller to allow additional functionality(i.e. gyro/accel and full Bluetooth mode), but the button and axis mappings were unchanged from the defaults so as not to introduce mapping discrepencies with older kernels which used the generic HID driver.  Note that changing them at this point *will* break a lot of software (SDL has had these mappings hardcoded into it’s controller database header for years now).
> 
> Ideally I would like to start following the spec (the current mapping is so awful), but SDL is the issue. Its database can be updated as the table uses a guid composed of bus_type | vendor_id | product_id | version. I don't know if it is allowed, but the version field could be utilized to negotiate a different button mapping... We wouldn't mind sending such patches.

The version is retrieved from the HID descriptor. So technically,
changing it has the risk of introducing a conflict with future devices.
The version field in input is a uint16, while the one in HID is a
uint32. usbhid retrieves a uint16 from the descriptor, so it's fine.
However, if you were to change the version, you should make sure your
company will not reuse the same version number (I guess doing "0x8000 |
version" might prevent future clashes if the version are incremented by
one).

> 
> >>>
> >>> Disabling the pointer emulation feature avoids the conflict. This
> >>> issue was only exposed
> >>> recently by adding of the input_mt_sync_frame call as recommended by Benjamin as
> >>> part of some improvements we did for touchpad handling in hid-sony.
> >>>
> 
> > Is input_mt_sync_frame even necessary?  The original implementation followed the protocol for a “Type B” device as defined in Documentation/input/multi-touch-protocol.txt which doesn’t require SYN_MT_REPORT messages.

input_mt_sync_frame doesn't add the SNY_MT_REPORT events, it introduces
the pointer emulation. And yes, I think I might just have screwed here
by recommending to use input_mt_sync_frame() because the call which is
actually needed is input_sync().

> 
> The call was added on recommendation from Benjamin Tissoires when we added support for touch history support. He said that if we didn't add this call the values would gt mangled by the kernel in evdev and data would get lost. For us the history was important for gesture reasons.

As I said, I screwed this. You should use input_sync(). However, as
Dmitry said, you need to also have ABS_X|Y in sync with
ABS_MT_POSITION_X|Y. So the split of the touchpad from the game
controller is the best move.

You can find an example of this in the hid tree in wacom_sys.c function
wacom_allocate_input(), or in hid-alps.c for a slightly less complex
driver :) (look for input_allocate_device).

Cheers,
Benjamin