[PATCH xserver] Convert server to masked input valuators

[Chase Douglas]

On Thu, 2010-10-14 at 13:31 +1000, Peter Hutterer wrote:
> On Tue, Oct 05, 2010 at 04:07:56PM -0400, Chase Douglas wrote:
> > XI2 allows for input event valuators to be masked. The current input
> > module API only allows for ranges to be specified. This fixes all
> > internal plumbing to use masks instead of ranges, and adds "M"
> > mask versions of xf86Post*Event() functions.
> > 
> > Note the minor version bump of the XInput ABI.
> 
> thanks, finally got to review it. a couple of comments, but nothing
> world-changing.
> 
> fwiw, I'm planning for this to be the last patch for the input ABI 12
> patchset, so the minor version bump won't be needed here.

I've still got a patch for mixed mode (absolute/relative) devices. I've
been holding off on it since it hits the same code paths and would be a
chore to maintain alongside this patch. Any issues with that going in as
well, assuming it passes review muster?

[...]

> >  /**
> > @@ -680,36 +696,37 @@ UpdateFromMaster(EventListPtr events, DeviceIntPtr dev, int type, int *num_event
> >   * @param dev The device which's pointer is to be moved.
> >   * @param x Returns the x position of the pointer after the move.
> >   * @param y Returns the y position of the pointer after the move.
> > - * @param first The first valuator in @valuators
> > - * @param num Total number of valuators in @valuators.
> > + * @param mask Bit mask of valid valuators.
> >   * @param valuators Valuator data for each axis between @first and
> >   *        @first+ at num.
> >   */
> >  static void
> >  moveAbsolute(DeviceIntPtr dev, int *x, int *y,
> > -             int first, int num, int *valuators)
> > +             ValuatorMask *mask, int *valuators)
> >  {
> >      int i;
> >  
> >  
> > -    if (num >= 1 && first == 0)
> > +    if (mask->len >= 1 && BitIsOn(mask->mask, 0))
> >          *x = *(valuators + 0);
> 
> if BitIsOn() returns 1 for the bit, then mask->len must be >= 1, so we can
> cut out the first bit here.

I try not to make any assumptions about the relationship between the
mask and the mask length. As coded, bits beyond the stated length of the
mask are undefined, not expected to be 0. So an empty mask would have a
mask->len == 0, but the zero'th bit may be 0 or 1. This is required as
there's no upper bounds on the mask length in the future. In this patch,
it's limited to MAX_VALUATORS, but MAX_VALUATORS may be bumped later. We
can't assume that any bits beyond the length given are actually part of
the ValuatorMask bitmask.

Here, I just matched the logic as close as possible. The previous
statement suggests that there may be a time when the number of valuators
passed to the function is 0. I think one such occurrence is a call to
GetPointerEvents where button presses are given but no valuator changes
are made.

> Generally it'd be good to do some sanity checking in the xf86Post* events to
> verify that the ValuatorMask supplied is actually valid. This one here would
> just fail quietly.

The check here isn't checking for sanity. An empty mask seems valid
given the manner in which this function is called elsewhere as noted
above. Evaluating to false here just means we shouldn't update the
coords or previous valuator values because nothing changed.

[...]

> > @@ -949,17 +969,15 @@ GetKeyboardValuatorEvents(EventList *events, DeviceIntPtr pDev, int type,
> >      events++;
> >      num_events++;
> >  
> > -    memcpy(valuators, valuators_in, num_valuators * sizeof(int));
> > +    if (valuators_in)
> > +        memcpy(valuators, valuators_in, MAX_VALUATORS * sizeof(int));
> 
> shouldn't this use len or so instead of MAX_VALUATORS?

Probably. I'll fix this for the next revision.

[...]

> > @@ -1088,13 +1104,15 @@ GetPointerEvents(EventList *events, DeviceIntPtr pDev, int type, int buttons,
> >  
> >      ms = GetTimeInMillis(); /* before pointer update to help precision */
> >  
> > -    if (!scr || !pDev->valuator || first_valuator < 0 ||
> > -        num_valuators > MAX_VALUATORS ||
> > -        ((num_valuators + first_valuator) > pDev->valuator->numAxes) ||
> > +    if (!scr || !pDev->valuator ||
> >          (type != MotionNotify && type != ButtonPress && type != ButtonRelease) ||
> >          (type != MotionNotify && !pDev->button) ||
> > -        ((type == ButtonPress || type == ButtonRelease) && !buttons) ||
> > -        (type == MotionNotify && num_valuators <= 0))
> > +        ((type == ButtonPress || type == ButtonRelease) && !buttons) || !mask ||
> > +        (mask->len > 0 && !valuators_in))
> > +        return 0;
> 
> the !mask/mask->len check is there often enough to warrant a mask_is_empty()
> macro/inline.

One checks whether a mask was passed in at all. The other checks whether
the mask says there are valuators but no valuators are actually passed
in. There's no "mask is empty" check here, an empty mask is valid.

[...]

> > @@ -1191,7 +1215,7 @@ GetPointerEvents(EventList *events, DeviceIntPtr pDev, int type, int buttons,
> >   */
> >  int
> >  GetProximityEvents(EventList *events, DeviceIntPtr pDev, int type,
> > -                   int first_valuator, int num_valuators, int *valuators_in)
> > +                       ValuatorMask *mask, int *valuators_in)
> 
> 
> indentation

Thanks for catching that! I'll fix it in the next revision.

> > --- a/hw/xfree86/common/xf86Xinput.c
> > +++ b/hw/xfree86/common/xf86Xinput.c
> > @@ -1004,6 +1004,20 @@ xf86PostMotionEventP(DeviceIntPtr	device,
> >                      int			num_valuators,
> >                      int			*valuators)
> >  {
> > +    ValuatorMask mask;
> > +
> > +    XI_VERIFY_VALUATORS(num_valuators);

Here it is! (See next comment...)

> > +
> > +    ValuatorRangeToMask(first_valuator, num_valuators, &mask);
> > +    xf86PostMotionEventM(device, is_absolute, &mask, valuators);
> > +}
> > +
> > +void
> > +xf86PostMotionEventM(DeviceIntPtr	device,
> > +                     int		is_absolute,
> > +                     ValuatorMask	*mask,
> > +                     int		*valuators)
> > +{
> >      int i = 0, nevents = 0;
> >      Bool drag = xf86SendDragEvents(device);
> >      DeviceEvent *event;
> > @@ -1014,8 +1028,6 @@ xf86PostMotionEventP(DeviceIntPtr	device,
> >      int dx = 0, dy = 0;
> >  #endif
> >  
> > -    XI_VERIFY_VALUATORS(num_valuators);
> > -
> 
> there should still be a check to warn in the log if a device sends more
> valuators than possible.

This diff algorithm got you :). I'll admit, it got me too when I was
reviewing the patch before sending. Due to the xf86PostMotionEventP
function becoming a simple wrapper around xf86PostMotionEventM, the
XI_VERIFY_VALUATORS looks like it moved, though it really didn't. Look
up :).

[...]

> > diff --git a/include/input.h b/include/input.h
> > index ffb1c33..8a1f41c 100644
> > --- a/include/input.h
> > +++ b/include/input.h
> > @@ -157,6 +157,11 @@ typedef struct _DeviceRec {
> >      Bool	on;			/* used by DDX to keep state */
> >  } DeviceRec, *DevicePtr;
> >  
> > +typedef struct _ValuatorMask {
> > +    uint8_t	len;
> > +    uint8_t	mask[(MAX_VALUATORS + 7) / 8];
> > +} ValuatorMask;
> > +
> 
> why not go the full step and add the int *valuators to this struct as well?
> Call it ValuatorData, with the mask plus the actual data in one struct.
> Makes for simpler passing and I think the changes elsewhere would still be
> reasonably small.
> 
> Without a comment, I also don't know what len stands for? Shouldn't that be
> implicit by the number of bits set in mask?
> Also, making mask explicit of MAX_VALUATORS means MAX_VALUATORS becomes ABI,
> which may not be the best of all things. Split it up into a uint8_t
> mask_len, a uint8_t *mask and the int* valuators to be arbitrary (that's
> also what we use for the protocol).

Ok, I think I can manage this.

> In regards to the use of the valuator array: I think it's better (if more
> complicated) to just pass the actual values around. Right now, if you update
> axis 32 only, you still have to pass in an array size 32 because the axis
> maps to the position in the array. It seems better

I'm not sure how much this would help. The two routes for ValuatorMask
usage are through an input module directly or through the
xf86Post*EventP wrapper functions. In an input module, the module should
preallocate one array for valuator values and be done with it. In the
wrapper functions, we either dynamically allocate the array to be a
minimum size (potentially slow, can't be done in signal context as the
input system is coded right now I think), or we allocate MAX_VALUATORS
elements on the stack.

[...]

> > @@ -560,4 +562,12 @@ extern _X_EXPORT void DDXRingBell(
> >     xfixes/cursor.c uses it to determine if the cursor is enabled */
> >  extern Bool EnableCursor;
> >  
> > +extern _X_EXPORT void ValuatorRangeToMask(int first_valuator,
> > +    int num_valuators,
> > +    ValuatorMask *mask);
> > +
> > +extern _X_EXPORT void ValuatorMaskSetBit(ValuatorMask *mask, int bit);
> > +
> > +extern _X_EXPORT int CountBits(uint8_t *mask, int len);
> 
> SetBit/BitIsOn/ClearBit are defined in inputstr.h, might be better to move
> CountBits there too.

Those three are simple one line macros. CountBits is a little more
complicated. I didn't put it in inputstr.h because it's a function
declaration, and it seemed only structure and macro definitions are
defined there.

I'm open to any resolution here, including making CountBits a large
macro or a static inline function in inputstr.h. What do you suggest?

[...]

Thanks for the review!

-- Chase