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

On Sat, 21 Dec 2024 20:44:24 +0100,
Kirill A. Korinsky <kirill@korins.ky> wrote:
> 
> 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.
> 

Oops, attached wrong diff. I mean this one:

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 20:03:25 -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];
 
@@ -1218,6 +1219,9 @@ uvideo_vs_parse_desc_alt(struct uvideo_s
 		}
 		desc = usbd_desc_iter_next(&iter);
 	}
+
+	vs->bulk_endpoint = 1;
+
 	while (desc) {
 		/* Crossed device function boundary. */
 		if (desc->bDescriptorType == UDESC_IFACE_ASSOC)
@@ -1248,12 +1252,20 @@ 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) {
 			vs->ifaceh = &sc->sc_udev->ifaces[iface];
@@ -1262,6 +1274,7 @@ uvideo_vs_parse_desc_alt(struct uvideo_s
 			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 +1298,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 +1332,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 && !vs->bulk_endpoint)
+			goto next;
 
 		/* save endpoint with requested bandwidth */
 		psize = UGETW(ed->wMaxPacketSize);


-- 
wbr, Kirill