[Linuxwacom-devel] [PATCH 1/2] Increase full-scale pressure range from 0..2047 to 0..65535

The TouchRing on the MobileStudio Pro has a range of only 0-35 instead
of 0-17 like prior devices. Because we hardcode an assumed range, the
driver mistakenly believes the jump from 35 to 0 (or 0 to 35, depending
on direction) when the user completes a revolution is actually caused
by the user reversing their finger direction. By reading the range from
the kernel, we can avoid this situation.

Note that the ABS_WHEEL axis is also used by (legacy) combined pen/pad
devices with a range corresponding to the puck fingerwheel. We need to
be careful to not read the value in these cases since it would lead to
erroneous behavior in existing setups. Devices with a range range
different from 0-71 will hopefully not be widely used on kernels prior
to 3.17 where pen and pad were split into seperate devices since there
is no way (short of peeking at the VID:PID or name) to have them work
correctly in the combined case.

Signed-off-by: Jason Gerecke <***@wacom.com>
---
src/wcmCommon.c | 5 +++--
src/wcmUSB.c | 10 ++++++++++
src/xf86Wacom.c | 8 ++++----
src/xf86WacomDefs.h | 9 ++++-----
4 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/src/wcmCommon.c b/src/wcmCommon.c
index 9408f42..0ba0304 100644
--- a/src/wcmCommon.c
+++ b/src/wcmCommon.c
@@ -361,6 +361,7 @@ static void sendWheelStripEvents(InputInfoPtr pInfo, const WacomDeviceState* ds,
int first_val, int num_vals, int *valuators)
{
WacomDevicePtr priv = (WacomDevicePtr) pInfo->private;
+ WacomCommonPtr common = priv->common;
int delta = 0, idx = 0;

DBG(10, priv, "\n");
@@ -396,7 +397,7 @@ static void sendWheelStripEvents(InputInfoPtr pInfo, const WacomDeviceState* ds,
}

/* emulate events for left touch ring */
- delta = getScrollDelta(ds->abswheel, priv->oldState.abswheel, MAX_PAD_RING, AXIS_INVERT);
+ delta = getScrollDelta(ds->abswheel, priv->oldState.abswheel, common->wcmMaxRing, AXIS_INVERT);
idx = getWheelButton(delta, WHEEL_ABS_UP, WHEEL_ABS_DN);
if (idx >= 0 && IsPad(priv) && priv->oldState.proximity == ds->proximity)
{
@@ -406,7 +407,7 @@ static void sendWheelStripEvents(InputInfoPtr pInfo, const WacomDeviceState* ds,
}

/* emulate events for right touch ring */
- delta = getScrollDelta(ds->abswheel2, priv->oldState.abswheel2, MAX_PAD_RING, AXIS_INVERT);
+ delta = getScrollDelta(ds->abswheel2, priv->oldState.abswheel2, common->wcmMaxRing, AXIS_INVERT);
idx = getWheelButton(delta, WHEEL2_ABS_UP, WHEEL2_ABS_DN);
if (idx >= 0 && IsPad(priv) && priv->oldState.proximity == ds->proximity)
{
diff --git a/src/wcmUSB.c b/src/wcmUSB.c
index dcb1690..2f9d93f 100644
--- a/src/wcmUSB.c
+++ b/src/wcmUSB.c
@@ -652,6 +652,16 @@ int usbWcmGetRanges(InputInfoPtr pInfo)
common->wcmMaxStripX = absinfo.maximum;
}

+ /* max touchring value for standalone pad tools */
+ common->wcmMinRing = 0;
+ common->wcmMaxRing = 71;
+ if (!ISBITSET(ev,EV_MSC) && ISBITSET(abs, ABS_WHEEL) &&
+ !ioctl(pInfo->fd, EVIOCGABS(ABS_WHEEL), &absinfo))
+ {
+ common->wcmMinRing = absinfo.minimum;
+ common->wcmMaxRing = absinfo.maximum;
+ }
+
/* X tilt range */
if (ISBITSET(abs, ABS_TILT_X) &&
!ioctl(pInfo->fd, EVIOCGABS(ABS_TILT_X), &absinfo))
diff --git a/src/xf86Wacom.c b/src/xf86Wacom.c
index 40b5c3d..b4cf25b 100644
--- a/src/xf86Wacom.c
+++ b/src/xf86Wacom.c
@@ -282,8 +282,8 @@ static int wcmInitAxes(DeviceIntPtr pWcm)
{
/* Touch ring */
label = XIGetKnownProperty(AXIS_LABEL_PROP_ABS_WHEEL);
- min = MIN_PAD_RING;
- max = MAX_PAD_RING;
+ min = common->wcmMinRing;
+ max = common->wcmMaxRing;
}

wcmInitAxis(pInfo->dev, index, label, min, max, res, min_res, max_res, mode);
@@ -298,8 +298,8 @@ static int wcmInitAxes(DeviceIntPtr pWcm)
mode = Absolute;
min_res = max_res = res = 1;

- min = MIN_PAD_RING;
- max = MAX_PAD_RING;
+ min = common->wcmMinRing;
+ max = common->wcmMaxRing;

wcmInitAxis(pInfo->dev, index, label, min, max, res, min_res, max_res, mode);
}
diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
index 9de9cab..3961545 100644
--- a/src/xf86WacomDefs.h
+++ b/src/xf86WacomDefs.h
@@ -48,9 +48,6 @@
#define TILT_MIN -64 /* Minimum reported tilt value */
#define TILT_MAX 63 /* Maximum reported tilt value */

-#define MIN_PAD_RING 0 /* I4 absolute scroll ring min value */
-#define MAX_PAD_RING 71 /* I4 absolute scroll ring max value */
-
/* I4 cursor tool has a rotation offset of 175 degrees */
#define INTUOS4_CURSOR_ROTATION_OFFSET 175

@@ -235,8 +232,8 @@ struct _WacomDeviceState
};

static const struct _WacomDeviceState OUTPROX_STATE = {
- .abswheel = MAX_PAD_RING + 1,
- .abswheel2 = MAX_PAD_RING + 1
+ .abswheel = INT_MAX,
+ .abswheel2 = INT_MAX
};

struct _WacomDeviceRec
@@ -436,6 +433,8 @@ struct _WacomCommonRec

int wcmMaxStripX; /* Maximum fingerstrip X */
int wcmMaxStripY; /* Maximum fingerstrip Y */
+ int wcmMinRing; /* Minimum touchring value */
+ int wcmMaxRing; /* Maximum touchring value */

WacomDevicePtr wcmDevices; /* list of devices sharing same port */
int wcmPktLength; /* length of a packet */

--
2.10.0

Peter Hutterer

2016-10-17 04:01:46 UTC

Post by Jason Gerecke
The TouchRing on the MobileStudio Pro has a range of only 0-35 instead
of 0-17 like prior devices. Because we hardcode an assumed range, the
driver mistakenly believes the jump from 35 to 0 (or 0 to 35, depending
on direction) when the user completes a revolution is actually caused
by the user reversing their finger direction. By reading the range from
the kernel, we can avoid this situation.
Note that the ABS_WHEEL axis is also used by (legacy) combined pen/pad
devices with a range corresponding to the puck fingerwheel. We need to
be careful to not read the value in these cases since it would lead to
erroneous behavior in existing setups. Devices with a range range
different from 0-71 will hopefully not be widely used on kernels prior
to 3.17 where pen and pad were split into seperate devices since there
is no way (short of peeking at the VID:PID or name) to have them work
correctly in the combined case.

Reviewed-by: Peter Hutterer <***@who-t.net>

on that note and because I saw the EVIOCGABS() - do we want to use libevdev?
given that we don't touch the evdev code itself much it doesn't gain us that
much (besides being able to drop the various wcmMax*/wcmMin* values) and it
would introduce a new dependency that's not present on all systems currently
supported. So I'm not convinced myself, but it doesn't hurt putting it out
there....

Cheers,
Peter

Post by Jason Gerecke
---
src/wcmCommon.c | 5 +++--
src/wcmUSB.c | 10 ++++++++++
src/xf86Wacom.c | 8 ++++----
src/xf86WacomDefs.h | 9 ++++-----
4 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/src/wcmCommon.c b/src/wcmCommon.c
index 9408f42..0ba0304 100644
--- a/src/wcmCommon.c
+++ b/src/wcmCommon.c
@@ -361,6 +361,7 @@ static void sendWheelStripEvents(InputInfoPtr pInfo, const WacomDeviceState* ds,
int first_val, int num_vals, int *valuators)
{
WacomDevicePtr priv = (WacomDevicePtr) pInfo->private;
+ WacomCommonPtr common = priv->common;
int delta = 0, idx = 0;
DBG(10, priv, "\n");
@@ -396,7 +397,7 @@ static void sendWheelStripEvents(InputInfoPtr pInfo, const WacomDeviceState* ds,
}
/* emulate events for left touch ring */
- delta = getScrollDelta(ds->abswheel, priv->oldState.abswheel, MAX_PAD_RING, AXIS_INVERT);
+ delta = getScrollDelta(ds->abswheel, priv->oldState.abswheel, common->wcmMaxRing, AXIS_INVERT);
idx = getWheelButton(delta, WHEEL_ABS_UP, WHEEL_ABS_DN);
if (idx >= 0 && IsPad(priv) && priv->oldState.proximity == ds->proximity)
{
@@ -406,7 +407,7 @@ static void sendWheelStripEvents(InputInfoPtr pInfo, const WacomDeviceState* ds,
}
/* emulate events for right touch ring */
- delta = getScrollDelta(ds->abswheel2, priv->oldState.abswheel2, MAX_PAD_RING, AXIS_INVERT);
+ delta = getScrollDelta(ds->abswheel2, priv->oldState.abswheel2, common->wcmMaxRing, AXIS_INVERT);
idx = getWheelButton(delta, WHEEL2_ABS_UP, WHEEL2_ABS_DN);
if (idx >= 0 && IsPad(priv) && priv->oldState.proximity == ds->proximity)
{
diff --git a/src/wcmUSB.c b/src/wcmUSB.c
index dcb1690..2f9d93f 100644
--- a/src/wcmUSB.c
+++ b/src/wcmUSB.c
@@ -652,6 +652,16 @@ int usbWcmGetRanges(InputInfoPtr pInfo)
common->wcmMaxStripX = absinfo.maximum;
}
+ /* max touchring value for standalone pad tools */
+ common->wcmMinRing = 0;
+ common->wcmMaxRing = 71;
+ if (!ISBITSET(ev,EV_MSC) && ISBITSET(abs, ABS_WHEEL) &&
+ !ioctl(pInfo->fd, EVIOCGABS(ABS_WHEEL), &absinfo))
+ {
+ common->wcmMinRing = absinfo.minimum;
+ common->wcmMaxRing = absinfo.maximum;
+ }
+
/* X tilt range */
if (ISBITSET(abs, ABS_TILT_X) &&
!ioctl(pInfo->fd, EVIOCGABS(ABS_TILT_X), &absinfo))
diff --git a/src/xf86Wacom.c b/src/xf86Wacom.c
index 40b5c3d..b4cf25b 100644
--- a/src/xf86Wacom.c
+++ b/src/xf86Wacom.c
@@ -282,8 +282,8 @@ static int wcmInitAxes(DeviceIntPtr pWcm)
{
/* Touch ring */
label = XIGetKnownProperty(AXIS_LABEL_PROP_ABS_WHEEL);
- min = MIN_PAD_RING;
- max = MAX_PAD_RING;
+ min = common->wcmMinRing;
+ max = common->wcmMaxRing;
}
wcmInitAxis(pInfo->dev, index, label, min, max, res, min_res, max_res, mode);
@@ -298,8 +298,8 @@ static int wcmInitAxes(DeviceIntPtr pWcm)
mode = Absolute;
min_res = max_res = res = 1;
- min = MIN_PAD_RING;
- max = MAX_PAD_RING;
+ min = common->wcmMinRing;
+ max = common->wcmMaxRing;
wcmInitAxis(pInfo->dev, index, label, min, max, res, min_res, max_res, mode);
}
diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
index 9de9cab..3961545 100644
--- a/src/xf86WacomDefs.h
+++ b/src/xf86WacomDefs.h
@@ -48,9 +48,6 @@
#define TILT_MIN -64 /* Minimum reported tilt value */
#define TILT_MAX 63 /* Maximum reported tilt value */
-#define MIN_PAD_RING 0 /* I4 absolute scroll ring min value */
-#define MAX_PAD_RING 71 /* I4 absolute scroll ring max value */
-
/* I4 cursor tool has a rotation offset of 175 degrees */
#define INTUOS4_CURSOR_ROTATION_OFFSET 175
@@ -235,8 +232,8 @@ struct _WacomDeviceState
};
static const struct _WacomDeviceState OUTPROX_STATE = {
- .abswheel = MAX_PAD_RING + 1,
- .abswheel2 = MAX_PAD_RING + 1
+ .abswheel = INT_MAX,
+ .abswheel2 = INT_MAX
};
struct _WacomDeviceRec
@@ -436,6 +433,8 @@ struct _WacomCommonRec
int wcmMaxStripX; /* Maximum fingerstrip X */
int wcmMaxStripY; /* Maximum fingerstrip Y */
+ int wcmMinRing; /* Minimum touchring value */
+ int wcmMaxRing; /* Maximum touchring value */
WacomDevicePtr wcmDevices; /* list of devices sharing same port */
int wcmPktLength; /* length of a packet */
--
2.10.0

Peter Hutterer

2016-10-17 03:57:11 UTC

Sorry about the delay, only been back from holidays since mid last week.

Add a pair of inline helper functions, that way you can have the 2048 in
only one place and you can add the documentation in one place too.

Cheers,
Peter

Post by Jason Gerecke
prop_threshold = InitWcmAtom(pInfo->dev, WACOM_PROP_PRESSURE_THRESHOLD, XA_INTEGER, 32, 1, values);
values[0] = common->wcmSuppress;
@@ -827,6 +828,7 @@ int wcmSetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
common->wcmCursorProxoutDist = value;
} else if (property == prop_threshold)
{
+ const INT32 MAXIMUM = 2048; /* backwards compatibility */
INT32 value;
if (prop->size != 1 || prop->format != 32)
@@ -836,8 +838,10 @@ int wcmSetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
if (value == -1)
value = DEFAULT_THRESHOLD;
- else if ((value < 1) || (value > FILTER_PRESSURE_RES))
+ else if ((value < 1) || (value > MAXIMUM))
return BadValue;
+ else
+ value *= (FILTER_PRESSURE_RES / MAXIMUM);
if (!checkonly)
common->wcmThreshold = value;
diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
index 1575960..9de9cab 100644
--- a/src/xf86WacomDefs.h
+++ b/src/xf86WacomDefs.h
@@ -183,7 +183,7 @@ struct _WacomModel
#define IsUSBDevice(common) ((common)->wcmDevCls == &gWacomUSBDevice)
-#define FILTER_PRESSURE_RES 2048 /* maximum points in pressure curve */
+#define FILTER_PRESSURE_RES 65536 /* maximum points in pressure curve */
/* Tested result for setting the pressure threshold to a reasonable value */
#define THRESHOLD_TOLERANCE (FILTER_PRESSURE_RES / 125)
#define DEFAULT_THRESHOLD (FILTER_PRESSURE_RES / 75)
--
2.10.0

Ping Cheng

2016-10-27 01:24:48 UTC

Reviewed-by: Ping Cheng <***@wacom.com> for the set of 2 patches.

Thank you for considering backward compatibility. That's a bonus for
many system admins who use xsetwacom/xinput to configure the driver.

Please update this patch as Peter suggested though.

Ping

Post by Jason Gerecke
---
src/wcmXCommand.c | 6 +++++-
src/xf86WacomDefs.h | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/wcmXCommand.c b/src/wcmXCommand.c
index 02278ba..403bc84 100644
--- a/src/wcmXCommand.c
+++ b/src/wcmXCommand.c
@@ -256,6 +256,7 @@ void InitWcmDeviceProperties(InputInfoPtr pInfo)
}
values[0] = (!common->wcmMaxZ) ? 0 : common->wcmThreshold;
+ values[0] /= (FILTER_PRESSURE_RES / 2048); /* backwards compatibility */
prop_threshold = InitWcmAtom(pInfo->dev, WACOM_PROP_PRESSURE_THRESHOLD, XA_INTEGER, 32, 1, values);
values[0] = common->wcmSuppress;
@@ -827,6 +828,7 @@ int wcmSetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
common->wcmCursorProxoutDist = value;
} else if (property == prop_threshold)
{
+ const INT32 MAXIMUM = 2048; /* backwards compatibility */
INT32 value;
if (prop->size != 1 || prop->format != 32)
@@ -836,8 +838,10 @@ int wcmSetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
if (value == -1)
value = DEFAULT_THRESHOLD;
- else if ((value < 1) || (value > FILTER_PRESSURE_RES))
+ else if ((value < 1) || (value > MAXIMUM))
return BadValue;
+ else
+ value *= (FILTER_PRESSURE_RES / MAXIMUM);
if (!checkonly)
common->wcmThreshold = value;
diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
index 1575960..9de9cab 100644
--- a/src/xf86WacomDefs.h
+++ b/src/xf86WacomDefs.h
@@ -183,7 +183,7 @@ struct _WacomModel
#define IsUSBDevice(common) ((common)->wcmDevCls == &gWacomUSBDevice)
-#define FILTER_PRESSURE_RES 2048 /* maximum points in pressure curve */
+#define FILTER_PRESSURE_RES 65536 /* maximum points in pressure curve */
/* Tested result for setting the pressure threshold to a reasonable value */
#define THRESHOLD_TOLERANCE (FILTER_PRESSURE_RES / 125)
#define DEFAULT_THRESHOLD (FILTER_PRESSURE_RES / 75)
--
2.10.0

Peter Hutterer

2016-10-27 06:36:10 UTC

Thank you for considering backward compatibility. That's a bonus for
many system admins who use xsetwacom/xinput to configure the driver.
Please update this patch as Peter suggested though.

uhm, just something else I noticed. we precalculate the pressure curve, so
we now have 256 KB in the driver allocated just for the values. We should
probably split this out to be allocated on demand (not everyone needs the
pressure curve) and to avoid blowing up our struct sizes even more.

Or, event better would be to have the pressure curve calculated based on the
device's actual min/max, so we only carry around as much as we need.

Cheers,
Peter

Post by Jason Gerecke
src/wcmXCommand.c | 6 +++++-
src/xf86WacomDefs.h | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/wcmXCommand.c b/src/wcmXCommand.c
index 02278ba..403bc84 100644
--- a/src/wcmXCommand.c
+++ b/src/wcmXCommand.c
@@ -256,6 +256,7 @@ void InitWcmDeviceProperties(InputInfoPtr pInfo)
}
values[0] = (!common->wcmMaxZ) ? 0 : common->wcmThreshold;
+ values[0] /= (FILTER_PRESSURE_RES / 2048); /* backwards compatibility */
prop_threshold = InitWcmAtom(pInfo->dev, WACOM_PROP_PRESSURE_THRESHOLD, XA_INTEGER, 32, 1, values);
values[0] = common->wcmSuppress;
@@ -827,6 +828,7 @@ int wcmSetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
common->wcmCursorProxoutDist = value;
} else if (property == prop_threshold)
{
+ const INT32 MAXIMUM = 2048; /* backwards compatibility */
INT32 value;
if (prop->size != 1 || prop->format != 32)
@@ -836,8 +838,10 @@ int wcmSetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
if (value == -1)
value = DEFAULT_THRESHOLD;
- else if ((value < 1) || (value > FILTER_PRESSURE_RES))
+ else if ((value < 1) || (value > MAXIMUM))
return BadValue;
+ else
+ value *= (FILTER_PRESSURE_RES / MAXIMUM);
if (!checkonly)
common->wcmThreshold = value;
diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
index 1575960..9de9cab 100644
--- a/src/xf86WacomDefs.h
+++ b/src/xf86WacomDefs.h
@@ -183,7 +183,7 @@ struct _WacomModel
#define IsUSBDevice(common) ((common)->wcmDevCls == &gWacomUSBDevice)
-#define FILTER_PRESSURE_RES 2048 /* maximum points in pressure curve */
+#define FILTER_PRESSURE_RES 65536 /* maximum points in pressure curve */
/* Tested result for setting the pressure threshold to a reasonable value */
#define THRESHOLD_TOLERANCE (FILTER_PRESSURE_RES / 125)
#define DEFAULT_THRESHOLD (FILTER_PRESSURE_RES / 75)
--
2.10.0

Ping Cheng

2016-10-27 06:56:57 UTC

Post by Jason Gerecke
than 2048 pressure levels are also normalized down to this range

though,

Post by Jason Gerecke
reducing their precision. In order to accomodate the new 8K pen (and

any

Post by Jason Gerecke
future pens with even higher precision), this patch bumps up the full-
scale range to be 0..65535. This number was chosen both because it far
exceeds anything currently known about, and also because it matches the
normalization range used over the wire by the Wayland tablet protocol.
Note that the WACOM_PROP_PRESSURE_THRESHOLD value has been tied to the
normalized (2048-level) pressure range for some time, meaning that we
cannot simply change the range without causing a change in the

perceived

Post by Jason Gerecke
threshold for users. To ensure compatibility, the value is interpreted
as a fraction of 2048 and then scaled to the actual normalization

range.
2 patches.

Post by Ping Cheng
Thank you for considering backward compatibility. That's a bonus for
many system admins who use xsetwacom/xinput to configure the driver.
Please update this patch as Peter suggested though.

uhm, just something else I noticed. we precalculate the pressure curve, so
we now have 256 KB in the driver allocated just for the values. We should
probably split this out to be allocated on demand

How do you plan to make it on demand? Will it add a new API?

Cheers,

Ping

Post by Jason Gerecke
(not everyone needs the
pressure curve) and to avoid blowing up our struct sizes even more.
Or, event better would be to have the pressure curve calculated based on the
device's actual min/max, so we only carry around as much as we need.
Cheers,
Peter

compatibility */

Post by Jason Gerecke
prop_threshold = InitWcmAtom(pInfo->dev,

WACOM_PROP_PRESSURE_THRESHOLD, XA_INTEGER, 32, 1, values);

Post by Jason Gerecke
values[0] = common->wcmSuppress;
@@ -827,6 +828,7 @@ int wcmSetProperty(DeviceIntPtr dev, Atom

property, XIPropertyValuePtr prop,

Post by Jason Gerecke
common->wcmCursorProxoutDist = value;
} else if (property == prop_threshold)
{
+ const INT32 MAXIMUM = 2048; /* backwards compatibility

Post by Jason Gerecke
INT32 value;
if (prop->size != 1 || prop->format != 32)
@@ -836,8 +838,10 @@ int wcmSetProperty(DeviceIntPtr dev, Atom

property, XIPropertyValuePtr prop,

Post by Jason Gerecke
if (value == -1)
value = DEFAULT_THRESHOLD;
- else if ((value < 1) || (value > FILTER_PRESSURE_RES))
+ else if ((value < 1) || (value > MAXIMUM))
return BadValue;
+ else
+ value *= (FILTER_PRESSURE_RES / MAXIMUM);
if (!checkonly)
common->wcmThreshold = value;
diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
index 1575960..9de9cab 100644
--- a/src/xf86WacomDefs.h
+++ b/src/xf86WacomDefs.h
@@ -183,7 +183,7 @@ struct _WacomModel
#define IsUSBDevice(common) ((common)->wcmDevCls == &gWacomUSBDevice)
-#define FILTER_PRESSURE_RES 2048 /* maximum points in pressure

curve */

Post by Jason Gerecke
+#define FILTER_PRESSURE_RES 65536 /* maximum points in pressure

curve */

Post by Jason Gerecke
/* Tested result for setting the pressure threshold to a reasonable

value */

Post by Jason Gerecke
#define THRESHOLD_TOLERANCE (FILTER_PRESSURE_RES / 125)
#define DEFAULT_THRESHOLD (FILTER_PRESSURE_RES / 75)
--
2.10.0

Peter Hutterer

2016-10-28 01:27:03 UTC

Post by Jason Gerecke
than 2048 pressure levels are also normalized down to this range

though,

Post by Jason Gerecke
reducing their precision. In order to accomodate the new 8K pen (and

any

perceived

Post by Jason Gerecke
threshold for users. To ensure compatibility, the value is interpreted
as a fraction of 2048 and then scaled to the actual normalization

range.
2 patches.

uhm, just something else I noticed. we precalculate the pressure curve, so
we now have 256 KB in the driver allocated just for the values. We should
probably split this out to be allocated on demand

How do you plan to make it on demand? Will it add a new API?

Changing pPressCurve[FILTER_PRESSURE_RES + 1] to a dynamically allocated
memory should be enough, no new API is needed, this is just an
implementation detail.

Cheers,
Peter

Ping Cheng

2016-10-28 02:27:27 UTC

<javascript:;>>
<javascript:;>

Post by Jason Gerecke
The driver has historically normalized the pressure range of all

kernel

Post by Jason Gerecke
devices to 0..2047 rather than using their native range to keep

things

Post by Jason Gerecke
like the application of the pressure curve simple. Pens that report

Post by Jason Gerecke
than 2048 pressure levels are also normalized down to this range

though,

Post by Jason Gerecke
reducing their precision. In order to accomodate the new 8K pen

(and

Post by Jason Gerecke
any

Post by Jason Gerecke
future pens with even higher precision), this patch bumps up the

full-

Post by Jason Gerecke
scale range to be 0..65535. This number was chosen both because it

far

Post by Jason Gerecke
exceeds anything currently known about, and also because it

matches the

Post by Jason Gerecke
normalization range used over the wire by the Wayland tablet

protocol.

Post by Jason Gerecke
Note that the WACOM_PROP_PRESSURE_THRESHOLD value has been tied to

the

Post by Jason Gerecke
normalized (2048-level) pressure range for some time, meaning that

Post by Jason Gerecke
cannot simply change the range without causing a change in the

perceived

Post by Jason Gerecke
threshold for users. To ensure compatibility, the value is

interpreted

Post by Jason Gerecke
as a fraction of 2048 and then scaled to the actual normalization

range.

<javascript:;> <javascript:;>>
<javascript:;>> for the set of

Post by Jason Gerecke
2 patches.

uhm, just something else I noticed. we precalculate the pressure

curve, so

Post by Jason Gerecke
we now have 256 KB in the driver allocated just for the values. We

should

Post by Jason Gerecke
probably split this out to be allocated on demand

How do you plan to make it on demand? Will it add a new API?

Changing pPressCurve[FILTER_PRESSURE_RES + 1] to a dynamically allocated
memory should be enough, no new API is needed, this is just an
implementation detail.

Great! That's smart. Then go for it ;).

Cheers,

Ping

Jason Gerecke

2016-11-01 18:43:43 UTC

Now that the pressure curve contains 65K points it takes up quite a bit
of memory, especially considering that the pressure curve may not need
to exist for some devices (e.g. pads) and may just be the default linear
curve even for those where it should exist. To reduce the amount of
memory used, we now lazily allocate space for the pressure curve tables
only when they are set to a non-default curve.

Signed-off-by: Jason Gerecke <***@wacom.com>
---
Changes from v1:
* Added this patch to lazilly allocate the pressure curve as requested

src/wcmCommon.c | 5 ++++-
src/wcmFilter.c | 30 ++++++++++++++++++++----------
src/xf86WacomDefs.h | 2 +-
3 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/wcmCommon.c b/src/wcmCommon.c
index 0ba0304..28e5488 100644
--- a/src/wcmCommon.c
+++ b/src/wcmCommon.c
@@ -1395,7 +1395,10 @@ static int applyPressureCurve(WacomDevicePtr pDev, const WacomDeviceStatePtr pSt
p = min(FILTER_PRESSURE_RES, p);

/* apply pressure curve function */
- return pDev->pPressCurve[p];
+ if (pDev->pPressCurve == NULL)
+ return p;
+ else
+ return pDev->pPressCurve[p];
}

/*****************************************************************************
diff --git a/src/wcmFilter.c b/src/wcmFilter.c
index e55ef0f..7ef6349 100644
--- a/src/wcmFilter.c
+++ b/src/wcmFilter.c
@@ -54,15 +54,30 @@ int wcmCheckPressureCurveValues(int x0, int y0, int x1, int y1)
void wcmSetPressureCurve(WacomDevicePtr pDev, int x0, int y0,
int x1, int y1)
{
- int i;
-
/* sanity check values */
if (!wcmCheckPressureCurveValues(x0, y0, x1, y1))
return;

- /* linear by default */
- for (i=0; i<=FILTER_PRESSURE_RES; ++i)
- pDev->pPressCurve[i] = i;
+ free(pDev->pPressCurve);
+ pDev->pPressCurve = NULL;
+
+ if (!(x0 == 0 && y0 == 0 && x1 == 100 && y1 == 100)) {
+ pDev->pPressCurve = calloc(FILTER_PRESSURE_RES+1, sizeof(*pDev->pPressCurve));
+ }
+
+ /* A NULL pPressCurve indicates the (default) linear curve */
+ if (!pDev->pPressCurve) {
+ pDev->nPressCtrl[0] = 0;
+ pDev->nPressCtrl[1] = 0;
+ pDev->nPressCtrl[2] = 100;
+ pDev->nPressCtrl[3] = 100;
+ return;
+ }
+
+ pDev->nPressCtrl[0] = x0;
+ pDev->nPressCtrl[1] = y0;
+ pDev->nPressCtrl[2] = x1;
+ pDev->nPressCtrl[3] = y1;

/* draw bezier line from bottom-left to top-right using ctrl points */
filterCurveToLine(pDev->pPressCurve,
@@ -71,11 +86,6 @@ void wcmSetPressureCurve(WacomDevicePtr pDev, int x0, int y0,
x0/100.0, y0/100.0, /* control point 1 */
x1/100.0, y1/100.0, /* control point 2 */
1.0, 1.0); /* top right */
-
- pDev->nPressCtrl[0] = x0;
- pDev->nPressCtrl[1] = y0;
- pDev->nPressCtrl[2] = x1;
- pDev->nPressCtrl[3] = y1;
}

/*
diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
index 3961545..c03d5d9 100644
--- a/src/xf86WacomDefs.h
+++ b/src/xf86WacomDefs.h
@@ -286,7 +286,7 @@ struct _WacomDeviceRec
int oldCursorHwProx; /* previous cursor hardware proximity */

/* JEJ - filters */
- int pPressCurve[FILTER_PRESSURE_RES + 1]; /* pressure curve */
+ int *pPressCurve; /* pressure curve */
int nPressCtrl[4]; /* control points for curve */
int minPressure; /* the minimum pressure a pen may hold */
int oldMinPressure; /* to record the last minPressure before going out of proximity */

--
2.10.1

Peter Hutterer

2016-11-01 22:47:36 UTC

Post by Jason Gerecke
Now that the pressure curve contains 65K points it takes up quite a bit
of memory, especially considering that the pressure curve may not need
to exist for some devices (e.g. pads) and may just be the default linear
curve even for those where it should exist. To reduce the amount of
memory used, we now lazily allocate space for the pressure curve tables
only when they are set to a non-default curve.
---
* Added this patch to lazilly allocate the pressure curve as requested
src/wcmCommon.c | 5 ++++-
src/wcmFilter.c | 30 ++++++++++++++++++++----------
src/xf86WacomDefs.h | 2 +-
3 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/src/wcmCommon.c b/src/wcmCommon.c
index 0ba0304..28e5488 100644
--- a/src/wcmCommon.c
+++ b/src/wcmCommon.c
@@ -1395,7 +1395,10 @@ static int applyPressureCurve(WacomDevicePtr pDev, const WacomDeviceStatePtr pSt
p = min(FILTER_PRESSURE_RES, p);
/* apply pressure curve function */
- return pDev->pPressCurve[p];
+ if (pDev->pPressCurve == NULL)
+ return p;
+ else
+ return pDev->pPressCurve[p];
}
/*****************************************************************************
diff --git a/src/wcmFilter.c b/src/wcmFilter.c
index e55ef0f..7ef6349 100644
--- a/src/wcmFilter.c
+++ b/src/wcmFilter.c
@@ -54,15 +54,30 @@ int wcmCheckPressureCurveValues(int x0, int y0, int x1, int y1)
void wcmSetPressureCurve(WacomDevicePtr pDev, int x0, int y0,
int x1, int y1)
{
- int i;
-
/* sanity check values */
if (!wcmCheckPressureCurveValues(x0, y0, x1, y1))
return;
- /* linear by default */
- for (i=0; i<=FILTER_PRESSURE_RES; ++i)
- pDev->pPressCurve[i] = i;
+ free(pDev->pPressCurve);
+ pDev->pPressCurve = NULL;
+
+ if (!(x0 == 0 && y0 == 0 && x1 == 100 && y1 == 100)) {

not a big fan of if (!(foo == x), very easy to overlook. You can just use
x0 != 0 || y0 != 0 ... here (but see below)

Post by Jason Gerecke
+ pDev->pPressCurve = calloc(FILTER_PRESSURE_RES+1, sizeof(*pDev->pPressCurve));
+ }
+
+ /* A NULL pPressCurve indicates the (default) linear curve */
+ if (!pDev->pPressCurve) {
+ pDev->nPressCtrl[0] = 0;
+ pDev->nPressCtrl[1] = 0;
+ pDev->nPressCtrl[2] = 100;
+ pDev->nPressCtrl[3] = 100;
+ return;
+ }
+
+ pDev->nPressCtrl[0] = x0;
+ pDev->nPressCtrl[1] = y0;
+ pDev->nPressCtrl[2] = x1;
+ pDev->nPressCtrl[3] = y1;

you can reshuffle this to save on code:

pDev->nPressCtrl[0] = x0;
pDev->nPressCtrl[1] = y0;
pDev->nPressCtrl[2] = x1;
pDev->nPressCtrl[3] = y1;

if (x0 == 0 && y0 == 0 && x1 == 100 && y1 == 100)
return;

calloc now

if calloc fails, print an error message and return. you could go back and
reset the control points to defaults, but for this niche case you might as
well keep going and just rely on applyPressureCurve() to not access a NULL
curve.

Post by Jason Gerecke
/* draw bezier line from bottom-left to top-right using ctrl points */
filterCurveToLine(pDev->pPressCurve,
@@ -71,11 +86,6 @@ void wcmSetPressureCurve(WacomDevicePtr pDev, int x0, int y0,
x0/100.0, y0/100.0, /* control point 1 */
x1/100.0, y1/100.0, /* control point 2 */
1.0, 1.0); /* top right */
-
- pDev->nPressCtrl[0] = x0;
- pDev->nPressCtrl[1] = y0;
- pDev->nPressCtrl[2] = x1;
- pDev->nPressCtrl[3] = y1;
}
/*
diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
index 3961545..c03d5d9 100644
--- a/src/xf86WacomDefs.h
+++ b/src/xf86WacomDefs.h
@@ -286,7 +286,7 @@ struct _WacomDeviceRec
int oldCursorHwProx; /* previous cursor hardware proximity */
/* JEJ - filters */

want to remove ^^ while we're at it? doesn't serve much purpose anymore.

Cheers,
Peter

Post by Jason Gerecke
- int pPressCurve[FILTER_PRESSURE_RES + 1]; /* pressure curve */
+ int *pPressCurve; /* pressure curve */
int nPressCtrl[4]; /* control points for curve */
int minPressure; /* the minimum pressure a pen may hold */
int oldMinPressure; /* to record the last minPressure before going out of proximity */
--
2.10.1

Jason Gerecke

2016-11-11 17:36:24 UTC

Now that the pressure curve contains 65K points it takes up quite a bit
of memory, especially considering that the pressure curve may not need
to exist for some devices (e.g. pads) and may just be the default linear
curve even for those where it should exist. To reduce the amount of
memory used, we now lazily allocate space for the pressure curve tables
only when they are set to a non-default curve.

Signed-off-by: Jason Gerecke <***@wacom.com>
---
Changes from v2:
* Use affirmative (instead of negating) check for default (0,0,100,100)
case.
* Shuffle code to reduce duplication.
* Log warning in case of memory allocation failure.
* Be even lazier (don't go through a free/calloc cycle if pPressCurve is
already allocated).

Peter:
I'm not totally happy with leaving the nPressCtrl points set to the wrong
value if a memory allocation failure happens. I suppose strict correctness
is probably moot in this situation since the server probably isn't long
for this world once we can't allocate a few hundred KB, but still... :/

I've re-shuffled things to try and minimize the amount of repeating of
myself -- let me know if its any better.

src/wcmCommon.c | 5 ++++-
src/wcmFilter.c | 38 +++++++++++++++++++++++++-------------
src/xf86WacomDefs.h | 3 +--
3 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/src/wcmCommon.c b/src/wcmCommon.c
index 0ba0304..28e5488 100644
--- a/src/wcmCommon.c
+++ b/src/wcmCommon.c
@@ -1395,7 +1395,10 @@ static int applyPressureCurve(WacomDevicePtr pDev, const WacomDeviceStatePtr pSt
p = min(FILTER_PRESSURE_RES, p);

/* apply pressure curve function */
- return pDev->pPressCurve[p];
+ if (pDev->pPressCurve == NULL)
+ return p;
+ else
+ return pDev->pPressCurve[p];
}

/*****************************************************************************
diff --git a/src/wcmFilter.c b/src/wcmFilter.c
index e55ef0f..aca5cd9 100644
--- a/src/wcmFilter.c
+++ b/src/wcmFilter.c
@@ -54,23 +54,35 @@ int wcmCheckPressureCurveValues(int x0, int y0, int x1, int y1)
void wcmSetPressureCurve(WacomDevicePtr pDev, int x0, int y0,
int x1, int y1)
{
- int i;
-
/* sanity check values */
if (!wcmCheckPressureCurveValues(x0, y0, x1, y1))
return;

- /* linear by default */
- for (i=0; i<=FILTER_PRESSURE_RES; ++i)
- pDev->pPressCurve[i] = i;
-
- /* draw bezier line from bottom-left to top-right using ctrl points */
- filterCurveToLine(pDev->pPressCurve,
- FILTER_PRESSURE_RES,
- 0.0, 0.0, /* bottom left */
- x0/100.0, y0/100.0, /* control point 1 */
- x1/100.0, y1/100.0, /* control point 2 */
- 1.0, 1.0); /* top right */
+ /* A NULL pPressCurve indicates the (default) linear curve */
+ if (x0 == 0 && y0 == 0 && x1 == 100 && y1 == 100) {
+ free(pDev->pPressCurve);
+ pDev->pPressCurve = NULL;
+ }
+ else if (!pDev->pPressCurve) {
+ pDev->pPressCurve = calloc(FILTER_PRESSURE_RES+1, sizeof(*pDev->pPressCurve));
+
+ if (!pDev->pPressCurve) {
+ LogMessageVerbSigSafe(X_WARNING, 0,
+ "Unable to allocate memory for pressure curve; using default.\n");
+ x0 = 0;
+ y0 = 0;
+ x1 = 100;
+ y1 = 100;
+ }
+ }
+
+ if (pDev->pPressCurve)
+ filterCurveToLine(pDev->pPressCurve,
+ FILTER_PRESSURE_RES,
+ 0.0, 0.0, /* bottom left */
+ x0/100.0, y0/100.0, /* control point 1 */
+ x1/100.0, y1/100.0, /* control point 2 */
+ 1.0, 1.0); /* top right */

pDev->nPressCtrl[0] = x0;
pDev->nPressCtrl[1] = y0;
diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
index 3961545..b10a114 100644
--- a/src/xf86WacomDefs.h
+++ b/src/xf86WacomDefs.h
@@ -285,8 +285,7 @@ struct _WacomDeviceRec
struct _WacomDeviceState oldState; /* previous state information */
int oldCursorHwProx; /* previous cursor hardware proximity */

- /* JEJ - filters */
- int pPressCurve[FILTER_PRESSURE_RES + 1]; /* pressure curve */
+ int *pPressCurve; /* pressure curve */
int nPressCtrl[4]; /* control points for curve */
int minPressure; /* the minimum pressure a pen may hold */
int oldMinPressure; /* to record the last minPressure before going out of proximity */

--
2.10.1

Peter Hutterer

2016-11-13 22:02:19 UTC

Reviewed-by: Peter Hutterer <***@who-t.net>

thanks, happy now :)

Cheers,
Peter

Post by Jason Gerecke
---
* Use affirmative (instead of negating) check for default (0,0,100,100)
case.
* Shuffle code to reduce duplication.
* Log warning in case of memory allocation failure.
* Be even lazier (don't go through a free/calloc cycle if pPressCurve is
already allocated).
I'm not totally happy with leaving the nPressCtrl points set to the wrong
value if a memory allocation failure happens. I suppose strict correctness
is probably moot in this situation since the server probably isn't long
for this world once we can't allocate a few hundred KB, but still... :/
I've re-shuffled things to try and minimize the amount of repeating of
myself -- let me know if its any better.
src/wcmCommon.c | 5 ++++-
src/wcmFilter.c | 38 +++++++++++++++++++++++++-------------
src/xf86WacomDefs.h | 3 +--
3 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/src/wcmCommon.c b/src/wcmCommon.c
index 0ba0304..28e5488 100644
--- a/src/wcmCommon.c
+++ b/src/wcmCommon.c
@@ -1395,7 +1395,10 @@ static int applyPressureCurve(WacomDevicePtr pDev, const WacomDeviceStatePtr pSt
p = min(FILTER_PRESSURE_RES, p);
/* apply pressure curve function */
- return pDev->pPressCurve[p];
+ if (pDev->pPressCurve == NULL)
+ return p;
+ else
+ return pDev->pPressCurve[p];
}
/*****************************************************************************
diff --git a/src/wcmFilter.c b/src/wcmFilter.c
index e55ef0f..aca5cd9 100644
--- a/src/wcmFilter.c
+++ b/src/wcmFilter.c
@@ -54,23 +54,35 @@ int wcmCheckPressureCurveValues(int x0, int y0, int x1, int y1)
void wcmSetPressureCurve(WacomDevicePtr pDev, int x0, int y0,
int x1, int y1)
{
- int i;
-
/* sanity check values */
if (!wcmCheckPressureCurveValues(x0, y0, x1, y1))
return;
- /* linear by default */
- for (i=0; i<=FILTER_PRESSURE_RES; ++i)
- pDev->pPressCurve[i] = i;
-
- /* draw bezier line from bottom-left to top-right using ctrl points */
- filterCurveToLine(pDev->pPressCurve,
- FILTER_PRESSURE_RES,
- 0.0, 0.0, /* bottom left */
- x0/100.0, y0/100.0, /* control point 1 */
- x1/100.0, y1/100.0, /* control point 2 */
- 1.0, 1.0); /* top right */
+ /* A NULL pPressCurve indicates the (default) linear curve */
+ if (x0 == 0 && y0 == 0 && x1 == 100 && y1 == 100) {
+ free(pDev->pPressCurve);
+ pDev->pPressCurve = NULL;
+ }
+ else if (!pDev->pPressCurve) {
+ pDev->pPressCurve = calloc(FILTER_PRESSURE_RES+1, sizeof(*pDev->pPressCurve));
+
+ if (!pDev->pPressCurve) {
+ LogMessageVerbSigSafe(X_WARNING, 0,
+ "Unable to allocate memory for pressure curve; using default.\n");
+ x0 = 0;
+ y0 = 0;
+ x1 = 100;
+ y1 = 100;
+ }
+ }
+
+ if (pDev->pPressCurve)
+ filterCurveToLine(pDev->pPressCurve,
+ FILTER_PRESSURE_RES,
+ 0.0, 0.0, /* bottom left */
+ x0/100.0, y0/100.0, /* control point 1 */
+ x1/100.0, y1/100.0, /* control point 2 */
+ 1.0, 1.0); /* top right */
pDev->nPressCtrl[0] = x0;
pDev->nPressCtrl[1] = y0;
diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
index 3961545..b10a114 100644
--- a/src/xf86WacomDefs.h
+++ b/src/xf86WacomDefs.h
@@ -285,8 +285,7 @@ struct _WacomDeviceRec
struct _WacomDeviceState oldState; /* previous state information */
int oldCursorHwProx; /* previous cursor hardware proximity */
- /* JEJ - filters */
- int pPressCurve[FILTER_PRESSURE_RES + 1]; /* pressure curve */
+ int *pPressCurve; /* pressure curve */
int nPressCtrl[4]; /* control points for curve */
int minPressure; /* the minimum pressure a pen may hold */
int oldMinPressure; /* to record the last minPressure before going out of proximity */
--
2.10.1

Peter Hutterer

2016-11-01 22:39:19 UTC