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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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 codeRelevant 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.
I have been using I2C in all my testing, the controllers I have access to are behind I2C only.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)?
Attached a tarball with both descriptors and their corresponding IDs (copied the R+N+I data from hid-recorder.)- 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.
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.
That sounds like something that would work yes, I did use workqueue before when this was a separate driver instead of a BPF program.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.
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.)"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.
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, BenjaminOne 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. -TeroCheers, Benjamin-Tero [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf
Attachment:
usi-rdescs.tar.gz
Description: application/gzip
