On Wed, Sep 23, 2009 at 01:43:55PM -0400, Thomas Jaeger wrote: > As noted in the gtk bug [1], XGetDeviceState returns incorrect valuator > information. I tracked this down to axisVal containing garbage. Rather > than figure out where the problem is, I think it's easier to get rid of > axisVal altogether and use dev->last.valuators instead.
not really. axisVal is the axis state that is sent to the client through the events. Anything in dev->last is the state inside the server as used during the signal handler. There's no guarantee that last.valuators is still on the correct value when the server processes the event and even worse - being used in the signal handler you can't even guarantee it's correct while you're copying it out. So for this case we actually need to find the bug and fix it. Cheers, Peter > [1] https://bugzilla.gnome.org/show_bug.cgi?id=588649 > From a1219f5cd34b8df4c6862caf3d783d0ef45beda1 Mon Sep 17 00:00:00 2001 > From: Thomas Jaeger <[email protected]> > Date: Wed, 23 Sep 2009 13:01:35 -0400 > Subject: [PATCH 1/2] Xi: Don't rely on axisVal for QueryState > > axisVal may contain garbage, so this lead to incorrect behavior > --- > Xi/queryst.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/Xi/queryst.c b/Xi/queryst.c > index 60ec32e..e0343f9 100644 > --- a/Xi/queryst.c > +++ b/Xi/queryst.c > @@ -85,7 +85,6 @@ ProcXQueryDeviceState(ClientPtr client) > xValuatorState *tv; > xQueryDeviceStateReply rep; > DeviceIntPtr dev; > - double *values; > > REQUEST(xQueryDeviceStateReq); > REQUEST_SIZE_MATCH(xQueryDeviceStateReq); > @@ -151,8 +150,8 @@ ProcXQueryDeviceState(ClientPtr client) > tv->num_valuators = v->numAxes; > tv->mode = v->mode; > buf += sizeof(xValuatorState); > - for (i = 0, values = v->axisVal; i < v->numAxes; i++) { > - *((int *)buf) = *values++; > + for (i = 0; i < v->numAxes; i++) { > + *((int *)buf) = dev->last.valuators[i]; > if (client->swapped) { > swapl((int *)buf, n); /* macro - braces needed */ > } > -- > 1.6.3.3 > > From 442efa5a67ab3db5279a017be6aea757448e8695 Mon Sep 17 00:00:00 2001 > From: Thomas Jaeger <[email protected]> > Date: Wed, 23 Sep 2009 13:10:53 -0400 > Subject: [PATCH 2/2] Kill off axisVal completely > > There's no point in having the same information stored in two different > places. > --- > Xi/exevents.c | 23 ++++++----------------- > Xi/xiquerydevice.c | 4 ++-- > dix/devices.c | 8 ++------ > hw/xfree86/common/xf86Xinput.c | 6 ++---- > include/inputstr.h | 1 - > 5 files changed, 12 insertions(+), 30 deletions(-) > > diff --git a/Xi/exevents.c b/Xi/exevents.c > index b0e0ede..558cfd0 100644 > --- a/Xi/exevents.c > +++ b/Xi/exevents.c > @@ -551,7 +551,6 @@ DeepCopyPointerClasses(DeviceIntPtr from, DeviceIntPtr to) > v->axes = (AxisInfoPtr)&v[1]; > memcpy(v->axes, from->valuator->axes, v->numAxes * sizeof(AxisInfo)); > > - v->axisVal = (double*)(v->axes + from->valuator->numAxes); > v->sourceid = from->id; > v->mode = from->valuator->mode; > } else if (to->valuator && !from->valuator) > @@ -799,16 +798,6 @@ UpdateDeviceState(DeviceIntPtr device, DeviceEvent* > event) > } > } > > - for (i = 0; i <= last_valuator && i < v->numAxes; i++) > - { > - if (BitIsOn(&event->valuators.mask, i)) > - { > - /* XXX: Relative/Absolute mode */ > - v->axisVal[i] = event->valuators.data[i]; > - v->axisVal[i] += event->valuators.data_frac[i]; > - } > - } > - > if (event->type == ET_KeyPress) { > if (!k) > return DONT_PROCESS; > @@ -1176,11 +1165,11 @@ FixDeviceStateNotify(DeviceIntPtr dev, > deviceStateNotify * ev, KeyClassPtr k, > ev->num_valuators = nval < 3 ? nval : 3; > switch (ev->num_valuators) { > case 3: > - ev->valuator2 = v->axisVal[first + 2]; > + ev->valuator2 = dev->last.valuators[first + 2]; > case 2: > - ev->valuator1 = v->axisVal[first + 1]; > + ev->valuator1 = dev->last.valuators[first + 1]; > case 1: > - ev->valuator0 = v->axisVal[first]; > + ev->valuator0 = dev->last.valuators[first]; > break; > } > } > @@ -1198,11 +1187,11 @@ FixDeviceValuator(DeviceIntPtr dev, deviceValuator * > ev, ValuatorClassPtr v, > ev->first_valuator = first; > switch (ev->num_valuators) { > case 3: > - ev->valuator2 = v->axisVal[first + 2]; > + ev->valuator2 = dev->last.valuators[first + 2]; > case 2: > - ev->valuator1 = v->axisVal[first + 1]; > + ev->valuator1 = dev->last.valuators[first + 1]; > case 1: > - ev->valuator0 = v->axisVal[first]; > + ev->valuator0 = dev->last.valuators[first]; > break; > } > first += ev->num_valuators; > diff --git a/Xi/xiquerydevice.c b/Xi/xiquerydevice.c > index 68d91fa..5c727f5 100644 > --- a/Xi/xiquerydevice.c > +++ b/Xi/xiquerydevice.c > @@ -338,8 +338,8 @@ ListValuatorInfo(DeviceIntPtr dev, xXIValuatorInfo* info, > int axisnumber) > info->min.frac = 0; > info->max.integral = v->axes[axisnumber].max_value; > info->max.frac = 0; > - info->value.integral = (int)v->axisVal[axisnumber]; > - info->value.frac = (int)(v->axisVal[axisnumber] * (1 << 16) * (1 << 16)); > + info->value.integral = dev->last.valuators[axisnumber]; > + info->value.frac = (int)(dev->last.remainder[axisnumber] * (1 << 16) * > (1 << 16)); > info->resolution = v->axes[axisnumber].resolution; > info->number = axisnumber; > info->mode = v->mode; /* Server doesn't have per-axis mode yet */ > diff --git a/dix/devices.c b/dix/devices.c > index e86e606..cbe646b 100644 > --- a/dix/devices.c > +++ b/dix/devices.c > @@ -571,10 +571,8 @@ CorePointerProc(DeviceIntPtr pDev, int what) > pDev->name); > return BadAlloc; /* IPDS only fails on allocs */ > } > - pDev->valuator->axisVal[0] = screenInfo.screens[0]->width / 2; > - pDev->last.valuators[0] = pDev->valuator->axisVal[0]; > - pDev->valuator->axisVal[1] = screenInfo.screens[0]->height / 2; > - pDev->last.valuators[1] = pDev->valuator->axisVal[1]; > + pDev->last.valuators[0] = screenInfo.screens[0]->width / 2; > + pDev->last.valuators[1] = screenInfo.screens[0]->height / 2; > break; > > case DEVICE_CLOSE: > @@ -1176,7 +1174,6 @@ InitValuatorClassDeviceStruct(DeviceIntPtr dev, int > numAxes, Atom *labels, > valc->numAxes = numAxes; > valc->mode = mode; > valc->axes = (AxisInfoPtr)(valc + 1); > - valc->axisVal = (double *)(valc->axes + numAxes); > dev->valuator = valc; > > AllocateMotionHistory(dev); > @@ -1184,7 +1181,6 @@ InitValuatorClassDeviceStruct(DeviceIntPtr dev, int > numAxes, Atom *labels, > for (i=0; i<numAxes; i++) { > InitValuatorAxisStruct(dev, i, labels[i], NO_AXIS_LIMITS, > NO_AXIS_LIMITS, > 0, 0, 0); > - valc->axisVal[i]=0; > } > > dev->last.numValuators = numAxes; > diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c > index b369537..62bb1e6 100644 > --- a/hw/xfree86/common/xf86Xinput.c > +++ b/hw/xfree86/common/xf86Xinput.c > @@ -1040,12 +1040,10 @@ void > xf86InitValuatorDefaults(DeviceIntPtr dev, int axnum) > { > if (axnum == 0) { > - dev->valuator->axisVal[0] = screenInfo.screens[0]->width / 2; > - dev->last.valuators[0] = dev->valuator->axisVal[0]; > + dev->last.valuators[0] = screenInfo.screens[0]->width / 2; > } > else if (axnum == 1) { > - dev->valuator->axisVal[1] = screenInfo.screens[0]->height / 2; > - dev->last.valuators[1] = dev->valuator->axisVal[1]; > + dev->last.valuators[1] = screenInfo.screens[0]->height / 2; > } > > if(axnum == 0) /* to prevent double invocation */ > diff --git a/include/inputstr.h b/include/inputstr.h > index 29ad5a8..61a107c 100644 > --- a/include/inputstr.h > +++ b/include/inputstr.h > @@ -236,7 +236,6 @@ typedef struct _ValuatorClassRec { > > AxisInfoPtr axes; > unsigned short numAxes; > - double *axisVal; /* always absolute, but device-coord system > */ > CARD8 mode; > ValuatorAccelerationRec accelScheme; > } ValuatorClassRec, *ValuatorClassPtr; > -- > 1.6.3.3 > _______________________________________________ xorg-devel mailing list [email protected] http://lists.x.org/mailman/listinfo/xorg-devel
