From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/uvideo: compute max_packet_size the same way in all cases
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org
Date: Sun, 23 Feb 2025 14:46:05 +0100

On Sat, Feb 15, 2025 at 03:32:58PM GMT, Kirill A. Korinsky wrote:

> tech@,
> 
> I'd like to extract code which computes packet size from usb web cam to
> a dedicated function which allows to compute it in the both cases the
> same way, which should improve selecting the endpoint with largest
> bandwish on some edge cases.

Make sense to me.  One little thing inline.
 
> Ok?
> 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.238 uvideo.c
> --- sys/dev/usb/uvideo.c	15 Feb 2025 09:05:15 -0000	1.238
> +++ sys/dev/usb/uvideo.c	15 Feb 2025 14:10:50 -0000
> @@ -114,6 +114,8 @@ void		uvideo_attach(struct device *, str
>  void		uvideo_attach_hook(struct device *);
>  int		uvideo_detach(struct device *, int);
> 
> +uint32_t	uvideo_vc_parse_max_packet_size(usb_endpoint_descriptor_t *);
> +

Can you please sort the function prototype to where you have inserted
the new function to?

>  usbd_status	uvideo_vc_parse_desc(struct uvideo_softc *);
>  usbd_status	uvideo_vc_parse_desc_header(struct uvideo_softc *,
>  		    const usb_descriptor_t *);
> @@ -1235,6 +1237,19 @@ uvideo_vs_parse_desc_frame_sub(struct uv
>  	return (USBD_NORMAL_COMPLETION);
>  }
> 
> +uint32_t
> +uvideo_vc_parse_max_packet_size(usb_endpoint_descriptor_t *ed)
> +{
> +	uint32_t psize;
> +
> +	/* XXX: get USB 3.0 speed from wBytesPerInterval */
> +
> +	psize = UGETW(ed->wMaxPacketSize);
> +	psize = UE_GET_SIZE(psize) * (1 + UE_GET_TRANS(psize));
> +
> +	return psize;
> +}
> +
>  usbd_status
>  uvideo_vs_parse_desc_alt(struct uvideo_softc *sc, int vs_nr, int iface, int numalts)
>  {
> @@ -1245,6 +1260,7 @@ uvideo_vs_parse_desc_alt(struct uvideo_s
>  	usb_endpoint_descriptor_t *ed;
>  	uint8_t ep_dir, ep_type;
>  	int bulk_endpoint;
> +	uint32_t psize;
> 
>  	vs = &sc->sc_vs_coll[vs_nr];
> 
> @@ -1306,13 +1322,14 @@ uvideo_vs_parse_desc_alt(struct uvideo_s
>  		if (bulk_endpoint && !vs->bulk_endpoint)
>  			goto next;
> 
> +		psize = uvideo_vc_parse_max_packet_size(ed);
>  		/* save endpoint with largest bandwidth */
> -		if (UGETW(ed->wMaxPacketSize) > vs->psize) {
> +		if (psize > vs->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->psize = psize;
>  			vs->iface = iface;
>  			vs->bulk_endpoint = bulk_endpoint;
>  		}
> @@ -1383,8 +1400,7 @@ uvideo_vs_set_alt(struct uvideo_softc *s
>  			goto next;
> 
>  		/* save endpoint with requested bandwidth */
> -		psize = UGETW(ed->wMaxPacketSize);
> -		psize = UE_GET_SIZE(psize) * (1 + UE_GET_TRANS(psize));
> +		psize = uvideo_vc_parse_max_packet_size(ed);
>  		if (psize >= max_packet_size)
>  			diff = psize - max_packet_size;
>  		else
>