It appears that the change in the input code where hotplugging was a part removed the call to the two conversion functions for the x&y axis defined in the device driver (reverse_conversion_proc and conversion_proc) and thus forcing the driver to scale the reported axis prior and not during event generation. This is a semantic change in the behavior of the XInput devices, has at least two drawbacks: * It is now impossible to report subpixel motion as an extension event as the the x&y axis must be reported after scaling. * The InputDevice must match its x&y axis to the screen resolution and therefore also must change these when the screen resolution changes (and notify all applications about it). One idea has been raised to generate the events, and afterwards scale the x&y axis in the genereated events. But as GetPointerEvents also moves the pointer on the screen the x&y must be in screen resolution and if the extension events should not loose precision when restoring the original x&y a backing store is needed inside the device driver. Another idea was to introduce a pair of new conversion functions that will replace the old ones. The included patch adds these two conversion functions to the DeviceIntRec structure so the conversion functions can be called during event generation, as before but now in the dix-code. See also the discussion on the xorg-mailinglist: http://lists.freedesktop.org/archives/xorg/2007-March/022288.html http://lists.freedesktop.org/archives/xorg/2007-March/022534.html
Created attachment 9204 [details] [review] Reintroduction of scaling in GetPointerEvents
* It is now impossible to report subpixel motion as an extension event as the the x&y axis must be reported after scaling. Speaking of that, is there any code in existence that would emit such an event? I'm asking since my patch bug#8583 could take advantage of such a thing.
(In reply to comment #2) > * It is now impossible to report subpixel motion as an extension event as the > the x&y axis must be reported after scaling. > > Speaking of that, is there any code in existence that would emit such an event? > I'm asking since my patch bug#8583 could take advantage of such a thing. Indeed there is. Wacom tablets have a higher resolution than the screen (from 3200*2300 (low-end) to 97000*61000 (high-end)), and the wacom driver report the full range of those axis today without scaling.
hi, the placement of reverse_conversion_proc doesn't seem quite right to me: it's only in the relative branch, and it always gets called with cp, instead of srcp. the other concern is the call to miPointerSetPosition being moved: as you have it, the motion history and last{x,y} will always remain unclipped, whereas we rely on the call being where it is now to clip the co-ordinates before it gets its way into history. other than that, the patch seems fine, including the cleanups. thanks!
(In reply to comment #4) Calling reverse_conversion_proc makes only any sense in relative mode. It's in that case where you need to find out where the pointer actually are in the input device coordinate system before moving it a little bit. In absolute mode we really don't care where it's been. We have a very specific point where it's supposed to be, so why bother with the old position? The use of cp in the reverse_conversion_proc is intentional. We should only convert the core coordinates. But you may have a point.. If we shouldn't send any core-events, we should probably be using the current coordinates from the input device instead of converting them from the core. Maybe always calling miPointerSetPosition confused me here. Should we really be moving the pointer even if we're not sending any core-events? Having updateMotionHistory where it is was based on that is should keep track of the motion history in the input device coordinate system, and in that case the x&y axis will be clipped by clipAxis a few lines up. If that is a faulty understanding it's definitely in the wrong place...
Created attachment 13491 [details] [review] Rescale patch without any interface changes
1. check is only for max > 0. I think it should be something like (min != -1 && max != -1) 2. we need a check that (max - min + 1) != 0, otherwise things can turn sour (division by 0). same thing in the VCP converstion. 3. lastx/y is only used here, nowhere else. I'd say it'd be best to store the unconverted coordinates (for the device anyway, VCP needs to store the converted obviously) 1 is a bit of a danger though, there may be devices that have -1 as min value. but then, there may be devices that only report negative coordinates?
I've had some more thoughts about the whole GetPointerEvent function, and there are a few things that should be addressed before I'm satisfied with it. * Arbitrary value ranges (scale if min < max, no limits on actual reported values) * Clip axis only in absolute mode or if not sending core events * Clip axis only if a value range is defined (min < max) * Should miPointerSetPosition ever be called for events that does not send core events? Feels a bit off where it is to be honest. (In reply to comment #7) > 1. check is only for max > 0. I think it should be something like (min != -1 && > max != -1) > 2. we need a check that (max - min + 1) != 0, otherwise things can turn sour > (division by 0). same thing in the VCP converstion. Arbitrary value ranges will take care of these two. > 3. lastx/y is only used here, nowhere else. I'd say it'd be best to store the > unconverted coordinates (for the device anyway, VCP needs to store the > converted obviously) You've lost me here; Is the current situation where pDev has the unconverted and cp the converted values the same as you describe above? > 1 is a bit of a danger though, there may be devices that have -1 > as min value. but then, there may be devices that only report > negative coordinates? Done right there's no danger at all.. I don't know any devices that does, but we can do something that will work with that kind too easily. I've started with changes that address these changes, I'll just clean them up, test them, and add them to the list here.
(In reply to comment #8) [...] > * Arbitrary value ranges (scale if min < max, no limits on actual reported > values) 'no limits on actual defined min and max' was what I meant..
Created attachment 13591 [details] [review] dix: Always add valuator information if present in GetPointerEvents See discussion on xorg-ml, this is a prereq for the next two patches. http://lists.freedesktop.org/pipermail/xorg/2007-December/031195.html
Created attachment 13592 [details] [review] dix: Do not call clipAxis for relative pointer events
Created attachment 13593 [details] [review] dix: Allow arbitrary value ranges in GetPointerEvents
(In reply to comment #10) > Created an attachment (id=13591) [details] > dix: Always add valuator information if present in GetPointerEvents ACK
(In reply to comment #11) > Created an attachment (id=13592) [details] > dix: Do not call clipAxis for relative pointer events when you're re-scaling pDev->lastx, shouldn't it be scr->width, not scr->height? otherwise ACK.
(In reply to comment #12) > Created an attachment (id=13593) [details] > dix: Allow arbitrary value ranges in GetPointerEvents > ACK
Created attachment 13624 [details] [review] Use right screen dimension in calculation after screen crossing (In reply to comment #14) > (In reply to comment #11) > > Created an attachment (id=13592) [details] [details] > > dix: Do not call clipAxis for relative pointer events > > when you're re-scaling pDev->lastx, shouldn't it be scr->width, not > scr->height? > otherwise ACK. Yes, it should be width not height, thanks!
Two more things that I've seen in the function. Can GetPointerEvents ever be called with the virtual core pointer as the reporting device? Should we call miPointerSetPosition for devices that are not reporting core-events? (== moving a pointer around on the screen?)
(In reply to comment #17) > Two more things that I've seen in the function. > > Can GetPointerEvents ever be called with the virtual core pointer as the > reporting device? > > Should we call miPointerSetPosition for devices that are not reporting > core-events? (== moving a pointer around on the screen?) Amusingly, your patch exposed a comment that is now very much incorrect. There's now more than one mi pointer, so really, all the miPointer functions (including SetPosition) should be always called per-device, and only called for the core pointer if we're sending a core event.
(In reply to comment #17) > Two more things that I've seen in the function. > > Can GetPointerEvents ever be called with the virtual core pointer as the > reporting device? yes, in response to a WarpPointer request for example. Look at miPointerMove in mi/mipointer.c. > Should we call miPointerSetPosition for devices that are not reporting > core-events? (== moving a pointer around on the screen?) no. (see daniel's comment).
(In reply to comment #19) > > Should we call miPointerSetPosition for devices that are not reporting > > core-events? (== moving a pointer around on the screen?) > > no. (see daniel's comment). But it still works as I expect it to.. Ok, that function will not do anything if the provided device should not send any core events. I leave the comment as it is, as I don't know what should be written instead..
Created attachment 13651 [details] [review] Add scaling to absolute reporting with missing x&y
Created attachment 13652 [details] [review] Move motion history update until after screen crossing and clipping
Created attachment 13653 [details] [review] Store coords when crossing screen or clipping for VCP
Created attachment 13679 [details] [review] dix: Call clipAxis for relative events in GetPointerEvents
(In reply to comment #21) > Created an attachment (id=13651) [details] > Add scaling to absolute reporting with missing x&y > ACK
(In reply to comment #22) > Created an attachment (id=13652) [details] > Move motion history update until after screen crossing and clipping > I think this is wrong. you also have to update the motion history if the event is != MotionNotify. Absolute devices may just send a button press at a different location, without a motion event first. In this case the motion history should still be updated I guess.
as for the others, it's getting a bit hard for me to keep track of consecutive diffs. daniels, any reason why magnus can't fix the remaining stuff on git master directly?
i don't have the bandwidth to look at it at the moment, but if you're happy with it, peter, then i don't see any reason to keep it out of master. after all, if it's broken, we can always fix it up later; cf. input-hotplug. ;)
Maybe I got a bit carried away with the motion history.. Figuring out which part of the old behaviour that was changed deliberately is not always easy. Yes, please add them to master if you think they're good enough. I've been waiting for this moment :) I hope they will eliminate more bugs than they will introduce. If it's possible to get the rescale patch included in the 1.4-branch I would be very grateful as that would mean that the workarounds in the linuxwacom driver that we had to add could be removed (and we can use the full potential of our tablets in X.org again).
(In reply to comment #28) > i don't have the bandwidth to look at it at the moment, but if you're happy > with it, peter, then i don't see any reason to keep it out of master. after > all, if it's broken, we can always fix it up later; cf. input-hotplug. ;) I think it's good enough, although lacking devices I haven't had a chance to fix it. and if we wait until it's perfect we're not going anywhere. especially since git master is a moving target. The other thing is that i-h has shown that most people who run master don't have absolute devices anyway, otherwise we'd have spotted this earlier :) so I think it's safe to get magnus an account and let him merge it. (In reply to comment #29) > Maybe I got a bit carried away with the motion history.. Figuring out which > part of the old behaviour that was changed deliberately is not always easy. > > Yes, please add them to master if you think they're good enough. I've been > waiting for this moment :) I hope they will eliminate more bugs than they will > introduce. > > If it's possible to get the rescale patch included in the 1.4-branch I would be > very grateful as that would mean that the workarounds in the linuxwacom driver > that we had to add could be removed (and we can use the full potential of our > tablets in X.org again). Magnus, please request a fdo account. See http://www.freedesktop.org/wiki/AccountRequests
A slightly modified patchset (so they can more easily be cherry-picked) are now committed to master. The change the patches does are the same though. Thanks for the review of my patches!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.