Message-ID: <hdahq2vfi3bnvaqswwdtave2kc2qm3ngvcwn6cgfiirfjfbqnz@zk77mbs3yktp>
Date: Wed, 2 Oct 2024 12:21:24 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Pavel Machek <pavel@....cz>
Cc: Werner Sembach <wse@...edocomputers.com>, Armin Wolf <W_Armin@....de>, 
	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

On Oct 02 2024, Pavel Machek wrote:
> On Wed 2024-10-02 10:13:10, Benjamin Tissoires wrote:
> > On Oct 01 2024, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > PPS: sorry for pushing that hard on HID-BPF, but I can see that it fits
> > > > all of the requirements here:
> > > > - need to be dynamic
> > > > - still unsure of the userspace implementation, meaning that userspace
> > > >   might do something wrong, which might require kernel changes
> > > > - possibility to extend later the kernel API
> > > > - lots of fun :)
> > > 
> > > Please don't do this.
> > > 
> > > We have real drivers for framebuffers. We don't make them emulate
> > > USB-display devices.
> > 
> > Except that they are not framebuffer for multiple reasons. I know you
> > disagree, but the DRM maintainers gave a strong NACK already for a
> 
> Person not linking the DRM stuff was not a maintainer.
> 
> > DRM-like interface, and the auxdisplay would need some sort of tweaking
> > that is going too far IMO (I'll explain below why I believe this).
> 
> 
> 
> > > Yes, this is a small display, and somewhat unusual with weird pixel
> > > positions, but it is common enough that we should have real driver for
> > > that, with real API.
> > 
> > It's not just weird pixel positions. It's also weird shapes. How are you
> > going to fit the space bar in a grid, unless you start saying that it
> > spans accross several pixels. But then users will want to address
> > individual "grouped" pixels, and you'll end up with a weird API. The
> > Enter key on non US layouts is also a problem: is it 1 or 2 pixels wide?
> > Is is 2 pixel in heights?
> 
> Have you seen one of those keyboards?

I already refrain from mention this once or twice, but please, try not
being aggressive in suggesting I'm dumb.

> 
> (Hint: it is LEDs below regular keyboard.)

Yes, I know, and if you read this email and the few others, you'll read
that I own a few of them already (for a long time), and I worked on a
cross vendor userspace API to configure them. So I know what I am
talking about.

> 
> > The positions of the pixels also depend on the physical layout of the
> > keyboard itself. So with the same vendor ID/Product ID, you might have
> > different pixel positions if the device is sold in Europe, or in the
> > US.
> 
> If vendor sells different hardware with same IDs, well 1) that's a
> nono, a 2) that's what kernel parameters are for.

This is already the case (hello hid-uclogic), and no, kernel parameters
are not helping. In that case (uclogic), we ask the device a specific
USB string which has the information, but is not part of HID. This is
dumb, but we don't control hardware makers.

> 
> > It's also luminance problem IIRC. Some keys might have a different range
> > of luminance than others. I remember Bastien Nocera telling me we
> 
> Have you seen one of those keyboards?

Again, please refrain any aggressive comments.

> 
> > But that's just the "easy" part. We can define a kernel API, for sure,
> > but then we need users. And there are several problems here:
> > 
> > - first, users of this new kernel API need to be root to address the
> >   LEDs. They probably won't, so they'll rely on a third party daemon for
> >   that, or just use uaccess (yay!). But that part is easy
> 
> Eventually, desktop environment should talk the interface. (Plus, how
> does HID or BPF craziness help with his?)

HID helps because we already have the case with game controllers. Steam
and SDL (both widely use), put rules giving uaccess to hidraw nodes on
those controllers. So we finally made the jump and now provide in v6.12
a new hidraw ioctl to allow logind to revoke the hidraw node. This
should allow us to not give uaccess to those hidraw nodes.

So in the near future, there will be a portal available, that says
"please give me a fd for this hidraw node", the compositor will then ask
logind to open the file for it and then will pass that fd to the final
application. Once there is a vt-switch, logind will revoke the fd,
meaning that the application will not have access to the device.

> 
> > - then, user sees a new kernel interface, and they have to implement it.
> >   OK, fair enough, but more code to maintain
> 
> Yep. At least there's single interface to talk to.
> 
> > - but that new kernel API doesn't give enough information: all you have
> >   is an approximation of the keyboard layout, which will not match
> >   the
> 
> Have you seen OpenRGB? It already aproximates keyboard as a grid. Or
> maybe we give them enough information.
> 
> Below you were just inventing problems.

A simple "IMO" would makes this kind of comments acceptable. But this is
really offensive TBH.

> 
> > - but then, even if you make everyones happy, the GUI project is
> >   actually cross-platform (OpenRGB is, Steam is, SDL is). And what is
> >   done on Windows is simple: raw access to the HID device. And the
> >   raw
> 
> Yes, Windows is a mess. We don't want to emulate them.
> 
> > I've been through this exact same process with Input and game
> > controllers, and even for libratbag for configuring gaming devices. In
> > the end, the kernel developer never wins, but the userspace
> 
> Yes, we have been in this exact situation. Userland was directly
> accessing mice. It was called "gpm" and we moved away from that for
> good reasons.

There is a slight difference between mouse support and LEDs on your
keyboard. The former is actually required to bring up the machine and to
use it, the latter is nice to have.

And if you want to take that mouse comparison, we are already seeing the
limits of the input subsystem, because we are running out of bits for
defining usages. A few years ago we talked about creating evdev2, but we
ended up nowhere. Now we are realizing that HID has way more
inforamtions on the device that the kernel provides, so we also need a
new way to export those information (pending proposal already).

> 
> > If you want a 100 lines of code program to control your keyboard, with
> > LampArray, you can, as long as you don't require a GUI and don't require
> > to be generic. Just write the values directly on the hidraw device,
> > and
> 
> Haha, no. Kernel part was 400+ lines, no way you can parse that in 100
> lines.

I'm not saying "parsing", I mean adapt to your use case. If you know
your device, your simple CLI is just writing a static array of bytes to
the hidraw interface.

> 
> > You might agree with me or not, but this is actually not relevant to the
> > discussion here IMO: all what Werner is doing (with crazy arrays) is to
> > take a proprietary protocol and convert into a HID standard. He is
> 
> Yes, we should never have had input subsystem. We should simply
> convert all mice to PS/2 standard protocol. ... And yes, we have that,
> that's /dev/mice, yet input layer is very useful.

Again, apple and oranges. Input is required for everything. The LEDs
under a keyboard is not a vital component. And there is already a HID
standard to it.

> 
> What is relevant that these crazy arrays are not going to be merged,
> and better solution is needed.

Again, you seemn to miss the point: those crazy arrays should have been
in the firmware from day one. They are not, so the idea is to convert
proprietary protocol into a standard. Then we can start thinking what
comes next.

> 
> > I'm also under the impression that you are scared by BPF. BPF is just a
> > tool here to "fix" the device with an easy path forward. BPF is
> > safer
> 
> I should not need to run just-in-time compiler to get support for my
> hardware. If you are not scared by BPF, take a look at modern CPU
> design, with emphasis on speculation vulnerabilities such as Spectre
> and Meltdown.
> 

Cheers,
Benjamin

Powered by blists - more mailing lists