| Message ID | 1384021557-24106-1-git-send-email-sven@narfation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Sat, 9 Nov 2013, Sven Eckelmann wrote: > Sony Dualshock 3 controllers have two motors which can be used to provide > simple force feedback rumble effects. The right motor is can be used to create > a weak rumble effect but does not allow to set the force. The left motor is > used to create a strong rumble effect with adjustable intensity. > > The state of both motors can be changed using HID_OUTPUT_REPORT packets and > have no timing information. FF memless is used to keep track of the timing and > the sony driver just generates the necessary URBs. > > Signed-off-by: Sven Eckelmann <sven@narfation.org> Applied, thanks.
> Sony Dualshock 3 controllers have two motors which can be used to provide > simple force feedback rumble effects. The right motor is can be used to > create > a weak rumble effect but does not allow to set the force. The left motor > is > used to create a strong rumble effect with adjustable intensity. Hi, This patch appears to work OK with my DualShock/SixAxis controller (USB connection), but causes a machine lockup when used with my Intec wired controller. The Intec controller works OK up to the point when I start rumble, sometimes the motor runs some times it doesn't. I am using SDL2's 'testhaptic' application. Syslog log attached detail controller. If you have any specific tests to run, please advise, Simon Nov 16 14:26:21 womble kernel: [ 95.296039] usb 3-1: new full-speed USB device number 2 using uhci_hcd Nov 16 14:26:22 womble kernel: [ 95.463964] usb 3-1: New USB device found, idVendor=054c, idProduct=0268 Nov 16 14:26:22 womble kernel: [ 95.463973] usb 3-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 Nov 16 14:26:22 womble kernel: [ 95.463978] usb 3-1: Product: PLAYSTATION(R)3 Controller Nov 16 14:26:22 womble kernel: [ 95.463982] usb 3-1: Manufacturer: GASIA CORP. Nov 16 14:26:22 womble mtp-probe: checking bus 3, device 2: "/sys/devices/pci0000:00/0000:00:1d.1/usb3/3-1" Nov 16 14:26:22 womble mtp-probe: bus: 3, device: 2 was not an MTP device Nov 16 14:26:22 womble kernel: [ 95.770400] usbcore: registered new interface driver usbhid Nov 16 14:26:22 womble kernel: [ 95.770408] usbhid: USB HID core driver Nov 16 14:26:22 womble kernel: [ 95.838174] sony 0003:054C:0268.0001: Fixing up Sony Sixaxis report descriptor Nov 16 14:26:22 womble kernel: [ 95.848763] input: GASIA CORP. PLAYSTATION(R)3 Controller as /devices/pci0000:00/0000:00:1d.1/usb3/3-1/3-1:1.0/input/input7 Nov 16 14:26:22 womble kernel: [ 95.849784] sony 0003:054C:0268.0001: input,hiddev0,hidraw0: USB HID v1.11 Joystick [GASIA CORP. PLAYSTATION(R)3 Controller] on usb-0000:00:1d.1-1/input0
> The Intec controller works OK up to the point when I start rumble, > sometimes the motor runs some times it doesn't. I am using SDL2's > 'testhaptic' application. That's not quite true. Both SixAxis and Intec lock up machine with 'testhaptic'. Using 'testrumble' the SixAxis works multiple times, Intec locks up straight away (sometimes starts motor). USBMon shows Testrumble does (on SixAxis) -- e9b23e40 2764496125 S Co:002:00 s 21 09 0201 0000 0023 35 = 00ff01ff 7f000000 0003ff27 100032ff 27100032 ff271000 32ff2710 00320000 e9b23e40 2764507425 C Co:002:00 0 35 > e99fe840 2766509714 S Co:002:00 s 21 09 0201 0000 0023 35 = 00ff00ff 00000000 0003ff27 100032ff 27100032 ff271000 32ff2710 00320000 e99fe840 2766521086 C Co:002:00 0 35 > e9b23cc0 2768521311 S Co:002:00 s 21 09 0201 0000 0023 35 = 00ff01ff 4c000000 0003ff27 100032ff 27100032 ff271000 32ff2710 00320000 e9b23cc0 2768532755 C Co:002:00 0 35 > e9b8e3c0 2770532954 S Co:002:00 s 21 09 0201 0000 0023 35 = 00ff00ff 00000000 0003ff27 100032ff 27100032 ff271000 32ff2710 00320000 e9b8e3c0 2770544425 C Co:002:00 0 35 > e9b8f780 2770545424 C Ii:002:01 -2 0 -- Attached is a script from a while ago, when I was trying to figure out how to get the Intec to rumble. Which runs without crashing machine. Simon
On Saturday 16 November 2013 17:30:25 simon@mungewell.org wrote: > This patch appears to work OK with my DualShock/SixAxis controller (USB > connection), but causes a machine lockup when used with my Intec wired > controller. > > The Intec controller works OK up to the point when I start rumble, > sometimes the motor runs some times it doesn't. I am using SDL2's > 'testhaptic' application. Thanks a lot for this bug report. The testhaptic was a good testcase because it is a heavy user and can reproduce the problem quite easily. I've only tested it using testrumble and own programs which didn't seem to trigger the problem here. The problem is easy to explain: * usb_control_msg/usb_interrupt_msg/usb_bulk_msg/... may sleep * sony_play_effect may gets called in an softirq context (atomic) So it is a bad choice to use hid_output_raw_report (which calls usb_control_msg) in a ff_memless control function. I will just send a revert patch in some minutes. Kind regards, Sven
Hi On Sun, Nov 17, 2013 at 10:36 AM, Sven Eckelmann <sven@narfation.org> wrote: > On Saturday 16 November 2013 17:30:25 simon@mungewell.org wrote: >> This patch appears to work OK with my DualShock/SixAxis controller (USB >> connection), but causes a machine lockup when used with my Intec wired >> controller. >> >> The Intec controller works OK up to the point when I start rumble, >> sometimes the motor runs some times it doesn't. I am using SDL2's >> 'testhaptic' application. > > Thanks a lot for this bug report. The testhaptic was a good testcase because > it is a heavy user and can reproduce the problem quite easily. I've only > tested it using testrumble and own programs which didn't seem to trigger the > problem here. The problem is easy to explain: > > * usb_control_msg/usb_interrupt_msg/usb_bulk_msg/... may sleep > * sony_play_effect may gets called in an softirq context (atomic) > > So it is a bad choice to use hid_output_raw_report (which calls > usb_control_msg) in a ff_memless control function. I will just send a revert > patch in some minutes. Yeah, the input-ff callbacks cannot be handled inline. You also get deadlocks with the input-spinlock. In the wiimote driver I simply dispatch the ff-events to a workqueue. You can have a look at drivers/hid/hid-wiimote-modules.c. You can get some ordering-problems then, but these can usually be ignored as they just collapse events. The related commit was: commit f50f9aabf32db7414551ffdfdccc71be5f3d6e7d Author: David Herrmann <dh.herrmann@gmail.com> Date: Wed Oct 2 13:47:28 2013 +0200 HID: wiimote: fix FF deadlock Thanks David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> So it is a bad choice to use hid_output_raw_report (which calls > usb_control_msg) in a ff_memless control function. I will just send a > revert > patch in some minutes. Rather than revert, can't we just replace the raw call with 'hid_hw_request()' to send the data? This is what we have in hid-lg4ff: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-lg4ff.c?id=refs/tags/v3.12#n402 Simon -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday 17 November 2013 12:38:43 simon@mungewell.org wrote: > > So it is a bad choice to use hid_output_raw_report (which calls > > usb_control_msg) in a ff_memless control function. I will just send a > > revert > > patch in some minutes. > > Rather than revert, can't we just replace the raw call with > 'hid_hw_request()' to send the data? This device doesn't have any report and I had not the time to check how to correctly generate the fields manually for the report/check why it doesn't have any. Kind regards, Sven
On Sun, 17 Nov 2013 10:36:55 +0100 Sven Eckelmann <sven@narfation.org> wrote: > On Saturday 16 November 2013 17:30:25 simon@mungewell.org wrote: > > This patch appears to work OK with my DualShock/SixAxis controller (USB > > connection), but causes a machine lockup when used with my Intec wired > > controller. > > > > The Intec controller works OK up to the point when I start rumble, > > sometimes the motor runs some times it doesn't. I am using SDL2's > > 'testhaptic' application. > > Thanks a lot for this bug report. The testhaptic was a good testcase because > it is a heavy user and can reproduce the problem quite easily. I've only > tested it using testrumble and own programs which didn't seem to trigger the > problem here. The problem is easy to explain: > > * usb_control_msg/usb_interrupt_msg/usb_bulk_msg/... may sleep > * sony_play_effect may gets called in an softirq context (atomic) > > So it is a bad choice to use hid_output_raw_report (which calls > usb_control_msg) in a ff_memless control function. I will just send a revert > patch in some minutes. > Sven, if you are going to resubmit another patch for this functionality (I've seen your v2 just before hitting "Send"), wouldn't it be better to advertise just FF_RUMBLE? AFAICS your first patch results in this (from evtest): .... Event type 21 (EV_FF) Event code 80 (FF_RUMBLE) Event code 81 (FF_PERIODIC) Event code 88 (FF_SQUARE) Event code 89 (FF_TRIANGLE) Event code 90 (FF_SINE) Event code 96 (FF_GAIN) I don't know if this is how it should be, I know nothing about FF stuff. Also please make sure that setting the rumble does not change the LEDs status if there is any set: HID output report 1 is used for both LEDs and rumble. In the bluez plugin[1] I plan on setting LEDs from userspace to match the joystick number, just as the PS3 does, it would be strange for the user if a rumble event would reset the LEDs status. Maybe only send up to the rumble related bytes, or do a read-modify-write if that is possible with HID output reports, if this cannot be done we will have to design a solution to set LEDs too in the kernel, in order to be able to keep some status around. Last comment, if we want a conditional config what about using CONFIG_HID_SONY_FF instead of CONFIG_SONY_FF? Not a big deal of course, just a suggestion. Sorry for the late comments but I got to see the patch only after Jiri had already picked it up. Thanks, Antonio [1] http://ao2.it/tmp/playstation-peripheral-pugin-v5.x-2013-11-13.patch
On Sunday 17 November 2013 23:25:59 Antonio Ospite wrote: > Sven, if you are going to resubmit another patch for this functionality > (I've seen your v2 just before hitting "Send"), wouldn't it be better > to advertise just FF_RUMBLE? AFAICS your first patch results in this > (from evtest): > > .... > Event type 21 (EV_FF) > Event code 80 (FF_RUMBLE) > Event code 81 (FF_PERIODIC) > Event code 88 (FF_SQUARE) > Event code 89 (FF_TRIANGLE) > Event code 90 (FF_SINE) > Event code 96 (FF_GAIN) I don't set them, ff_memless is doing that in input_ff_create_memless: /* we can emulate periodic effects with RUMBLE */ if (test_bit(FF_RUMBLE, ff->ffbit)) { set_bit(FF_PERIODIC, dev->ffbit); set_bit(FF_SINE, dev->ffbit); set_bit(FF_TRIANGLE, dev->ffbit); set_bit(FF_SQUARE, dev->ffbit); } > Also please make sure that setting the rumble does not change the LEDs > status if there is any set: HID output report 1 is used for both LEDs > and rumble. In the bluez plugin[1] I plan on setting LEDs from userspace > to match the joystick number, just as the PS3 does, it would be strange > for the user if a rumble event would reset the LEDs status. I never used the LEDs and therefore cannot say anything about it (I don't have a specification for the used command format). Maybe I can try to play with them next week. But you're patch has some comments in set_leds. Do I correctly interpret the byte 10 in leds_report as "only make changes to following LEDs"? So setting it to 1 would make the command not change the LEDs at all? > Maybe only send up to the rumble related bytes, or do a > read-modify-write if that is possible with HID output reports, if this > cannot be done we will have to design a solution to set LEDs too in the > kernel, in order to be able to keep some status around. The in-kernel state seems to be interesting because it is already done for the Sony Buzz LEDs. But I this is only a wild guess because I've never checked this code path and never used it. > Last comment, if we want a conditional config what about using > CONFIG_HID_SONY_FF instead of CONFIG_SONY_FF? Not a big deal of course, > just a suggestion. Most other hid devices omit the HID_ part in the _FF setting. This is also the reason why I've removed it too. $ grep 'config ' ./drivers/hid/Kconfig|grep -B1 _FF config HID_ACRUX config HID_ACRUX_FF -- config HID_DRAGONRISE config DRAGONRISE_FF config HID_EMS_FF -- config HID_HOLTEK config HOLTEK_FF -- config HID_LOGITECH_DJ config LOGITECH_FF config LOGIRUMBLEPAD2_FF config LOGIG940_FF config LOGIWHEELS_FF -- config HID_PANTHERLORD config PANTHERLORD_FF -- config HID_GREENASIA config GREENASIA_FF -- config HID_SMARTJOYPLUS config SMARTJOYPLUS_FF -- config HID_THRUSTMASTER config THRUSTMASTER_FF -- config HID_ZEROPLUS config ZEROPLUS_FF Kind regards, Sven -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 18 November 2013 00:12:14 Sven Eckelmann wrote: > > Also please make sure that setting the rumble does not change the LEDs > > status if there is any set: HID output report 1 is used for both LEDs > > and rumble. In the bluez plugin[1] I plan on setting LEDs from userspace > > to match the joystick number, just as the PS3 does, it would be strange > > for the user if a rumble event would reset the LEDs status. > > I never used the LEDs and therefore cannot say anything about it (I don't > have a specification for the used command format). Maybe I can try to play > with them next week. > > But you're patch has some comments in set_leds. Do I correctly interpret the > byte 10 in leds_report as "only make changes to following LEDs"? So setting > it to 1 would make the command not change the LEDs at all? Ok, just tried it and it seems this byte is really for the LEDs. But unfortunately, it is enabling/disabling the LEDs completely and not the configuration. And sending less bytes just lets everything fail. Kind regards, Sven
On Mon, 18 Nov 2013 00:12:14 +0100 Sven Eckelmann <sven@narfation.org> wrote: > On Sunday 17 November 2013 23:25:59 Antonio Ospite wrote: > > Sven, if you are going to resubmit another patch for this functionality > > (I've seen your v2 just before hitting "Send"), wouldn't it be better > > to advertise just FF_RUMBLE? AFAICS your first patch results in this > > (from evtest): > > > > .... > > Event type 21 (EV_FF) > > Event code 80 (FF_RUMBLE) > > Event code 81 (FF_PERIODIC) > > Event code 88 (FF_SQUARE) > > Event code 89 (FF_TRIANGLE) > > Event code 90 (FF_SINE) > > Event code 96 (FF_GAIN) > > I don't set them, ff_memless is doing that in input_ff_create_memless: > > /* we can emulate periodic effects with RUMBLE */ > if (test_bit(FF_RUMBLE, ff->ffbit)) { > set_bit(FF_PERIODIC, dev->ffbit); > set_bit(FF_SINE, dev->ffbit); > set_bit(FF_TRIANGLE, dev->ffbit); > set_bit(FF_SQUARE, dev->ffbit); > } > I see now, thanks. > > Also please make sure that setting the rumble does not change the LEDs > > status if there is any set: HID output report 1 is used for both LEDs > > and rumble. In the bluez plugin[1] I plan on setting LEDs from userspace > > to match the joystick number, just as the PS3 does, it would be strange > > for the user if a rumble event would reset the LEDs status. > > I never used the LEDs and therefore cannot say anything about it (I don't have > a specification for the used command format). Maybe I can try to play with > them next week. > > But you're patch has some comments in set_leds. Do I correctly interpret the > byte 10 in leds_report as "only make changes to following LEDs"? So setting it > to 1 would make the command not change the LEDs at all? > That would be an interesting approach, I'll do some experiment about that too. > > Maybe only send up to the rumble related bytes, or do a > > read-modify-write if that is possible with HID output reports, if this > > cannot be done we will have to design a solution to set LEDs too in the > > kernel, in order to be able to keep some status around. > > The in-kernel state seems to be interesting because it is already done for the > Sony Buzz LEDs. But I this is only a wild guess because I've never checked > this code path and never used it. > > > Last comment, if we want a conditional config what about using > > CONFIG_HID_SONY_FF instead of CONFIG_SONY_FF? Not a big deal of course, > > just a suggestion. > > Most other hid devices omit the HID_ part in the _FF setting. This is also the > reason why I've removed it too. > OK, I didn't know, I guess I am fine with that if others are doing so too. > $ grep 'config ' ./drivers/hid/Kconfig|grep -B1 _FF [...] Thanks, Antonio
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index a27e531..329fbb9 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -618,6 +618,14 @@ config HID_SONY * Sony PS3 Blue-ray Disk Remote Control (Bluetooth) * Logitech Harmony adapter for Sony Playstation 3 (Bluetooth) +config SONY_FF + bool "Sony PS2/3 accessories force feedback support" + depends on HID_SONY + select INPUT_FF_MEMLESS + ---help--- + Say Y here if you have a Sony PS2/3 accessory and want to enable force + feedback support for it. + config HID_SPEEDLINK tristate "Speedlink VAD Cezanne mouse support" depends on HID diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c index bc37a18..da551d1 100644 --- a/drivers/hid/hid-sony.c +++ b/drivers/hid/hid-sony.c @@ -614,6 +614,54 @@ static void buzz_remove(struct hid_device *hdev) drv_data->extra = NULL; } +#ifdef CONFIG_SONY_FF +static int sony_play_effect(struct input_dev *dev, void *data, + struct ff_effect *effect) +{ + unsigned char buf[] = { + 0x01, + 0x00, 0xff, 0x00, 0xff, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x03, + 0xff, 0x27, 0x10, 0x00, 0x32, + 0xff, 0x27, 0x10, 0x00, 0x32, + 0xff, 0x27, 0x10, 0x00, 0x32, + 0xff, 0x27, 0x10, 0x00, 0x32, + 0x00, 0x00, 0x00, 0x00, 0x00 + }; + __u8 left; + __u8 right; + struct hid_device *hid = input_get_drvdata(dev); + + if (effect->type != FF_RUMBLE) + return 0; + + left = effect->u.rumble.strong_magnitude / 256; + right = effect->u.rumble.weak_magnitude ? 1 : 0; + + buf[3] = right; + buf[5] = left; + + return hid->hid_output_raw_report(hid, buf, sizeof(buf), + HID_OUTPUT_REPORT); +} + +static int sony_init_ff(struct hid_device *hdev) +{ + struct hid_input *hidinput = list_entry(hdev->inputs.next, + struct hid_input, list); + struct input_dev *input_dev = hidinput->input; + + input_set_capability(input_dev, EV_FF, FF_RUMBLE); + return input_ff_create_memless(input_dev, NULL, sony_play_effect); +} + +#else +static int sony_init_ff(struct hid_device *hdev) +{ + return 0; +} +#endif + static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) { int ret; @@ -663,6 +711,10 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) if (ret < 0) goto err_stop; + ret = sony_init_ff(hdev); + if (ret < 0) + goto err_stop; + return 0; err_stop: hid_hw_stop(hdev);
Sony Dualshock 3 controllers have two motors which can be used to provide simple force feedback rumble effects. The right motor is can be used to create a weak rumble effect but does not allow to set the force. The left motor is used to create a strong rumble effect with adjustable intensity. The state of both motors can be changed using HID_OUTPUT_REPORT packets and have no timing information. FF memless is used to keep track of the timing and the sony driver just generates the necessary URBs. Signed-off-by: Sven Eckelmann <sven@narfation.org> --- drivers/hid/Kconfig | 8 ++++++++ drivers/hid/hid-sony.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) The device will not have any reports. Thus the payload is generated on the fly as it is already done in different other places in the driver.