Re: [PATCH 0/4] HID: lg-g15: G15 v2 LCD and game-mode support

[Hans de Goede <hansg@kernel.org>]

Hi,

+To: Benjamin Valentin <benpicco@googlemail.com>

Who also just submitted a G15 LCD HID framebuffer driver:

https://lore.kernel.org/linux-input/20261108173920.424072-1-benpicco@googlemail.com/

On 9-Nov-26 10:44, Torstein Eide wrote:
> This series adds support for more of the Logitech G15 v2 hardware to
> hid-lg-g15, plus a fix for the remove path.
> 
> Patch 1 exposes the GamePanel LCD contrast as a g15::lcd_contrast LED.
> 
> Patch 2 adds an optional fbdev framebuffer (HID_LOGITECH_G15_FB) for
> the 160x43 monochrome LCD, updated through deferred I/O, so userspace
> can draw on the LCD or bind fbcon to it without libg15/g15daemon.

Ok, so both of you are trying to turn this into a /dev/fb# device, which
I can understand but I'm not sure this is the best idea.

The /dev/fb# API is sorta deprecated in favor of the /dev/dri/card#
kernel-mode-setting (KMS) interface. Note this is not an invitation to
turn this into a KMS driver instead. Please do NOT tell your LLM agent
to turn this into a KMS driver.

Regardless of /dev/fb# vs KMS the problem is that userspace code
like GNOME/KDE will try to treat this as an actual display / an extra
monitor and that userspace code will now need some special code to not
treat it as such and may break without special handling added to it.

And we *never* break userspace so using a userspace API which is
intended for normal (extra) monitor / LCD panel  display output
is not really appropriate IMHO.

Which brings us to the question, why handle this in the kernel at
all. I realize that libg15/g15daemon are no longer maintained and
are obsolete. This is part of why I wrote the drivers/hid/hid-lg-g15.c
driver to move handling of the key-presses into the kernel.

But while handling key-presses is typically always done in the kernel
so far these kind of small status displays (again not a full blown
monitor / display output) has typically been done in userspace.

There are several projects for this, e.g. lcdproc, which is still
actively developed on github:

https://github.com/lcdproc/lcdproc

This works more or less out of the box with the G15 display, it does
still rely on the libg15render library. But that can easily be fixed
as shown by these kernel patches which in essence integrate a copy of
the relevant libg15render bits.

So maybe instead of writing a kernel driver you can write a PR for
lcdproc to remove the dependency on the unmaintained libg15render,
instead folding the render bits directly into the lcdproc code.

Note that on Fedora (you can try by booting a F45 livecd) after
installing lcdprod "sudo dnf install lcdproc" just replugging
the G15 keyboard will trigger a udev script which automatically
creates a lcdproc conf file to use the keyboard if no lcdproc.conf
exists yet. So this pretty much works plug and play without any
kernel changes...

> Patch 3 binds the G15 v2's second interface (046D:C226), which is
> currently handled by hid-generic, and exposes the Super-key-lock
> ("Game Mode") state as a read-only g15::game_mode LED. Normal key
> input on that interface is left to hid-input.

Patches 1 and 3 do sound potentially interesting although I wonder
how these interact with e.g. the existing lcdproc support which
uses this interface directly in hidraw mode.

Note again we never break userspace. So existing lcdproc setups
must keep working with these changes.

> Patch 4 fixes a use-after-free on remove: the LED work can be
> rescheduled by a key press between cancel_work_sync() and
> hid_hw_stop(). The bug predates this series, but the patch is
> written against the remove path as changed by patches 1-3, so
> it does not apply to the current tree on its own.

I'll reply to this patch seperately.

Regards,

Hans


> 
> The LCD wire format and contrast commands follow libg15, hence the
> Based-on-work-by tags for its authors.
> 
> All patches were tested on a Logitech G15 v2.
> 
> Torstein Eide (4):
>   HID: lg-g15: add LCD contrast LED
>   HID: lg-g15: add LCD pixel-content framebuffer
>   HID: lg-g15: report Super-key-lock state on G15 v2's keyboard
>     interface
>   HID: lg-g15: cancel LED work only after stopping the hardware
> 
>  drivers/hid/Kconfig      |  13 +
>  drivers/hid/hid-ids.h    |   1 +
>  drivers/hid/hid-lg-g15.c | 599 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 606 insertions(+), 7 deletions(-)
>