Messages in this thread | | | Date | Sat, 8 Oct 2022 09:34:37 -0700 | From | Eirin Nya <> | Subject | Re: [PATCH 3/3] Input: Fix incorrectly halved touchpad range on ELAN v3 touchpads |
| |
On Fri, 07 Oct 2022 16:06:08 +0200 Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:
> On Thu, Sep 29, 2022 at 01:21, Eirin Nya <nyanpasu256@gmail.com> > wrote: > > > On Linux 5.19.10, on my laptop (Dell Inspiron 15R SE 7520) with an > > Elan v3 touchpad (dmesg says "with firmware version 0x450f02"), the > > reported size of my touchpad (in userspace by calling > > mtdev_configure() and libevdev_get_abs_maximum(), in kernel space > > elantech_device_info::x_max/y_max, either way 1470 by 700) is half > > that of the actual touch range (2940 by 1400), and the upper half > > of my touchpad reports negative values. As a result, with the > > Synaptics or libinput X11 driver set to edge scrolling mode, the > > entire right half of my touchpad has x-values past evdev's reported > > maximum size, and acts as a giant scrollbar! > > > > The problem is that elantech_setup_ps2() -> > > elantech_set_absolute_mode() sets up absolute mode and doubles the > > hardware resolution (doubling the hardware's maximum reported x/y > > coordinates and its response to ETP_FW_ID_QUERY), *after* > > elantech_query_info() fetches the touchpad coordinate system size > > using ETP_FW_ID_QUERY, which gets cached and reported to userspace > > through ioctl(fd, EVIOCGABS(ABS_X/Y), ...). So the touchpad size > > reported to userspace (and used to subtract vertical coordinates > > from) is half the maximum position of actual touches. > > ... > > This seems like a candidate patch for stable kernels as well. > > Maybe consider adding the following in the commit message footer: > Fixes: 28f49616113f ("Input: elantech - add v3 hardware support")
Interestingly, if I understand correctly, the bug was actually *not* introduced by 28f49616113f ("Input: elantech - add v3 hardware support"). In that commit, elantech_init called (elantech_set_absolute_mode -> elantech_write_reg(...0x0b)) to set absolute double-size mode, though bit 3 controlling double-resolution was not documented at this point, then (elantech_set_input_params -> elantech_set_range -> ETP_FW_ID_QUERY) *after* enabling double-size mode. This code structure was maintained through 36189cc3cd57 ("Input: elantech - fix touchpad initialization on Gigabyte U2442"), which introduced etd->set_hw_resolution.
By the time of f07875920116 ("Input: elantech - split device info into a separate structure"), elantech_setup_ps2 called (elantech_set_absolute_mode -> elantech_write_reg(...0x0b or 0x01) (later changed to 0x03)) before (elantech_set_input_params -> elantech_set_range -> ETP_FW_ID_QUERY).
The bug was actually introduced by 37548659bb22 ("Input: elantech - query the min/max information beforehand too"), which removed elantech_set_range() and moved ETP_FW_ID_QUERY into elantech_query_info(). At this point, elantech_init_ps2() for non-SMBus touchpads calls (elantech_query_info() -> ETP_FW_ID_QUERY) before (elantech_setup_ps2 -> elantech_set_absolute_mode -> elantech_write_reg), so now the size is queried *before* setting doubled size for the touchpad.
Why was this change made? The commit message says:
> For the latest generation of Elantech touchpads, we need to forward > the min/max information from PS/2 to SMBus.
I don't know which generation of touchpad that is, and whether it correlates with protocol version 3 vs. 4 or not. I'm guessing that in the process of adding/fixing support for newer touchpads, Benjamin Tissoires subtly broke size queries for older v3 PS/2 touchpads. At this point, is it worth asking him about this bug?
In any case, given the complexity of this code designed to handle multiple hardware types, including SMBus Elan touchpads I cannot test myself, I now feel that my surgical patch is the safest way to fix v3 touchpads. It's probably error-prone to *conditionally* query size later on in elantech_set_input_params for v3 (and below?) touchpads, without breaking hardware fixed by 37548659bb22.
Should we move (elantech_set_absolute_mode -> elantech_write_reg(...0x0b or 0x01)) *earlier* into elantech_query_info() before "query range information"? I don't know if that will break anything either, since I can't test on anything but v3 touchpads. But it's an option to explore if you want to avoid querying range twice.
| |