From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: sys/uvideo: support devices which reports bulk and isochronous endpoints
To: Marcus Glocker <marcus@nazgul.ch>
Cc: tech@openbsd.org
Date: Sat, 21 Dec 2024 20:44:24 +0100

On Sat, 21 Dec 2024 19:03:29 +0100,
Marcus Glocker <marcus@nazgul.ch> wrote:
>
> On Sat, Dec 21, 2024 at 12:35:41AM GMT, Kirill A. Korinsky wrote:
>
> > tech@,
> >
> > A user send a debug log for Elgato HD60 S+ which reports both UE_BULK
> > and UE_ISOCHRONOUS endpoints.
> >
> > The code selected endpoint with largest bandwidth, but vs->bulk_endpoint
> > depends on order of endpoints, and the last one is used, not selected one.
> >
> > Ok?
>
> Yep, nice one.
>
> ok mglocker@
>

Thus, this fix raises the next quesiton: which endpoint should be preffered?

After some reading if the specification I noticed that Section 2.4.3 does
not prohibit the mix of bulk and isochronous endpoints when the bulk
endpoints are used solely for still image transfer.

So, here an updated diff which prefered isochronous endpoints when both are
available on the device.

Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -r1.228 uvideo.c
--- sys/dev/usb/uvideo.c	14 Dec 2024 09:58:04 -0000	1.228
+++ sys/dev/usb/uvideo.c	21 Dec 2024 19:43:19 -0000
@@ -1204,6 +1204,7 @@ uvideo_vs_parse_desc_alt(struct uvideo_s
 	usb_interface_descriptor_t *id;
 	usb_endpoint_descriptor_t *ed;
 	uint8_t ep_dir, ep_type;
+	int bulk_endpoint;

 	vs = &sc->sc_vs_coll[vs_nr];

@@ -1248,20 +1249,29 @@ uvideo_vs_parse_desc_alt(struct uvideo_s
 		ep_dir = UE_GET_DIR(ed->bEndpointAddress);
 		ep_type = UE_GET_XFERTYPE(ed->bmAttributes);
 		if (ep_dir == UE_DIR_IN && ep_type == UE_ISOCHRONOUS)
-			vs->bulk_endpoint = 0;
+			bulk_endpoint = 0;
 		else if (ep_dir == UE_DIR_IN && ep_type == UE_BULK)
-			vs->bulk_endpoint = 1;
+			bulk_endpoint = 1;
 		else
 			goto next;

+		/*
+		 * Section 2.4.3 does not prohibit the mix of bulk and
+		 * isochronous endpoints when the bulk endpoints are
+		 * used solely for still image transfer.
+		 */
+		if (bulk_endpoint && !vs->bulk_endpoint)
+			goto next;
+
 		/* save endpoint with largest bandwidth */
-		if (UGETW(ed->wMaxPacketSize) > vs->psize) {
+		if (UGETW(ed->wMaxPacketSize) > psize) {
 			vs->ifaceh = &sc->sc_udev->ifaces[iface];
 			vs->endpoint = ed->bEndpointAddress;
 			vs->numalts = numalts;
 			vs->curalt = id->bAlternateSetting;
 			vs->psize = UGETW(ed->wMaxPacketSize);
 			vs->iface = iface;
+			vs->bulk_endpoint = bulk_endpoint;
 		}
 next:
 		desc = usbd_desc_iter_next(&iter);
@@ -1285,9 +1295,10 @@ uvideo_vs_set_alt(struct uvideo_softc *s
 	const usb_descriptor_t *desc;
 	usb_interface_descriptor_t *id;
 	usb_endpoint_descriptor_t *ed;
-	int diff, best_diff = INT_MAX;
+	int diff, best_diff = INT_MAX, bulk_endpoint;
 	usbd_status error;
 	uint32_t psize;
+	uint8_t ep_type;

 	usbd_desc_iter_init(sc->sc_udev, &iter);
 	desc = usbd_desc_iter_next(&iter);
@@ -1318,6 +1329,15 @@ uvideo_vs_set_alt(struct uvideo_softc *s
 		if (desc->bDescriptorType != UDESC_ENDPOINT)
 			goto next;
 		ed = (usb_endpoint_descriptor_t *)(uint8_t *)desc;
+
+		ep_type = UE_GET_XFERTYPE(ed->bmAttributes);
+		if (ep_type == UE_ISOCHRONOUS)
+			bulk_endpoint = 0;
+		else if (ep_type == UE_BULK)
+			bulk_endpoint = 1;
+
+		if (bulk_endpoint != sc->sc_vs_cur->bulk_endpoint)
+			goto next;

 		/* save endpoint with requested bandwidth */
 		psize = UGETW(ed->wMaxPacketSize);


-- 
wbr, Kirill