Re: ims(4) touchscreen support for the SGB4E

On Tue, 26 May 2026 21:04:59 +0200,
Marcus Glocker <[email protected]> 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