Re: [PATCH resend] [media] uvcvideo: zero seq number when disabling stream

[Laurent Pinchart]

Hi Hans,

On Thursday, 19 October 2017 10:23:06 EEST Hans Yang wrote:
> On 2017å10æ18æ 16:52, Guennadi Liakhovetski wrote:
> > On Mon, 16 Oct 2017, Laurent Pinchart wrote:
> >> 
> >> On Friday, 15 September 2017 09:27:51 EEST Hans Yang wrote:
> >>> For bulk-based devices, when disabling the video stream,
> >>> in addition to issue CLEAR_FEATURE(HALT), it is better to set
> >>> alternate setting 0 as well or the sequnce number in host
> >>> side will probably not reset to zero.
> >> 
> >> The USB 2.0 specificatin states in the description of the SET_INTERFACE
> >> request that "If a device only supports a default setting for the
> >> specified interface, then a STALL may be returned in the Status stage of
> >> the request".
> >> 
> >> The Linux implementation of usb_set_interface() deals with this by
> >> handling STALL conditions and manually clearing HALT for all endpoints in
> >> the interface, but I'm still concerned that this change could break
> >> existing bulk- based cameras. Do you know what other operating systems do
> >> when disabling the stream on bulk cameras ? According to a comment in the
> >> driver Windows calls CLEAR_FEATURE(HALT), but the situation might have
> >> changed since that was tested.
> 
> Sorry I did not mention that it is about SS bulk-based cameras using
> sequence number technique.
>  From my observations, invoking usb_clear_halt() will reset the endpoint
> in the device side via CLEAR_FEATURE(HALT)
> and reset the sequence number as well; however usb_reset_endpoint() does
> not reset the host side endpoint with xhci driver,
> then the sequence number will mismatch in next time stream enable.
> I can always observe this through a bus analyzer on Linux implementation
> whatever X86 or ARM based.
> This is not observed on the Windows.

According to the USB 3.0 specification, section 9.4.5 ("Get Status"),

"Regardless of whether an endpoint has the Halt feature set, a 
ClearFeature(ENDPOINT_HALT) request always results in the data sequence being 
reinitialized to zero, and if Streams are enabled, the Stream State Machine 
shall be reinitialized to the Disabled state."

If Linux doesn't reset the sequence number when issuing a CLEAR_FEATURE(HALT), 
isn't it a Linux USB stack bug that should be fixed in the USB core ?

> >> Guennadi, how do your bulk-based cameras handle this ?
> > 
> > I don't know what design decisions are implemented there, but I tested a
> > couple of cameras for sending a STREAMOFF after a few captured buffers,
> > sleeping for a couple of seconds, requeuing buffers, sending STREAMON and
> > capturing a few more - that seems to have worked. "Seems" because I only
> > used a modified version of capture-example, I haven't checked buffer
> > contents.
> > 
> > BTW, Hans, may I ask - what cameras did you use?
> 
> I have three SS bulk-based cameras as follows:
> e-con Systems See3CAM_CU130 (2560:c1d0)
> Leopard ZED (2b03:f580)
> Intel(R) RealSense(TM) Camera SR300 (8086:0aa5)
> 
> I can observe the same issue on all above;
> besides, to reproduce issue do not let the camera enter suspend because
> it will *help* to reset the sequence number through usb_set_interface(0).
> 
> >>> Then in next time video stream start, the device will expect
> >>> host starts packet from 0 sequence number but host actually
> >>> continue the sequence number from last transaction and this
> >>> causes transaction errors.
> >> 
> >> Do you mean the DATA0/DATA1 toggle ? Why does the host continue toggling
> >> the PID from the last transation ? The usb_clear_halt() function calls
> >> usb_reset_endpoint() which should reset the DATA PID toggle.
> >> 
> >>> This commit fixes this by adding set alternate setting 0 back
> >>> as what isoch-based devices do.
> >>> 
> >>> Below error message will also be eliminated for some devices:
> >>> uvcvideo: Non-zero status (-71) in video completion handler.
> >>> 
> >>> Signed-off-by: Hans Yang <hansy@xxxxxxxxxx>
> >>> ---
> >>> 
> >>>   drivers/media/usb/uvc/uvc_video.c | 5 ++---
> >>>   1 file changed, 2 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/drivers/media/usb/uvc/uvc_video.c
> >>> b/drivers/media/usb/uvc/uvc_video.c index fb86d6af398d..ad80c2a6da6a
> >>> 100644
> >>> --- a/drivers/media/usb/uvc/uvc_video.c
> >>> +++ b/drivers/media/usb/uvc/uvc_video.c
> >>> @@ -1862,10 +1862,9 @@ int uvc_video_enable(struct uvc_streaming
> >>> *stream, int enable)
> >>> 
> >>>   	if (!enable) {
> >>>   		uvc_uninit_video(stream, 1);
> >>> -		if (stream->intf->num_altsetting > 1) {
> >>> -			usb_set_interface(stream->dev->udev,
> >>> +		usb_set_interface(stream->dev->udev,
> >>>   					  stream->intfnum, 0);
> >>> -		} else {
> >>> +		if (stream->intf->num_altsetting == 1) {
> >>>   			/* UVC doesn't specify how to inform a bulk-based device
> >>>   			 * when the video stream is stopped. Windows sends a
> >>>   			 * CLEAR_FEATURE(HALT) request to the video streaming

-- 
Regards,

Laurent Pinchart