On Wed, Nov 27, 2024 at 08:12:51PM GMT, Kirill A. Korinsky wrote:

> On Wed, 27 Nov 2024 19:09:47 +0100,
> Stuart Henderson <stu@spacehopper.org> wrote:
> > 
> > On 2024/11/27 17:54, Kirill A. Korinsky wrote:
> > >  
> > > +	/* do device negotiation with commit after set alternate interface */
> > 
> > would "renegotiate after setting alternate interface" make sense?
> >
> 
> Thanks, I think renegotiate is better.
> 
> > > +	error = uvideo_vs_negotiation(sc, 1);
> > > +	if (error != USBD_NORMAL_COMPLETION)
> > > +		printf("%s: could not negotiate after setting alternate interface!\n",
> > > +		    DEVNAME(sc));
> > > +
> > >  	DPRINTF(1, "%s: open pipe for bEndpointAddress=0x%02x\n",
> > >  	    DEVNAME(sc), sc->sc_vs_cur->endpoint);
> > 
> > please stick to <80 columns (either shorter text, or split the
> > string across two lines)
> >
> > should that return (error) too?
> > 
> 
> This is a point that I'm thinking a lot. From one hand it makes perfect
> sense to return error here, from another hand it may break some webcam which
> doesn't support it, but may continue to work, or may not.

I was testing your diff with two cams.  One still did work, the other
one did break.  Although the additional negotiate didn't return an
error, the cam wasn't able to stream anymore:

	video: ioctl VIDIOC_DQBUF: Invalid argument

Therefore, can you please add an quirk around the new re-negotiation
code for your cam which requires this?
 
> Thus, Linux uses 0 as a hardcoded alternate interface, has comments that
> soem device crashes without it, and returns error:
> https://github.com/torvalds/linux/blob/v6.12/drivers/media/usb/uvc/uvc_video.c#L2180-L2220
> 
> Base on all this return error is right move, I guess.
> 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.223 uvideo.c
> --- sys/dev/usb/uvideo.c	27 Nov 2024 11:37:23 -0000	1.223
> +++ sys/dev/usb/uvideo.c	27 Nov 2024 18:45:42 -0000
> @@ -1899,6 +1899,14 @@ uvideo_vs_open(struct uvideo_softc *sc)
>  		return (USBD_INVAL);
>  	}
>  
> +	/* renegotiate with commit after setting alternate interface */
> +	error = uvideo_vs_negotiation(sc, 1);
> +	if (error != USBD_NORMAL_COMPLETION) {
> +		printf("%s: could not renegotiate after setting "
> +		    "alternate interface!\n", DEVNAME(sc));
> +		return (error);
> +	}
> +
>  	DPRINTF(1, "%s: open pipe for bEndpointAddress=0x%02x\n",
>  	    DEVNAME(sc), sc->sc_vs_cur->endpoint);
>  	error = usbd_open_pipe(
> 
> 
> -- 
> wbr, Kirill