| Message ID | 20221019151832.44522-1-jason.gerecke@wacom.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | a0f5276716c80415230b47e321e1c46357d70993 |
| Headers | show |
| Series | HID: Recognize "Digitizer" as a valid input application | expand |
On Wed, Oct 19, 2022 at 5:18 PM Gerecke, Jason <killertofu@gmail.com> wrote: > > From: Jason Gerecke <jason.gerecke@wacom.com> > > "Digitizer" is a generic usage that may be used by various devices but > which is particularly used by non-display pen tablets. This patch adds the > usage to the list of values matched by the IS_INPUT_APPLICATION() macro > that determines if an input device should be allocated or not. > > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> > Reviewed-by: Ping Cheng <ping.cheng@wacom.com> > --- > include/linux/hid.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/hid.h b/include/linux/hid.h > index 4363a63b9775..07803e144d98 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -883,7 +883,7 @@ static inline bool hid_is_usb(struct hid_device *hdev) > /* We ignore a few input applications that are not widely used */ > #define IS_INPUT_APPLICATION(a) \ > (((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \ > - || ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \ FWIW, this has always been problematic, and I am pretty sure this is breaking existing devices. Have you been running the hid-tools testsuite to see if there were any regressions? Cheers, Benjamin > + || ((a >= HID_DG_DIGITIZER) && (a <= HID_DG_WHITEBOARD)) \ > || (a == HID_GD_SYSTEM_CONTROL) || (a == HID_CP_CONSUMER_CONTROL) \ > || (a == HID_GD_WIRELESS_RADIO_CTLS)) > > -- > 2.38.0 >
On Wed, Oct 19, 2022 at 8:40 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Wed, Oct 19, 2022 at 5:18 PM Gerecke, Jason <killertofu@gmail.com> wrote: > > > > From: Jason Gerecke <jason.gerecke@wacom.com> > > > > "Digitizer" is a generic usage that may be used by various devices but > > which is particularly used by non-display pen tablets. This patch adds the > > usage to the list of values matched by the IS_INPUT_APPLICATION() macro > > that determines if an input device should be allocated or not. > > > > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> > > Reviewed-by: Ping Cheng <ping.cheng@wacom.com> > > --- > > include/linux/hid.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/hid.h b/include/linux/hid.h > > index 4363a63b9775..07803e144d98 100644 > > --- a/include/linux/hid.h > > +++ b/include/linux/hid.h > > @@ -883,7 +883,7 @@ static inline bool hid_is_usb(struct hid_device *hdev) > > /* We ignore a few input applications that are not widely used */ > > #define IS_INPUT_APPLICATION(a) \ > > (((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \ > > - || ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \ > > FWIW, this has always been problematic, and I am pretty sure this is > breaking existing devices. > > Have you been running the hid-tools testsuite to see if there were any > regressions? > > Cheers, > Benjamin > I was slightly worried that this usage might have been explicitly excluded for some compatibility reason, but I didn't see anything in the commit history that said that. I also had a hard time convincing myself that allocating an input device for a weird device where it is unnecessary would cause too much trouble. I didn't see any regressions when running the hid-tools testsuite. The output from the 5.15.74 kernel with / without the patch applied is almost identical (there are numerous test failures in test_tablet.py even in the unpatched case; maybe I should re-run with Linus's latest master instead?) Jason > > + || ((a >= HID_DG_DIGITIZER) && (a <= HID_DG_WHITEBOARD)) \ > > || (a == HID_GD_SYSTEM_CONTROL) || (a == HID_CP_CONSUMER_CONTROL) \ > > || (a == HID_GD_WIRELESS_RADIO_CTLS)) > > > > -- > > 2.38.0 > > >
Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... On Thu, Oct 20, 2022 at 7:48 AM Jason Gerecke <killertofu@gmail.com> wrote: > > On Wed, Oct 19, 2022 at 8:40 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > On Wed, Oct 19, 2022 at 5:18 PM Gerecke, Jason <killertofu@gmail.com> wrote: > > > > > > From: Jason Gerecke <jason.gerecke@wacom.com> > > > > > > "Digitizer" is a generic usage that may be used by various devices but > > > which is particularly used by non-display pen tablets. This patch adds the > > > usage to the list of values matched by the IS_INPUT_APPLICATION() macro > > > that determines if an input device should be allocated or not. > > > > > > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> > > > Reviewed-by: Ping Cheng <ping.cheng@wacom.com> > > > --- > > > include/linux/hid.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/linux/hid.h b/include/linux/hid.h > > > index 4363a63b9775..07803e144d98 100644 > > > --- a/include/linux/hid.h > > > +++ b/include/linux/hid.h > > > @@ -883,7 +883,7 @@ static inline bool hid_is_usb(struct hid_device *hdev) > > > /* We ignore a few input applications that are not widely used */ > > > #define IS_INPUT_APPLICATION(a) \ > > > (((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \ > > > - || ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \ > > > > FWIW, this has always been problematic, and I am pretty sure this is > > breaking existing devices. > > > > Have you been running the hid-tools testsuite to see if there were any > > regressions? > > > > Cheers, > > Benjamin > > > > I was slightly worried that this usage might have been explicitly > excluded for some compatibility reason, but I didn't see anything in > the commit history that said that. I also had a hard time convincing > myself that allocating an input device for a weird device where it is > unnecessary would cause too much trouble. > > I didn't see any regressions when running the hid-tools testsuite. The > output from the 5.15.74 kernel with / without the patch applied is > almost identical (there are numerous test failures in test_tablet.py > even in the unpatched case; maybe I should re-run with Linus's latest > master instead?) > > Jason > (Apologies for this doubled message, Benjamin -- I didn't "reply all" on my previous attempt...) I re-ran the tests with the Linus's latest 6.1-rc2 and have good results with / without the patch there. The test_tablet.py failures I previously saw no longer occur, so seem to be specific to stable. There are still two tests that fail regardless of if the patch is applied or not, but the patch doesn't seem to introduce any new failures of its own. Jason
> On Thu, Oct 20, 2022 at 7:48 AM Jason Gerecke <killertofu@gmail.com> wrote: > > > > On Wed, Oct 19, 2022 at 8:40 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > On Wed, Oct 19, 2022 at 5:18 PM Gerecke, Jason <killertofu@gmail.com> wrote: > > > > > > > > From: Jason Gerecke <jason.gerecke@wacom.com> > > > > > > > > "Digitizer" is a generic usage that may be used by various devices but > > > > which is particularly used by non-display pen tablets. This patch adds the > > > > usage to the list of values matched by the IS_INPUT_APPLICATION() macro > > > > that determines if an input device should be allocated or not. > > > > > > > > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com> > > > > Reviewed-by: Ping Cheng <ping.cheng@wacom.com> > > > > --- > > > > include/linux/hid.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/include/linux/hid.h b/include/linux/hid.h > > > > index 4363a63b9775..07803e144d98 100644 > > > > --- a/include/linux/hid.h > > > > +++ b/include/linux/hid.h > > > > @@ -883,7 +883,7 @@ static inline bool hid_is_usb(struct hid_device *hdev) > > > > /* We ignore a few input applications that are not widely used */ > > > > #define IS_INPUT_APPLICATION(a) \ > > > > (((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \ > > > > - || ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \ > > > > > > FWIW, this has always been problematic, and I am pretty sure this is > > > breaking existing devices. > > > > > > Have you been running the hid-tools testsuite to see if there were any > > > regressions? > > > > > > Cheers, > > > Benjamin > > > > > > > I was slightly worried that this usage might have been explicitly > > excluded for some compatibility reason, but I didn't see anything in > > the commit history that said that. I also had a hard time convincing > > myself that allocating an input device for a weird device where it is > > unnecessary would cause too much trouble. > > > > I didn't see any regressions when running the hid-tools testsuite. The > > output from the 5.15.74 kernel with / without the patch applied is > > almost identical (there are numerous test failures in test_tablet.py > > even in the unpatched case; maybe I should re-run with Linus's latest > > master instead?) > > > > Jason > > > > (Apologies for this doubled message, Benjamin -- I didn't "reply all" > on my previous attempt...) > > I re-ran the tests with the Linus's latest 6.1-rc2 and have good > results with / without the patch there. The test_tablet.py failures I > previously saw no longer occur, so seem to be specific to stable. > There are still two tests that fail regardless of if the patch is > applied or not, but the patch doesn't seem to introduce any new > failures of its own. > > Jason Still waiting to hear back about this. Are positive results from the hid-tools tests sufficient, or is there additional work that should be done for this patch? Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours....
Hi Jason, > Still waiting to hear back about this. Are positive results from the > hid-tools tests sufficient, or is there additional work that should be > done for this patch? A while ago a similar patch was sent [1] and it was in a similar status to your patch for a while, so I decided to fix the issue in the UCLogic driver [2]. I can not tell you if this patch needs additional work, but you might be interested in fixing it in the Wacom driver, where it'd be easier for you to test that no regressions are introduced. I hope this helps a bit, Jose [1] https://lore.kernel.org/linux-input/20220804151832.30373-1-openglfreak@googlemail.com/ [2] https://lore.kernel.org/linux-input/d08049f2-443b-f769-cfde-629cdfb96fc0@alexyzhang.dev/T/
Thanks for the response, Jose. Unfortunately this isn't an issue that we can fix in the Wacom driver since it affects devices that don't use the Wacom driver. While we could theoretically adopt affected VID:PIDs into the Wacom driver on a case-by-case basis, there would be a large time lag between us adopting a device and users running the required kernel. This should really be something that is fixed in the hid-generic driver, even if it means some pain trying to ensure we don't break things in the process... I'm still hoping to hear something back from Benjamin about my hid-tools test results or if additional testing is needed. Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... On Sat, Dec 3, 2022 at 3:43 AM José Expósito <jose.exposito89@gmail.com> wrote: > > Hi Jason, > > > Still waiting to hear back about this. Are positive results from the > > hid-tools tests sufficient, or is there additional work that should be > > done for this patch? > > A while ago a similar patch was sent [1] and it was in a similar status > to your patch for a while, so I decided to fix the issue in the UCLogic > driver [2]. > > I can not tell you if this patch needs additional work, but you might be > interested in fixing it in the Wacom driver, where it'd be easier for you > to test that no regressions are introduced. > > I hope this helps a bit, > Jose > > [1] https://lore.kernel.org/linux-input/20220804151832.30373-1-openglfreak@googlemail.com/ > [2] https://lore.kernel.org/linux-input/d08049f2-443b-f769-cfde-629cdfb96fc0@alexyzhang.dev/T/
Following up on this since it has been a while since the last attempt. I'm still hoping to get this patch merged if possible :) The patch still applies cleanly against the for-6.4/core branch and the hid-tools test suite still runs without any (new*) test failures when the patch is applied. Please let me know if there's anything I could / should do to continue moving this forward. *There are four failures in test_sony.py, but they occur on the for-6.4/core branch regardless of if the patch is applied or not. Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... On Thu, Dec 15, 2022 at 8:11 AM Jason Gerecke <killertofu@gmail.com> wrote: > > Thanks for the response, Jose. > > Unfortunately this isn't an issue that we can fix in the Wacom driver > since it affects devices that don't use the Wacom driver. While we > could theoretically adopt affected VID:PIDs into the Wacom driver on a > case-by-case basis, there would be a large time lag between us > adopting a device and users running the required kernel. This should > really be something that is fixed in the hid-generic driver, even if > it means some pain trying to ensure we don't break things in the > process... I'm still hoping to hear something back from Benjamin about > my hid-tools test results or if additional testing is needed. > > Jason > --- > Now instead of four in the eights place / > you’ve got three, ‘Cause you added one / > (That is to say, eight) to the two, / > But you can’t take seven from three, / > So you look at the sixty-fours.... > > On Sat, Dec 3, 2022 at 3:43 AM José Expósito <jose.exposito89@gmail.com> wrote: > > > > Hi Jason, > > > > > Still waiting to hear back about this. Are positive results from the > > > hid-tools tests sufficient, or is there additional work that should be > > > done for this patch? > > > > A while ago a similar patch was sent [1] and it was in a similar status > > to your patch for a while, so I decided to fix the issue in the UCLogic > > driver [2]. > > > > I can not tell you if this patch needs additional work, but you might be > > interested in fixing it in the Wacom driver, where it'd be easier for you > > to test that no regressions are introduced. > > > > I hope this helps a bit, > > Jose > > > > [1] https://lore.kernel.org/linux-input/20220804151832.30373-1-openglfreak@googlemail.com/ > > [2] https://lore.kernel.org/linux-input/d08049f2-443b-f769-cfde-629cdfb96fc0@alexyzhang.dev/T/
On Wed, 19 Oct 2022 08:18:32 -0700, Gerecke, Jason wrote: > "Digitizer" is a generic usage that may be used by various devices but > which is particularly used by non-display pen tablets. This patch adds the > usage to the list of values matched by the IS_INPUT_APPLICATION() macro > that determines if an input device should be allocated or not. > > Applied to hid/hid.git (for-6.4/core), thanks! [1/1] HID: Recognize "Digitizer" as a valid input application https://git.kernel.org/hid/hid/c/a0f5276716c8 Cheers,
diff --git a/include/linux/hid.h b/include/linux/hid.h index 4363a63b9775..07803e144d98 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -883,7 +883,7 @@ static inline bool hid_is_usb(struct hid_device *hdev) /* We ignore a few input applications that are not widely used */ #define IS_INPUT_APPLICATION(a) \ (((a >= HID_UP_GENDESK) && (a <= HID_GD_MULTIAXIS)) \ - || ((a >= HID_DG_PEN) && (a <= HID_DG_WHITEBOARD)) \ + || ((a >= HID_DG_DIGITIZER) && (a <= HID_DG_WHITEBOARD)) \ || (a == HID_GD_SYSTEM_CONTROL) || (a == HID_CP_CONSUMER_CONTROL) \ || (a == HID_GD_WIRELESS_RADIO_CTLS))