n Thu, Nov 29, 2018 at 10:08 AM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote: > On Wed, Nov 28, 2018 at 11:27 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > I must say, I am *very* reluctant at adding an other gaming specific > driver in the kernel. We have a nice external project named libratbag > that has the same goal, which is much easier to update and use from > the users. This project announces itself as targeted for gaming mice. I guess it is also for keyboard? Razer keyboards have mice interfaces, but these interfaces are mainly used for control messages, not actual mouse events. > Also, adding yet an other gaming device in the kernel means > you'll have to support yet an other kernel ABI, and this is just wrong > because you can't know what the future will be. Well the ABI used is mainly the LED ABI, with 1 (one) additional sysfs file for controlling the enumerated LED effects. It's not especially sophisticated. If you insist, I can drop that sysfs file and then we only use the existing LED ABI and I do not add anything. As far as I understand one does not exclude the other here, if I add the majority of this driver for key translation and LEDs that do not add any userspace ABIs at all, then hidraw can still be used for the remaining portions of the device, right? > Anyway, you'll need a > userspace tool (or at least a shell script) to enable the various > settings, so using hidraw directly is usually the best choice. So hidraw is the kernel ABI that libratbag is using IIUC? I am not a fan of this design. It is essentially a microkernel architecture where device drivers are relayed to userspace daemons. And it's not just my personal opinion or gut feeling, it is my experience after spending years in the past writing and maintaining libmtp which is another microkernelish userspace that handle MTP devices (like Android phones). https://elinux.org/images/8/85/Media_Transfer_Protocol.pdf I might be grumpy. But the sum total of my experience is that sooner or later you *will* run into a situation where the kernel is way better suited to handle the device and you are essentially writing substandard device drivers in userspace. I have absolutely no hard proof for this. The second concern is community-related and concerns the fact that such projects are usually dependent on one person and when this person lose interest, development stalls and updates stop coming out. But I take it RedHat et al are officially backing libratbag so it is a minor concern. The third concern is that there will be distributions that do not package this ratbag library in their default install and that is going to be pretty annoying to users. So in total, all such external userspace comes with a maintenance price elsewhere and the price of a kernel module is IMO way lower. Especially for these keyboards. But maybe the best is a compromise, a reasonable kernel driver and all extra icing relayed to userspace? > That being said, if the device does some fancy features like switching > to a proprietary protocol, then yes, having a kernel driver is the > only option. This is the case ... the kernel driver switches the keyboard around into a "driver" mode, which is what the OS is expected to do, the keyboard comes up in some compatibility mode which is for interacting with BIOS and boot loader, but it is not supposed to be used during main operation. > But as far as setting the LEDs effects, I would > definitively prefer having this done in userspace. It could be a both/and rather than an either/or approach I think. > > The extra functions have been supported for years by an > > out-of-tree project, OpenRazer. However out-of-tree code > > annoys me because special keys and features don't "just > > work" out of the box with the mainline kernel. > > IIRC, I opened a yet-to-be-finished PR on this very same project to > get rid off the dkms and only rely on userspace bits: > https://github.com/openrazer/openrazer/pull/382 > This made the whole openRazer project much easier to install from a > user and to upgrade for developers. So this looks stalled to me. It's been sitting there for over a year. It surely illustrates that the OpenRazer project is in maintenance mode not development mode. Also illustrates the point that odd userspace projects behave like this when initial development has slowed down: sporadic maintenance and lack of interest and thus manpower except for the odd creative burst. This just motivates me to work even more on this kernel module TBH. But please share your view of it. > Sorry, but you'll have to convince me to review this patch. A 2700 > lines of code patch is not something to review lightly This patch is long but it is because: 1. It contains a length device database in C99 notation i.e. { .foo = bar, .baz = fnord, }, 2. It uses the new kerneldoc documentation style: struct foo { /** * @bar: This is a member */ bool bar; (...) These two things add to the linecount considerably. The first can be amended by switching to either a separate file+patch for the device database or using macros (which is subject to maintainer preference). The second can be amended by using the old compact kerneldoc on top of the struct. This more verbose style is preferred by eg DRM and I thought it's neat but if you don't like it I can ditch it. > and I am > already convinced we should strip off most of it to only handle what > is not reasonable to ask user space to do (key translation and other > basic functionalities). Does this mean that users who don't have the ratbag library (daemon?) and the right udev scripts and whatnot in place are at loss? I guess that is OK if the library+daemon has widespread acceptance and is part of Fedora and Ubuntu default installs. Is it? > > The driver registers an additional input device to funnel the > > special keys (these report themselves as "game controller" > > events) and register LEDs with the LED subsystem for the > > backlight and special LEDs on the Razer keyboards. > > The additional input is fine to have. I am not convinced about the > LEDs, but this is standard API so why not. However, I doubt the > effects are standard API, so I do not really see how everything fits > together. It's one LED for the backlight, standard brightness, then that has one (1) additional sysfs file to control effects. > > The "game" LED goes on/off in response to pushing the > > special "game" key (sometimes prefixed with the Fn key). > > Is this HW triggered or triggered by your driver? Triggered by the driver. In response to pressing that special key, this special LED shall go on. It also activates hardware features for the gaming mode. It's pretty low-level control here. It is a per-keyboard state, so it is not comparable to e.g. caps lock, which should be across all keyboards. Keeping this control to the driver instance thus neatly isolates it. > > This on some keyboards also has the side effect of inhibiting > > the "super" key (Windows) while active. Apparently gamers > > often accidentally push the super key. The HID USB device > > reports two individual keyboard interfaces: one for common > > mode and one for game mode. Some developer noted it is > > a bit over the top to have a separate HID device for a > > game mode that only inhibits a single key, but that is > > how it works. > > The least we do in the kernel, the better. We could merge the 2 input > nodes, but what's the point? Especially if Razer decides to have > completely different keyboards structures between gaming and non > gaming mode. Agreed. I just leave it as-is. > > The "macro" LED goes to on when pressed, the macro is > > then supposed to be recorded, second time it is pressed > > the LED starts flashing meaning it is time to store the > > macro, and after pressing a key where the macro is > > stored, the LED goes off. ESC can abort macro recording. > > The LED is all handled in the driver, and each time we press > > the macro key the standard Linux key KEY_MACRO is reported > > to userspace, that should take care of the actual macro > > recording. > > I don't get this part. Are you also handling the macro recording in > the driver? Or is it done in HW? Neither. It is keeping track of the state of the macro key and LED per se. Userspace should deal with any macro recording and playback. Thus it emits KEY_MACRO when starting and ending the macro recording. In my limited understanding that was the only reasonable usage of KEY_MACRO. > This seems to be an awful lot of states to deal with, and I would > definitively prefer having such thing done in userspace. It is really just keeping track of this one LED in response to this one key. > > The special keys to increase/decrease backlight on the > > keyboard itself are handled directly in the driver. > > The input subsystem only supports KEY_LIGHTS_TOGGLE, but > > I guess if people prefer we could add KEY_LIGHTS_UP > > and KEY_LIGHTS_DOWN and handle this in userspace, > > looping back to the backlight. I am uncertain about that. > > I am surprised we don't already have a solution for that. Having more > than one backlight level is quite common and I can see on my t450s, > that gnome knows about the 3 states the backlight can have. Though I > do not see any events coming out of the keyboards/special keys. Do you think I should add KEY_LIGHTS_UP and KEY_LIGHTS_DOWN then? I can do that as a separate patch. > > The special matrix effects are fun but (IMHO) pointless to > > abstract further than a plain selection item. They are > > controlled by a sysfs file named "matrix_effect" in the > > class device for the "backlight" LED like this: > > > > > cd /sys/class/leds/razer:rgb:backlight > > > cat matrix_effect > > > [none] static spectrum wave reactive breathing starlight ripple fire > > > echo wave > matrix_effect > > So you have only one LED for the entire keyboard? Yes. > How is the effect implemented, in the driver or you are just sending > it once to the device and the HW takes care of the rest? The latter. Razer have some effects that the hardware handles directly (I guess the microcontroller in the keyboard). So it is just selecting an item in a list. Then they have some effects that are just created by manipulating all the LEDs individually. I do not implement that, I think that should be done by userspace. > Can we make this new sysfs standard so other HW vendors can hook up > their effects to it? Sure, I can just add something in Documentation/ABI/testing/sysfs-class-led it's just an array of strings when read, and a single string on write, standard sysfs format. > > TODO: > > - No support for non-keyboard devices. People can send > > patches. > > - There is no RGB LED color control for the backlight or > > anything else. Just leave it for now. > > - There is no individual LED control: each key on the > > keyboard can be assigned a custom color, but we do > > not support this yet. > > - I imagine RGB LED control need some work in the LED > > subsystem or alternatively custom sysfs files. > > There are at least 4 vendors I know that have custom way of settings > the LEDs (Logitech, Razer, Corsair, Roccat). There are many others, > and I really don't think having custom sysfs is the way to go. OK I am fine with handling customized LED patterns in userspace. People want to do crazy things like lighting the LEDs in response to music level and things like that, better not do that in a kernel driver indeed. > In libratbag we tried to only support the common features so we can > have a common tool for every supported device. This doesn't prevent to > interact with other vendor specific projects to implement the missing > functionalities. OK > > - I kept some sysfs files from openrazer giving the product > > name, firmware version and serial number, albeit the usage > > is dubious. Should I just drop them? > > This can be retrieved through hidraw, so I'd say yes, drop them. OK > Can you please double check which features should reasonably be in the > kernel (i.e. the device doesn't work without it), and which features > would be better implemented in userspace by a daemon (those that do > not require to be time sensitive). I would say I would keep key translation, LED control and possibly the built-in LED effects so the keyboard "just works" when plugged in. > If you think we need to have the macros in software implemented in the > kernel, I don't think that belongs in the kernel. That is why I'm just sending userspace KEY_MACRO and let it deal with it. I'm just curious if anyone has seen a userspace that can deal with KEY_MACRO... > I'd like to have a standard API that we could reuse for other > drivers (vendors) and also for mice. This is a functionality we don't > have in libratbag as we only fire libratbag once to configure teh HW, > and rely mostly on the HW for the dynamic part. This works well for > most Logitech devices, but some other vendors have limitted macro > capabilities that don't fit in this model. Hm this looks different... I'm not familiar with the Logitechs. Yours, Linus Walleij
