Message-ID: <kywhqw5ef6hioemoydwub57dcmfuu3bwqpz3vjur4pkabboydo@2hrqj3zy4txv>
Date: Tue, 22 Oct 2024 10:51:33 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Armin Wolf <W_Armin@....de>, Pavel Machek <pavel@....cz>, 
	Werner Sembach <wse@...edocomputers.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

On Oct 22 2024, Hans de Goede wrote:
> Hi Armin,
> 
> On 21-Oct-24 10:26 PM, Armin Wolf wrote:
> > Am 11.10.24 um 17:26 schrieb Pavel Machek:
> > 
> >> Hi!
> >>
> >>>> 1.
> >>>> https://lore.kernel.org/all/6b32fb73-0544-4a68-95ba-e82406a4b188@gmx.de/
> >>>> -> Should be no problem? Because this is not generally exposing wmi
> >>>> calls, just mapping two explicitly with sanitized input (whitelisting
> >>>> basically).
> >>> It would be OK to expose a selected set of WMI calls to userspace and sanitizing the input of protect potentially buggy firmware from userspace.
> >>>
> >> I don't believe this is good idea. Passthrough interfaces where
> >> userland talks directly to hardware are very tricky.
> >>
> >>> Regarding the basic idea of having a virtual HID interface: i would prefer to create a illumination subsystem instead, but i have to agree that we should be doing this
> >>> only after enough drivers are inside the kernel, so we can design a
> >>> suitable interface for them. For now, creating a virtual HID
> >>> interface seems to be good enough.
> >> I have an RGB keyboard, and would like to get it supported. I already
> >> have kernel driver for LEDs (which breaks input functionality). I'd
> >> like to cooperate on "illumination" subsystem.
> >>
> >> Best regards,
> >>                                 Pavel
> > 
> > Sorry for taking a bit long to respond.
> > 
> > This "illumination" subsystem would (from my perspective) act like some sort of LED subsystem
> > for devices with a high count of LEDs, like some RGB keyboards.
> > 
> > This would allow us too:
> > - provide an abstract interface for userspace applications like OpenRGB
> > - provide an generic LED subsystem emulation on top of the illumination device (optional)
> > - support future RGB controllers in a generic way
> > 
> > Advanced features like RGB effects, etc can be added later should the need arise.
> > 
> > I would suggest that we model it after the HID LampArray interface:
> > 
> > - interface for querying:
> >  - number of LEDs
> >  - supported colors, etc of those LEDs
> >  - position of those LEDs if available
> >  - kind (keyboard, ...)
> >  - latency, etc
> > - interface for setting multiple LEDs at once
> > - interface for setting a range of LEDs at once
> > - interface for getting the current LED colors
> > 
> > Since sysfs has a "one value per file" rule, i suggest that we use a chardev interface
> > for querying per-LED data and for setting/getting LED colors.
> > 
> > I do not know if mixing sysfs (for controller attributes like number of LEDs, etc) and IOCTL
> > (for setting/getting LED colors) is a good idea, any thoughts?
> 
> I wonder what the advantage of this approach is over simply using HID LampArray
> (emulation), openRGB is already going to support HID LampArray and since Microsoft
> is pushing this we will likely see it getting used more and more.
> 
> Using HID LampArray also has the advantage that work has landed and is landing
> to allow safely handing over raw HID access to userspace programs or even
> individual graphical apps with the option to revoke that access when it is
> no longer desired for the app to have access.
> 
> HID LampArray gives us a well designed API + a safe way to give direct access
> to e.g. games to control the lighting. I really don't see the advantage of
> inventing our own API here only to then also have to design + code some way to
> safely give access to sandboxed apps.
> 
> Note that giving access to sandboxed apps is a lot of work, it is not just
> kernel API it also requires designing a portal interface + implementing
> that portal for at least GNOME, KDE and wlroots.
> 
> Personally I really like the idea to just emulate a HID LampArray device
> for this instead or rolling our own API.  I believe there need to be
> strong arguments to go with some alternative NIH API and I have not
> heard such arguments yet.

Agreed on everything Hans said.

I'll personnaly fight against any new "illumination" API as long as we
don't have committed users. This is the same policy the DRM folks are
applying and it makes a lot of sense:
We can devise a lot about this new API, but if we don't have users for
it, it's energy wasted.

When I mean users, I'm not talking about an example in the kernel tree
or some quick prototype. I mean talking to the existing folks already
doing that and getting their stamp of approval and have an actual
integrated prototype.

We know that OpenRGB and probably others will implement LampArray, if
not for Linux, at least for Mac and Windows. So we will have users for
this protocol. A new Linux specific protocol should be discussed with
them, but I doubt that they'll be happy writing an entirely new
abstraction layer because of Linux.

Cheers,
Benjamin

Powered by blists - more mailing lists