From: Thomas Dettbarn Subject: Re: imt(4) Patch To Increase Number Of Usable Devices To: tech@openbsd.org Date: Wed, 28 May 2025 08:07:39 +0200 Hello. In accordance with slide 16 of the wonderful document https://www.openbsd.org/papers/eurobsdcon2017-device-drivers.pdf this is me, following up on my own thread after one week. During said week, a new corner case came to mind, so the patch below is an improvement upon the first improvment. There are still two things with which I need help: 1. Testing on more than one computer 2. Reformatting the code in style(9) Thomas xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx Patch 2 by Thomas Dettbarn (git diff against c6ac02a2b3a608d4bafeca03d190519141521fad) The original code does not respect Logical Collections inside Application collections from the report descriptor. Instead, it is searching for the LOWEST report ID inside an application collection with the correct usage code (000D/0005). The report descriptor, which was causing issues, looked like the one below. It ended with a vendor specific section, that defined a vendor specific report 06, even though the input report was defined as report 0x54. What this patch does is to restrict the reports which can be found to the ones that are ACTUALLY providing X and Y coordinates. (Device: ihidev0 at iic0 addr 0x15 gpio 171, vendor 0x4f3 product 0x3195, ELAN0676) usage page 000D/0005 (Digitizer/Touch Pad ) collection 01 (Application) report ID 0x54 usage 0009/0001 (Button/Button 1) 1 Bit padding 3 Bit usage 000D/0054 (Digitizer/Contact Count) 4 Bit usage 000D/0056 (Digitizer/Scan Count) 16 Bit usage 000D/0022 (Digitizer/Finger) collection 02 (Logical) usage 000D/0047 (Digitizer/Touch Valid) 1 Bit usage 000D/0042 (Digitizer/Tip Switch) 1 Bit padding 2 Bit usage 000D/0051 (Digitizer/Contact Identifier) 4 Bit usage 0001/0030 (Generic Desktop/X) 16 Bit usage 0001/0031 (Generic Desktop/Y) 16 Bit collection end collection 02 (Logical) usage 000D/0047 (Digitizer/Touch Valid) 1 Bit usage 000D/0042 (Digitizer/Tip Switch) 1 Bit padding 2 Bit usage 000D/0051 (Digitizer/Contact Identifier) 4 Bit usage 0001/0030 (Generic Desktop/X) 16 Bit usage 0001/0031 (Generic Desktop/Y) 16 Bit collection end collection 02 (Logical) usage 000D/0047 (Digitizer/Touch Valid) 1 Bit usage 000D/0042 (Digitizer/Tip Switch) 1 Bit padding 2 Bit usage 000D/0051 (Digitizer/Contact Identifier) 4 Bit usage 0001/0030 (Generic Desktop/X) 16 Bit usage 0001/0031 (Generic Desktop/Y) 16 Bit collection end collection 02 (Logical) usage 000D/0047 (Digitizer/Touch Valid) 1 Bit usage 000D/0042 (Digitizer/Tip Switch) 1 Bit padding 2 Bit usage 000D/0051 (Digitizer/Contact Identifier) 4 Bit usage 0001/0030 (Generic Desktop/X) 16 Bit usage 0001/0031 (Generic Desktop/Y) 16 Bit collection end collection 02 (Logical) usage 000D/0047 (Digitizer/Touch Valid) 1 Bit usage 000D/0042 (Digitizer/Tip Switch) 1 Bit padding 2 Bit usage 000D/0051 (Digitizer/Contact Identifier) 4 Bit usage 0001/0030 (Generic Desktop/X) 16 Bit usage 0001/0031 (Generic Desktop/Y) 16 Bit collection end report ID 02 .... Capabilities report ID 07 .... Latency Mode report ID 06 usage page FF00/00C5 (Vendor specific) 2048 Bit collection end diff --git sys/dev/hid/hidmt.c sys/dev/hid/hidmt.c index 62b500a4f44..b393b40e9e5 100644 --- sys/dev/hid/hidmt.c +++ sys/dev/hid/hidmt.c @@ -175,45 +175,46 @@ hidmt_setup(struct device *self, struct hidmt *mt, void *desc, int dlen) if (h.report_ID != mt->sc_rep_input) continue; - if (h.kind != hid_input) - continue; - - switch (h.usage) { - /* contact level usages */ - case HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_X): - if (h.logical_maximum - h.logical_minimum) { - mt->sc_minx = h.logical_minimum; - mt->sc_maxx = h.logical_maximum; - mt->sc_resx = hidmt_get_resolution(&h); - } - break; - case HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_Y): - if (h.logical_maximum - h.logical_minimum) { - mt->sc_miny = h.logical_minimum; - mt->sc_maxy = h.logical_maximum; - mt->sc_resy = hidmt_get_resolution(&h); - } - break; - case HID_USAGE2(HUP_DIGITIZERS, HUD_TIP_SWITCH): - case HID_USAGE2(HUP_DIGITIZERS, HUD_CONFIDENCE): - case HID_USAGE2(HUP_DIGITIZERS, HUD_WIDTH): - case HID_USAGE2(HUP_DIGITIZERS, HUD_HEIGHT): - case HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTID): - - /* report level usages */ - case HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTCOUNT): - case HID_USAGE2(HUP_BUTTON, 0x01): - case HID_USAGE2(HUP_BUTTON, 0x02): - case HID_USAGE2(HUP_BUTTON, 0x03): - break; - default: - continue; - } input = malloc(sizeof(*input), M_DEVBUF, M_NOWAIT | M_ZERO); memcpy(&input->loc, &h.loc, sizeof(struct hid_location)); - input->usage = h.usage; + input->kind = h.kind; + + if (h.kind == hid_input) { + switch (h.usage) { + /* contact level usages */ + case HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_X): + if (h.logical_maximum - h.logical_minimum) { + mt->sc_minx = h.logical_minimum; + mt->sc_maxx = h.logical_maximum; + mt->sc_resx = hidmt_get_resolution(&h); + } + break; + case HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_Y): + if (h.logical_maximum - h.logical_minimum) { + mt->sc_miny = h.logical_minimum; + mt->sc_maxy = h.logical_maximum; + mt->sc_resy = hidmt_get_resolution(&h); + } + break; + case HID_USAGE2(HUP_DIGITIZERS, HUD_TIP_SWITCH): + case HID_USAGE2(HUP_DIGITIZERS, HUD_CONFIDENCE): + case HID_USAGE2(HUP_DIGITIZERS, HUD_WIDTH): + case HID_USAGE2(HUP_DIGITIZERS, HUD_HEIGHT): + case HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTID): + + /* report level usages */ + case HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTCOUNT): + case HID_USAGE2(HUP_BUTTON, 0x01): + case HID_USAGE2(HUP_BUTTON, 0x02): + case HID_USAGE2(HUP_BUTTON, 0x03): + break; + default: + continue; + } + input->usage = h.usage; + } SIMPLEQ_INSERT_TAIL(&mt->sc_inputs, input, entry); } hid_end_parse(hd); @@ -300,6 +301,15 @@ hidmt_input(struct hidmt *mt, uint8_t *data, u_int len) struct hidmt_contact hc; int32_t d, firstu = 0; int contactcount = 0, seencontacts = 0, tips = 0, buttons = 0, i, s, z; + /* There are two strategies for handling multiple inputs. + * The first one strategy = 0, is plan A: There are logical + * collections inside the report. strategy = 1 is plan B: + * Simply search for repeating usages. + */ + int collection_cnt; + int endcollection_cnt; + int usagey_cnt; + int strategy = 1; // enable plan B. if (len != mt->sc_rep_input_size) { DPRINTF(("%s: %s: length %d not %d, ignoring\n", @@ -317,10 +327,22 @@ hidmt_input(struct hidmt *mt, uint8_t *data, u_int len) * report with contactid=0 but contactids are zero-based, find * contactcount first. */ + collection_cnt = 0; + endcollection_cnt = 0; + usagey_cnt = 0; SIMPLEQ_FOREACH(hi, &mt->sc_inputs, entry) { - if (hi->usage == HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTCOUNT)) + if (hi->kind == hid_collection) + collection_cnt++; + if (hi->kind == hid_endcollection) + endcollection_cnt++; + if (hi->kind == hid_input && hi->usage == HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_Y)) + usagey_cnt++; + if (hi->kind == hid_input && hi->usage == HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTCOUNT)) contactcount = hid_get_udata(data, len, &hi->loc); } + if (collection_cnt == endcollection_cnt && collection_cnt >= usagey_cnt && collection_cnt >= contactcount) { + strategy = 0; // plan A is possible + } if (contactcount) mt->sc_cur_contactcount = contactcount; @@ -358,66 +380,69 @@ hidmt_input(struct hidmt *mt, uint8_t *data, u_int len) */ bzero(&hc, sizeof(struct hidmt_contact)); SIMPLEQ_FOREACH(hi, &mt->sc_inputs, entry) { - d = hid_get_udata(data, len, &hi->loc); - - if (firstu && hi->usage == firstu) { + if ((!strategy && hi->kind == hid_endcollection) || + (strategy && firstu && hi->usage == firstu) + ) { if (seencontacts < contactcount) { hc.seen = 1; i = wsmouse_id_to_slot( - mt->sc_wsmousedev, hc.contactid); + mt->sc_wsmousedev, hc.contactid); if (i >= 0) memcpy(&mt->sc_contacts[i], &hc, - sizeof(struct hidmt_contact)); + sizeof(struct hidmt_contact)); seencontacts++; } bzero(&hc, sizeof(struct hidmt_contact)); } - else if (!firstu) - firstu = hi->usage; - - switch (hi->usage) { - case HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_X): - hc.x = d; - break; - case HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_Y): - if (mt->sc_flags & HIDMT_REVY) - hc.y = mt->sc_maxy - d; - else - hc.y = d; - break; - case HID_USAGE2(HUP_DIGITIZERS, HUD_TIP_SWITCH): - hc.tip = d; - if (d) - tips++; - break; - case HID_USAGE2(HUP_DIGITIZERS, HUD_CONFIDENCE): - hc.confidence = d; - break; - case HID_USAGE2(HUP_DIGITIZERS, HUD_WIDTH): - hc.width = d; - break; - case HID_USAGE2(HUP_DIGITIZERS, HUD_HEIGHT): - hc.height = d; - break; - case HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTID): - hc.contactid = d; - break; - - /* these will only appear once per report */ - case HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTCOUNT): - if (d) - contactcount = d; - break; - case HID_USAGE2(HUP_BUTTON, 0x01): - case HID_USAGE2(HUP_BUTTON, 0x02): - if (d != 0) - buttons |= 1; - break; - case HID_USAGE2(HUP_BUTTON, 0x03): - if (d != 0) - buttons |= 1 << 2; - break; + else if (hi->kind == hid_input) { + d = hid_get_udata(data, len, &hi->loc); + if (!firstu) + firstu = hi->usage; + + switch (hi->usage) { + case HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_X): + hc.x = d; + break; + case HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_Y): + if (mt->sc_flags & HIDMT_REVY) + hc.y = mt->sc_maxy - d; + else + hc.y = d; + break; + case HID_USAGE2(HUP_DIGITIZERS, HUD_TIP_SWITCH): + hc.tip = d; + if (d) + tips++; + break; + case HID_USAGE2(HUP_DIGITIZERS, HUD_CONFIDENCE): + hc.confidence = d; + break; + case HID_USAGE2(HUP_DIGITIZERS, HUD_WIDTH): + hc.width = d; + break; + case HID_USAGE2(HUP_DIGITIZERS, HUD_HEIGHT): + hc.height = d; + break; + case HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTID): + hc.contactid = d; + break; + + /* these will only appear once per report */ + case HID_USAGE2(HUP_DIGITIZERS, HUD_CONTACTCOUNT): + if (d) + contactcount = d; + break; + case HID_USAGE2(HUP_BUTTON, 0x01): + case HID_USAGE2(HUP_BUTTON, 0x02): + if (d != 0) + buttons |= 1; + break; + case HID_USAGE2(HUP_BUTTON, 0x03): + if (d != 0) + buttons |= 1 << 2; + break; + } } } if (seencontacts < contactcount) { diff --git sys/dev/hid/hidmtvar.h sys/dev/hid/hidmtvar.h index dabe5867b7e..dd4560b70ff 100644 --- sys/dev/hid/hidmtvar.h +++ sys/dev/hid/hidmtvar.h @@ -16,6 +16,7 @@ */ struct hidmt_data { + enum hid_kind kind; int32_t usage; struct hid_location loc; SIMPLEQ_ENTRY(hidmt_data) entry; diff --git sys/dev/i2c/imt.c sys/dev/i2c/imt.c index 5dcdbed57e9..875e9120e4e 100644 --- sys/dev/i2c/imt.c +++ sys/dev/i2c/imt.c @@ -116,12 +116,18 @@ imt_find_winptp_reports(struct ihidev_softc *parent, void *desc, int size, continue; if (hid_is_collection(desc, size, repid, - HID_USAGE2(HUP_DIGITIZERS, HUD_TOUCHPAD))) { + HID_USAGE2(HUP_DIGITIZERS, HUD_TOUCHPAD)) && + hid_locate(desc, size, HID_USAGE2(HUP_GENERIC_DESKTOP, + HUG_X), repid, hid_input, NULL, NULL) && + hid_locate(desc, size, HID_USAGE2(HUP_GENERIC_DESKTOP, + HUG_Y), repid, hid_input, NULL, NULL)) { input = 1; if (sc != NULL && sc->sc_rep_input == -1) sc->sc_rep_input = repid; } else if (hid_is_collection(desc, size, repid, - HID_USAGE2(HUP_DIGITIZERS, HUD_CONFIG))) { + HID_USAGE2(HUP_DIGITIZERS, HUD_CONFIG)) && + hid_locate(desc, size, HID_USAGE2(HUP_DIGITIZERS, + HUD_INPUT_MODE), repid, hid_feature, NULL, NULL)) { conf = 1; if (sc != NULL && sc->sc_rep_config == -1) sc->sc_rep_config = repid;