Re: [PATCH 1/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO NB04 devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 28.09.24 um 12:05 schrieb Benjamin Tissoires:
On Sep 28 2024, Werner Sembach wrote:
Hi,

Am 28.09.24 um 09:27 schrieb Benjamin Tissoires:
On Sep 28 2024, Armin Wolf wrote:
Am 27.09.24 um 23:01 schrieb Pavel Machek:

Hi!

The TUXEDO Sirius 16 Gen1 and TUXEDO Sirius 16 Gen2 devices have a per-key
controllable RGB keyboard backlight. The firmware API for it is implemented
via WMI.
Ok.

To make the backlight userspace configurable this driver emulates a
LampArray HID device and translates the input from hidraw to the
corresponding WMI calls. This is a new approach as the leds subsystem lacks
a suitable UAPI for per-key keyboard backlights, and like this no new UAPI
needs to be established.
Please don't.

a) I don't believe emulating crazy HID interface si right thing to
do. (Ton of magic constants. IIRC it stores key positions with
micrometer accuracy or something that crazy. How is userland going to
use this? Will we update micrometers for every single machine?)
This is exactly why I suggest to make use of HID-BPF. The machine
specifics is going to be controlled by userspace, leaving out the crazy
bits out of the kernel.
 From just a quick look at
https://www.kernel.org/doc/html/latest/hid/hid-bpf.html HID-BPF is some kind
HID remapping?
Yes. HID-BPF allows to customize a HID device by changing the report
descriptor and/or the events, and the requests made from hidraw.

It's a HID -> HID conversion, but controlled by userspace.

See [0] for a tutorial.

But the device in question nativly does not have a hid interface for the
backlight. It is controlled via WMI calls.

Afaik userspace on linux has no access to WMI? How could HID-BPF implement
the WMI calls?
You'll need a thin WMI to HID wrapper, but without LampArray.
Then you load the HID-BPF program from userspace, that program knows
about the specifics of the device, and can do the LampArray transform.

Which means that once the wmi-to-hid driver specific to this device is
built in the kernel, you can adjust your LampArray implementation (the
device specifics micrometers and what not) from usersapce.

Even if it is,

b) The emulation should go to generic layer, it is not specific to
your hardware.
Well, there is not so much about an emulation here. It's a different way
of presenting the information.
But given that HID LampArray is a HID standard, userspace is able to
implement it once for all the operating systems, which is why this is so
appealing for them. For reference, we have the same issue with SDL and
Steam regarding advanced game controller: they very much prefer to
directly use HID(raw) to talk to the device instead of having a Linux
specific interface.

Also, starting with v6.12, systemd (logind) will be able to provide
hidraw node access to non root applications (in the same way you can
request an input evdev node). So HID LampArray makes a lot of sense IMO.

Maybe introducing a misc-device which provides an ioctl-based API similar
to the HID LampArray would be a solution?

Basically we would need:
- ioctl for querying the supported LEDs and their properties
- ioctl for enabling/disabling autonomous mode
- ioctl for updating a range of LEDs
- ioctl for updating multiple LEDs at once
You'll definitely get the API wrong at first, then you'll need to adapt
for a new device, extend it, etc... But then, you'll depend on one
userspace application that can talk to your custom ioctls, because cross
platform applications will have to implement LampArray, and they'ĺl
probably skip your custom ioctls. And once that userspace application is
gone, you'll still have to maintain this forever.

Also, the application needs to have root access to that misc device, or
you need to add extra support for it in systemd...

If we implement this as a separate subsystem ("illumination subsystem"), then different
drivers could use this. This would also allow us to add additional ioctl calls later
for more features.
Again, I strongly advise against this.

I'll just reiterate what makes the more sense to me:
- provide a thin wmi-to-hid layer that creates a normal regular HID
    device from your device (could be using vendor collections)
This is what this driver tries to be.
Except that your current implementation also does the LampArray
conversion. I think it'll make more sense to provide an almost raw
access to the underlying protocol (think of it like your own Tuxedo
vendor collection in HID), and handle the LampArray weirdeness in bpf:
definition of the device physicals, conversion from HID LampArray
commands into Tuxedo specifics.

- deal with the LampArray bits in the HID stack, that we can reuse for
    other devices (I was planing on getting there for my Corsair and
    Logitech keyboads).
If a greater efford in the hid stack is planed here i would be all for it.
That's what makes more sense to me at least. Other operating systems
export the HID nodes directly, so userspace prefers to talk to the
device directly. So I'd rather rely on a standard than trying to fit the
current use case in a new interface that will probably fail.

On my todolist i would try to integrate the leds subsystem with the
LampArray interface next, just a simple implementation treating the whole
keyboard as a single led.
That could be done in HID-core as well. Making it part of HID-core also
means that once we get an actual LampArray device, we'll get support for
it from day one.

- Meanwhile, while prototyping the LampArray support in userspace and
    kernelspace, make use of HID-BPF to transform your vendor protocol
    into LampArray. This will allow to fix things without having to
    support them forever. This is why HID-BPF exists: so we can create
    crazy but safe kernel interfaces, without having to support them
    forever.
I guess i have to do some readup xD.

Please have a look at the tutorial[0]. That tutorial is missing the
couple of new hooks you'll need to change the requests emitted from
hidraw as LampArray into Tuxedo, but I can also give you a help into
making it happening.

Basically, you also need to define a .hid_hw_request callback in your
HID_BPF_OPS and extract all of the code you have here into that bpf
program (which is roughly C code).

Cheers,
Benjamin


[0] https://libevdev.pages.freedesktop.org/udev-hid-bpf/tutorial.html

2 question left on my side:

- Does the BPF approach have performance/latency impact?

- Does it work during boot? (e.g. early control via the leds subsystem to stop firmware induced rainbow puke)





[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux