Re: Requesting your attention and expertise regarding a Tablet/Kernel issue

[Benjamin Tissoires]

On Thu, Nov 9, 2023 at 12:19 AM David Revoy <davidrevoy@xxxxxxxxxxxxxx> wrote:
>
> > BTW, David, were you able to do a revert of 276e14e6c3?
>
> I'm sorry Benjamin: I did some research on how to build a kernel [1], on how to revert a commit (easy, I know a bit of Git), and started following it step by step. Result: I failed and concluded that it probably requires too much computer knowledge compared to what I can do now. I'm afraid I won't be able to build a custom kernel for testing.

No worries. And I'm actually happy, because you definitely fit into
the HID-BPF model where I want to fix a user's device without
requiring kernel compilation, and fixing the device in a reliable way
that we can do the general fix without impacting the reporter.

Cheers,
Benjamin

>
> [1] https://docs.fedoraproject.org/en-US/quick-docs/kernel-build-custom/#_building_a_vanilla_upstream_kernel
>
>
> On Tuesday, November 7th, 2023 at 08:59, Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote:
>
>
> > On Mon, Nov 6, 2023 at 9:06 PM Illia Ostapyshyn
> > ostapyshyn@xxxxxxxxxxxxxxxxxxx wrote:
> >
> > > On 11/6/23 17:59, Benjamin Tissoires wrote:
> > >
> > > > If the pen has 2 buttons, and an eraser side, it would be a serious
> > > > design flow for XPPEN to report both as eraser.
> > > >
> > > > Could you please use sudo hid-recorder from hid-tools[1] on any kernel
> > > > version and send us the logs here?
> > > > I'll be able to replay the events locally, and understand why the
> > > > kernel doesn't work properly.
> > > >
> > > > And if there is a design flaw that can be fixed, we might even be able
> > > > to use hid-bpf to change it :)
> > >
> > > My wild guess is that XP-Pen 16 Artist Pro reports an Eraser usage
> > > without Invert for the upper button and Eraser with Invert for the
> > > eraser tip. A device-specific driver could work with that, but there
> > > seems to be no way to incorporate two different erasers (thus, allowing
> > > userspace to map them to different actions arbitrarily) in the generic
> > > driver currently.
> >
> >
> > That's exactly why I want to see the exact event flow. We can not do
> > "wild guesses" unfortunately (not meaning any offenses).
> > And I am very suspicious about the fact that the stylus reports 2
> > identical erasers. Because in the past David seemed to be able to have
> > 2 distincts behaviors for the 2 "buttons" (physical button and eraser
> > tail).
> >
> > > > Generally speaking, relying on X to fix your hardware is going to be a
> > > > dead end. When you switch to wayland, you'll lose all of your fixes,
> > > > which isn't great.
> > >
> > > > AFAIU, the kernel now "merges" both buttons, which is a problem. It
> > > > seems to be a serious regression. This case is also worrying because I
> > > > added regression tests on hid, but I don't have access to all of the
> > > > various tablets, so I implemented them from the Microsoft
> > > > specification[0]. We need a special case for you here.
> > >
> > > The issue preventing David from mapping HID_DG_ERASER to BTN_STYLUS2 is
> > > that the hidinput_hid_event is not compatible with hidinput_setkeycode.
> > > If usage->code is no longer BTN_TOUCH after remapping, it won't be
> > > released when Eraser reports 0. A simple fix is:
> >
> >
> > I must confess, being the one who refactored everything, I still don't
> > believe this is as simple as it may seem. I paged out all of the
> > special cases, and now, without seeing the event flow I just can not
> > understand why this would fix the situation.
> >
> > And BTW, if you have a tool affected by 276e14e6c3, I'd be curious to
> > get a hid-recorder sample for it so I can get regression tests for it.
> >
> > > --- a/drivers/hid/hid-input.c
> > > +++ b/drivers/hid/hid-input.c
> > > @@ -1589,7 +1589,7 @@ void hidinput_hid_event(struct hid_device *hid,
> > > struct hid_field field, struct
> > > / value is off, tool is not rubber, ignore */
> > > return;
> > > else if (*quirks & HID_QUIRK_NOINVERT &&
> > > - !test_bit(BTN_TOUCH, input->key)) {
> > > + !test_bit(usage->code, input->key)) {
> >
> >
> > I don't want to be rude, but this feels very much like black magic,
> > especially because there is a comment just below and it is not
> > updated. So either the explanation was wrong, or it's not explaining
> > the situation (I also understand that this is not a formal submission,
> > so maybe that's the reason why the comment is not updated).
> >
> > > /*
> > > * There is no invert to release the tool, let hid_input
> > > * send BTN_TOUCH with scancode and release the tool after.
> > >
> > > This change alone fixes David's problem and the right-click mapping in
> > > hwdb works again. However, the tool switches to rubber for the remapped
> > > eraser (here BTN_STYLUS2) events, both for devices with and without
> > > Invert. This does no harm but is not useful either. A cleaner solution
> > > for devices without Invert would be to omit the whole tool switching
> > > logic in this case:
> > >
> > > @@ -1577,6 +1577,9 @@ void hidinput_hid_event(struct hid_device *hid,
> > > struct hid_field *field, struct
> > >
> > > switch (usage->hid) {
> > > case HID_DG_ERASER:
> > > + if (*quirks & HID_QUIRK_NOINVERT && usage->code != BTN_TOUCH)
> > > + break;
> > > +
> > > report->tool_active |= !!value;
> > >
> > > Remapping Invert does not work anyway as the Invert tool is hardcoded in
> > > hidinput_hid_event. Even worse, I guess (not tested) trying to do so
> > > would mask BTN_TOOL_RUBBER from dev->keybit and could cause weird
> > > behavior similar to one between 87562fcd1342 and 276e14e6c3. This
> > > raises the question: should users be able to remap Invert after all?
> >
> >
> > The kernel is supposed to transfer what the device is. So if it says
> > this is an eraser, we should not try to change it. Users can then
> > tweak their own device if they wish through hid-bpf or through
> > libinput quirks, but when you install a fresh kernel without tweaks,
> > we should be as accurate as possible.
> >
> > My main concern is that now we have a device which exports 2 different
> > interactions as being the same. So either the firmware is wrong, and
> > we need to quirk it, or the kernel is wrong and merges both, and this
> > needs fixes as well.
> >
> > Once every interaction on the device gets its own behavior, userspace
> > can do whatever they want. It's not the kernel's concern anymore.
> >
> > BTW, David, were you able to do a revert of 276e14e6c3?
> >
> > Cheers,
> > Benjamin
>