Bug 23126

Please attach your Xorg.log file and the output of lshal.

Created attachment 28329 [details]
lshal output

Created attachment 28330 [details]
Xorg log w/ 2.2.3

Created attachment 28331 [details]
Xorg log w/ 2.2.2

Please download http://people.freedesktop.org/~whot/evtest.c, compile it
with gcc -o evtest evtest.c and then run it as root against the devile file.
The device file is /dev/input/eventX, where X is a number. You can get the
right device file by looking into /proc/bus/input/devices, 

I think you might be a victim of Bug 21832, the only difference is the "ignoring absolute events" message. I need to see your device capabilities though (the evtest output).

Created attachment 28334 [details] [review]
Fix device initialization code to handle rel/abs axes better

I've attached a patch that should fix/improve the relative/absolute axes problem by taking touch{pads,screens} into account. It also includes better error checking.

(This is a first patch on evdev for me, so be kind if it's wrong; however, it *does* work for me and is noticeably better than before, because it eliminates a coordinates glitch).

*** Bug 17637 has been marked as a duplicate of this bug. ***

Created attachment 28340 [details] [review]
Sexier and safer (I hope) version of the first patch

This second patch has different formatting, is slightly more verbose, and includes an extra check as to whether !Success should be returned.

It seems to be working...

Created attachment 28341 [details]
evtest log

(In reply to comment #5)
> Please download http://people.freedesktop.org/~whot/evtest.c, compile it
> with gcc -o evtest evtest.c and then run it as root against the devile file.
> The device file is /dev/input/eventX, where X is a number. You can get the
> right device file by looking into /proc/bus/input/devices, 
> 

/proc/bus/input/devices includes

I: Bus=0011 Vendor=0002 Product=0008 Version=7301
N: Name="AlpsPS/2 ALPS DualPoint TouchPad"
P: Phys=isa0060/serio1/input0
S: Sysfs=/devices/platform/i8042/serio1/input/input9
U: Uniq=
H: Handlers=mouse1 event9 
B: EV=f
B: KEY=420 70000 0 0 0 0
B: REL=3
B: ABS=1000003

so that's event9. (With xf86-input-evdev installed, if that matters), running

  sudo ./evtest /dev/input/event9

and playing with the touchpad a bit yields the log I've attached.

(In reply to comment #10)
> so that's event9. (With xf86-input-evdev installed, if that matters), running

I meant to say "With xf86-input-evdev 2.2.2 installed".

(In reply to comment #9)
> Created an attachment (id=28340) [details]
> Sexier and safer (I hope) version of the first patch
> 
> This second patch has different formatting, is slightly more verbose, and
> includes an extra check as to whether !Success should be returned.
> 
> It seems to be working...

That patch appears to fix it!

Like Elias, I have an "AlpsPS/2 ALPS DualPoint TouchPad", and it registers with BOTH relative and absolute axes, yet seems to produce only absolute events (from my tests and from Elias's evtest output)---presumably because it is a touchpad (I presume this, because the evdev code shows that a prerequisite to being a touchpad or touchscreen is having absolute axes).

Here's what I don't quite understand: Why does this device register as having relative axes?

Created attachment 28355 [details] [review]
0001-Hack-around-touchpads-with-relative-axes-23126.patch

simplified version of your patch.

The patch causing this regression: "evdev: Prevent driver from processing motion events that it has not configured. #21832"
The internal path before this patch was:
- device set up with relative axes, absolute ignored
- absolute events come in
- absolute events are converted to relative and posted as relative

Now we ignore events that we haven't configured the axes for, i.e. if we don't configure the absolute axes all abs events are ignored.
Have a look at this patch, it's based on your idea but simplifies the whole thing a bit. All we need to do for touchpads is unset the relative bit and hope that the abs axis stuff picks it up then.

I also removed the touchscreen handling. if a touchscreen has absolute and relative at the same time then we need to sort this out but for 2.2.4 i'd like to keep the behaviour as-is.

> Have a look at this patch, it's based
> on your idea but simplifies the whole
> thing a bit. All we need to do for
> touchpads is unset the relative bit
> and hope that the abs axis stuff picks
> it up then.

Your version certainly makes the *patch* simpler, but that's because it doesn't touch the existing code; you'll notice that my first patch is exactly the same as your patch, except that I

* include EVDEV_TOUCHSCREEN
* link the conditionals with an `else if' (thereby avoiding a redundant check)
* add some error checking at the bottom

The second patch I submitted expanded upon this by

* linking all conditionals together (thereby avoiding redundant checks).
* making the logic paths explicit (but still quite compact), which helps comprehension of what's going on.
* adding one last error-check, which was simpler and cleaner with the explicit logic paths.
* cleaning up the formatting.

The relevant code that results from your patch is 22 lines long, while the code that results from my patch is 36 lines long---approximately 12 of which are blank lines to aid reading.

My vote is still with my second patch (with EVDEV_TOUCHSCREEN removed, perhaps); I think it's not just a fix, but also a general improvement.

> I also removed the touchscreen handling.
> if a touchscreen has absolute and
> relative at the same time then we need
> to sort this out but for 2.2.4 i'd like
> to keep the behaviour as-is.

However, the AlpsPS/2 DualPoint TouchPad (which gave both Elias and me the same trouble) also advertises itself as having relative *and* absolute axes, but then only generates absolute events. Are we to assume that this touchpad advertises itself incorrectly? Are all touchpads actually solely absolute?

Currently the driver initialization code only labels a device as an EVDEV_TOUCHPAD or EVDEV_TOUCHSCREEN if it has_abs_axes [see EvdevProbe()]. Is this incorrect (for touchscreens at least)?

sorry, I didn't look at the first patch since it was obsoleted by the second one by the time I got to it.

on general terms, the importance of keeping a patch simple is high. in this case, with reformatting plus indentation changes I had to apply the patch, look at the resulting code and compare it to the existing code. If the patch is simpler it eases review a great deal. even more so as this will be a fix to address a regression on the stable branch.

anyway, you're changing the behaviour of the driver itself. Previously, a device could have failed setting up the relative axis and succeeded with the absolute axis. more so, a device that had buttons and/or keys would have worked even if the axes failed to initialize. After your patch, this isn't possible anymore since the device fails init.

To give you one example of a device that is affected by this: the griffin powermate only has one single axis, REL_DIAL. this axis is however counted as scroll button and EvdevAddRelClass always fails on it. With your patch applied, this device would always fail initialization. This isn't really acceptable when addressing a regression on a stable branch.

> However, the AlpsPS/2 DualPoint TouchPad (which gave both Elias and me the same
> trouble) also advertises itself as having relative *and* absolute axes, but
> then only generates absolute events. Are we to assume that this touchpad
> advertises itself incorrectly? Are all touchpads actually solely absolute?

the single exception I've seen so far is a touchpad integrated with a keyboard. this one only advertises REL_X/Y and nothing else. Synaptics and appletouch have ABS_X/Y. So this seems to be a case of the Alps being special for some reason.
 
> Currently the driver initialization code only labels a device as an
> EVDEV_TOUCHPAD or EVDEV_TOUCHSCREEN if it has_abs_axes [see EvdevProbe()]. Is
> this incorrect (for touchscreens at least)?
 
no, I haven't seen a touchscreen yet that has relative axes. It wouldn't make much sense anyway since touchscreens are (usually) direct-touch interfaces.

Created attachment 28364 [details] [review]
The sexy version, but now clear bits instead of failing

> anyway, you're changing the behaviour
> of the driver itself. Previously, a
> device could have failed setting up
> the relative axis and succeeded with
> the absolute axis.

Actually, the patch allows that, but...

> more so, a device
> that had buttons and/or keys would have
> worked even if the axes failed to
> initialize. After your patch, this isn't
> possible anymore since the device fails
> init.

Good point! However, it seems odd just to ignore errors. How about replacing `return !Success;' with code that clears the relevant bits?

See the attached patch, which applies on top of my second patch (so that the patch is very simple, though there is a little noise because I swapped some lines to be more consistent with the flow of logic from a reader's point of view).

> no, I haven't seen a touchscreen yet that
> has relative axes. It wouldn't make much
> sense anyway since touchscreens are
> (usually) direct-touch interfaces.

Therefore, I've left EVDEV_TOUCHSCREEN in this latest patch. Why did you want to remove it again?

Created attachment 28388 [details] [review]
Patch 04: Corrected logging types, abstracted logic into 3 functions, simpler from reuse.

I think you'll like this version of the patch; it's simpler and the logging is better.

****

This patch applies directly to the HEAD of master (69d6ff3e01263ce2d52ed18b08f054bf3fdb923c):

evdev: Use the EvdevPost...Event() functions in the emulation code.

****

In particular, the code for initializing abs/rel axes has been abstracted out into 3 functions, so that initialization in EvdevInit(device) is as easy as:

    if (pEvdev->flags & (EVDEV_TOUCHPAD | EVDEV_TOUCHSCREEN))
        EvdevInitTouchDevice(device, pEvdev);
    else if (pEvdev->flags & EVDEV_RELATIVE_EVENTS)
        EvdevInitRelClass(device, pEvdev);
    else if (pEvdev->flags & EVDEV_ABSOLUTE_EVENTS)
        EvdevInitAbsClass(device, pEvdev);

excellent, thanks. Pushed as f352598e45be86f9e24d9dba88c657f03f3b168e.
Closing this bug as fixed, 2.2.4 will be out shortly.