Hi,
this is the v2 of my series which reworks the HID report processing.
I took Ping's comments into account, and amended my MR with the
regression tests[0].
More specifically, the tests (and thus this new version of the series)
enforces that only one BTN_TOOL_* event gets forwarded between each
EV_SYN frame, and that BTN_TOUCH are properly translated too.
This also magivally solved some worrying transitions we had in the
pen state machine where the pen was jumping from "eraser" to "in
contact". This new behavior enforces a "out-of-range" state in the
middle, making it easier for userspace to understand now.
Again, tests are welcome :)
Cheers,
Benjamin
[0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127
Benjamin Tissoires (12):
HID: core: statically allocate read buffers
HID: core: de-duplicate some code in hid_input_field()
HID: core: split data fetching from processing in hid_input_field()
HID: input: tag touchscreens as such if the physical is not there
HID: input: rework spaghetti code with switch statements
HID: input: move up out-of-range processing of input values
HID: compute an ordered list of input fields to process
HID: core: for input reports, process the usages by priority list
HID: input: enforce Invert usage to be processed before InRange
HID: input: remove the need for HID_QUIRK_INVERT
HID: input: accommodate priorities for slotted devices
Input: docs: add more details on the use of BTN_TOOL
Documentation/input/event-codes.rst | 6 +-
drivers/hid/hid-core.c | 280 ++++++++++++++++++---
drivers/hid/hid-input.c | 364 ++++++++++++++++++++++------
include/linux/hid.h | 23 +-
4 files changed, 568 insertions(+), 105 deletions(-)
--
2.33.1
This is a preparation patch for rethinking the generic processing
of HID reports.
We can actually pre-allocate all of our memory instead of dynamically
allocating/freeing it whenever we parse a report.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-core.c | 12 +++++-------
include/linux/hid.h | 1 +
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index f1aed5bbd000..75e7b8447bf7 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
field = kzalloc((sizeof(struct hid_field) +
usages * sizeof(struct hid_usage) +
- usages * sizeof(unsigned)), GFP_KERNEL);
+ 2 * usages * sizeof(unsigned int)), GFP_KERNEL);
if (!field)
return NULL;
@@ -109,6 +109,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
report->field[field->index] = field;
field->usage = (struct hid_usage *)(field + 1);
field->value = (s32 *)(field->usage + usages);
+ field->new_value = (s32 *)(field->value + usages);
field->report = report;
return field;
@@ -1541,9 +1542,8 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
__s32 max = field->logical_maximum;
__s32 *value;
- value = kmalloc_array(count, sizeof(__s32), GFP_ATOMIC);
- if (!value)
- return;
+ value = field->new_value;
+ memset(value, 0, count * sizeof(__s32));
for (n = 0; n < count; n++) {
@@ -1557,7 +1557,7 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
value[n] >= min && value[n] <= max &&
value[n] - min < field->maxusage &&
field->usage[value[n] - min].hid == HID_UP_KEYBOARD + 1)
- goto exit;
+ return;
}
for (n = 0; n < count; n++) {
@@ -1581,8 +1581,6 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field,
}
memcpy(field->value, value, count * sizeof(__s32));
-exit:
- kfree(value);
}
/*
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 7487b0586fe6..3fbfe0986659 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -476,6 +476,7 @@ struct hid_field {
unsigned report_count; /* number of this field in the report */
unsigned report_type; /* (input,output,feature) */
__s32 *value; /* last known value(s) */
+ __s32 *new_value; /* newly read value(s) */
__s32 logical_minimum;
__s32 logical_maximum;
__s32 physical_minimum;
--
2.33.1
Some devices (Elan, Synaptics...) are sometimes not setting a physical
in their finger collections. hid-input will consider them to be pen
devices, leading to some wrong behavior in user space.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-input.c | 29 +++++++++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 112901d2d8d2..d2562497a726 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -828,10 +828,31 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
break;
case 0x32: /* InRange */
- switch (field->physical & 0xff) {
- case 0x21: map_key(BTN_TOOL_MOUSE); break;
- case 0x22: map_key(BTN_TOOL_FINGER); break;
- default: map_key(BTN_TOOL_PEN); break;
+ switch (field->physical) {
+ case HID_DG_PUCK:
+ map_key(BTN_TOOL_MOUSE);
+ break;
+ case HID_DG_FINGER:
+ map_key(BTN_TOOL_FINGER);
+ break;
+ default:
+ /*
+ * If the physical is not given,
+ * rely on the application.
+ */
+ if (!field->physical) {
+ switch (field->application) {
+ case HID_DG_TOUCHSCREEN:
+ case HID_DG_TOUCHPAD:
+ map_key_clear(BTN_TOOL_FINGER);
+ break;
+ default:
+ map_key_clear(BTN_TOOL_PEN);
+ }
+ } else {
+ map_key(BTN_TOOL_PEN);
+ }
+ break;
}
break;
--
2.33.1
Multitouch devices in hybrid mode are reporting multiple times the
same collection. We should accommodate for this in our handling
of priorities by defining the slots they belong to.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-input.c | 103 ++++++++++++++++++++++++++++++++++++----
include/linux/hid.h | 1 +
2 files changed, 96 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 9f8853640648..56d4e91c4750 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -48,6 +48,16 @@ static const struct {
__s32 y;
} hid_hat_to_axis[] = {{ 0, 0}, { 0,-1}, { 1,-1}, { 1, 0}, { 1, 1}, { 0, 1}, {-1, 1}, {-1, 0}, {-1,-1}};
+struct usage_priority {
+ __u32 usage; /* the HID usage associated */
+ bool global; /* we assume all usages to be slotted,
+ * unless global
+ */
+ unsigned int slot_overwrite; /* for globals: allows to set the usage
+ * before or after the slots
+ */
+};
+
/*
* hid-input will convert this list into priorities:
* the first element will have the highest priority
@@ -57,17 +67,30 @@ static const struct {
* hid-input will then shift the priority by 8 bits to leave some space
* in case drivers want to interleave other fields.
*
+ * To accommodate slotted devices, the slot priority is
+ * defined in the next 8 bits (defined by 0xff - slot).
+ *
* If drivers want to add fields before those, hid-input will
* leave out the first 8 bits of the priority value.
*
* This still leaves us 65535 individual priority values.
*/
-static const __u32 hidinput_usages_priorities[] = {
- HID_DG_ERASER, /* Eraser (eraser touching) must always come before tipswitch */
- HID_DG_INVERT, /* Invert must always come before In Range */
- HID_DG_TIPSWITCH, /* Is the tip of the tool touching? */
- HID_DG_TIPPRESSURE, /* Tip Pressure might emulate tip switch */
- HID_DG_INRANGE, /* In Range needs to come after the other tool states */
+static const struct usage_priority hidinput_usages_priorities[] = {
+ { /* Eraser (eraser touching) must always come before tipswitch */
+ .usage = HID_DG_ERASER,
+ },
+ { /* Invert must always come before In Range */
+ .usage = HID_DG_INVERT,
+ },
+ { /* Is the tip of the tool touching? */
+ .usage = HID_DG_TIPSWITCH,
+ },
+ { /* Tip Pressure might emulate tip switch */
+ .usage = HID_DG_TIPPRESSURE,
+ },
+ { /* In Range needs to come after the other tool states */
+ .usage = HID_DG_INRANGE,
+ },
};
#define map_abs(c) hid_map_usage(hidinput, usage, &bit, &max, EV_ABS, (c))
@@ -612,6 +635,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
{
struct input_dev *input = hidinput->input;
struct hid_device *device = input_get_drvdata(input);
+ const struct usage_priority *usage_priority = NULL;
int max = 0, code;
unsigned int i = 0;
unsigned long *bit = NULL;
@@ -633,13 +657,26 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
/* assign a priority based on the static list declared here */
for (i = 0; i < ARRAY_SIZE(hidinput_usages_priorities); i++) {
- if (usage->hid == hidinput_usages_priorities[i]) {
+ if (usage->hid == hidinput_usages_priorities[i].usage) {
+ usage_priority = &hidinput_usages_priorities[i];
+
field->usages_priorities[usage_index] =
(ARRAY_SIZE(hidinput_usages_priorities) - i) << 8;
break;
}
}
+ /*
+ * For slotted devices, we need to also add the slot index
+ * in the priority.
+ */
+ if (usage_priority && usage_priority->global)
+ field->usages_priorities[usage_index] |=
+ usage_priority->slot_overwrite;
+ else
+ field->usages_priorities[usage_index] |=
+ (0xff - field->slot_idx) << 16;
+
if (device->driver->input_mapping) {
int ret = device->driver->input_mapping(device, hidinput, field,
usage, &bit, &max);
@@ -2068,7 +2105,57 @@ static struct hid_input *hidinput_match_application(struct hid_report *report)
static inline void hidinput_configure_usages(struct hid_input *hidinput,
struct hid_report *report)
{
- int i, j;
+ int i, j, k;
+ int first_field_index = 0;
+ int slot_collection_index = -1;
+ int prev_collection_index = -1;
+ unsigned int slot_idx = 0;
+ struct hid_field *field;
+
+ /*
+ * First tag all the fields that are part of a slot,
+ * a slot needs to have one Contact ID in the collection
+ */
+ for (i = 0; i < report->maxfield; i++) {
+ field = report->field[i];
+
+ /* ignore fields without usage */
+ if (field->maxusage < 1)
+ continue;
+
+ /*
+ * janitoring when collection_index changes
+ */
+ if (prev_collection_index != field->usage->collection_index) {
+ prev_collection_index = field->usage->collection_index;
+ first_field_index = i;
+ }
+
+ /*
+ * if we already found a Contact ID in the collection,
+ * tag and continue to the next.
+ */
+ if (slot_collection_index == field->usage->collection_index) {
+ field->slot_idx = slot_idx;
+ continue;
+ }
+
+ /* check if the current field has Contact ID */
+ for (j = 0; j < field->maxusage; j++) {
+ if (field->usage[j].hid == HID_DG_CONTACTID) {
+ slot_collection_index = field->usage->collection_index;
+ slot_idx++;
+
+ /*
+ * mark all previous fields and this one in the
+ * current collection to be slotted.
+ */
+ for (k = first_field_index; k <= i; k++)
+ report->field[k]->slot_idx = slot_idx;
+ break;
+ }
+ }
+ }
for (i = 0; i < report->maxfield; i++)
for (j = 0; j < report->field[i]->maxusage; j++)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index feb8df61168f..4363a63b9775 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -492,6 +492,7 @@ struct hid_field {
/* hidinput data */
struct hid_input *hidinput; /* associated input structure */
__u16 dpad; /* dpad input code */
+ unsigned int slot_idx; /* slot index in a report */
};
#define HID_MAX_FIELDS 256
--
2.33.1
This will be used in a later commit:
we build a list of input fields (and usage_index) that is ordered based
on a usage priority.
Changing the usage priority allows to re-order the processed list, meaning
that we can enforce some usages to be process before others.
For instance, before processing InRange in the HID tablets, we need to
know if we are using the eraser (side or button). Enforcing a higher
(lower number) priority for Invert allows to force the input stack to
process that field before.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-core.c | 105 ++++++++++++++++++++++++++++++++++++++++-
include/linux/hid.h | 10 ++++
2 files changed, 114 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 34188d7ac0f7..b38528118642 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -81,6 +81,7 @@ struct hid_report *hid_register_report(struct hid_device *device,
report_enum->report_id_hash[id] = report;
list_add_tail(&report->list, &report_enum->report_list);
+ INIT_LIST_HEAD(&report->field_entry_list);
return report;
}
@@ -101,7 +102,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
field = kzalloc((sizeof(struct hid_field) +
usages * sizeof(struct hid_usage) +
- 2 * usages * sizeof(unsigned int)), GFP_KERNEL);
+ 3 * usages * sizeof(unsigned int)), GFP_KERNEL);
if (!field)
return NULL;
@@ -110,6 +111,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
field->usage = (struct hid_usage *)(field + 1);
field->value = (s32 *)(field->usage + usages);
field->new_value = (s32 *)(field->value + usages);
+ field->usages_priorities = (s32 *)(field->new_value + usages);
field->report = report;
return field;
@@ -657,6 +659,8 @@ static void hid_free_report(struct hid_report *report)
{
unsigned n;
+ kfree(report->field_entries);
+
for (n = 0; n < report->maxfield; n++)
kfree(report->field[n]);
kfree(report);
@@ -1667,6 +1671,103 @@ static void hid_process_report(struct hid_device *hid,
}
}
+/*
+ * Insert a given usage_index in a field in the list
+ * of processed usages in the report.
+ *
+ * The elements of lower priority score are processed
+ * first.
+ */
+static void __hid_insert_field_entry(struct hid_device *hid,
+ struct hid_report *report,
+ struct hid_field_entry *entry,
+ struct hid_field *field,
+ unsigned int usage_index)
+{
+ struct hid_field_entry *next;
+
+ entry->field = field;
+ entry->index = usage_index;
+ entry->priority = field->usages_priorities[usage_index];
+
+ /* insert the element at the correct position */
+ list_for_each_entry(next,
+ &report->field_entry_list,
+ list) {
+ /*
+ * the priority of our element is strictly higher
+ * than the next one, insert it before
+ */
+ if (entry->priority > next->priority) {
+ list_add_tail(&entry->list, &next->list);
+ return;
+ }
+ }
+
+ /* lowest priority score: insert at the end */
+ list_add_tail(&entry->list, &report->field_entry_list);
+}
+
+static void hid_report_process_ordering(struct hid_device *hid,
+ struct hid_report *report)
+{
+ struct hid_field *field;
+ struct hid_field_entry *entries;
+ unsigned int a, u, usages;
+ unsigned int count = 0;
+
+ /* count the number of individual fields in the report */
+ for (a = 0; a < report->maxfield; a++) {
+ field = report->field[a];
+
+ if (field->flags & HID_MAIN_ITEM_VARIABLE)
+ count += field->report_count;
+ else
+ count++;
+ }
+
+ /* allocate the memory to process the fields */
+ entries = kcalloc(count, sizeof(*entries), GFP_KERNEL);
+ if (!entries)
+ return;
+
+ report->field_entries = entries;
+
+ /*
+ * walk through all fields in the report and
+ * store them by priority order in report->field_entry_list
+ *
+ * - Var elements are individualized (field + usage_index)
+ * - Arrays are taken as one, we can not chose an order for them
+ */
+ usages = 0;
+ for (a = 0; a < report->maxfield; a++) {
+ field = report->field[a];
+
+ if (field->flags & HID_MAIN_ITEM_VARIABLE) {
+ for (u = 0; u < field->report_count; u++) {
+ __hid_insert_field_entry(hid, report,
+ &entries[usages],
+ field, u);
+ usages++;
+ }
+ } else {
+ __hid_insert_field_entry(hid, report, &entries[usages],
+ field, 0);
+ usages++;
+ }
+ }
+}
+
+static void hid_process_ordering(struct hid_device *hid)
+{
+ struct hid_report *report;
+ struct hid_report_enum *report_enum = &hid->report_enum[HID_INPUT_REPORT];
+
+ list_for_each_entry(report, &report_enum->report_list, list)
+ hid_report_process_ordering(hid, report);
+}
+
/*
* Output the field into the report.
*/
@@ -2050,6 +2151,8 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
return -ENODEV;
}
+ hid_process_ordering(hdev);
+
if ((hdev->claimed & HID_CLAIMED_INPUT) &&
(connect_mask & HID_CONNECT_FF) && hdev->ff_init)
hdev->ff_init(hdev);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index cf79eb3da465..f25020c0d6b8 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -477,6 +477,7 @@ struct hid_field {
unsigned report_type; /* (input,output,feature) */
__s32 *value; /* last known value(s) */
__s32 *new_value; /* newly read value(s) */
+ __s32 *usages_priorities; /* priority of each usage when reading the report */
__s32 logical_minimum;
__s32 logical_maximum;
__s32 physical_minimum;
@@ -493,13 +494,22 @@ struct hid_field {
#define HID_MAX_FIELDS 256
+struct hid_field_entry {
+ struct list_head list;
+ struct hid_field *field;
+ unsigned int index;
+ __s32 priority;
+};
+
struct hid_report {
struct list_head list;
struct list_head hidinput_list;
+ struct list_head field_entry_list; /* ordered list of input fields */
unsigned int id; /* id of this report */
unsigned int type; /* report type */
unsigned int application; /* application usage for this report */
struct hid_field *field[HID_MAX_FIELDS]; /* fields of the report */
+ struct hid_field_entry *field_entries; /* allocated memory of input field_entry */
unsigned maxfield; /* maximum valid field index */
unsigned size; /* size of the report (bits) */
struct hid_device *device; /* associated device */
--
2.33.1
On Thu, 3 Feb 2022, Benjamin Tissoires wrote:
> Hi,
>
> this is the v2 of my series which reworks the HID report processing.
>
> I took Ping's comments into account, and amended my MR with the
> regression tests[0].
> More specifically, the tests (and thus this new version of the series)
> enforces that only one BTN_TOOL_* event gets forwarded between each
> EV_SYN frame, and that BTN_TOUCH are properly translated too.
>
> This also magivally solved some worrying transitions we had in the
> pen state machine where the pen was jumping from "eraser" to "in
> contact". This new behavior enforces a "out-of-range" state in the
> middle, making it easier for userspace to understand now.
>
> Again, tests are welcome :)
>
> Cheers,
> Benjamin
>
> [0] https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/127
>
> Benjamin Tissoires (12):
> HID: core: statically allocate read buffers
> HID: core: de-duplicate some code in hid_input_field()
> HID: core: split data fetching from processing in hid_input_field()
> HID: input: tag touchscreens as such if the physical is not there
> HID: input: rework spaghetti code with switch statements
> HID: input: move up out-of-range processing of input values
> HID: compute an ordered list of input fields to process
> HID: core: for input reports, process the usages by priority list
> HID: input: enforce Invert usage to be processed before InRange
> HID: input: remove the need for HID_QUIRK_INVERT
> HID: input: accommodate priorities for slotted devices
> Input: docs: add more details on the use of BTN_TOOL
>
> Documentation/input/event-codes.rst | 6 +-
> drivers/hid/hid-core.c | 280 ++++++++++++++++++---
> drivers/hid/hid-input.c | 364 ++++++++++++++++++++++------
> include/linux/hid.h | 23 +-
> 4 files changed, 568 insertions(+), 105 deletions(-)
This is now in hid.git#for-5.18/core.
Thanks a lot Benjamin, very nice work.
--
Jiri Kosina
SUSE Labs