| Message ID | 20211104133701.1733551-1-tero.kristo@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | HID: add support for USI style pens | expand |
Hi Tero, [just a quick note, I am supposed to be on holiday this week] On Thu, Nov 4, 2021 at 2:38 PM Tero Kristo <tero.kristo@linux.intel.com> wrote: > > Hi, > > This series is an RFC for USI (Universal Stylus Interface) style pen > support. This is based on documentation from USB org describing the HID > usage tables for digitizers (page 0x0D) and experimentation with actual > USI capable controllers. > > This series introduces the USI support with a new HID driver, which > applies the controller specific quirks. The most problematic part of the > USI support is handling of the pen parameters (color, line width, line > style), which are not immediately available from the controller from pen > down event, but must be cached and queried separately from the controller. > In addition to that, when a get-feature report is sent to the > controller, there is a delay before the proper value is reported out; it > is not part of the feature report coming back immediately. > Most of the code in the driver is to handle this (otherwise we could > just use hid-generic.) > > This also boils down to the reason why this series is an RFC, I would like > to receive some feedback which option to pick for programming of the new > values for the programmable pen parameters; whether to parse the input > events so userspace can directly write the new values to the input event > file handle, or whether to use IOCTL. Patches #7 / #8 are sort of optional > choices towards this, but are there to show that both approaches can be > done. Direct write to evdev causes some confusion on the driver level > though, thus patch #7 is there to avoid some of that introducing new > input events for writing the parameters. IOCTL might be the cleanest > approach and I am slightly leaning towards that myself (see patch #8, > this would need to be squashed and cleaned up a bit though but would > effectively get rid of some code from patch #6 and completely rid patch #7.) This series unfortunately raised quite a few red flags for me, and I am glad this is just an RFC. Let me enumerate them first and discuss a little bit more about those: 1. USI is supposed to be generic, so why is there a new driver for it instead of being handled by hid-input.c? 2. new MSC_EVENTS are created without Dmitry or Peter being CC-ed 3. new ioctls??? 4. direct write to evdev to write parameters 5. patch 1/8 doesn't compile without 5/8 6. no tests :) 1. new driver After quickly reading the RFC, I think the main issue there is that we are now having a transducer index which is incompatible with the way input and evedev works nowadays. Yay, we have a new hid-multitouch for pen :( Wacom has been dealing with that situation for years by tweaking the protocol and by just emitting a different serial number (roughly). I think the safest approach would be to keep the existing protocol running so that our user space can handle it properly. I'd need to read the rest of the code more carefully, but if we could have a basic generic handling (without the fancy features like changing the pen style/color) I'd be happier. 2. MSC_* events there is an issue with those: they are not cached like the ABS_* ones. Meaning that each report will wake up userspace for something which basically doesn't change. I know ABS_* is saturated, but I'd like to have reviews from others on what could be done here instead of just using MSC_* as a new ABS_* 3. ioctls this is problematic to me. Any new kernel ABI is problematic to me, and I'd much rather not add any new ones. My new set of mind is because of the recent work I have been conducting regarding eBPF. Basically I managed to have eBPF programs handling the device configuration and event processing in a local branch. I should be able to push a WIP next week, but basically this should allow me to not have to deal with new kernel APIs besides the generic eBPF one. We can imagine a generic hid-input.c processing for those tablets, and have a new userspace component that loads an eBPF program with its own userspace API which is capable of the fancy features. For instance, my current playground is setting the haptic feedback of the Surface Dial depending on the resolution I set on it. Furthermore, ioctls on a new cdev means that the classic userspace libraries will not have access to it without some heavy tuning in the systemd space (libinput only has read/write access to /dev/input/event*). 4. direct write to evdev We enabled that once for LEDs, and it's a pain to maintain. Maybe we can make a use case for it but given that you don't seem very enthusiastic about it too, I wonder if this is not a dead end. 5. patch ordering doesn't compile I guess this is just a rebase hiccup. Not an issue for an RFC 6. tests For these kinds of new classes of devices, I'd like to have tests in the https://gitlab.freedesktop.org/libevdev/hid-tools repository. There is already an initial MR for tablet support (!115 in this project), and we should extend it with more tests. I'd happily help with those tests if you could share the report descriptors and some device dumps made with the hid-recorder tool from that repository. > > The driver has been tested with chromebooks that contain either Goodix > or Elan manufactured USI capable touchscreen controllers in them. > > Any feedback appreciated! I'll try to have a deeper look next week (though it seems a few bits stacked up during my week off, sigh). Cheers, Benjamin > > -Tero > >
Hi Benjamin, Thanks for your feedback! Couple of quick replies below. On 05/11/2021 20:22, Benjamin Tissoires wrote: > Hi Tero, > > [just a quick note, I am supposed to be on holiday this week] > > On Thu, Nov 4, 2021 at 2:38 PM Tero Kristo <tero.kristo@linux.intel.com> wrote: >> Hi, >> >> This series is an RFC for USI (Universal Stylus Interface) style pen >> support. This is based on documentation from USB org describing the HID >> usage tables for digitizers (page 0x0D) and experimentation with actual >> USI capable controllers. >> >> This series introduces the USI support with a new HID driver, which >> applies the controller specific quirks. The most problematic part of the >> USI support is handling of the pen parameters (color, line width, line >> style), which are not immediately available from the controller from pen >> down event, but must be cached and queried separately from the controller. >> In addition to that, when a get-feature report is sent to the >> controller, there is a delay before the proper value is reported out; it >> is not part of the feature report coming back immediately. >> Most of the code in the driver is to handle this (otherwise we could >> just use hid-generic.) >> >> This also boils down to the reason why this series is an RFC, I would like >> to receive some feedback which option to pick for programming of the new >> values for the programmable pen parameters; whether to parse the input >> events so userspace can directly write the new values to the input event >> file handle, or whether to use IOCTL. Patches #7 / #8 are sort of optional >> choices towards this, but are there to show that both approaches can be >> done. Direct write to evdev causes some confusion on the driver level >> though, thus patch #7 is there to avoid some of that introducing new >> input events for writing the parameters. IOCTL might be the cleanest >> approach and I am slightly leaning towards that myself (see patch #8, >> this would need to be squashed and cleaned up a bit though but would >> effectively get rid of some code from patch #6 and completely rid patch #7.) > This series unfortunately raised quite a few red flags for me, and I > am glad this is just an RFC. > Let me enumerate them first and discuss a little bit more about those: > 1. USI is supposed to be generic, so why is there a new driver for it > instead of being handled by hid-input.c? > 2. new MSC_EVENTS are created without Dmitry or Peter being CC-ed > 3. new ioctls??? > 4. direct write to evdev to write parameters > 5. patch 1/8 doesn't compile without 5/8 > 6. no tests :) > > 1. new driver > After quickly reading the RFC, I think the main issue there is that we > are now having a transducer index which is incompatible with the way > input and evedev works nowadays. Yay, we have a new hid-multitouch for > pen :( > > Wacom has been dealing with that situation for years by tweaking the > protocol and by just emitting a different serial number (roughly). I > think the safest approach would be to keep the existing protocol > running so that our user space can handle it properly. > > I'd need to read the rest of the code more carefully, but if we could > have a basic generic handling (without the fancy features like > changing the pen style/color) I'd be happier. The USI pen support is partially compatible with existing input framework, e.g. co-ordinates + touch events work out of box with no-modification to the kernel whatsoever, just by using hid-generic driver. What is missing completely is the new features; color/width/style. It would be possible to move all these to the hid-input driver obviously, I don't think there is anything to prevent that. And, I could split up the series so that in the initial patches we would only support reporting current color/width/style parameters, and add the programmability as a subsequent patch if that would be better. It would also be possible to move parts of the code to the input subsystem from HID (some initial patches from our side were done this way, but I don't think this is necessary.) > > 2. MSC_* events > there is an issue with those: they are not cached like the ABS_* ones. > Meaning that each report will wake up userspace for something which > basically doesn't change. > I know ABS_* is saturated, but I'd like to have reviews from others on > what could be done here instead of just using MSC_* as a new ABS_* In my tests, it seems like MSC_* from the USI pens with these patches only get reported to userspace if something changes. Otherwise they do not get through at all. I have a small quirk in the driver to address this for a case where a new userspace handle is opened while pen is already in use; it does not get the params reported at all unless I flush the current entries out (by reporting a bogus value followed immediately by the real value.) Anyways, ABS events would be fine for the parameters also I believe if this is desirable. > > 3. ioctls > this is problematic to me. Any new kernel ABI is problematic to me, > and I'd much rather not add any new ones. I agree I am torn between the ioctl / direct evdev write. Both have their bad sides to them, thus I provided sort of support for both. :) > > My new set of mind is because of the recent work I have been > conducting regarding eBPF. > Basically I managed to have eBPF programs handling the device > configuration and event processing in a local branch. > I should be able to push a WIP next week, but basically this should > allow me to not have to deal with new kernel APIs besides the generic > eBPF one. > We can imagine a generic hid-input.c processing for those tablets, and > have a new userspace component that loads an eBPF program with its own > userspace API which is capable of the fancy features. > > For instance, my current playground is setting the haptic feedback of > the Surface Dial depending on the resolution I set on it. > > Furthermore, ioctls on a new cdev means that the classic userspace > libraries will not have access to it without some heavy tuning in the > systemd space (libinput only has read/write access to > /dev/input/event*). So, you are saying it would be possible to use this to support the USI pen parameters also somehow? I can take a look at this once available. > > 4. direct write to evdev > > We enabled that once for LEDs, and it's a pain to maintain. Maybe we > can make a use case for it but given that you don't seem very > enthusiastic about it too, I wonder if this is not a dead end. Well, we need to be able to program the pen parameters somehow from userspace, I am open to any suggestions how this could/should be done. > > 5. patch ordering doesn't compile > I guess this is just a rebase hiccup. Not an issue for an RFC Yeah sorry about that. Will fix all those for next rev. > > 6. tests > For these kinds of new classes of devices, I'd like to have tests in > the https://gitlab.freedesktop.org/libevdev/hid-tools repository. > There is already an initial MR for tablet support (!115 in this > project), and we should extend it with more tests. Ok I can take a look at these, thanks for the pointer. I am quite new to the input side of things and have been using whatever I have been able to craft myself, or found via googling. > > I'd happily help with those tests if you could share the report > descriptors and some device dumps made with the hid-recorder tool from > that repository. Yeah, I can share these in your preferred format once I figure out the test tools. I have been using some custom tools to parse things myself so far (mostly kernel traces for both HID + I2C subsystem where my USI controller is connected.) -Tero > >> The driver has been tested with chromebooks that contain either Goodix >> or Elan manufactured USI capable touchscreen controllers in them. >> >> Any feedback appreciated! > I'll try to have a deeper look next week (though it seems a few bits > stacked up during my week off, sigh). > > Cheers, > Benjamin > >> -Tero >> >>
Hi Tero, On Mon, Nov 8, 2021 at 8:44 AM Tero Kristo <tero.kristo@linux.intel.com> wrote: > > Hi Benjamin, > > Thanks for your feedback! Couple of quick replies below. > > On 05/11/2021 20:22, Benjamin Tissoires wrote: > > Hi Tero, > > > > [just a quick note, I am supposed to be on holiday this week] > > > > On Thu, Nov 4, 2021 at 2:38 PM Tero Kristo <tero.kristo@linux.intel.com> wrote: > >> Hi, > >> > >> This series is an RFC for USI (Universal Stylus Interface) style pen > >> support. This is based on documentation from USB org describing the HID > >> usage tables for digitizers (page 0x0D) and experimentation with actual > >> USI capable controllers. > >> > >> This series introduces the USI support with a new HID driver, which > >> applies the controller specific quirks. The most problematic part of the > >> USI support is handling of the pen parameters (color, line width, line > >> style), which are not immediately available from the controller from pen > >> down event, but must be cached and queried separately from the controller. > >> In addition to that, when a get-feature report is sent to the > >> controller, there is a delay before the proper value is reported out; it > >> is not part of the feature report coming back immediately. > >> Most of the code in the driver is to handle this (otherwise we could > >> just use hid-generic.) > >> > >> This also boils down to the reason why this series is an RFC, I would like > >> to receive some feedback which option to pick for programming of the new > >> values for the programmable pen parameters; whether to parse the input > >> events so userspace can directly write the new values to the input event > >> file handle, or whether to use IOCTL. Patches #7 / #8 are sort of optional > >> choices towards this, but are there to show that both approaches can be > >> done. Direct write to evdev causes some confusion on the driver level > >> though, thus patch #7 is there to avoid some of that introducing new > >> input events for writing the parameters. IOCTL might be the cleanest > >> approach and I am slightly leaning towards that myself (see patch #8, > >> this would need to be squashed and cleaned up a bit though but would > >> effectively get rid of some code from patch #6 and completely rid patch #7.) > > This series unfortunately raised quite a few red flags for me, and I > > am glad this is just an RFC. > > Let me enumerate them first and discuss a little bit more about those: > > 1. USI is supposed to be generic, so why is there a new driver for it > > instead of being handled by hid-input.c? > > 2. new MSC_EVENTS are created without Dmitry or Peter being CC-ed > > 3. new ioctls??? > > 4. direct write to evdev to write parameters > > 5. patch 1/8 doesn't compile without 5/8 > > 6. no tests :) > > > > 1. new driver > > After quickly reading the RFC, I think the main issue there is that we > > are now having a transducer index which is incompatible with the way > > input and evedev works nowadays. Yay, we have a new hid-multitouch for > > pen :( > > > > Wacom has been dealing with that situation for years by tweaking the > > protocol and by just emitting a different serial number (roughly). I > > think the safest approach would be to keep the existing protocol > > running so that our user space can handle it properly. > > > > I'd need to read the rest of the code more carefully, but if we could > > have a basic generic handling (without the fancy features like > > changing the pen style/color) I'd be happier. > > The USI pen support is partially compatible with existing input > framework, e.g. co-ordinates + touch events work out of box with > no-modification to the kernel whatsoever, just by using hid-generic > driver. What is missing completely is the new features; > color/width/style. It would be possible to move all these to the > hid-input driver obviously, I don't think there is anything to prevent > that. And, I could split up the series so that in the initial patches we > would only support reporting current color/width/style parameters, and > add the programmability as a subsequent patch if that would be better. > It would also be possible to move parts of the code to the input > subsystem from HID (some initial patches from our side were done this > way, but I don't think this is necessary.) I think I'd like to see the common behavior in hid-input.c, yes. This is mostly because other drivers will be able to use that for free (hid-multitouch for devices that are both touch and pen capable for instance). We can deal with the programmability from userspace later. As for merging part of the code from HID to input, I'd like to see what you are talking about. > > > > > 2. MSC_* events > > there is an issue with those: they are not cached like the ABS_* ones. > > Meaning that each report will wake up userspace for something which > > basically doesn't change. > > I know ABS_* is saturated, but I'd like to have reviews from others on > > what could be done here instead of just using MSC_* as a new ABS_* > In my tests, it seems like MSC_* from the USI pens with these patches > only get reported to userspace if something changes. Otherwise they do > not get through at all. I have a small quirk in the driver to address > this for a case where a new userspace handle is opened while pen is > already in use; it does not get the params reported at all unless I > flush the current entries out (by reporting a bogus value followed > immediately by the real value.) Anyways, ABS events would be fine for > the parameters also I believe if this is desirable. That is by design. When a userspace program opens a device node, it has to query its current state by running a few ioctls. We have libevdev for that that simplifies everything for the userspace. libevdev does a lot more than just that, and every userspace program should use it to not have to deal with SYN_DROPPED and other subtleties in the protocol. > > > > 3. ioctls > > this is problematic to me. Any new kernel ABI is problematic to me, > > and I'd much rather not add any new ones. > I agree I am torn between the ioctl / direct evdev write. Both have > their bad sides to them, thus I provided sort of support for both. :) > > > > My new set of mind is because of the recent work I have been > > conducting regarding eBPF. > > Basically I managed to have eBPF programs handling the device > > configuration and event processing in a local branch. > > I should be able to push a WIP next week, but basically this should > > allow me to not have to deal with new kernel APIs besides the generic > > eBPF one. > > We can imagine a generic hid-input.c processing for those tablets, and > > have a new userspace component that loads an eBPF program with its own > > userspace API which is capable of the fancy features. > > > > For instance, my current playground is setting the haptic feedback of > > the Surface Dial depending on the resolution I set on it. > > > > Furthermore, ioctls on a new cdev means that the classic userspace > > libraries will not have access to it without some heavy tuning in the > > systemd space (libinput only has read/write access to > > /dev/input/event*). > > So, you are saying it would be possible to use this to support the USI > pen parameters also somehow? I can take a look at this once available. Yeah, basically once you load the ebpf program in the kernel, you have a shared memory with userspace that you can use to create your own API. You could even ditch entirely the input subsystem and do the processing in the eBPF program and pull the data from that shared memory. The way I see is the following: - hid-tools (or maybe an other repo, not sure there) is responsible for holding the list of bpf programs that we need to maintain (it's the new upstream for those kind of quirks) - we have one or more userspace program to load those eBPF program as an intrinsic in udev - those userspace program could be a one shot (attach a bpf program, pin it and return) - but userspace program could also present any RPC mechanism (dbus, unix pipe, etc) The advantage here is that we can control the API from the userspace: if we do not use it anymore, we can just ditch the eBPF program (or not load it). We can also rely on classic lib versioning or dbus versioning. I just pushed a branch "hid-bpf-2" at https://gitlab.freedesktop.org/bentiss/hid/-/commits/hid-bpf-2 with the first examples. The interesting commits are between the tip and the `build` branch. > > > > > 4. direct write to evdev > > > > We enabled that once for LEDs, and it's a pain to maintain. Maybe we > > can make a use case for it but given that you don't seem very > > enthusiastic about it too, I wonder if this is not a dead end. > Well, we need to be able to program the pen parameters somehow from > userspace, I am open to any suggestions how this could/should be done. > > > > 5. patch ordering doesn't compile > > I guess this is just a rebase hiccup. Not an issue for an RFC > Yeah sorry about that. Will fix all those for next rev. > > > > 6. tests > > For these kinds of new classes of devices, I'd like to have tests in > > the https://gitlab.freedesktop.org/libevdev/hid-tools repository. > > There is already an initial MR for tablet support (!115 in this > > project), and we should extend it with more tests. > Ok I can take a look at these, thanks for the pointer. I am quite new to > the input side of things and have been using whatever I have been able > to craft myself, or found via googling. > > > > I'd happily help with those tests if you could share the report > > descriptors and some device dumps made with the hid-recorder tool from > > that repository. > > Yeah, I can share these in your preferred format once I figure out the > test tools. I have been using some custom tools to parse things myself > so far (mostly kernel traces for both HID + I2C subsystem where my USI > controller is connected.) Maybe just add a new test in https://gitlab.freedesktop.org/libevdev/hid-tools/-/blob/master/tests/test_tablet.py like https://gitlab.freedesktop.org/libevdev/hid-tools/-/commit/7fa34c2c86e1380eb9c233422567b24a0fd6541f To extract the report descriptor, use `sudo hid-recorder` at the root of this repo, and copy the line starting with `R:` or use your favorite tool :). Cheers, Benjamin > > -Tero > > > > >> The driver has been tested with chromebooks that contain either Goodix > >> or Elan manufactured USI capable touchscreen controllers in them. > >> > >> Any feedback appreciated! > > I'll try to have a deeper look next week (though it seems a few bits > > stacked up during my week off, sigh). > > > > Cheers, > > Benjamin > > > >> -Tero > >> > >> >
Hi Benjamin, On 09/11/2021 19:02, Benjamin Tissoires wrote: > Hi Tero, > > On Mon, Nov 8, 2021 at 8:44 AM Tero Kristo <tero.kristo@linux.intel.com> wrote: >> Hi Benjamin, >> >> Thanks for your feedback! Couple of quick replies below. >> >> On 05/11/2021 20:22, Benjamin Tissoires wrote: >>> Hi Tero, >>> >>> [just a quick note, I am supposed to be on holiday this week] >>> >>> On Thu, Nov 4, 2021 at 2:38 PM Tero Kristo <tero.kristo@linux.intel.com> wrote: >>>> Hi, >>>> >>>> This series is an RFC for USI (Universal Stylus Interface) style pen >>>> support. This is based on documentation from USB org describing the HID >>>> usage tables for digitizers (page 0x0D) and experimentation with actual >>>> USI capable controllers. >>>> >>>> This series introduces the USI support with a new HID driver, which >>>> applies the controller specific quirks. The most problematic part of the >>>> USI support is handling of the pen parameters (color, line width, line >>>> style), which are not immediately available from the controller from pen >>>> down event, but must be cached and queried separately from the controller. >>>> In addition to that, when a get-feature report is sent to the >>>> controller, there is a delay before the proper value is reported out; it >>>> is not part of the feature report coming back immediately. >>>> Most of the code in the driver is to handle this (otherwise we could >>>> just use hid-generic.) >>>> >>>> This also boils down to the reason why this series is an RFC, I would like >>>> to receive some feedback which option to pick for programming of the new >>>> values for the programmable pen parameters; whether to parse the input >>>> events so userspace can directly write the new values to the input event >>>> file handle, or whether to use IOCTL. Patches #7 / #8 are sort of optional >>>> choices towards this, but are there to show that both approaches can be >>>> done. Direct write to evdev causes some confusion on the driver level >>>> though, thus patch #7 is there to avoid some of that introducing new >>>> input events for writing the parameters. IOCTL might be the cleanest >>>> approach and I am slightly leaning towards that myself (see patch #8, >>>> this would need to be squashed and cleaned up a bit though but would >>>> effectively get rid of some code from patch #6 and completely rid patch #7.) >>> This series unfortunately raised quite a few red flags for me, and I >>> am glad this is just an RFC. >>> Let me enumerate them first and discuss a little bit more about those: >>> 1. USI is supposed to be generic, so why is there a new driver for it >>> instead of being handled by hid-input.c? >>> 2. new MSC_EVENTS are created without Dmitry or Peter being CC-ed >>> 3. new ioctls??? >>> 4. direct write to evdev to write parameters >>> 5. patch 1/8 doesn't compile without 5/8 >>> 6. no tests :) >>> >>> 1. new driver >>> After quickly reading the RFC, I think the main issue there is that we >>> are now having a transducer index which is incompatible with the way >>> input and evedev works nowadays. Yay, we have a new hid-multitouch for >>> pen :( >>> >>> Wacom has been dealing with that situation for years by tweaking the >>> protocol and by just emitting a different serial number (roughly). I >>> think the safest approach would be to keep the existing protocol >>> running so that our user space can handle it properly. >>> >>> I'd need to read the rest of the code more carefully, but if we could >>> have a basic generic handling (without the fancy features like >>> changing the pen style/color) I'd be happier. >> The USI pen support is partially compatible with existing input >> framework, e.g. co-ordinates + touch events work out of box with >> no-modification to the kernel whatsoever, just by using hid-generic >> driver. What is missing completely is the new features; >> color/width/style. It would be possible to move all these to the >> hid-input driver obviously, I don't think there is anything to prevent >> that. And, I could split up the series so that in the initial patches we >> would only support reporting current color/width/style parameters, and >> add the programmability as a subsequent patch if that would be better. >> It would also be possible to move parts of the code to the input >> subsystem from HID (some initial patches from our side were done this >> way, but I don't think this is necessary.) > I think I'd like to see the common behavior in hid-input.c, yes. This > is mostly because other drivers will be able to use that for free > (hid-multitouch for devices that are both touch and pen capable for > instance). I have looked at this a bit now, and mostly the initial patches in this series are good for the purpose, just need to ditch #6+ and replace those with something for programming the pen parameters via BPF. I will clean up whatever I have atm and post that as RFCv2. > We can deal with the programmability from userspace later. > > As for merging part of the code from HID to input, I'd like to see > what you are talking about. The split was mostly for programming the pen parameters. It had callbacks to hid-core though, and the API wasn't too clean between hid<->input subsystem in this case. Anyways, if BPF is preferred way to handle the programming of pen parameters, this split between hid<->input is not needed. > >>> 2. MSC_* events >>> there is an issue with those: they are not cached like the ABS_* ones. >>> Meaning that each report will wake up userspace for something which >>> basically doesn't change. >>> I know ABS_* is saturated, but I'd like to have reviews from others on >>> what could be done here instead of just using MSC_* as a new ABS_* >> In my tests, it seems like MSC_* from the USI pens with these patches >> only get reported to userspace if something changes. Otherwise they do >> not get through at all. I have a small quirk in the driver to address >> this for a case where a new userspace handle is opened while pen is >> already in use; it does not get the params reported at all unless I >> flush the current entries out (by reporting a bogus value followed >> immediately by the real value.) Anyways, ABS events would be fine for >> the parameters also I believe if this is desirable. > That is by design. When a userspace program opens a device node, it > has to query its current state by running a few ioctls. > We have libevdev for that that simplifies everything for the userspace. > libevdev does a lot more than just that, and every userspace program > should use it to not have to deal with SYN_DROPPED and other > subtleties in the protocol. Ok, that seems fine. I need to check how to compile / patch libevdev myself to accommodate the changes needed. What is your recommendation for the pen events? Use ABS_x at the end of range (0x40+), or stay with MSC_x? Either way, I need to allocate some event numbers for these. > >>> 3. ioctls >>> this is problematic to me. Any new kernel ABI is problematic to me, >>> and I'd much rather not add any new ones. >> I agree I am torn between the ioctl / direct evdev write. Both have >> their bad sides to them, thus I provided sort of support for both. :) >>> My new set of mind is because of the recent work I have been >>> conducting regarding eBPF. >>> Basically I managed to have eBPF programs handling the device >>> configuration and event processing in a local branch. >>> I should be able to push a WIP next week, but basically this should >>> allow me to not have to deal with new kernel APIs besides the generic >>> eBPF one. >>> We can imagine a generic hid-input.c processing for those tablets, and >>> have a new userspace component that loads an eBPF program with its own >>> userspace API which is capable of the fancy features. >>> >>> For instance, my current playground is setting the haptic feedback of >>> the Surface Dial depending on the resolution I set on it. >>> >>> Furthermore, ioctls on a new cdev means that the classic userspace >>> libraries will not have access to it without some heavy tuning in the >>> systemd space (libinput only has read/write access to >>> /dev/input/event*). >> So, you are saying it would be possible to use this to support the USI >> pen parameters also somehow? I can take a look at this once available. > Yeah, basically once you load the ebpf program in the kernel, you have > a shared memory with userspace that you can use to create your own > API. > You could even ditch entirely the input subsystem and do the > processing in the eBPF program and pull the data from that shared > memory. > > The way I see is the following: > - hid-tools (or maybe an other repo, not sure there) is responsible > for holding the list of bpf programs that we need to maintain (it's > the new upstream for those kind of quirks) > - we have one or more userspace program to load those eBPF program as > an intrinsic in udev > - those userspace program could be a one shot (attach a bpf program, > pin it and return) > - but userspace program could also present any RPC mechanism (dbus, > unix pipe, etc) > > The advantage here is that we can control the API from the userspace: > if we do not use it anymore, we can just ditch the eBPF program (or > not load it). We can also rely on classic lib versioning or dbus > versioning. > > I just pushed a branch "hid-bpf-2" at > https://gitlab.freedesktop.org/bentiss/hid/-/commits/hid-bpf-2 with > the first examples. The interesting commits are between the tip and > the `build` branch. I did experiment with this a bit, and looked at the code. So, it seems like there is possibility for raw_event re-mapping which would be needed to fix some quirks for the current USI controllers, so it seems I can do whatever needed with the support available there. However, I did run into some problems while trying out the hid-bpf samples myself; they seem to fail either during the object load or bpf program attach phases. Are the new samples tested, and with what clang version? There seem to be plenty of pitfalls with the clang version used. And yes, at least some of the other programs under samples/bpf/ do work for me so I do believe I am compiling these properly. > >>> 4. direct write to evdev >>> >>> We enabled that once for LEDs, and it's a pain to maintain. Maybe we >>> can make a use case for it but given that you don't seem very >>> enthusiastic about it too, I wonder if this is not a dead end. >> Well, we need to be able to program the pen parameters somehow from >> userspace, I am open to any suggestions how this could/should be done. >>> 5. patch ordering doesn't compile >>> I guess this is just a rebase hiccup. Not an issue for an RFC >> Yeah sorry about that. Will fix all those for next rev. >>> 6. tests >>> For these kinds of new classes of devices, I'd like to have tests in >>> the https://gitlab.freedesktop.org/libevdev/hid-tools repository. >>> There is already an initial MR for tablet support (!115 in this >>> project), and we should extend it with more tests. >> Ok I can take a look at these, thanks for the pointer. I am quite new to >> the input side of things and have been using whatever I have been able >> to craft myself, or found via googling. >>> I'd happily help with those tests if you could share the report >>> descriptors and some device dumps made with the hid-recorder tool from >>> that repository. >> Yeah, I can share these in your preferred format once I figure out the >> test tools. I have been using some custom tools to parse things myself >> so far (mostly kernel traces for both HID + I2C subsystem where my USI >> controller is connected.) > Maybe just add a new test in > https://gitlab.freedesktop.org/libevdev/hid-tools/-/blob/master/tests/test_tablet.py > like https://gitlab.freedesktop.org/libevdev/hid-tools/-/commit/7fa34c2c86e1380eb9c233422567b24a0fd6541f > > To extract the report descriptor, use `sudo hid-recorder` at the root > of this repo, and copy the line starting with `R:` or use your > favorite tool :). Attached the rdesc for now, I can record things once we figure out which event codes to use (got the hid-tools working with my buildroot env.) -Tero > > Cheers, > Benjamin > >> -Tero >> >>>> The driver has been tested with chromebooks that contain either Goodix >>>> or Elan manufactured USI capable touchscreen controllers in them. >>>> >>>> Any feedback appreciated! >>> I'll try to have a deeper look next week (though it seems a few bits >>> stacked up during my week off, sigh). >>> >>> Cheers, >>> Benjamin >>> >>>> -Tero >>>> >>>> R: 1174 05 0d 09 04 a1 01 85 01 09 22 a1 02 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 55 0f 65 11 35 00 45 ff 09 48 81 02 09 49 81 02 09 30 81 02 95 01 05 01 a4 26 cf 0f 75 10 55 0f 65 11 09 30 35 00 46 26 01 95 01 81 02 26 77 0a 46 a6 00 09 31 81 02 b4 c0 05 0d 09 22 a1 02 05 0d 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 55 0f 65 11 35 00 45 ff 09 48 81 02 09 49 81 02 09 30 81 02 95 01 05 01 a4 26 cf 0f 75 10 55 0f 65 11 09 30 35 00 46 26 01 95 01 81 02 26 77 0a 46 a6 00 09 31 81 02 b4 c0 05 0d 09 22 a1 02 05 0d 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 55 0f 65 11 35 00 45 ff 09 48 81 02 09 49 81 02 09 30 81 02 95 01 05 01 a4 26 cf 0f 75 10 55 0f 65 11 09 30 35 00 46 26 01 95 01 81 02 26 77 0a 46 a6 00 09 31 81 02 b4 c0 05 0d 09 22 a1 02 05 0d 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 55 0f 65 11 35 00 45 ff 09 48 81 02 09 49 81 02 09 30 81 02 95 01 05 01 a4 26 cf 0f 75 10 55 0f 65 11 09 30 35 00 46 26 01 95 01 81 02 26 77 0a 46 a6 00 09 31 81 02 b4 c0 05 0d 09 22 a1 02 05 0d 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 75 06 09 51 25 3f 81 02 26 ff 00 75 08 55 0f 65 11 35 00 45 ff 09 48 81 02 09 49 81 02 09 30 81 02 95 01 05 01 a4 26 cf 0f 75 10 55 0f 65 11 09 30 35 00 46 26 01 95 01 81 02 26 77 0a 46 a6 00 09 31 81 02 b4 c0 05 0d 09 54 25 7f 96 01 00 75 08 81 02 85 0a 09 55 25 0a b1 02 85 44 06 00 ff 09 c5 16 00 00 26 ff 00 75 08 96 00 01 b1 02 c0 06 ff 01 09 01 a1 01 85 02 16 00 00 26 ff 00 75 08 95 40 09 00 81 02 c0 06 00 ff 09 01 a1 01 85 03 75 08 95 20 09 01 91 02 c0 06 00 ff 09 01 a1 01 85 06 09 03 75 08 95 12 91 02 09 04 75 08 95 03 b1 02 c0 06 01 ff 09 01 a1 01 85 04 15 00 26 ff 00 75 08 95 13 09 00 81 02 c0 05 0d 09 02 a1 01 85 07 35 00 09 20 a1 00 09 32 09 42 09 44 09 3c 09 45 15 00 25 01 75 01 95 05 81 02 95 03 81 03 05 01 09 30 75 10 95 01 a4 55 0f 65 11 46 26 01 26 1c 48 81 42 09 31 46 a6 00 26 bc 2f 81 42 b4 05 0d 09 30 26 00 10 81 02 75 08 95 01 09 3b 25 64 81 42 09 38 15 00 25 02 81 02 09 5c 26 ff 00 81 02 09 5e 81 02 09 70 a1 02 15 01 25 06 09 72 09 73 09 74 09 75 09 76 09 77 81 20 09 5b 25 ff 75 40 81 02 c0 06 00 ff 75 08 95 02 09 01 81 02 c0 05 0d 85 60 09 81 a1 02 09 38 75 08 95 01 15 00 25 02 81 02 09 81 15 01 25 04 09 82 09 83 09 84 09 85 81 20 c0 85 61 09 5c a1 02 15 00 26 ff 00 75 08 95 01 09 38 b1 02 09 5c 26 ff 00 b1 02 09 5d 75 01 95 01 25 01 b1 02 95 07 b1 03 c0 85 62 09 5e a1 02 09 38 15 00 25 02 75 08 95 01 b1 02 09 5e 26 ff 00 b1 02 09 5f 75 01 25 01 b1 02 75 07 b1 03 c0 85 63 09 70 a1 02 75 08 95 01 15 00 25 02 09 38 b1 02 09 70 a1 02 25 06 09 72 09 73 09 74 09 75 09 76 09 77 b1 20 c0 09 71 75 01 25 01 b1 02 75 07 b1 03 c0 85 64 09 80 15 00 25 ff 75 40 95 01 b1 02 85 65 09 44 a1 02 09 38 75 08 95 01 25 02 b1 02 15 01 25 03 09 44 a1 02 09 a4 09 44 09 5a 09 45 09 a3 b1 20 c0 09 5a a1 02 09 a4 09 44 09 5a 09 45 09 a3 b1 20 c0 09 45 a1 02 09 a4 09 44 09 5a 09 45 09 a3 b1 20 c0 c0 85 66 75 08 95 01 05 0d 09 90 a1 02 09 38 25 02 b1 02 09 91 75 10 26 ff 0f b1 02 09 92 75 40 25 ff b1 02 05 06 09 2a 75 08 26 ff 00 a1 02 09 2d b1 02 09 2e b1 02 c0 c0 85 67 05 06 09 2b a1 02 05 0d 25 02 09 38 b1 02 05 06 09 2b a1 02 09 2d 26 ff 00 b1 02 09 2e b1 02 c0 c0 85 68 06 00 ff 09 01 a1 02 05 0d 09 38 75 08 95 01 25 02 b1 02 06 00 ff 09 01 75 10 27 ff ff 00 00 b1 02 c0 85 69 05 0d 09 38 75 08 95 01 15 00 25 02 b1 02 c0 06 00 ff 09 81 a1 01 85 17 75 08 95 1f 09 05 81 02 c0