Messages in this thread | | | Subject | Re: [RFCv2 0/8] USI stylus support series | From | Tero Kristo <> | Date | Thu, 9 Dec 2021 10:55:47 +0200 |
| |
Hi Benjamin,
On 08/12/2021 16:56, Benjamin Tissoires wrote: > Hi Tero, > > On Tue, Nov 30, 2021 at 5:13 PM Tero Kristo <tero.kristo@linux.intel.com> wrote: >> Hi Benjamin, >> >> On 30/11/2021 16:44, Benjamin Tissoires wrote: >>> Hi Tero, >>> >>> On Fri, Nov 26, 2021 at 2:02 PM Tero Kristo <tero.kristo@linux.intel.com> wrote: >>>> Hi, >>>> >>>> This series is an update based on comments from Benjamin. What is done >>>> is this series is to ditch the separate hid-driver for USI, and add the >>>> generic support to core layers. This part basically brings the support >>>> for providing USI events, without programmability (patches 1-6). >>> That part seems to be almost good for now. I have a few things to check: >>> - patch2: "HID: hid-input: Add suffix also for HID_DG_PEN" I need to >>> ensure there are no touchscreens affected by this (there used to be a >>> mess with some vendors where they would not declare things properly) >>> - patch5: "HID: core: map USI pen style reports directly" this one >>> feels plain wrong. I would need to have a look at the report >>> descriptor but this is too specific in a very generic code >> Relevant part of the report descriptor is here: >> >> Field(8) >> Physical(Digitizers.Stylus) >> Logical(Digitizers.Preferred Line Style) >> Application(Digitizers.Pen) >> Usage(6) >> Digitizers.Ink >> Digitizers.Pencil >> Digitizers.Highlighter >> Digitizers.Chisel Marker >> Digitizers.Brush >> Digitizers.No Preference >> Logical Minimum(1) >> Logical Maximum(6) >> Physical Minimum(0) >> Physical Maximum(255) >> Unit Exponent(-1) >> Unit(SI Linear : Centimeter) >> Report Size(8) >> Report Count(1) >> Report Offset(88) >> Flags( Variable Absolute NoPreferredState ) >> >> To me, it looks almost like it is a bug in the report descriptor itself; >> as you see there are 6 usage values but the report size / count is 1 >> byte. The fact that there are 6 usage values in the field confuses >> hid-core. Basically the usage values are used as encoded content for the >> field. > It took me a few days but I finally understand that this report > descriptor is actually correct. > > The descriptor gives an array of 1 element of size 8, which is enough > to give an index within the available values being [Digitizers.Ink, > Digitizers.Pencil, Digitizers.Highlighter, Digitizers.Chisel Marker, > Digitizers.Brush, Digitizers.No Preference] > > Given that logical min is 1, this index is 1-based. > > So the job of the kernel is to provide the event > Digitizers.Highlighter whenever the value here is 3. The mapping 3 <-> > Digitizers.Highlighter is specific to this report descriptor and > should not be forwarded to user space.
Yes, all this is true. I also see you re-wrote this part a bit in the series to add individual events for all the different line styles. I'll give this a shot and see how it works out. A problem I see is that we need to be able to program the pen line style also somehow, do we just set a single pen style to "enabled" and all the rest get set to "disabled" under the hood?
> >> Alternatively I think this could be patched up in the BPF program, as I >> am modifying the content of the raw hid report already; I could just as >> well modify this one also. Or, maybe I could fix the report descriptor >> itself to act as a sane variable, as I am parsing the report descriptor >> already? > I couldn't understand the fix you did in the BPF program. Can you > explain it by also giving me an example of raw event from the device > and the outputs (fixed and not fixed) of the kernel?
The fix in the BPF code is this (under process_tag()):
/* * Force flags for line style. This makes it act * as a simple variable from HID core point of view. */ bpf_hid_set_data(ctx, (*idx + 1) << 3, 8, 0x2);
After that, the pen line style gets forwarded as a simple integer value to input-core / userspace also. raw events did not need modification after all, I just modified the report descriptor.
> > > Talking about that, I realized that you gave me the report descriptor > of the Acer panel in an other version of this RFC. Could you give me: > - the bus used (USB or I2C)? I have been using I2C in all my testing, the controllers I have access to are behind I2C only. > - the vendor ID? > - the product ID? > - and the same for the other panel, with its report descriptor? > > This way I can add them in the testsuite, and start playing with them. Attached a tarball with both descriptors and their corresponding IDs (copied the R+N+I data from hid-recorder.) > >>>> Additionally, a HID-BPF based sample is provided which can be used to >>>> program / query pen parameters in comparison to the old driver level >>>> implementation (patches 7-8, patch #8 is an incremental change on top of >>>> patch #7 which just converts the fifo to socket so that the client can >>>> also get results back from the server.) >>> After a few more thoughts, I wondered what your input is on this. We >>> should be able to do the very same with plain hidraw... However, you >>> added a `hid/raw_event` processing that will still be kept in the >>> kernel, so maybe bpf would be useful for that at least. >> Yes, plain hidraw can be sort of used to program the values, however the >> interface is kind of annoying to use for the USI pens. You need to be >> touching the display with the pen before anything is accepted. Maybe >> writing some support code to the libevdev would help. >> >> The hidraw hook is needed for processing the cached values also, USI >> pens report their parameters with a delay of some few hundred ms >> depending on controller vendor. And in some cases they don't report >> anything back before forcibly querying the value from the controller, >> and also the write mechanism acts differently; some controllers report >> the programmed value back, others keep reporting the old value until the >> pen leaves the screen and touches it again. > Hmm, not sure I follow this entirely. I guess I would need to have one > of such devices in my hands :(
Yes, it is kind of confusing, I was also trying to figure out the details with a remote proxy (someone telling me how things behave) until I decided to order a second chromebook that had the same controller. I can try to provide logs of the different cases if you want though. The quirks I know of at the moment:
1) controller does not immediately report "correct" values when pen touches screen (ELAN)
2) controller does never report "correct" values when pen touches screen (must do a force GET_REPORT) (GOODIX)
3) controller does not report "correct" values after SET_REPORT (reporting old value) (ELAN)
4) controller responds with bogus data in GET_REPORT (does not know the correct value yet) (ELAN + GOODIX)
I believe other vendors have different behavior with their controllers also, as the specs are not 100% clear on multiple things.
> >> >>>> The whole series is based on top of Benjamin's hid-bpf support work, and >>>> I've pushed a branch at [1] with a series that works and brings in >>>> the dependency. There are also a few separate patches in this series to >>>> fix the problems I found from Benjamin's initial work for hid-bpf; I >>>> wasn't able to get things working without those. The branch is also >>>> based on top of 5.16-rc2 which required some extra changes to the >>>> patches from Benjamin. >>> Yeah, I also rebased on top of 5.16 shortly after sharing that branch >>> and got roughly the same last fix (HID: bpf: compile fix for >>> bpf_hid_foreach_rdesc_item). I am *very* interested in your "HID: bpf: >>> execute BPF programs in proper context" because that is something that >>> was bothering me a lot :) >> Right, I think I have plenty of lockdep / scheduler checks enabled in my >> kernel. They generate plenty of spam with i2c-hid without that patch. >> The same issue may not be visible with some other low level hid devices >> though, I don't have testing capability for anything but the i2c-hid >> right now. I2C is quite notorious for the locking aspects as it is slow >> and is used to control some pretty low level stuff like power management >> etc. > As a rule of thumb, hid_hw_raw_request() can not and should not be > called in IRQ. > I tested your patch with a USB device, and got plenty of complaints too. > > I know bpf now has the ability to defer a function call with timers, > so maybe that's what we need here. That sounds like something that would work yes, I did use workqueue before when this was a separate driver instead of a BPF program. > >>> "HID: bpf: add expected_attach_type to bpf prog during detach" is >>> something I'll need to bring in too >>> >>> but "HID: bpf: fix file mapping" is actually wrong. I initially wanted >>> to attach BPF programs to hidraw, but shortly realized that this is >>> not working because the `hid/rdesc_fixup` kills the hidraw node and so >>> releases the BPF programs. The way I am now attaching it is to use the >>> fd associated with the modalias in the sysfs file (for instance: `sudo >>> ./hid_surface_dial /sys/bus/hid/devices/0005:045E:091B.*/modalias`). >>> This way, the reference to the struct hid_device is kept even if we >>> disconnect the device and reprobe it. >> Ok I can check this out if it works me also. The samples lead me to >> /dev/hidraw usage. >>> Thanks again for your work, and I'd be curious to have your thoughts >>> on hid-bpf and if you think it is better than hidraw/evdev write/new >>> ioctls for your use case. >> The new driver was 777 lines diff, the BPF one is 496 lines so it >> appears smaller. The driver did support two different vendors though >> (ELAN+Goodix, with their specific quirks in place), the BPF only a >> single one right now (ELAN). >> >> The vendor specific quirks are a question, do we want to support that >> somehow in a single BPF binary, or should we attach vendor specific BPF >> programs? > Good question. > The plan I had was to basically pre-compile BPF programs for the > various devices, but having them separated into generic + vendor > specifics seems interesting too. > > I don't have a good answer right now. At least for USI purposes, ELAN+GOODIX controllers have pretty different quirks for them and it seems like having separate BPF programs might be better; trying to get the same BPF program to run for both sounds painful (it was rather painful to get this to work for single vendor.) > >> Chromium-os devices are one of the main customers for USI pens right >> now, and I am not sure how well they will take the BPF concept. :) I did >> ask their feedback though, and I'll come back on this once I have something. > Cool thanks. > >> Personally, I don't have much preference either way at this moment, both >> seem like feasible options. I might lean a bit towards evdev/ioctl as it >> seems a cleaner implementation as of now. The write mechanism I >> implemented for the USI-BPF is a bit hacky, as it just directly writes >> to a shared memory buffer and the buffer gets parsed by the kernel part >> when it processes hidraw event. Anyways, do you have any feedback on >> that part? BPF is completely new to me again so would love to get some >> feedback. > Yeah, this feels wrong to me too. > I guess what we want is to run a BPF call initiated from the > userspace. I am not sure if this is doable. I'll need to dig further > too (I am relatively new to BPF too as a matter of facts).
I could not find a way to initiate BPF call from userspace, thats the reason I implemented it this way. That said, I don't see any case where this would fail though; we only ever write the values from single source (userspace) and read them from kernel. If we miss a write, we just get the old value and report the change later on.
To initiate a BPF call from userspace we would need some sort of hid-bpf callback to a BPF program, which gets triggered by an ioctl or evdev write or something coming from userspace. Which brings us back to the chicken-egg problem we have with USI right now. :)
-Tero
> Cheers, > Benjamin > >> One option is of course to push the write portion of the code to >> userspace and just use hidraw, but we still need to filter out the bogus >> events somehow, and do that in vendor specific manner. I don't think >> this can be done on userspace, as plenty of information that would be >> needed to do this properly has been lost at the input-event level. >> >> -Tero >> >>> Cheers, >>> Benjamin >>> >>>> -Tero >>>> >>>> [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf >>>> >>>> [unhandled content-type:application/gzip] | |