From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/uvideo: clarify bulk endpoint stream closure
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org, laurie@tratt.net
Date: Sat, 15 Feb 2025 09:37:34 +0100

On Tue, Feb 11, 2025 at 09:13:08AM GMT, Kirill A. Korinsky wrote:

> tech@,
> 
> Right now, we're using the same method to close the video stream for
> both bulk and non-bulk endpoints. This approach switches back to the
> default (zero) interface and turns off the camera LED. While this logic
> is valid for non-bulk endpoints, bulk endpoints always use the default
> interface. As a result, the switch is likely ignored by the device or
> may trigger a reset.
> 
> Some devices can enter a broken state and won't function until they are
> reattached. One such example is the Elgato Game Capture HD60 S+. After
> the user presses 'q' in ffmpeg to stop the stream, the device becomes
> unusable until it is reattached. For embedded USB webcams, this issue
> could require a reboot to resolve.
> 
> UVC does not specify how to notify a bulk-based device when the video
> stream stops. Both Linux and Windows send a CLEAR_FEATURE(HALT) request
> to the video streaming bulk endpoint.
> 
> I've implemented the same logic in our codebase.
> 
> I've tested it on my bulk webcams, and no regressions were observed, and by
> Laurence (cc'ed) who has access to Elgato device and it fixed the issue.
> 
> Tests? Ok?

Seems reasonable to me.  No regression found with my cams.

ok mglocker@
 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.237 uvideo.c
> --- sys/dev/usb/uvideo.c	6 Feb 2025 13:15:50 -0000	1.237
> +++ sys/dev/usb/uvideo.c	8 Feb 2025 22:14:19 -0000
> @@ -2031,15 +2031,27 @@ uvideo_vs_close(struct uvideo_softc *sc)
>  		sc->sc_vs_cur->pipeh = NULL;
>  	}
>  
> -	/*
> -	 * Some devices need time to shutdown before we switch back to
> -	 * the default interface (0).  Not doing so can leave the device
> -	 * back in a undefined condition.
> -	 */
> -	usbd_delay_ms(sc->sc_udev, 100);
> +	if (sc->sc_vs_cur->bulk_endpoint) {
> +		/*
> +		 * UVC doesn't specify how to inform a bulk-based device
> +		 * when the video stream is stopped. Both, Linux and
> +		 * Windows send a CLEAR_FEATURE(HALT) request to the
> +		 * video streaming bulk endpoint.
> +		 */
> +		if (usbd_clear_endpoint_feature(sc->sc_udev,
> +		    sc->sc_vs_cur->endpoint, UF_ENDPOINT_HALT))
> +			printf("%s: clear endpoints failed!\n", __func__);
> +	} else {
> +		/*
> +		 * Some devices need time to shutdown before we switch back to
> +		 * the default interface (0).  Not doing so can leave the device
> +		 * back in a undefined condition.
> +		 */
> +		usbd_delay_ms(sc->sc_udev, 100);
>  
> -	/* switch back to default interface (turns off cam LED) */
> -	(void)usbd_set_interface(sc->sc_vs_cur->ifaceh, 0);
> +		/* switch back to default interface (turns off cam LED) */
> +		(void)usbd_set_interface(sc->sc_vs_cur->ifaceh, 0);
> +	}
>  }
>  
>  usbd_status
> 
> -- 
> wbr, Kirill
>