| Message ID | 20220126161832.3193805-1-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | HID: fix for generic input processing | expand |
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?
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?
On Tue, Feb 8, 2022 at 8:20 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. Sorry, no timeline on when we will be able to merge this. I was enjoying some time off last week, but now I need to check whether the comments I received need a v3 or not. > > > 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? > As mentioned above, no, this patch series does not have the multitouch support in it. Let me answer your other email you sent while I am typing this one :) Cheers, Benjamin
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? >