Re: [RFCv5 0/2] HID: Add USI support

[Tero Kristo]

Hi Benjamin,

On 16/12/2021 12:36, Benjamin Tissoires wrote:
> Hi Tero,
>
> On Wed, Dec 15, 2021 at 2:42 PM Tero Kristo <tero.kristo@xxxxxxxxxxxxxxx> wrote:
> > Hi,
> >
> > These two patches add the missing pieces for HID USI support. First one
> > adds the HID core changes to support the new Misc events for pen ID,
> > line color and line style. The second patch adds a BPF program on top of
> > the HID-BPF driver which adds support for writing the Pen parameters
> > from userspace, and to add filtering of HID low level events for ELAN
> > USI controller. The BPF programs are not built by the kernel as of now
> > (there are no Makefile changes), as there is a plan to most likely
> > integrate these to a kernel external repository. I have tested these in
> > my own external build setup though, and I can provide the makefile for
> > that if needed. Also a sample client program is provided for
> > communicating with the D-BUS server.
> I had a deeper look at the recordings, and I am very worried in what I
> am seeing:
> - the USI parameters seems to be transmitted only after the touch
Yes, they don't get updated before the pen touches the screen.
> - the USI parameters takes *a lot* of time to be transmitted (2 bytes
> every 2 reports)
Yes, there appears to be a very large latency when they are transmitted 
from the pen to controller. Apparently the controller queries each 
parameter from the pen individually and caches the received values 
internally, and then passes these out via input reports.
> - the recording of the goodix one starts with a stylus touch without hovering
This is a glitch I think, I'll capture a new log.
> - the only "reliable" information we get when hovering seems to be the
> transducer index

Yes, however even this one is a static constant of "1" for the current 
controllers.


> So I am wondering a few things:
> - what happens when you switch between pens?
>    * Do we immediately get a different transducer index?
No, the transducer index gets re-allocated, effectively the first pen is 
going to use transducer index 1, and the second pen is going to re-use 
that and still report it as 1. This is a highly annoying feature.
>    * Are the values right there or do they also take time to be updated?
They take time to update. If you are using the same pen, the values are 
retained in the cache and report correct values immediately, but if you 
switch pens, they start updating with the same latency as initial pen touch.
> - on the goodix one, do you still need to issue a get_report on the
> feature to get the USI parameters, even when you change the pen?

Yes, goodix reports 0xff,0xff,0x77 for the parameters until a GET_REPORT 
is issued, after which it notices the parameter that was queried has 
changed and updates its cached value. Also, see how it reports 0x77, 
instead of an index to the usages array.


> Could you give me the following recording (with an updated hid-tools
> master branch):
> - on the Elan:
>    * start the recording from a fresh boot (no BPF loaded)
>    * hover for a few secs the first USI pen you have
>    * touch it for a few secs
>    * release, then out of proximity
>    * approach the other pen
>    * touch
>    * release, out of prox
>    * then once again with the first pen
>    * then once again with the second pen
>
> - on the goodix: same thing
>
> - on the goodix: same thing but with a BPF program to trigger the
> GET_REPORT if you can cook one quickly (not a big issue if you can
> not).
>
> The reason I am asking about those recordings is because with the 2
> logs you kindly provided, there is no way we can forward the raw
> information to userspace. So I am slightly tempted to only rely on a
> USI manager, in the form of the BPF program in 2/2 to transmit that
> information to userspace.
>
> If this is bulky just for the first event, then the input events might
> be OK, we can assume when the application needs those events they will
> be there.

See attached tarball. All three logs are in it, I crafted a BPF program 
for Goodix controller also. Wasn't that bad actually, a few line change 
so it might be possible to use the same program at least for these two 
controllers.


> > I have also a kernel testing branch available at [1], which contains a
> > few fix patches on top of Benjamin's HID-BPF driver work, and is rebased
> > on top of latest hid/for-next. The HID-BPF fixes have been cleaned up a
> > bit compared to previous setup. There are also a couple of new patches
> > for adding support for a delayed_work BPF program on top of the
> > hid-bpf driver; this is used to execute the raw_requests in non-irq
> > context.
> Thanks for that. I had a very quick look. I thought we could directly
> use the bpf_timer_* functions instead of having to cook another API.
> I'll play around with this, but thanks for pushing forward :)

Yeah, I am kicked by someone on this so want to proceed. ;)

According to documentation, bpf_timer callbacks are executed in soft-irq 
context, so we can't use it at least for sending the raw_requests which 
is needed for USI.

> IIRC you asked me when I was counting on submitting the HID BPF work.
>
> So my answer is that I wanted to submit it by the end of 2021, but it
> looks like I have only one week to finalize this :/
Ok, sounds like you want to submit it probably early next year then. I 
just wanted to have some sort of ballpark estimate, and this is good 
enough for me, thanks.
> The current missing points are:
> - add selftests for all the new API introduced
> - review the entire API to not have to deal with a mistake forever
> - rebase against bpf-next
If you need my help on any of these items, let me know.
> One part of the API I am wondering is whether it is good or not to
> have bpf_hid_foreach_rdesc_item(). This function is complex and we
> could in theory parse the report descriptor in userspace, even before
> we load the program. So all the parameters you need in the various
> raw_event functions could be computed in user space, leading to a much
> smaller API. The other benefit would be that the API would only deal
> with arrays of bytes, which is a small enough and versatile enough API
> :)

This might be a good idea. Assuming the BPF programs end up in the 
hid-tools repo, there could be a generic library for parsing the rdescs 
also which the userspace programs could use.

-Tero

> Cheers,
> Benjamin
>
> > -Tero
> >
> > [1] https://github.com/t-kristo/linux/tree/usi-5.16-v5-bpf

Attachment: usi-logs-2.tar.gz
Description: application/gzip