Bug 88134

Created attachment 112503 [details]
Add a Resolution option

Add "Resolution" option for mice to the evdev driver.

It can be used to scale the resolution of a mouse to that of a 1000 DPI mouse. This can be useful to make high resolution mice less sensitive without turning off acceleration. The target of 1000 DPI is used as the same default is used in libinput. If the option is not set no scaling will be done.

This is my first patch to X. Please review accordingly.

Comment on attachment 112503 [details]
Add a Resolution option

Just a couple of minor nitpicks, the rest looks good. Please remember to sign
off your patch though.

>--- a/man/evdev.man
>+++ b/man/evdev.man
>@@ -238,6 +238,10 @@ Default: "1".  Property: "Evdev Scrolling Distance".
> .BI "Option \*qDialDelta\*q \*q" integer \*q
> The amount of motion considered one unit of turning the dial.  Default: "1".
> Property: "Evdev Scrolling Distance".
>+.TP 7
>+.BI "Option \*qResolution\*q \*q" integer \*q
>+Sets the resolution of the device in counts per inch. The resolution is used
>+to scale relative motion events from mouse devices to 1000 DPI resolution.

this needs a bit more explanation to be really clear (especially about what
the default behaviour is). If it gets too verbose you can add a separate
section and refer to that if you want.

> 
> .SH SUPPORTED PROPERTIES
> The following properties are provided by the
>diff --git a/src/evdev.c b/src/evdev.c
>index 2d99f07..039abc5 100644
>--- a/src/evdev.c
>+++ b/src/evdev.c
>@@ -25,6 +25,7 @@
>  *	Adam Jackson (ajax@redhat.com)
>  *	Peter Hutterer (peter.hutterer@redhat.com)
>  *	Oliver McFadden (oliver.mcfadden@nokia.com)
>+ *	Thomas H.P. Andersen (phomes@gmail.com)
>  */
> 
> #ifdef HAVE_CONFIG_H
>@@ -462,6 +463,11 @@ EvdevProcessValuators(InputInfoPtr pInfo)
>             deltaY = tmp;
>         }
> 
>+        if (pEvdev->resolution > 0) {
>+            deltaX *= (double)DEFAULT_MOUSE_DPI / pEvdev->resolution;
>+            deltaY *= (double)DEFAULT_MOUSE_DPI / pEvdev->resolution;
>+        }
>+

if you change DEFAULT_MOUSE_DPI to be 1000.0, you don't need the double cast.

>         if (pEvdev->invert_x)
>             deltaX *= -1;
>         if (pEvdev->invert_y)
>@@ -2327,6 +2333,7 @@ EvdevProbe(InputInfoPtr pInfo)
>         pEvdev->invert_x = xf86SetBoolOption(pInfo->options, "InvertX", FALSE);
>         pEvdev->invert_y = xf86SetBoolOption(pInfo->options, "InvertY", FALSE);
>         pEvdev->swap_axes = xf86SetBoolOption(pInfo->options, "SwapAxes", FALSE);
>+        pEvdev->resolution = xf86SetIntOption(pInfo->options, "Resolution", 0);

needs error checking for negative resolutions and a resolution of 0 as
well. It's fine to print an error and reset to 0 for invalid cases, that
leaves the device useable and allows the user to fix up the bad config.

Created attachment 112571 [details] [review]
v2

Thanks for the review. Here is an updated patch for your comments.

It has a slightly longer description including the default value. Does that suffice or did you mean something as verbose as the description in libinput?

I added an error message if the resolution was set to a negative number and then set it back to 0. Instead of warning about a 0 resolution I documented it to mean "no scaling". I did this because 0 is the default value and it seemed strange to not allow that value explicitly as well.

almost pushed it and then I noticed: this doesn't work for high-res devices. I tried for a resolution option of 5500 (on Logitech G500s) and it makes the mouse unusable on lower resolutions: the scale factor is 0.18, so lower deltas get truncated to 0. we need to switch to doubles here, the various valuator mask functions have a _double equivalent. The server should be fine with that, just make sure that the rest of evdev is too.

ping?

Thanks for the ping. I will send an updated patch later this week.

Created attachment 117775 [details] [review]
v3

This version uses the _double variants. The normal calls with ints are actually just calling the _double variants on the server. So this should definitely be fine there.

I do not have a mouse with extremely high resolution but I have tested it with a setting of 5000 on a 1000 DPI mouse. There was no problem with that test.

Comment on attachment 117775 [details] [review]
v3

Review of attachment 117775 [details] [review]:
-----------------------------------------------------------------

::: src/evdev.c
@@ +466,4 @@
>          else
>              valuator_mask_unset(pEvdev->rel_vals, REL_Y);
>  
> +        Evdev3BEmuProcessRelMotion(pInfo, trunc(deltaX), trunc(deltaY));

this isn't enough, same as the previous problem. 3B emulation is the right-button emulation. If you truncate here, any high-res mouse will just pass 0 down to this function. That's what I meant in my previous comment "make sure that the rest of evdev is too.". it's a bit like a jenga tower :)

Rest of the patch is fine though.

Created attachment 117830 [details] [review]
v4

Thanks. I see the problem now.

This one works with right mouse emulation. Delta is now a double. I have kept threshold and startpos as ints as they are that format in the configuration and external api.

Thanks for your efforts and patience. All good, tested it, works, pushed.


commit 034be31159f22ce28d84994d541a45ee44963fd8
Author: Thomas Hindoe Paaboel Andersen <phomes@gmail.com>
Date:   Tue Jan 20 00:44:40 2015 +0100

    Add "Resolution" option for mice to the evdev driver