Message-ID: <CAO-hwJKVwZK00yZFjuyyR9Xt4Y2-r8eLJNZfnyeopHxoZQ0eGA@mail.gmail.com>
Date:   Tue, 7 Nov 2023 08:59:50 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Illia Ostapyshyn <ostapyshyn@....uni-hannover.de>
Cc:     David Revoy <davidrevoy@...tonmail.com>, jkosina@...e.cz,
        jason.gerecke@...om.com, jose.exposito89@...il.com,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        nils@...sfuhler.de, peter.hutterer@...-t.net, ping.cheng@...om.com,
        bagasdotme@...il.com
Subject: Re: Requesting your attention and expertise regarding a Tablet/Kernel issue

On Mon, Nov 6, 2023 at 9:06 PM Illia Ostapyshyn
<ostapyshyn@....uni-hannover.de> 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

Powered by blists - more mailing lists