Message-ID: <rszv4p34oivysoyi337dxwooebipiikzd3pyq7rof5r3agbzce@xejutpd4jcfv>
Date: Tue, 8 Oct 2024 17:21:38 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Werner Sembach <wse@...edocomputers.com>
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

On Oct 08 2024, Werner Sembach wrote:
> 
> Am 08.10.24 um 14:18 schrieb Benjamin Tissoires:
> > On Oct 08 2024, Werner Sembach wrote:
> > > Am 08.10.24 um 11:53 schrieb Benjamin Tissoires:
> > > > On Oct 07 2024, Werner Sembach wrote:
> > > > > Hi,
> > > > > 
> > > > > Am 02.10.24 um 10:31 schrieb Benjamin Tissoires:
> > > > > > On Oct 01 2024, Werner Sembach wrote:
> > > > > > > Hi Benjamin,
> > > > > > > 
> > > > > > > Am 01.10.24 um 15:41 schrieb Benjamin Tissoires:
> > > > > > > > [...]
> > > > > > > > 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
> > > > > > > Well the reference implementetion for the arduiono macropad from microsoft
> > > > > > > ignores the intensity (brightness) channel on rgb leds contrary to the HID
> > > > > > > spec, soo yeah you have a point here ...
> > > > > > Heh :)
> > > > > > 
> > > > > > > > - possibility to extend later the kernel API
> > > > > > > > - lots of fun :)
> > > > > > > You advertise it good ;). More work for me now but maybe less work for me
> > > > > > > later, I will look into it.
> > > > > > Again, I'm pushing this because I see the benefits and because I can
> > > > > > probably reuse the same code on my Corsair and Logitech keyboards. But
> > > > > > also, keep in mind that it's not mandatory because you can actually
> > > > > > attach the BPF code on top of your existing driver to change the way it
> > > > > > behaves. It'll be slightly more complex if you don't let a couple of
> > > > > > vendor passthrough reports that we can use to directly talk to the
> > > > > > device without any tampering, but that's doable. But if you want to keep
> > > > > > the current implementation and have a different layout, this can easily
> > > > > > be done in BPF on top.
> > > > > > 
> > > > > > Cheers,
> > > > > > Benjamin
> > > > > > 
> > > > > > 
> > > > > > [0] https://lore.kernel.org/linux-input/20241001-hid-bpf-hid-generic-v3-0-2ef1019468df@kernel.org/T/#t
> > > > > Thinking about the minimal WMI to HID today, but found a problem: a HID
> > > > > feature report is either strictly input or output afaik, but the WMI
> > > > > interface has both in some functions.
> > > > Not sure you are talking about feature reports, because they are
> > > > read/write. It's just that they are synchronous over the USB control
> > > > endpoint (on USB).
> > > I'm confused about the split between get and send feature reports
> > > https://www.kernel.org/doc/html/latest/hid/hidraw.html
> > > 
> > > I guess then a get feature report can also carry input data and the
> > > difference is that a send feature report doesn't wait for a reply? but then
> > > what is it's reason of existence in contrast to an output report?
> > I'm under the impression you are mixing the 3 types of reports (just
> > re-stating that here in case I wasn't clear).
> > 
> > - Input reports:
> >    `Input()` in the report descriptor
> >    -> data emitted by the device to the host, and notified through an IRQ
> >    mechanism
> >    -> obtained in hidraw through a blocking read() operation
> > - Output reports:
> >    `Output()` in the report descriptor
> >    -> data sent asynchronously by the host to the device.
> >    -> sent from hidraw by calling write() on the dev node (no feedback
> >    except how many bytes were sent)
> > - Feature reports:
> >    `Feature()` in the report descriptor
> >    -> way to synchronously configure the device. Think of it like a
> >    register on the device: you can read it, write it, but you never get
> >    an interrupt when there is a change
> >    -> read/written by using an ioctl on the hidraw node
> 
> From userspace there are the HIDIOCSFEATURE and HIDIOCGFEATURE ioctls.
> 
> From the hid 1.11 spec:
> 
> "
> 
> 7.2.2 Set_Report Request
> 
> [...]
> 
> The meaning of the request fields for the Set_Report request is the same as for
> the Get_Report request, however the data direction is reversed and the Report
> Data is sent from host to device.
> 
> "
> 
> and from the hut 1.5, some of the LampArray feature reports are meant to be
> used with set report and some with get report

Yeah, it just means that you can query or send the data. You can also
use HIDIOCGINPUT() and HIDIOCSOUTPUT() to get a current input report and
set an output report through the hidraw ioctl...

Internally, HIDIOCGINPUT() uses the same code path than
HIDIOCGFEATURE(), but with the report type being an Input instead of a
Feature. Same for HIDIOCSOUTPUT() and HIDIOCSFEATURE().

> 
> (well as far as I can tell the hut doesn't actual specify, if they need to
> be feature reports, or am I missing something?)

They can be both actually. The HUT is missing what's expected here :(.

However, looking at the HUT RR 84:
https://www.usb.org/sites/default/files/hutrr84_-_lighting_and_illumination_page.pdf

There is an example of a report descriptor, and they are using Features.
Not Input+Output.

And looking even further (above), in 3.5 Usage Definitions:
3.5.2, 3.5.3 and 3.5.5 all of them are meant to be a feature, like:
LampArrayAttributesReport CL – Feature -
LampAttributesRequestReport CL – Feature –
LampAttributesResponseReport CL – Feature –
LampArrayControlReport CL – Feature –

3.5.4: can be either feature or output, like:
LampMultiUpdateReport CL – Feature/Output –
LampRangeUpdateReport CL – Feature/ Output –

So I guess the MS implementation can handle Feature only for all but the
update commands.

> 
> and there is the pair with LampAttributesRequestReport and
> LampAttributesResponseReport.

Yeah, not a big deal. The bold IN and OUT are just to say that calling a
setReport on a LampAttributesResponseReport is just ignored AFAIU.

> 
> Sorry for my confusion over the hid spec.

No worries. It is definitely confusing :)

Cheers,
Benjamin

> 
> > 
> > And BTW, it's perfectly fine to have a dedicated report ID which has
> > Input, Output and Feature attached to it :)
> > 
> > > > An input report is strictly directed from the device, and an output
> > > > report is from the host to the device.
> > > > 
> > > > But a feature report is bidirectional.
> > > > 
> > > > > How would I map that?
> > > > Depending on the WMI interface, if you want this to be synchronous,
> > > > defining a Feature report is correct, otherwise (if you don't need
> > > > feedback from WMI), you can declare the commands to WMI as Output
> > > > reports.
> > > Thanks for reminding me that output reports exist xD.
> > hehe
> > 
> > > > > If I split everything in input and output the new interface wouldn't
> > > > > actually be much smaller.
> > > > The HID report descriptor doesn't need to be smaller. The fact that by
> > > > default it exposes only one or two LEDs so we don't have the micrometers
> > > > arrays is the only purpose.
> > > > 
> > > > But if we also implement a not-full HID implementation of LampArray, we
> > > > should be able to strip out the parts that we don't care in the LED
> > > > class implementation, like the exact positioning, or the multiupdate.
> > > > 
> > > > > Also what would I write for the usage for the reserved padding in the report
> > > > > descriptor. Usage: 0x00?
> > > > padding are ignored by HID. So whatever current usage you have is fine.
> > > > 
> > > > However, if you are talking about the custom WMI vendor access, I'd go
> > > > with a vendor collection (usage page 0xff00, usage 0x08 for the 8 bytes
> > > > long WMI command for instance, 0x10 for the 16 bytes long one).
> > > > 
> > > > Side note: in drivers/hid/bpf/progs/hid_report_helpers.h we have some
> > > > autogenerated macros to help writing report descriptors (see
> > > > drivers/hid/bpf/progs/Huion__Dial-2.bpf.c for an example of usage). It's
> > > > in the hid-bpf tree but I think we might be able to include this in
> > > > other drivers (or do a minimal rewrite/move into include).
> > > > I'm not asking you to use it on your code right now, but this has the
> > > > advantage of becoming less "binary blob" in your code, and prevent
> > > > mistakes where you edit the comments but not the values.
> > > I will look into it.
> > > 
> > > Since the interface is fixed I don't need to flesh out the whole descriptor
> > > (which i thought i must do) and usage page (0xff42, because NB04 and the wmi
> > > has 2 other ec controlling wmi interfaces besides the AB one), report usage
> > > (matching the wmi comand id's) and report size should be enough.
> > I'm a little confused by that last sentence. But yeah, I would expect
> > some minimal sanity check before handing over the HID report to the WMI
> > interface :)
> > 
> > Cheers,
> > Benjamin
> > 

Powered by blists - more mailing lists