Re: [RFCv2 0/8] USI stylus support series

[Tero Kristo <tero.kristo@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 12970 bytes --]

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
>>>>
>>>>

[-- Attachment #2: usi-rdescs.tar.gz --]
[-- Type: application/gzip, Size: 1301 bytes --]