Date:   Mon, 26 Sep 2016 12:53:06 +0300
From:   Nikolai Kondrashov <spbnick@...il.com>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Ioan-Adrian Ratiu <adi@...rat.com>,
        Vladislav Naumov <vnaum@...um.com>
Cc:     jikos@...nel.org, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] hid: input: add HID_QUIRK_REUSE_AXES and fix
 dragonrise

Hi Benjamin,

On 09/26/2016 12:29 PM, Benjamin Tissoires wrote:
> Ideally, we need to have Dragon Rise 0x0011 tested too. Nick, would you
> mind checking it if you still have this particular device?

I never had it, but perhaps Vladislav still has some.

Vladislav, would you be able to test a change to the kernel module for your
Dragonrise gamepads?

Please see below for context.

Thank you.

Nick

On 09/26/2016 12:29 PM, Benjamin Tissoires wrote:
> On Sep 26 2016 or thereabouts, Ioan-Adrian Ratiu wrote:
>> Commit 79346d620e9d ("HID: input: force generic axis to be mapped to their
>> user space axis") made mapping generic axes to their userspace equivalents
>> mandatory and some lower end gamepads which were depending on the previous
>> behaviour suffered severe regressions because they were reusing axes and
>> expecting hid-input to multiplex their map to the respective userspace axis
>> by always searching for and using the next available axis.
>
> Yes, I apologies for the breakage and the regression, though I must say
> that for now, only one hardware maker and one device (or range of devices
> from the look of it) has needed to be quirked.
>
>>
>> Now the result is that different device axes appear on a single axis in
>> userspace, which is clearly a regression in the hid-input driver because it
>> needs to continue to handle this hardware as expected before the forcing
>> to provide the same interface to userspace.
>>
>> Since these lower-end gamepads like 0079:0006 are definitely the exception,
>> create a quirk to fix them.
>
> Given that we only have this particular vendor that is an issue, I'd
> rather see the fix in hid-dr.c. The reason being that you actually don't
> need to have a global quirk and this simplifies the path in hid-input.
> Plus for users, they can just upgrade hid-dr without having to recompile
> their kernel when hid-core is not compiled as a module.
>
> The cleanest solution that wouldn't require any quirk in hid-core is to
> simply add an .input_mapping() callback in hid-dr.c.
>
> The code of the callback could be something like (untested):
>
> static int dr_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> 		struct hid_field *field, struct hid_usage *usage,
> 		unsigned long **bit, int *max)
> {
> 	switch (usage->hid) {
> 	/*
> 	 * revert the old hid-input behavior where axes
> 	 * can be randomly assigned when the hid usage is
> 	 * reused.
> 	 */
> 	case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
> 	case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
> 		if (field->flags & HID_MAIN_ITEM_RELATIVE)
> 			map_rel(usage->hid & 0xf);
> 		else
> 			map_abs(usage->hid & 0xf);
> 		return 1;
> 	}
> 	
> 	return 0;
> }
>
> Hopefully, something like this should revert the old behavior for all
> hid-dr touchpads.
>
> Ideally, we need to have Dragon Rise 0x0011 tested too. Nick, would you
> mind checking it if you still have this particular device?
>
> Cheers,
> Benjamin
>
>>
>> Signed-off-by: Ioan-Adrian Ratiu <adi@...rat.com>
>> ---
>>  drivers/hid/hid-dr.c    |  2 ++
>>  drivers/hid/hid-input.c | 16 +++++++++++-----
>>  include/linux/hid.h     |  1 +
>>  3 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
>> index 2523f8a..27fc826 100644
>> --- a/drivers/hid/hid-dr.c
>> +++ b/drivers/hid/hid-dr.c
>> @@ -274,6 +274,8 @@ static int dr_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>  			hid_hw_stop(hdev);
>>  			goto err;
>>  		}
>> +		/* has only 5 axes and reuses X, Y */
>> +		hdev->quirks |= HID_QUIRK_REUSE_AXES;
>>  		break;
>>  	}
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index fb9ace1..1cc6fe4 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -633,11 +633,17 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>>  		/* These usage IDs map directly to the usage codes. */
>>  		case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
>>  		case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
>> -			if (field->flags & HID_MAIN_ITEM_RELATIVE)
>> -				map_rel(usage->hid & 0xf);
>> -			else
>> -				map_abs_clear(usage->hid & 0xf);
>> -			break;
>> +
>> +			/* if quirk is active don't force the userspace mapping,
>> +			 * instead search and use the next available axis.
>> +			 */
>> +			if (!(device->quirks & HID_QUIRK_REUSE_AXES)) {
>> +				if (field->flags & HID_MAIN_ITEM_RELATIVE)
>> +					map_rel(usage->hid & 0xf);
>> +				else
>> +					map_abs_clear(usage->hid & 0xf);
>> +				break;
>> +			}
>>
>>  		case HID_GD_SLIDER: case HID_GD_DIAL: case HID_GD_WHEEL:
>>  			if (field->flags & HID_MAIN_ITEM_RELATIVE)
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 75b66ec..0979920 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -320,6 +320,7 @@ struct hid_item {
>>  #define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
>>  #define HID_QUIRK_NO_INIT_INPUT_REPORTS		0x00000200
>>  #define HID_QUIRK_ALWAYS_POLL			0x00000400
>> +#define HID_QUIRK_REUSE_AXES			0x00000800
>>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
>>  #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
>>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	0x00040000
>> --
>> 2.10.0
>>

Powered by blists - more mailing lists