Ptr accel API changes

[Peter Hutterer]

On Sat, Jun 20, 2009 at 07:09:39PM +0200, Simon Thum wrote:
> Peter Hutterer wrote:
>> On Sat, Jun 06, 2009 at 03:45:13PM +0200, Simon Thum wrote:
>>> I would like to see the first of the attached patches in the server   
>>> before 1.7 is branched. This is to have the accel API be sensible 
>>> enough  to finally solve the synaptics-and-acceleration problem 
>>> hiding in its  corner since the jura, approximately. The synaptics 
>>> accel code (or the  driver as a whole) could be depending on 1.7 
>>> then, so this has my priority.
>>
>> can you give an example of how this would be used in the driver? Will the
>> driver provide it's custom acceleration function or just rely on the server?
>> If the former, why not the latter? :)
> See attached synaptics patch (POC code only!). The driver provides its  
> own function to implement the pressure-related acceleration feature,  
> which I think is worth the sweat.
>
> Without the API change, it would be hard to do this properly because  
> it's hard to infer the device from a DeviceVelocityPtr alone. (Ok, I'm  
> yet to see a 2-synaptics-pads-box, but you get the point.)
>
> Also, now a driver can create own accel contexts, e.g. to accelerate  
> scrolling or other independent axes. (I could have before but it would  
> be unnecessarily verbose).
>
>> right now, the server provides a couple of options to configure pointer
>> acceleration on a per-device basis. Why can't we set these options in the
>> driver to something resembling sensible defaults (IIRC synaptics requires
>> constant deceleration of at least 3 or so, it'd be a single line to add this
>> to the driver).
> That's what the patch also does. Or, does try to, at least.
>
>> I wish we could get rid of the pDev and pVel and pSomething and just use
>> dev, vel, and something instead. (I know, I'm guilty of that myself)
>> same goes for the rest of the patch.
> I was merely aligning to what I found; I'm not a great supporter of that  
> hungarian notation thingy either. Anyway, your wishes have come true  
> (for ptrveloc, at least :)



> From aa117b1023023cfdd7748536266dd7004efe388b Mon Sep 17 00:00:00 2001
> From: Simon Thum <simon.thum at gmx.de>
> Date: Sat, 20 Jun 2009 16:07:43 +0200
> Subject: [PATCH] Setup dix pointer acceleration from synaptics
> 
> Tries to setup dix accel so configured synaptics devices
> behave alike while benefiting from dix velocity approximation.
> Pressure-dependent acceleration is performed in a device-specific
> profile.
> ---
>  src/synaptics.c |  124 +++++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 89 insertions(+), 35 deletions(-)
> 
> diff --git a/src/synaptics.c b/src/synaptics.c
> index 6b902e9..94eed2f 100644
> --- a/src/synaptics.c
> +++ b/src/synaptics.c
> @@ -77,6 +77,10 @@
>  #include "synapticsstr.h"
>  #include "synaptics-properties.h"
>  
> +#include <ptrveloc.h>
> +#include <xserver-properties.h>
> +#include <X11/Xatom.h> /* for XA_INTEGER */
> +
>  typedef enum {
>      BOTTOM_EDGE = 1,
>      TOP_EDGE = 2,
> @@ -515,6 +519,46 @@ static void set_default_parameters(LocalDevicePtr local)
>      }
>  }
>  
> +static float SynapticsAccelerationProfile
> +    (DeviceIntPtr dev,
 please move this up instead of onto a new line

> +     DeviceVelocityPtr vel,
> +     float velocity,
> +     float thr,
> +     float acc) {
> +    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;
> +
> +    if (accelfct > para->max_speed) {  /* clip speed factor */
> +	accelfct = para->max_speed;
> +    } else if (accelfct < para->min_speed) {
> +	/* this may be bogus since dix also enforces a min accel */
> +	accelfct = para->min_speed;
> +    }

I think the min/max macros in misc.h may be useful here.

> +
> +    /* 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
>   */
> @@ -793,6 +837,8 @@ 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;
> @@ -818,6 +864,45 @@ DeviceInit(DeviceIntPtr dev)
>  			    GetMotionHistorySize(), 2
>  #endif
>  			    );
> +
> +    /*
> +     * setup dix acceleration to match legacy synaptics settings, and
> +     * etablish a device-specific profile to do stuff like pressure-related
> +     * acceleration.
> +     */
> +    DeviceVelocityPtr pVel = GetDevicePredictableAccelData(dev);
> +    if(pVel){
> +	SetDeviceSpecificAccelerationProfile(pVel, SynapticsAccelerationProfile);
> +
> +	/* float property type */
> +	float_type = XIGetKnownProperty(XATOM_FLOAT);
> +
> +	/* translate MinAcc to constant deceleration.
> +	 * May be overridden in xf86IVD */
> +	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);
> +    }
> +
>      /* X valuator */
>      if (priv->minx < priv->maxx)
>      {
> @@ -861,11 +946,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
> @@ -1474,9 +1554,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;
> @@ -1556,36 +1635,11 @@ 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 */
> +	    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.0.6

fair enough, makes sense. go ahead with the API changes.

> From bf0dc3f31c6eae5bcf8b4de0eba631d20e1b659d Mon Sep 17 00:00:00 2001
> From: Simon Thum <simon.thum at gmx.de>
> Date: Sat, 6 Jun 2009 14:49:43 +0200
> Subject: [PATCH] dix: improve pointer acceleration API
> 
> This makes the ptr accel api actually sensible from a driver
> perspective, since the device is now a parameter of the
> device-specific accel callback. Also, make independent accel
> contexts possible and align argument naming.

big NACK to this patch. there's two completely unrelated changes here, one
is the search/replace of the hungarian notation and the other one the actual
API change. The API change is well hidden behind lots of variable renaming
(I think the first interesting bit is in hunk 27 or so, way after the point
where concentration has gone home).
please split it up into one patch with all the renaming and then the other
one with the actual API changes.

> From 68c18962c80df28224f1716c129b30402e92d7ea Mon Sep 17 00:00:00 2001
> From: Simon Thum <simon.thum at gmx.de>
> Date: Sat, 20 Jun 2009 18:57:22 +0200
> Subject: [PATCH] dix: make part of ptrveloc.h internal
> 
> Though this is a SDK header, some functions are intended solely
> for the server.
> ---
>  include/ptrveloc.h |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/ptrveloc.h b/include/ptrveloc.h
> index 317bc26..fa2156b 100644
> --- a/include/ptrveloc.h
> +++ b/include/ptrveloc.h
> @@ -106,7 +106,7 @@ BasicComputeAcceleration(DeviceIntPtr dev, DeviceVelocityPtr vel,
>  extern _X_EXPORT void
>  FreeVelocityData(DeviceVelocityPtr vel);
>  
> -extern _X_EXPORT BOOL
> +extern _X_INTERNAL BOOL
>  InitializePredictableAccelerationProperties(DeviceIntPtr dev);
>  
>  extern _X_EXPORT int
> @@ -119,14 +119,14 @@ extern _X_EXPORT void
>  SetDeviceSpecificAccelerationProfile(DeviceVelocityPtr vel,
>                                       PointerAccelerationProfileFunc profile);
>  
> -extern _X_EXPORT void
> +extern _X_INTERNAL void
>  AccelerationDefaultCleanup(DeviceIntPtr dev);
>  
> -extern _X_EXPORT void
> +extern _X_INTERNAL void
>  acceleratePointerPredictable(DeviceIntPtr dev, int first_valuator,
>                               int num_valuators, int *valuators, int evtime);
>  
> -extern _X_EXPORT void
> +extern _X_INTERNAL void
>  acceleratePointerLightweight(DeviceIntPtr dev, int first_valuator,
>                               int num_valuators, int *valuators, int ignored);
>  

ACK to this, I'll apply it once I get the other patches.

Cheers,
  Peter