From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933561AbeBVRtA (ORCPT ); Thu, 22 Feb 2018 12:49:00 -0500 Received: from mail-wr0-f175.google.com ([209.85.128.175]:45252 "EHLO mail-wr0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933447AbeBVRs6 (ORCPT ); Thu, 22 Feb 2018 12:48:58 -0500 X-Google-Smtp-Source: AH8x226Mjh9qnli3WTBIVs1gzSXSA/ciMXqy2e+ZjforADsT0NafwK9vXhbg2tHhzHvuYbsAzqmvrg== Date: Thu, 22 Feb 2018 18:48:53 +0100 From: Rodrigo Rivas Costa To: Benjamin Tissoires Cc: "Pierre-Loup A. Griffais" , Jiri Kosina , lkml , linux-input@vger.kernel.org Subject: Re: [PATCH v2 0/3] new driver for Valve Steam Controller Message-ID: <20180222174853.GB21600@casa> References: <20180220193306.28748-1-rodrigorivascosta@gmail.com> <673a2510-7d82-1b24-1085-9f5aa2bb9998@valvesoftware.com> <20180220232035.GA28798@casa> <52b10af3-d93b-34b2-e6ba-22621511b16f@valvesoftware.com> <20180221202107.GA21140@casa> <77b62733-a1ed-16f2-f942-39c27c0f5924@valvesoftware.com> <20180222163143.GA21600@casa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.3 (2018-01-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Feb 22, 2018 at 06:06:30PM +0100, Benjamin Tissoires wrote: > On Thu, Feb 22, 2018 at 5:31 PM, Rodrigo Rivas Costa > wrote: > > On Thu, Feb 22, 2018 at 10:10:40AM +0100, Benjamin Tissoires wrote: > >> On Thu, Feb 22, 2018 at 1:13 AM, Pierre-Loup A. Griffais > >> wrote: > >> > > >> > > >> > On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote: > >> >> > >> >> On Tue, Feb 20, 2018 at 04:09:39PM -0800, Pierre-Loup A. Griffais wrote: > >> >>> > >> >>> On 02/20/2018 03:20 PM, Rodrigo Rivas Costa wrote: > >> >>>> > >> >>>> On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote: > >> >>>> About the left trackpad/joystick, currently I'm not treating them > >> >>>> different. I'm out of ABS axes, and anyway, it is not likely that the > >> >>>> left pad and the joystick be used at the same time (I only have one left > >> >>>> thumb). Nevertheless, if we really want to make them apart, we can use > >> >>>> bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the > >> >>>> details in [2], but I'm not currently doing that in this driver. > >> >>> > >> >>> > >> >>> I didn't necessarily mean exposing it, but in the event a user is using > >> >>> both > >> >>> at the same time (it happens, using claw grip with some games is > >> >>> necessary > >> >>> to use the D-pad while deflecting the stick), then you'll most likely run > >> >>> into issues unless you demux the inbound data, since we were also out of > >> >>> left analog fields and mux them together if used at the same time. Not > >> >>> sure > >> >>> if you're already handling that case, but not doing it would result in > >> >>> the > >> >>> stick appearing to fully deflect every other input report if the user is > >> >>> doing both at the same time. > >> >> > >> >> > >> >> "Claw grip"? Is that a real thing? Yes, it is! Well, you're totally > >> >> right. I think that it will be best to fully separate the left-pad and > >> >> the joystick. Maybe the left-pad can be mapped to ABS_HAT1{X,Y]... > >> >> > >> >>>> About the gyroscope/acceleration/quaternion, you know the issue > >> >>>> that the wireless gives gyro plus quat but no accel, while the wired > >> >>>> gives all three. My general idea is to create an additional input device > >> >>>> with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that > >> >>>> the wireless gives no accel, maybe there is some command to enable it? > >> >>> > >> >>> > >> >>> It should be three neighboring bits for that setting; does this not work > >> >>> for > >> >>> you? > >> >>> > >> >>> // Bitmask that define which IMU features to enable. > >> >>> typedef enum > >> >>> { > >> >>> SETTING_GYRO_MODE_OFF = 0x0000, > >> >>> SETTING_GYRO_MODE_STEERING = 0x0001, > >> >>> SETTING_GYRO_MODE_TILT = 0x0002, > >> >>> SETTING_GYRO_MODE_SEND_ORIENTATION = 0x0004, > >> >>> SETTING_GYRO_MODE_SEND_RAW_ACCEL = 0x0008, > >> >>> SETTING_GYRO_MODE_SEND_RAW_GYRO = 0x0010, > >> >>> } SettingGyroMode; > >> >> > >> >> > >> >> Thanks for that, it will be useful! Those are the values to be written > >> >> to what I called "register 0x30" with {0x87 0x03 0x30 X 0x00}. Currently > >> >> I am writing 0x14 to enable and 0x00 to disable. I suspected it was a > >> >> bit-field... > >> >> > >> >>> In general I'm concerned about sending of the gyro-enable message > >> >>> potentially impairing Steam functionality if it's running at the same > >> >>> time > >> >>> (eg. if gyro gets disabled over the radio while Steam still needs it), or > >> >>> sending any stateful messages to the device. For instance, are you ever > >> >>> sending the "blank configuration" setting? I assume you must be or users > >> >>> would still get keyboard/mouse input over these USB endpoints while > >> >>> trying > >> >>> to interact with the controller. But sending this after Steam programmed > >> >>> a > >> >>> setting, or instructed the controller to go back to lizard mode because > >> >>> it > >> >>> was requested by a configuration, would be problematic. > >> >> > >> >> > >> >> Sure, but this patchset should be safe, shouldn't it? > >> >> Maube future patches that play with the gyro/force-feedback could be > >> >> disabled by default, and could need a module parameter to be enabled. > >> >> That way Steam Client will work out-of-the-box anywhere but proficient > >> >> users could use the extra functionality easily. > >> >> > >> >> Related to that, Benjamin Tissoires replied to 1/3: > >> >>>> > >> >>>> --- a/drivers/hid/hid-quirks.c > >> >>>> +++ b/drivers/hid/hid-quirks.c > >> >>>> @@ -629,6 +629,10 @@ static const struct hid_device_id > >> >>>> hid_have_special_driver[] = { > >> >>>> #if IS_ENABLED(CONFIG_HID_SPEEDLINK) > >> >>>> { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS, > >> >>>> USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) }, > >> >>>> #endif > >> >>>> +#if IS_ENABLED(CONFIG_HID_STEAM) > >> >>>> + { HID_USB_DEVICE(USB_VENDOR_ID_VALVE, > >> >>>> USB_DEVICE_ID_STEAM_CONTROLLER) }, > >> >>>> + { HID_USB_DEVICE(USB_VENDOR_ID_VALVE, > >> >>>> USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) }, > >> >>>> +#endif > >> >>> > >> >>> > >> >>> In addition to the discussion in 0/3, I wonder if you should not > >> >>> remove this hunk. Unless having hid-generic binding the device before > >> >>> your hid-steam driver, it would be better not force the Steam boxes to > >> >>> use your driver. > >> >> > >> >> > >> >> I don't really see the point on that. If we do that I'll have to unbind > >> >> and bind the device manually, and that is a pain, and impossible without > >> >> root (my ultimate goal is to use this controller with my Tizen TV ;-P). > >> > >> I guess you are not running v4.16-rc2 :) (see Clement answers too) > >> > >> Since v4.16, there is no need to absolutely blacklist the devices in > >> this list. hid-generic will bind them first, but as soon as an other > >> driver claims the device, hid-generic unbinds itself and let the other > >> driver to handle the device. Without any user input! > > > > No, I'm still in v4.15.3, and I thought I was bleeding edge... > > I've seen those patches in the hid git tree, but I thought it was > > experimental. Good to know it is upstream, that quirks array was > > weird... I'll try to upgrade and see what happens. > >> > >> >> And anyway this driver is mostly harmless, the only visible changes from > >> >> userspace are the new input and power devices, and that the virtual > >> >> keyboard/mouse are no more. > >> > >> Pierre-Loup mentioned that sometime the Steam client needs those > >> interfaces for the Desktop mode. I have the feeling things are going > >> to be too intricated for us to gracefully accept the driver unless > >> there is a clear ACK from Valve that they are happy with the new > >> driver. > >> > >> >> If the virtual devices are really missed we > >> >> could use: > >> >> > >> >> hid_hw_start(hdev, HID_CONNECT_DEFAULT); > >> >> > >> >> on them, maybe with a module parameter? Note that one of the first > >> >> things the Steam Client does is to disable the virtual devices (with a > >> >> command to the device). > >> >> (TBH I always had the impression that those virtual devices are there > >> >> to move the Grub menu...) > >> >> > >> >> If Valve people really feel that this driver is not needed for SteamOS, > >> >> because the Steam Client is always running, then SteamOS can always do > >> >> CONFIG_HID_STEAM=n. > >> > >> That is what I was expecting, but if the driver also breaks the > >> regular Steam client, we have a situation :) > >> > >> > > >> > > >> > Yes, certainly; that isn't really the usecase I'm worried about. What I'm > >> > worried about is behavior changing for existing users of the controller on > >> > normal desktop distributions. Currently the Steam client will program these > >> > pieces of state (enable/disable direct keyboard/mouse HID functionality, > >> > enable/disable gyro/accel) based on the user's configuration, and a user > >> > getting a kernel update that includes a driver that also programs that > >> > without their intervention is bound to affect the behavior. If there was a > >> > way to make it so the states won't be programmed until it's pretty clear the > >> > user is trying to use that driver's functionality, that would feel safer. > >> > >> I do not think there is a way to know beforehand, given that the > >> kernel module will be loaded first, and sometimes even without the > >> root file system if the driver has been included in the initramfs > >> (which is the point of not having the device blacklisted in > >> hid-quirks.c) > >> > >> I guess the only reasonable solution would be for the kernel HID > >> driver to expose all the interfaces (keyboard, touchpad, gyro, etc...) > >> but not take any action on whether which mode it switches into. Then, > >> (and unfortunatelly) though a custom sysfs API, we could teach SDL or > >> any other configurator to change the device into the desired mode. > >> I would prefer not having a custom sysfs API, but this would allow us > >> to change the owner to a non-root user while hidraw needs to have the > >> root access. > >> > >> An other solution than the sysfs API, would be to have a small driver > >> in libratbag (or a similar daemon) that sends the mode switch command > >> as if the joystick where a special gaming mouse. > > > > Ok, I agree that we cannot remove the keyboard/mouse interfaces. What > > about a module parameter? Something like `disable_lizard_mode` or > > `disable_hid_devices`. That would be 0 by default, so no user-mode > > breakage. BTW, are module parameters considered userspace API stable? > > There are 2 issues with parameters modules: > - yes, it's kernel API (if you change it and one user in the South > Pole has it in the grub config and it now breaks the device, it will > be considered as a regression) > - they are usually taken into account at loading time, and you need > root to change them Well, that parameter would be using at probe time. So you will need to unplug and replug the controller to take effect. > > > > Note that when this parameter is set, the driver will not send any > > command to the controller, (as Steam Client does). Instead, it will > > simply not call hid_hw_start() on those interfaces. I'm not sure how the > > no-quirks hid-generic will behave in this case... I'll try and see... > > > > In the future, we could add a few other modules paramteres to enable the > > gyro and the force-feedback, as these functions could very well break > > Steam. But these functions will be welcomed by DIY enthusiasts. > > The more I think of it, the more I think you need to split the "driver" in 2: > - the kernel part that will be able to understand the raw values from > the device and inject the correct events in the correct input nodes > - the config part that will control how the device behaves. This > config part is the one interfering with Steam, so you might simply > want to have a standalone tool (or in libratbag, and integrate it in > SDL) to send the commands to switch the device into the desired mode. > > Note that we all tried to use custom sysfs API for configuring our > fancy input devices, and we all came down to the conclusion that it > was better to leave the kernel to the minimum and have external tool > to talk to the hardware for the config. This way, you can break the > API as you want (it's internal to your tool), and you can also adapt > much quicker to new devices. Nice idea. If we are relying in user-mode external tools, no need for sysfs, we already can send commands using hidraw. Maybe I can write a simple command-line tool to do that. Would adding a link to my github page with that tool be considered too much self-promotion? The future situation with the gyro will be trikier, though. Because the driver should send the enable/disable gyro commands when the corresponding input-gyro device is opened/closed. And that cannot be done with a user-mode tool. But one problem at a time... I'll modify the driver to leave the lizard mode alone, and see what you all think, ok? Thank you. Rodrigo