Message-ID: <586a1c41-bbe0-4912-b7c7-1716d886c198@tuxedocomputers.com>
Date: Mon, 30 Sep 2024 17:35:31 +0200
From: Werner Sembach <wse@...edocomputers.com>
To: Benjamin Tissoires <bentiss@...nel.org>
Cc: Armin Wolf <W_Armin@....de>, Pavel Machek <pavel@....cz>,
 Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 dri-devel@...ts.freedesktop.org, jelle@...aa.nl, jikos@...nel.org,
 lee@...nel.org, linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-leds@...r.kernel.org, miguel.ojeda.sandonis@...il.com,
 ojeda@...nel.org, onitake@...il.com, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 1/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO
 NB04 devices

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)

Powered by blists - more mailing lists