[PATCH xf86-input-evdev 2/2 v2] Add support for masked valuators

Chase Douglas chase.douglas at canonical.com
Sat Jan 22 13:26:35 PST 2011


On 01/05/2011 10:33 PM, Peter Hutterer wrote:
> On Wed, Jan 05, 2011 at 03:27:06PM -0500, Chase Douglas wrote:
>> With the X server now supporting masked valuators for XI2, enable
>> support in X evdev.
>>
>> This kills backwards compatibility with X Input ABI < 12
> 
> thanks! we need some extra configure checks to prevent building against
> older servers, and once we do that, all current ABI_XINPUT checks can go
> too.

I've added a commit to do this.

>>      /* convert to relative motion for touchpads */
>>      if (pEvdev->abs_queued && (pEvdev->flags & EVDEV_RELATIVE_MODE)) {
>>          if (pEvdev->in_proximity) {
>> -            if (pEvdev->old_vals[0] != -1)
>> -                pEvdev->delta[REL_X] = pEvdev->vals[0] - pEvdev->old_vals[0];
>> -            if (pEvdev->old_vals[1] != -1)
>> -                pEvdev->delta[REL_Y] = pEvdev->vals[1] - pEvdev->old_vals[1];
>> -            if (pEvdev->abs_queued & ABS_X_VALUE)
>> -                pEvdev->old_vals[0] = pEvdev->vals[0];
>> -            if (pEvdev->abs_queued & ABS_Y_VALUE)
>> -                pEvdev->old_vals[1] = pEvdev->vals[1];
>> +            if (valuator_mask_isset(pEvdev->mask, 0) &&
>> +                valuator_mask_isset(pEvdev->oldMask, 0))
>> +                pEvdev->delta[REL_X] = valuator_mask_get(pEvdev->mask, 0) -
>> +                                       valuator_mask_get(pEvdev->oldMask, 0);
>> +            if (valuator_mask_isset(pEvdev->mask, 1) &&
>> +                valuator_mask_isset(pEvdev->oldMask, 1))
>> +                pEvdev->delta[REL_Y] = valuator_mask_get(pEvdev->mask, 1) -
>> +                                       valuator_mask_get(pEvdev->oldMask, 1);
>> +            if (valuator_mask_isset(pEvdev->mask, 0))
>> +                valuator_mask_set(pEvdev->oldMask, 0,
>> +                                  valuator_mask_get(pEvdev->mask, 0));
>> +            if (valuator_mask_isset(pEvdev->mask, 1))
>> +                valuator_mask_set(pEvdev->oldMask, 1,
>> +                                  valuator_mask_get(pEvdev->mask, 1));
> 
> please rearrange this into
> 
>     if (valuator_mask_isset(mask, 0))
>     {
>             if (valuator_mask_isset(oldmask, 0)
>                     delta = ...
>             valuator_mask_set(oldMask)
>     }

Done.

>> +        for (i = 0; i <= 1; i++) {
>> +            int val;
>>  
>> -        if (pEvdev->flags & EVDEV_CALIBRATED)
>> -        {
>> -            v[0] = xf86ScaleAxis(v[0],
>> -                    pEvdev->absinfo[ABS_X].maximum,
>> -                    pEvdev->absinfo[ABS_X].minimum,
>> -                    pEvdev->calibration.max_x, pEvdev->calibration.min_x);
>> -            v[1] = xf86ScaleAxis(v[1],
>> -                    pEvdev->absinfo[ABS_Y].maximum,
>> -                    pEvdev->absinfo[ABS_Y].minimum,
>> -                    pEvdev->calibration.max_y, pEvdev->calibration.min_y);
>> -        }
>> +            if (!valuator_mask_isset(pEvdev->mask, i))
>> +                continue;
>>  
>> -        if (pEvdev->invert_x)
>> -            v[0] = (pEvdev->absinfo[ABS_X].maximum - v[0] +
>> -                    pEvdev->absinfo[ABS_X].minimum);
>> -        if (pEvdev->invert_y)
>> -            v[1] = (pEvdev->absinfo[ABS_Y].maximum - v[1] +
>> -                    pEvdev->absinfo[ABS_Y].minimum);
>> +            val = valuator_mask_get(pEvdev->mask, i);
> 
> val here has the same value as unswapped_x/y, so you can skip those two and
> just work with val.

val is retrieved during the loop. The first time through the loop this
would work. But then the x value will change for the second loop. Thus,
I use val as a temporary variable.

>> -        *num_v = pEvdev->num_vals;
>> -        *first_v = 0;
>> +            if (pEvdev->swap_axes)
>> +                val = xf86ScaleAxis((i == 0 ? unswapped_y : unswapped_x),
>> +                                    pEvdev->absinfo[i].maximum,
>> +                                    pEvdev->absinfo[i].minimum,
>> +                                    pEvdev->absinfo[1 - i].maximum,
>> +                                    pEvdev->absinfo[1 - i].minimum);
>> +
>> +            if (pEvdev->flags & EVDEV_CALIBRATED)
>> +                val = xf86ScaleAxis(val, pEvdev->absinfo[i].maximum,
>> +                                    pEvdev->absinfo[i].minimum,
>> +                                    (i == 0 ? pEvdev->calibration.max_x :
>> +                                     pEvdev->calibration.max_y),
>> +                                    (i == 0 ? pEvdev->calibration.min_x :
>> +                                     pEvdev->calibration.min_y));
> 
> please, no two-line ternery conditions in function calls. just use a temp
> variable here. mind you, if you want to get really fancy you can change
> calibration into a struct { int min; int max; } calibration[2]. but I'm not
> sure how much that gains us here besides saving one line.

I added temporary variables.

>> +
>> +            if ((i == 0 && pEvdev->invert_x) || (i == 1 && pEvdev->invert_y))
>> +                val = (pEvdev->absinfo[i].maximum - val +
>> +                       pEvdev->absinfo[i].minimum);
>> +
>> +            valuator_mask_set(pEvdev->mask, i, val);
>> +        }
>>      }
>>  }
>>  
>> @@ -518,11 +502,15 @@ EvdevProcessProximityState(InputInfoPtr pInfo)
>>      int prox_state = 0;
>>      int i;
>>  
>> +    /* Does this device have any proximity keys? */
> 
> s/keys/axes/

Done.

>>      pEvdev->num_vals = num_axes;
>> -    memset(pEvdev->vals, 0, num_axes * sizeof(int));
>> -    memset(pEvdev->old_vals, -1, num_axes * sizeof(int));
>> +    if (num_axes > 0) {
>> +        pEvdev->mask = valuator_mask_new(num_axes);
>> +        if (!pEvdev->mask) {
>> +            xf86Msg(X_ERROR, "%s: failed to allocate valuator mask.\n",
>> +                    device->name);
>> +            return !Success;
> 
> shouldn't this be goto out as well?

Sure.

>> +        }
>> +        pEvdev->oldMask = valuator_mask_new(num_axes);
>> +        if (!pEvdev->oldMask) {
>> +            xf86Msg(X_ERROR, "%s: failed to allocate old valuator mask.\n",
>> +                    device->name);
> 
> imo, one error message for both mask and oldMask is enough (mind you, if we
> fail here we probably topple soon anyway).

Yeah, I've simplified it.

>> @@ -1488,6 +1510,11 @@ EvdevAddRelClass(DeviceIntPtr device)
>>      free(atoms);
>>  
>>      return Success;
>> +
>> +out:
>> +    free(pEvdev->mask);
>> +    pEvdev->mask = NULL;
>> +    return !Success;
>>  }
> 
> please split the "goto out" parts into a separate patch that the valuator
> mask patch only contains valuator mask portions.

Done.

>>  
>>  static int
>> @@ -1759,6 +1786,9 @@ EvdevProc(DeviceIntPtr device, int what)
>>              close(pInfo->fd);
>>              pInfo->fd = -1;
>>          }
>> +        free(pEvdev->mask);
>> +        free(pEvdev->oldMask);
>> +        free(pEvdev->proxMask);
> 
> seeing this, I just realised we need a valuator_mask_free() in the server,
> otherwise the whole point of making the valuator mask opague goes away and
> we're stuck with a single memory area for the mask. patch is out.

I've switched this now.

>> diff --git a/src/evdev.h b/src/evdev.h
>> index f640fdd..6af145f 100644
>> --- a/src/evdev.h
>> +++ b/src/evdev.h
>> @@ -121,8 +121,9 @@ typedef struct {
>>  
>>      int num_vals;           /* number of valuators */
>>      int axis_map[max(ABS_CNT, REL_CNT)]; /* Map evdev <axis> to index */
>> -    int vals[MAX_VALUATORS];
>> -    int old_vals[MAX_VALUATORS]; /* Translate absolute inputs to relative */
>> +    ValuatorMask *mask;
>> +    ValuatorMask *oldMask;
>> +    ValuatorMask *proxMask;
> 
> this is possibly bikeshedding, but why not keep using the names vals and
> old_vals? the type changes, but it's still used as a valuator array (more or
> less). please add some comments as to what values the various masks hold, I
> have to admit the proximity mask confused me a bit.
> 
> also, evdev is trying to follow a foo_bar naming convention internally where
> possible.

Ok, fixed up.

Thanks for the review! I will send the patches out shortly.

-- Chase


More information about the xorg-devel mailing list