| Message ID | 20240820142529.9380-1-max@enpas.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] hid-playstation: DS4: Update rumble and lightbar together | expand |
On Tue, 20 Aug 2024, Max Staudt wrote: > Some 3rd party gamepads expect updates to rumble and lightbar together, > and setting one may cancel the other. > > Let's maximise compatibility for these controllers by sending rumble > and lightbar updates together, even when only one has been scheduled. > > The quirky controllers are matched by a known CRC32 over their HID > report descriptor (hdev->rdesc), since they seem to share the same > descriptor, while pretending to be a Sony DS4 v2.0. > > Signed-off-by: Max Staudt <max@enpas.org> Roderick, does you Ack from v2 still hold? Thanks,
On Wed, Aug 21, 2024 at 7:33 AM Jiri Kosina <jikos@kernel.org> wrote: > > On Tue, 20 Aug 2024, Max Staudt wrote: > > > Some 3rd party gamepads expect updates to rumble and lightbar together, > > and setting one may cancel the other. > > > > Let's maximise compatibility for these controllers by sending rumble > > and lightbar updates together, even when only one has been scheduled. > > > > The quirky controllers are matched by a known CRC32 over their HID > > report descriptor (hdev->rdesc), since they seem to share the same > > descriptor, while pretending to be a Sony DS4 v2.0. > > > > Signed-off-by: Max Staudt <max@enpas.org> > > Roderick, does you Ack from v2 still hold? Ideally I would like some type of quirk, but I have no good way of verifying (also across different 'clone' devices) how reliable this current check would be. It feels a bit too much towards a magical value. I don't see why the reports would differ besides some engineer having made a typo. My gut feeling is that the previous fix is less fragile, so let's opt for that one. Roderick > > Thanks, > > -- > Jiri Kosina > SUSE Labs > >
On 8/21/24 17:03, Roderick Colenbrander wrote: > Ideally I would like some type of quirk, but I have no good way of > verifying (also across different 'clone' devices) how reliable this > current check would be. It feels a bit too much towards a magical > value. I don't see why the reports would differ besides some engineer > having made a typo. The descriptors differ in several ways - why, I don't know. - Across a few DS4 2.0 that I have looked at, they seemed to all have the same descriptors. - Quite a few clones seemed to expose the exact same descriptor on USB, and those didn't need this quirk either. - I have seen two controllers that need this quirk, and they both have the *same* descriptor, which is close to, but still notably different from, the DS4 v2.0 descriptor. So, while I don't have a large base to infer from, it seems like this particular "magic" descriptor and the quirk are correlated. Do you have another idea for a specific enough difference that I could look for? > My gut feeling is that the previous fix is less fragile, so let's opt > for that one. Let me know if you wish for me to resend v2 with a Signed-off-by. Or, I guess Jiri might just manually patch it in - please see this as permission to do so if you prefer this. Max
On Wed, 21 Aug 2024, Max Staudt wrote: > > My gut feeling is that the previous fix is less fragile, so let's opt > > for that one. > > Let me know if you wish for me to resend v2 with a Signed-off-by. Or, I guess > Jiri might just manually patch it in - please see this as permission to do so > if you prefer this. Now in hid.git: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-6.12/hid-playstation Thanks,
On 8/22/24 00:25, Jiri Kosina wrote: > On Wed, 21 Aug 2024, Max Staudt wrote: > >>> My gut feeling is that the previous fix is less fragile, so let's opt >>> for that one. >> >> Let me know if you wish for me to resend v2 with a Signed-off-by. Or, I guess >> Jiri might just manually patch it in - please see this as permission to do so >> if you prefer this. > > Now in hid.git: > > https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-6.12/hid-playstation Thank you all for your help in getting this patch landed! Max
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c index e7c309cfe3a0..0b48ca7bfe22 100644 --- a/drivers/hid/hid-playstation.c +++ b/drivers/hid/hid-playstation.c @@ -356,6 +356,9 @@ struct dualsense_output_report { #define DS4_TOUCHPAD_WIDTH 1920 #define DS4_TOUCHPAD_HEIGHT 942 +/* Quirks for third-party controllers */ +#define DS4_QUIRK_OUTPUT_LIGHTBAR_RUMBLE_TOGETHER BIT(0) + enum dualshock4_dongle_state { DONGLE_DISCONNECTED, DONGLE_CALIBRATING, @@ -405,6 +408,8 @@ struct dualshock4 { struct work_struct output_worker; bool output_worker_initialized; void *output_report_dmabuf; + + unsigned long quirks; }; struct dualshock4_touch_point { @@ -2143,6 +2148,20 @@ static void dualshock4_output_worker(struct work_struct *work) spin_lock_irqsave(&ds4->base.lock, flags); + /* + * Some 3rd party gamepads expect updates to rumble and lightbar + * together, and sending only one at a time may cancel the other. + * + * If this is such a quirky controller, force sending both + * updates, even if only one has been scheduled. + */ + if (DS4_QUIRK_OUTPUT_LIGHTBAR_RUMBLE_TOGETHER) { + if (ds4->update_rumble || ds4->update_lightbar) { + ds4->update_rumble = true; /* 0x01 */ + ds4->update_lightbar = true; /* 0x02 */ + } + } + if (ds4->update_rumble) { /* Select classic rumble style haptics and enable it. */ common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_MOTOR; @@ -2558,6 +2577,30 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev) */ hdev->version |= HID_PLAYSTATION_VERSION_PATCH; + /* + * Some 3rd party gamepads expect updates to rumble and lightbar + * together, and sending only one at a time may cancel the other. + * + * Set a quirk bit if this is a controller known to behave this way. + */ + if (hdev->vendor == USB_VENDOR_ID_SONY && + hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_2) { + /* Match quirky controllers by their HID report descriptor. */ + if (hdev->bus == BUS_USB && hdev->rsize >= 507 && + crc32_le(0xffffffff, hdev->rdesc, 507) == 0xabc63a20) + ds4->quirks |= DS4_QUIRK_OUTPUT_LIGHTBAR_RUMBLE_TOGETHER; + + /* + * The descriptor via Bluetooth differs from the USB one. + * Allow for 1 byte extra, because there may be a trailing + * 0x00 byte. + */ + if (hdev->bus == BUS_BLUETOOTH && + (hdev->rsize == 430 || hdev->rsize == 431) && + crc32_le(0xffffffff, hdev->rdesc, 430) == 0x4194b762) + ds4->quirks |= DS4_QUIRK_OUTPUT_LIGHTBAR_RUMBLE_TOGETHER; + } + ps_dev = &ds4->base; ps_dev->hdev = hdev; spin_lock_init(&ps_dev->lock);
Some 3rd party gamepads expect updates to rumble and lightbar together, and setting one may cancel the other. Let's maximise compatibility for these controllers by sending rumble and lightbar updates together, even when only one has been scheduled. The quirky controllers are matched by a known CRC32 over their HID report descriptor (hdev->rdesc), since they seem to share the same descriptor, while pretending to be a Sony DS4 v2.0. Signed-off-by: Max Staudt <max@enpas.org> --- Changes in v3 -> v4: - Reduced quirk matching to exact values of hdev->rsize. - Adjusted comments to describe quirk detection. The patch no longer applies to all controllers. Changes in v2 -> v3: - Introduced a quirk bit, so this hack only applies to controllers known to be quirky. Changes in v1 -> v2: - Simplified the code, comment, and commit message. --- drivers/hid/hid-playstation.c | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)