On Sun, Dec 22, 2024 at 12:03:49AM GMT, Kirill A. Korinsky wrote:

> On Sat, 21 Dec 2024 21:04:13 +0100,
> Kirill A. Korinsky <kirill@korins.ky> wrote:
> > 
> > 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:
> >
> 
> I don't understand how it had happened, but the second attempt to attach the
> right diff had failed. Here the third one which I double check that it is
> that I really mean.
> 
> Sorry for the noise.

I was reviewing your patch in the meantime.  So it will always prefer
ISOC over BULK endpoints, if there are both in a device, and just if
there would be only BULK endpoints, it would pick the one with the
largest wMaxPackSize.  That make sense to me.

I've also tested your patch with some of my devices and couldn't spot
any regression.

ok mglocker@
 
> 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 22:59:45 -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 && !sc->sc_vs_cur->bulk_endpoint)
> +			goto next;
>  
>  		/* save endpoint with requested bandwidth */
>  		psize = UGETW(ed->wMaxPacketSize);
> 
> 
> -- 
> wbr, Kirill
>