mbox series

[00/12] HID: fix for generic input processing

Message ID 20220126161832.3193805-1-benjamin.tissoires@redhat.com
Headers show
Series HID: fix for generic input processing | expand

Message

Benjamin Tissoires Jan. 26, 2022, 4:18 p.m. UTC
Hi,

This is a followup of the discussion we had between Wacom and
the maintainers, and a followup of those 2 patch series:

https://lore.kernel.org/r/20211022232837.18988-1-ping.cheng@wacom.com/
https://lore.kernel.org/r/2ca91ac7cf92e3048a236db3cd519f04e12c1e61.1615224800.git.nabijaczleweli@nabijaczleweli.xyz/

It took me a while to get it right, but I finally can submit the
series:

- the first 8 patches are some cleanup in the hid-input.c and
  hid-core.c code. They also create a list of input fields that
  is then used to process the event, in the priority we think
  is good.

  For instance, on multitouch devices, it is better to have
  Contact Count before processing all touches, and in each
  touch, having Contact ID first is better. This series doesn't
  cover hid-multitouch, but I have a series on top of this one that
  does cover it.

  Anyway, in our case, here, we need to process Invert before
  In Range for tablets so we can make a decision whether the user
  has the intend to erase or not.

- patch 9 enforces the invert usage before In Range as mentioned
  above

- patch 10 is the actual bulk of processing that should fix the
  generic tablet handling. Now that we have a reliable ordering
  of the fields, we can compute the state of the tool in a reliable
  way, and be kinder to userspace by not sending to it 2 tools at
  the same time.

  This patch has been extensively tested by hid-tools with the new
  MR I submitted that add tests for tablets [0].

- patch 11 is a nice to have that I need for my second series regarding
  hid-multitouch. It is not mandatory with that series, but given
  that it changes the format of the priorities in hid-input.c I thought
  it would be best to send it as part of this series.

  Note that now we are tagging the *reports* and the individual fields
  when they are part of a multitouch collection, which should help
  the drivers that implement this processing (hid-multitouch and wacom).

- last, patch 12 is an attempt at fixing the documentation regarding
  BTN_TOOL_* (requested by Peter).

  Dmitry, feel free to take this one through your tree if you prefer
  to do so (and if you are happy with it), otherwise we can take it
  through the hid tree.

As mentioned above, I have a followup series not entirely tidied up
that implements the processing of Win8 mutltiouch devices in
hid-input.c.
There are several benefits for that: we should be able to drop the
multitouch code in wacom.ko, we can simplify part of hid-multitouch,
and we will be able to quirk a particular device in a separate module,
without touching at the generic code (in hid-multitouch or hid-input).

Anyway, I am missing a few bits for that so that's coming in later.

Cheers,
Benjamin


[0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127

Benjamin Tissoires (12):
  HID: core: statically allocate read buffers
  HID: core: de-duplicate some code in hid_input_field()
  HID: core: split data fetching from processing in hid_input_field()
  HID: input: tag touchscreens as such if the physical is not there
  HID: input: rework spaghetti code with switch statements
  HID: input: move up out-of-range processing of input values
  HID: compute an ordered list of input fields to process
  HID: core: for input reports, process the usages by priority list
  HID: input: enforce Invert usage to be processed before InRange
  HID: input: remove the need for HID_QUIRK_INVERT
  HID: input: accommodate priorities for slotted devices
  Input: docs: add more details on the use of BTN_TOOL

 Documentation/input/event-codes.rst |   5 +-
 drivers/hid/hid-core.c              | 280 ++++++++++++++++++++---
 drivers/hid/hid-input.c             | 330 ++++++++++++++++++++++------
 include/linux/hid.h                 |  23 +-
 4 files changed, 533 insertions(+), 105 deletions(-)

Comments

Angela Czubak Feb. 8, 2022, 7:19 p.m. UTC | #1
Hi Benjamin,

On Wed, Jan 26, 2022 at 5:18 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi,
>
> This is a followup of the discussion we had between Wacom and
> the maintainers, and a followup of those 2 patch series:
>
> https://lore.kernel.org/r/20211022232837.18988-1-ping.cheng@wacom.com/
> https://lore.kernel.org/r/2ca91ac7cf92e3048a236db3cd519f04e12c1e61.1615224800.git.nabijaczleweli@nabijaczleweli.xyz/
>
> It took me a while to get it right, but I finally can submit the
> series:
>
> - the first 8 patches are some cleanup in the hid-input.c and
>   hid-core.c code. They also create a list of input fields that
>   is then used to process the event, in the priority we think
>   is good.
>
>   For instance, on multitouch devices, it is better to have
>   Contact Count before processing all touches, and in each
>   touch, having Contact ID first is better. This series doesn't
>   cover hid-multitouch, but I have a series on top of this one that
>   does cover it.
>
>   Anyway, in our case, here, we need to process Invert before
>   In Range for tablets so we can make a decision whether the user
>   has the intend to erase or not.
>
> - patch 9 enforces the invert usage before In Range as mentioned
>   above
>
> - patch 10 is the actual bulk of processing that should fix the
>   generic tablet handling. Now that we have a reliable ordering
>   of the fields, we can compute the state of the tool in a reliable
>   way, and be kinder to userspace by not sending to it 2 tools at
>   the same time.
>
>   This patch has been extensively tested by hid-tools with the new
>   MR I submitted that add tests for tablets [0].
>
> - patch 11 is a nice to have that I need for my second series regarding
>   hid-multitouch. It is not mandatory with that series, but given
>   that it changes the format of the priorities in hid-input.c I thought
>   it would be best to send it as part of this series.
>
>   Note that now we are tagging the *reports* and the individual fields
>   when they are part of a multitouch collection, which should help
>   the drivers that implement this processing (hid-multitouch and wacom).
>
> - last, patch 12 is an attempt at fixing the documentation regarding
>   BTN_TOOL_* (requested by Peter).
>
>   Dmitry, feel free to take this one through your tree if you prefer
>   to do so (and if you are happy with it), otherwise we can take it
>   through the hid tree.
>
> As mentioned above, I have a followup series not entirely tidied up
> that implements the processing of Win8 mutltiouch devices in
> hid-input.c.
> There are several benefits for that: we should be able to drop the
> multitouch code in wacom.ko, we can simplify part of hid-multitouch,
> and we will be able to quirk a particular device in a separate module,
> without touching at the generic code (in hid-multitouch or hid-input).
>
> Anyway, I am missing a few bits for that so that's coming in later.
>

Is there any timeline for the followup series? I am wondering how that
would affect haptic support implementation.

> Cheers,
> Benjamin
>
>
> [0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127
>
> Benjamin Tissoires (12):
>   HID: core: statically allocate read buffers
>   HID: core: de-duplicate some code in hid_input_field()
>   HID: core: split data fetching from processing in hid_input_field()
>   HID: input: tag touchscreens as such if the physical is not there
>   HID: input: rework spaghetti code with switch statements
>   HID: input: move up out-of-range processing of input values
>   HID: compute an ordered list of input fields to process
>   HID: core: for input reports, process the usages by priority list
>   HID: input: enforce Invert usage to be processed before InRange
>   HID: input: remove the need for HID_QUIRK_INVERT
>   HID: input: accommodate priorities for slotted devices
>   Input: docs: add more details on the use of BTN_TOOL
>
>  Documentation/input/event-codes.rst |   5 +-
>  drivers/hid/hid-core.c              | 280 ++++++++++++++++++++---
>  drivers/hid/hid-input.c             | 330 ++++++++++++++++++++++------
>  include/linux/hid.h                 |  23 +-
>  4 files changed, 533 insertions(+), 105 deletions(-)
>
> --
> 2.33.1
>

Does this patch series introduce the leaf driver support you mentioned
in the haptic review?
Angela Czubak Feb. 14, 2022, 10:51 a.m. UTC | #2
On Tue, Feb 8, 2022 at 8:19 PM Angela Czubak <acz@semihalf.com> wrote:
>
> Hi Benjamin,
>
> On Wed, Jan 26, 2022 at 5:18 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > Hi,
> >
> > This is a followup of the discussion we had between Wacom and
> > the maintainers, and a followup of those 2 patch series:
> >
> > https://lore.kernel.org/r/20211022232837.18988-1-ping.cheng@wacom.com/
> > https://lore.kernel.org/r/2ca91ac7cf92e3048a236db3cd519f04e12c1e61.1615224800.git.nabijaczleweli@nabijaczleweli.xyz/
> >
> > It took me a while to get it right, but I finally can submit the
> > series:
> >
> > - the first 8 patches are some cleanup in the hid-input.c and
> >   hid-core.c code. They also create a list of input fields that
> >   is then used to process the event, in the priority we think
> >   is good.
> >
> >   For instance, on multitouch devices, it is better to have
> >   Contact Count before processing all touches, and in each
> >   touch, having Contact ID first is better. This series doesn't
> >   cover hid-multitouch, but I have a series on top of this one that
> >   does cover it.
> >
> >   Anyway, in our case, here, we need to process Invert before
> >   In Range for tablets so we can make a decision whether the user
> >   has the intend to erase or not.
> >
> > - patch 9 enforces the invert usage before In Range as mentioned
> >   above
> >
> > - patch 10 is the actual bulk of processing that should fix the
> >   generic tablet handling. Now that we have a reliable ordering
> >   of the fields, we can compute the state of the tool in a reliable
> >   way, and be kinder to userspace by not sending to it 2 tools at
> >   the same time.
> >
> >   This patch has been extensively tested by hid-tools with the new
> >   MR I submitted that add tests for tablets [0].
> >
> > - patch 11 is a nice to have that I need for my second series regarding
> >   hid-multitouch. It is not mandatory with that series, but given
> >   that it changes the format of the priorities in hid-input.c I thought
> >   it would be best to send it as part of this series.
> >
> >   Note that now we are tagging the *reports* and the individual fields
> >   when they are part of a multitouch collection, which should help
> >   the drivers that implement this processing (hid-multitouch and wacom).
> >
> > - last, patch 12 is an attempt at fixing the documentation regarding
> >   BTN_TOOL_* (requested by Peter).
> >
> >   Dmitry, feel free to take this one through your tree if you prefer
> >   to do so (and if you are happy with it), otherwise we can take it
> >   through the hid tree.
> >
> > As mentioned above, I have a followup series not entirely tidied up
> > that implements the processing of Win8 mutltiouch devices in
> > hid-input.c.
> > There are several benefits for that: we should be able to drop the
> > multitouch code in wacom.ko, we can simplify part of hid-multitouch,
> > and we will be able to quirk a particular device in a separate module,
> > without touching at the generic code (in hid-multitouch or hid-input).
> >
> > Anyway, I am missing a few bits for that so that's coming in later.
> >
>
> Is there any timeline for the followup series? I am wondering how that
> would affect haptic support implementation.

Hi Benjamin,

just pinging in hope of receiving some answer :)
I am thinking of preparing another version of haptic support patches
(https://lore.kernel.org/all/20220114183152.1691659-1-acz@semihalf.com/T/)
and if I could already start remodelling them based on your changes so that
it is actually a haptic hid driver and not and API that would be great :)
I am simply wondering when multitouch driver is going to be expressed simply
by your changes.

>
> > Cheers,
> > Benjamin
> >
> >
> > [0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127
> >
> > Benjamin Tissoires (12):
> >   HID: core: statically allocate read buffers
> >   HID: core: de-duplicate some code in hid_input_field()
> >   HID: core: split data fetching from processing in hid_input_field()
> >   HID: input: tag touchscreens as such if the physical is not there
> >   HID: input: rework spaghetti code with switch statements
> >   HID: input: move up out-of-range processing of input values
> >   HID: compute an ordered list of input fields to process
> >   HID: core: for input reports, process the usages by priority list
> >   HID: input: enforce Invert usage to be processed before InRange
> >   HID: input: remove the need for HID_QUIRK_INVERT
> >   HID: input: accommodate priorities for slotted devices
> >   Input: docs: add more details on the use of BTN_TOOL
> >
> >  Documentation/input/event-codes.rst |   5 +-
> >  drivers/hid/hid-core.c              | 280 ++++++++++++++++++++---
> >  drivers/hid/hid-input.c             | 330 ++++++++++++++++++++++------
> >  include/linux/hid.h                 |  23 +-
> >  4 files changed, 533 insertions(+), 105 deletions(-)
> >
> > --
> > 2.33.1
> >
>
> Does this patch series introduce the leaf driver support you mentioned
> in the haptic review?
Benjamin Tissoires Feb. 14, 2022, 11:03 a.m. UTC | #3
On Mon, Feb 14, 2022 at 11:51 AM Angela Czubak <acz@semihalf.com> wrote:
>
> On Tue, Feb 8, 2022 at 8:19 PM Angela Czubak <acz@semihalf.com> wrote:
> >
> > Hi Benjamin,
> >
> > On Wed, Jan 26, 2022 at 5:18 PM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > This is a followup of the discussion we had between Wacom and
> > > the maintainers, and a followup of those 2 patch series:
> > >
> > > https://lore.kernel.org/r/20211022232837.18988-1-ping.cheng@wacom.com/
> > > https://lore.kernel.org/r/2ca91ac7cf92e3048a236db3cd519f04e12c1e61.1615224800.git.nabijaczleweli@nabijaczleweli.xyz/
> > >
> > > It took me a while to get it right, but I finally can submit the
> > > series:
> > >
> > > - the first 8 patches are some cleanup in the hid-input.c and
> > >   hid-core.c code. They also create a list of input fields that
> > >   is then used to process the event, in the priority we think
> > >   is good.
> > >
> > >   For instance, on multitouch devices, it is better to have
> > >   Contact Count before processing all touches, and in each
> > >   touch, having Contact ID first is better. This series doesn't
> > >   cover hid-multitouch, but I have a series on top of this one that
> > >   does cover it.
> > >
> > >   Anyway, in our case, here, we need to process Invert before
> > >   In Range for tablets so we can make a decision whether the user
> > >   has the intend to erase or not.
> > >
> > > - patch 9 enforces the invert usage before In Range as mentioned
> > >   above
> > >
> > > - patch 10 is the actual bulk of processing that should fix the
> > >   generic tablet handling. Now that we have a reliable ordering
> > >   of the fields, we can compute the state of the tool in a reliable
> > >   way, and be kinder to userspace by not sending to it 2 tools at
> > >   the same time.
> > >
> > >   This patch has been extensively tested by hid-tools with the new
> > >   MR I submitted that add tests for tablets [0].
> > >
> > > - patch 11 is a nice to have that I need for my second series regarding
> > >   hid-multitouch. It is not mandatory with that series, but given
> > >   that it changes the format of the priorities in hid-input.c I thought
> > >   it would be best to send it as part of this series.
> > >
> > >   Note that now we are tagging the *reports* and the individual fields
> > >   when they are part of a multitouch collection, which should help
> > >   the drivers that implement this processing (hid-multitouch and wacom).
> > >
> > > - last, patch 12 is an attempt at fixing the documentation regarding
> > >   BTN_TOOL_* (requested by Peter).
> > >
> > >   Dmitry, feel free to take this one through your tree if you prefer
> > >   to do so (and if you are happy with it), otherwise we can take it
> > >   through the hid tree.
> > >
> > > As mentioned above, I have a followup series not entirely tidied up
> > > that implements the processing of Win8 mutltiouch devices in
> > > hid-input.c.
> > > There are several benefits for that: we should be able to drop the
> > > multitouch code in wacom.ko, we can simplify part of hid-multitouch,
> > > and we will be able to quirk a particular device in a separate module,
> > > without touching at the generic code (in hid-multitouch or hid-input).
> > >
> > > Anyway, I am missing a few bits for that so that's coming in later.
> > >
> >
> > Is there any timeline for the followup series? I am wondering how that
> > would affect haptic support implementation.
>
> Hi Benjamin,
>
> just pinging in hope of receiving some answer :)
> I am thinking of preparing another version of haptic support patches
> (https://lore.kernel.org/all/20220114183152.1691659-1-acz@semihalf.com/T/)
> and if I could already start remodelling them based on your changes so that
> it is actually a haptic hid driver and not and API that would be great :)
> I am simply wondering when multitouch driver is going to be expressed simply
> by your changes.

Hi Angela,

FWIW, I got a public branch that has the multitouch changes at
https://gitlab.freedesktop.org/bentiss/hid/-/commits/wip/input-mt-v5

The logic in the multitouch processing is correct but it is missing a
few bits IIRC:
- suspend/resume doesn't unset/set the multitouch parameters (doesn't
seem to be an issue on my devel laptop though)
- scantime is not properly handled
- width/height is not using the same path than hid-multitouch (and
probably not reported at all)
- hid-multitouch needs to be cleaned up to use the new core changes
instead of re-doing stuffs itself

I think that you should be able to experiment your hid-haptic changes
already, and see if that is indeed easier to use than creating an API
driver.

Cheers,
Benjamin

>
> >
> > > Cheers,
> > > Benjamin
> > >
> > >
> > > [0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127
> > >
> > > Benjamin Tissoires (12):
> > >   HID: core: statically allocate read buffers
> > >   HID: core: de-duplicate some code in hid_input_field()
> > >   HID: core: split data fetching from processing in hid_input_field()
> > >   HID: input: tag touchscreens as such if the physical is not there
> > >   HID: input: rework spaghetti code with switch statements
> > >   HID: input: move up out-of-range processing of input values
> > >   HID: compute an ordered list of input fields to process
> > >   HID: core: for input reports, process the usages by priority list
> > >   HID: input: enforce Invert usage to be processed before InRange
> > >   HID: input: remove the need for HID_QUIRK_INVERT
> > >   HID: input: accommodate priorities for slotted devices
> > >   Input: docs: add more details on the use of BTN_TOOL
> > >
> > >  Documentation/input/event-codes.rst |   5 +-
> > >  drivers/hid/hid-core.c              | 280 ++++++++++++++++++++---
> > >  drivers/hid/hid-input.c             | 330 ++++++++++++++++++++++------
> > >  include/linux/hid.h                 |  23 +-
> > >  4 files changed, 533 insertions(+), 105 deletions(-)
> > >
> > > --
> > > 2.33.1
> > >
> >
> > Does this patch series introduce the leaf driver support you mentioned
> > in the haptic review?
>