From: Kirill A. Korinsky Subject: Re: ims(4) touchscreen support for the SGB4E To: Marcus Glocker Cc: tech@openbsd.org Date: Mon, 01 Jun 2026 00:39:43 +0200 On Tue, 26 May 2026 21:04:59 +0200, Marcus Glocker wrote: > > Here's a status update for this diff: > > - The DTS part (the power-on path for the Samsung touchscreen) was > committed this morning, so it's out of the picture for this diff now. > > - I received a few test reports, all reporting no regressions. > > What the diff does: > > Today ihidev(4) doesn't handle empty packets for polling devices at > all -- that's the > > /* > * TODO: all fingers are up, should we pass to hid > * layer? > */ > > comment, which this diff completes with an edge-triggered check. > ihidev(4) remembers the report-id of the last non-empty report. When > we have delivered a report for some report-id and then read an empty > packet while polling, the device has gone idle (all contacts up). We > then hand that report-id's subdevice a zeroed report, so the upper > layer can register the release. The edge condition makes this fire > once, on the transition, not on every idle poll. > > Interrupt-driven devices are not affected -- their empty-packet path > still just returns as before. > > The zeroed report then reaches hidms. For an interrupt-driven > absolute device the real release report carries the exact coordinates; > for our polled device the synthesized report carries 0,0. Currently > hidms just sets the new absolute coordinates, so for the polled > device the 0,0 snaps the pointer to the upper-left corner. To prevent > that, we add a check in hidms: if a button was pressed before and > is now released, we skip setting the absolute coordinates on that > report and leave the pointer where it was. For an interrupt-driven > device this is invisible -- the previous position is the same point > the release report would have set -- and for the polled device it > prevents the corner snap. > > Importantly, this hidms check only does anything for devices that > report absolute coordinates (HIDMS_ABSX/ABSY). Relative pointers -- > ordinary mice and touchpads, that is nearly all ums(4) users -- skip > it entirely, which is why the test reports (all relative devices) show > no change. The only absolute device exercising this path is the > touchscreen. > > On regression risk: > > - For ihidev(4) I think it's close to zero: we've only completed a > code path that wasn't implemented for polling devices, and > interrupt-driven devices are untouched. > > - For hidms I think it's low as well: we now hold the last position > on release instead of re-setting it to the position from the release > report, which is the same point, so there's no visible change for > existing absolute (interrupt-driven) devices and it's a no-op for > relative ones. hidms_input() is used by ims(4) and ums(4); uwacom(4) > uses hidms for setup but parses input itself, so it is not affected. > If a regression does show up, the change is small and trivial to back > out. > > Based on that, ok? > > Tested for about a week on my main daily driver. No regression to report, but some feedback bellow. > Index: sys/dev/hid/hidms.c > =================================================================== > RCS file: /cvs/src/sys/dev/hid/hidms.c,v > diff -u -p -u -p -r1.11 hidms.c > --- sys/dev/hid/hidms.c 10 May 2024 10:49:10 -0000 1.11 > +++ sys/dev/hid/hidms.c 23 May 2026 14:30:24 -0000 > @@ -533,7 +533,7 @@ hidms_input(struct hidms *ms, uint8_t *d > { > int dx, dy, dz, dw; > u_int32_t buttons = 0; > - int i, s; > + int i, s, released; > > DPRINTFN(5,("hidms_input: len=%d\n", len)); > > @@ -595,17 +595,26 @@ hidms_input(struct hidms *ms, uint8_t *d > buttons != ms->sc_buttons) { > DPRINTFN(10, ("hidms_input: x:%d y:%d z:%d w:%d buttons:0x%x\n", > dx, dy, dz, dw, buttons)); > + released = (ms->sc_buttons != 0 && buttons == 0); > ms->sc_buttons = buttons; > if (ms->sc_wsmousedev != NULL) { > s = spltty(); > + /* > + * A polled touchscreen reports finger-up as a zeroed > + * packet; emitting its (0,0) as an absolute position > + * would snap the pointer to the corner. On a button > + * release edge, hold the last position instead. > + */ > if (ms->sc_flags & HIDMS_ABSX) { > - wsmouse_set(ms->sc_wsmousedev, > - WSMOUSE_ABS_X, dx, 0); > + if (!released) > + wsmouse_set(ms->sc_wsmousedev, > + WSMOUSE_ABS_X, dx, 0); > dx = 0; > } > if (ms->sc_flags & HIDMS_ABSY) { > - wsmouse_set(ms->sc_wsmousedev, > - WSMOUSE_ABS_Y, dy, 0); > + if (!released) > + wsmouse_set(ms->sc_wsmousedev, > + WSMOUSE_ABS_Y, dy, 0); > dy = 0; > } > WSMOUSE_INPUT(ms->sc_wsmousedev, I think this hidms part is broader than the stated touchscreen fix. The new released flag is true for every absolute HID device whose buttons transition from nonzero to zero; it is not tied to the synthetic zeroed report generated by the polled ihidev path, if I not mistaken. Consequently, a real release report from an interrupt driven absolute device, with valid final X/Y coordinates, will have those coordinates suppressed, and the button release will be delivered at the previous stored position. Not sure does it bring any issue, or just impossible case. > Index: sys/dev/i2c/ihidev.c > =================================================================== > RCS file: /cvs/src/sys/dev/i2c/ihidev.c,v > diff -u -p -u -p -r1.43 ihidev.c > --- sys/dev/i2c/ihidev.c 23 May 2026 11:10:57 -0000 1.43 > +++ sys/dev/i2c/ihidev.c 23 May 2026 14:30:24 -0000 > @@ -142,6 +142,7 @@ ihidev_attach(struct device *parent, str > sc->sc_tag = ia->ia_tag; > sc->sc_addr = ia->ia_addr; > sc->sc_hid_desc_addr = ia->ia_size; > + sc->sc_lastrepid = -1; > > if (ihidev_hid_command(sc, I2C_HID_CMD_DESCR, NULL) || > ihidev_hid_desc_parse(sc)) { > @@ -159,10 +160,13 @@ ihidev_attach(struct device *parent, str > > /* find largest report size and allocate memory for input buffer */ > sc->sc_isize = letoh16(sc->hid_desc.wMaxInputLength); > + sc->sc_repsizes = mallocarray(sc->sc_nrepid, sizeof(int), > + M_DEVBUF, M_WAITOK | M_ZERO); > for (repid = 0; repid < sc->sc_nrepid; repid++) { > repsz = hid_report_size(sc->sc_report, sc->sc_reportlen, > hid_input, repid); > repsizes[repid] = repsz; > + sc->sc_repsizes[repid] = repsz; > if (repsz > sc->sc_isize) > sc->sc_isize = repsz; > if (repsz != 0) > @@ -255,6 +259,9 @@ ihidev_detach(struct device *self, int f > if (sc->sc_report != NULL) > free(sc->sc_report, M_DEVBUF, sc->sc_reportlen); > > + if (sc->sc_repsizes != NULL) > + free(sc->sc_repsizes, M_DEVBUF, sc->sc_nrepid * sizeof(int)); > + > return (0); > } > > @@ -718,10 +725,21 @@ ihidev_intr(void *arg) > psize = sc->sc_ibuf[0] | sc->sc_ibuf[1] << 8; > if (psize <= 2 || psize > sc->sc_isize) { > if (sc->sc_poll) { > - /* > - * TODO: all fingers are up, should we pass to hid > - * layer? > - */ > + /* empty packet: hand the last subdev a zeroed report > + * once so it releases its contacts (polled finger-up) */ > + int lrep = sc->sc_lastrepid; > + int rsz; > + > + if (lrep >= 0 && lrep < sc->sc_nrepid && > + (scd = sc->sc_subdevs[lrep]) != NULL && > + (scd->sc_state & IHIDEV_OPEN) && !sc->sc_dying) { > + rsz = sc->sc_repsizes[lrep]; > + if (rsz > 0 && rsz <= sc->sc_isize) { > + memset(sc->sc_ibuf, 0, rsz); > + scd->sc_intr(scd, sc->sc_ibuf, rsz); > + } > + sc->sc_lastrepid = -1; > + } > sc->sc_fastpoll = 0; > goto more_polling; > } else I think on an empty polled packet it sends a zeroed input report to whichever subdevice last produced data. So the behaviour applies to other polled I2C HID child, not only ims. Not sure how safe it is. > @@ -770,8 +788,10 @@ ihidev_intr(void *arg) > return (1); > } > > - if (!sc->sc_dying) > + if (!sc->sc_dying) { > + sc->sc_lastrepid = rep; > scd->sc_intr(scd, p, psize); > + } > > if (sc->sc_poll && (fast != sc->sc_fastpoll)) { > DPRINTF(("%s: %s->%s polling\n", sc->sc_dev.dv_xname, > Index: sys/dev/i2c/ihidev.h > =================================================================== > RCS file: /cvs/src/sys/dev/i2c/ihidev.h,v > diff -u -p -u -p -r1.11 ihidev.h > --- sys/dev/i2c/ihidev.h 7 Jan 2025 19:26:14 -0000 1.11 > +++ sys/dev/i2c/ihidev.h 23 May 2026 14:30:24 -0000 > @@ -85,6 +85,8 @@ struct ihidev_softc { > > u_int sc_isize; > u_char *sc_ibuf; > + int sc_lastrepid; /* report id of last non-empty input */ > + int *sc_repsizes; /* per-report input size, for poll path */ > > int sc_refcnt; > > -- wbr, Kirill