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

[David Revoy]

> 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.

[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