Testers wanted: Synaptics acceleration patch

[Peter Hutterer]

I've had this running on my laptop pretty much since you sent out the patch
and given that I was at a conference and on holidays after, I pretty much
exclusively used the touchpad for more than two weeks.

My first impression was - it's slow. Much slower than the current method and
while I played around with xset initially to make it faster, I eventually
stopped doing that, frequent reboots make it cumbersome. There's some
additional knobs I guess that I can use, but I haven't tried tweaking them
yet.

That aside, I like it a lot. I have a lot more control over the pointer,
don't overshoot anymore and generally the pointer now feels it's doing what
it should. I didn't realize how much I got used to it until a recent Fedora
update changed my test setup and went back to the old accel.


On Sat, Jan 16, 2010 at 06:40:59PM +0100, Simon Thum wrote:
> this patch fixes the long-standing issue of synaptics being accelerated
> twice, once by itself and once by the dix, based on different settings.
> This typically makes synaptics feel awkward, since one needs a careful
> setup to avoid the issue.
> 
> This patch fixes that by taking most of acceleration out of the driver,
> utilizing the accel framework in dix. It should behave quite the same,
> except that the double-accel is now missing, so you may need to adapt to
> that.
> 
> Unfortunately, I don't have matching a HW+SW to test at the moment. A
> previous version of that patch did work though. So I ask for volunteers,
> and the maintainers to pick it up when feedback is positive.
> 
> Prerequisites:
> X.Org Server 1.7.0+ or git master
> xf86-input-synaptics from git (1.2.0 may be fine too) plus patch
> 
> 
> What to look out for:
> 
> If you are fine with your synaptics, and it stays like that after issuing
> 
>   xset m 1 1
> 
> (or selecting the none (-1) accel profile using xinput, for that matter)
> then this patch should not change much for you, because your setup is
> already good. A small change in behaviour is acceptable, and should be
> for the better. In particular, pressure-sensitive acceleration should
> remain working, but AFAIK that feature isn't on by default.
> 
> If the xset m ... changed synaptics behaviour, then the patch should
> change it likewise. You can readjust as usual. In the end, it should
> behave a bit more predictable.
> 
> Anyone who feels qualified, please test it and give feedback!
> 

> From 0c30a80cb0e32b89a93c2497ebd7ef97652cf211 Mon Sep 17 00:00:00 2001
> From: Simon Thum <simon.thum at gmx.de>
> Date: Wed, 9 Sep 2009 14:41:08 +0200
> Subject: [PATCH] Setup pointer acceleration for synaptics
> 
> Setup dix pointer accel from the synaptics driver so synaptics devices
> behave like before while benefiting from dix velocity approximation.
> 
> This fixes the longstanding issue with synaptics being
> accelerated twice with different algorithms. The pressure-dependent
> synaptics acceleration is now performed in a device-specific profile.
> 
> Signed-off-by: Simon Thum <simon.thum at gmx.de>
> ---
>  src/synaptics.c |  123 +++++++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 88 insertions(+), 35 deletions(-)
> 
> diff --git a/src/synaptics.c b/src/synaptics.c
> index 0fdc496..93f716d 100644
> --- a/src/synaptics.c
> +++ b/src/synaptics.c
> @@ -80,6 +80,7 @@
>  #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7
>  #include <X11/Xatom.h>
>  #include <xserver-properties.h>
> +#include <ptrveloc.h>
>  #endif
>  
>  typedef enum {
> @@ -543,6 +544,45 @@ static void set_default_parameters(LocalDevicePtr local)
>      }
>  }
>  
> +static float SynapticsAccelerationProfile
> +    (DeviceIntPtr dev,
> +     DeviceVelocityPtr vel,
> +     float velocity,
> +     float thr,
> +     float acc) {

This is not the coding style used in most of the driver, should be
static float foobar(param 1,
                    param 2)
{


> +    LocalDevicePtr local = (LocalDevicePtr) dev->public.devicePrivate;
> +    SynapticsPrivate *priv = (SynapticsPrivate *) (local->private);
> +    SynapticsParameters* para = &priv->synpara;
> +
> +    double accelfct;
> +
> +    /* speed up linear with finger velocity */
> +    accelfct = velocity * para->accl;
> +
> +    /* clip acceleration factor */
> +    if (accelfct > para->max_speed)
> +	accelfct = para->max_speed;
> +    else if (accelfct < para->min_speed)
> +	accelfct = para->min_speed;
> +
> +    /* modify speed according to pressure */
> +    if (priv->moving_state == MS_TOUCHPAD_RELATIVE) {
> +	int minZ = para->press_motion_min_z;
> +	int maxZ = para->press_motion_max_z;
> +	double minFctr = para->press_motion_min_factor;
> +	double maxFctr = para->press_motion_max_factor;
> +	if (priv->hwState.z <= minZ) {
> +	    accelfct *= minFctr;
> +	} else if (priv->hwState.z >= maxZ) {
> +	    accelfct *= maxFctr;
> +	} else {
> +	    accelfct *= minFctr + (priv->hwState.z - minZ) * (maxFctr - minFctr) / (maxZ - minZ);
> +	}
> +    }
> +
> +    return accelfct;
> +}
> +
>  /*
>   *  called by the module loader for initialization
>   */
> @@ -852,12 +892,15 @@ DeviceInit(DeviceIntPtr dev)
>  {
>      LocalDevicePtr local = (LocalDevicePtr) dev->public.devicePrivate;
>      SynapticsPrivate *priv = (SynapticsPrivate *) (local->private);
> +    Atom float_type, prop;
> +    float tmpf;
>      unsigned char map[SYN_MAX_BUTTONS + 1];
>      int i;
>      int min, max;
>  #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7
>      Atom btn_labels[SYN_MAX_BUTTONS] = { 0 };
>      Atom axes_labels[2] = { 0 };
> +    DeviceVelocityPtr pVel;
>  
>      InitAxesLabels(axes_labels, 2);
>      InitButtonLabels(btn_labels, SYN_MAX_BUTTONS);
> @@ -890,6 +933,46 @@ DeviceInit(DeviceIntPtr dev)
>                              , axes_labels
>  #endif
>  			    );
> +
> +    /*
> +     * setup dix acceleration to match legacy synaptics settings, and
> +     * etablish a device-specific profile to do stuff like pressure-related
> +     * acceleration.
> +     */
> +#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 7
> +    if (NULL != (pVel = GetDevicePredictableAccelData(dev))) {
> +	SetDeviceSpecificAccelerationProfile(pVel, SynapticsAccelerationProfile);
> +
> +	/* float property type */
> +	float_type = XIGetKnownProperty(XATOM_FLOAT);
> +
> +	/* translate MinAcc to constant deceleration.
> +	 * May be overridden in xf86IVD */
                                please write out which function call that
                                is, even I had to think twice here


> +	tmpf = 1.0 / priv->synpara.min_speed;
> +
> +	xf86Msg(X_CONFIG, "%s: (accel) MinSpeed is now constant deceleration %.1f\n",
> +	        dev->name, tmpf);
> +	prop = XIGetKnownProperty(ACCEL_PROP_CONSTANT_DECELERATION);
> +	XIChangeDeviceProperty(dev, prop, float_type, 32,
> +	                       PropModeReplace, 1, &tmpf, FALSE);
> +
> +	/* adjust accordingly */
> +	priv->synpara.max_speed /= priv->synpara.min_speed;
> +	/* leave priv->synpara.accl alone since velocity includes const decel */
> +	priv->synpara.min_speed = 1.0;
> +
> +	xf86Msg(X_CONFIG, "%s: MaxSpeed is now %.1f\n",
> +		dev->name, priv->synpara.max_speed);
> +	xf86Msg(X_CONFIG, "%s: AccelFactor is now %.1f\n",
> +		dev->name, priv->synpara.accl);
> +
> +	prop = XIGetKnownProperty(ACCEL_PROP_PROFILE_NUMBER);
> +	i = AccelProfileDeviceSpecific;
> +	XIChangeDeviceProperty(dev, prop, XA_INTEGER, 32,
> +	                       PropModeReplace, 1, &i, FALSE);
> +    }
> +#endif
> +
>      /* X valuator */
>      if (priv->minx < priv->maxx)
>      {
> @@ -941,11 +1024,6 @@ DeviceInit(DeviceIntPtr dev)
>      return Success;
>  }
>  
> -static int
> -move_distance(int dx, int dy)
> -{
> -    return sqrt(SQR(dx) + SQR(dy));
> -}
>  
>  /*
>   * Convert from absolute X/Y coordinates to a coordinate system where
> @@ -1585,9 +1663,8 @@ ComputeDeltas(SynapticsPrivate *priv, struct SynapticsHwState *hw,
>  {
>      SynapticsParameters *para = &priv->synpara;
>      enum MovingState moving_state;
> -    int dist;
>      double dx, dy;
> -    double speed, integral;
> +    double integral;
>      int delay = 1000000000;
>  
>      dx = dy = 0;
> @@ -1667,36 +1744,12 @@ ComputeDeltas(SynapticsPrivate *priv, struct SynapticsHwState *hw,
>  		}
>  	    }
>  
> -	    /* speed depending on distance/packet */
> -	    dist = move_distance(dx, dy);
> -	    speed = dist * para->accl;
> -	    if (speed > para->max_speed) {  /* set max speed factor */
> -		speed = para->max_speed;
> -	    } else if (speed < para->min_speed) { /* set min speed factor */
> -		speed = para->min_speed;
> -	    }
> -
> -	    /* modify speed according to pressure */
> -	    if (priv->moving_state == MS_TOUCHPAD_RELATIVE) {
> -		int minZ = para->press_motion_min_z;
> -		int maxZ = para->press_motion_max_z;
> -		double minFctr = para->press_motion_min_factor;
> -		double maxFctr = para->press_motion_max_factor;
> -
> -		if (hw->z <= minZ) {
> -		    speed *= minFctr;
> -		} else if (hw->z >= maxZ) {
> -		    speed *= maxFctr;
> -		} else {
> -		    speed *= minFctr + (hw->z - minZ) * (maxFctr - minFctr) / (maxZ - minZ);
> -		}
> -	    }
> -
> -	    /* save the fraction, report the integer part */
> -	    tmpf = dx * speed + x_edge_speed * dtime + priv->frac_x;
> +	    /* report edge speed as synthetic motion. Of course, it would be
> +	     * cooler to report floats than to buffer, but anyway. */
> +	    tmpf = dx + x_edge_speed * dtime + priv->frac_x;
>  	    priv->frac_x = modf(tmpf, &integral);
>  	    dx = integral;
> -	    tmpf = dy * speed + y_edge_speed * dtime + priv->frac_y;
> +	    tmpf = dy + y_edge_speed * dtime + priv->frac_y;
>  	    priv->frac_y = modf(tmpf, &integral);
>  	    dy = integral;
>  	}
> -- 
> 1.6.4.4

aside from that, looks good to me and I'd be happy to put that one in. We'll
see if we get positive feedback.

Cheers,
  Peter