Bug 88134 - Add Resolution option
Summary: Add Resolution option
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Input/evdev (show other bugs)
Version: git
Hardware: Other All
: medium enhancement
Assignee: Thomas H.P. Andersen
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-07 05:12 UTC by Peter Hutterer
Modified: 2015-08-27 05:05 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Add a Resolution option (2.98 KB, text/plain)
2015-01-19 23:47 UTC, Thomas H.P. Andersen
no flags Details
v2 (3.33 KB, patch)
2015-01-20 23:47 UTC, Thomas H.P. Andersen
no flags Details | Splinter Review
v3 (4.67 KB, patch)
2015-08-18 21:09 UTC, Thomas H.P. Andersen
no flags Details | Splinter Review
v4 (6.90 KB, patch)
2015-08-20 21:10 UTC, Thomas H.P. Andersen
no flags Details | Splinter Review

Description Peter Hutterer 2015-01-07 05:12:00 UTC
Add a "resolution" option to evdev to scale relative motion events from mouse devices into some normalized resolution. This would make it possible to set the resolution for high-res mice, making them more useful without having to turn accel all the way down (or using ConstantDeceleration).

Default should be unset so we don't break any setups.
Comment 1 Thomas H.P. Andersen 2015-01-19 23:47:59 UTC
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 2 Peter Hutterer 2015-01-20 01:30:30 UTC
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.
Comment 3 Thomas H.P. Andersen 2015-01-20 23:47:11 UTC
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.
Comment 4 Peter Hutterer 2015-01-22 23:38:06 UTC
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.
Comment 5 Peter Hutterer 2015-08-12 05:08:44 UTC
ping?
Comment 6 Thomas H.P. Andersen 2015-08-17 13:56:12 UTC
Thanks for the ping. I will send an updated patch later this week.
Comment 7 Thomas H.P. Andersen 2015-08-18 21:09:51 UTC
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 8 Peter Hutterer 2015-08-19 06:12:12 UTC
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.
Comment 9 Thomas H.P. Andersen 2015-08-20 21:10:18 UTC
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.
Comment 10 Peter Hutterer 2015-08-27 05:05:01 UTC
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


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.