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

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

[Hans Yang]

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.

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.

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@nvidia.com>
---
 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
-- 
2.1.4

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

[Laurent Pinchart]

Hi Hans,

(CC'ing Guennadi Liakhovetski)

Thank you for the patch.

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.

Guennadi, how do your bulk-based cameras handle this ?

> 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@nvidia.com>
> ---
>  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

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

[Guennadi Liakhovetski]

Hi Laurent,

On Mon, 16 Oct 2017, Laurent Pinchart wrote:

> Hi Hans,
> 
> (CC'ing Guennadi Liakhovetski)
> 
> Thank you for the patch.
> 
> 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.
> 
> 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?

Thanks
Guennadi

> > 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@nvidia.com>
> > ---
> >  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
> 

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

[Hans Yang]

Hi Laurent and Guennadi,

On 2017年10月18日 16:52, Guennadi Liakhovetski wrote:
> Hi Laurent,
> 
> On Mon, 16 Oct 2017, Laurent Pinchart wrote:
> 
>> Hi Hans,
>>
>> (CC'ing Guennadi Liakhovetski)
>>
>> Thank you for the patch.
>>
>> 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.

>>
>> 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).

> 
> Thanks
> Guennadi
> 
>>> 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@nvidia.com>
>>> ---
>>>   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
>>

Regards,
Hans Yang
--nvpublic--

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@nvidia.com>
> >>> ---
> >>> 
> >>>   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

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

[Laurent Pinchart]

Hi Hans,

(CC'ing the linux-usb mailing list)

On Thursday, 19 October 2017 15:03:34 EEST Laurent Pinchart wrote:
> 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.

Do your USB analyser traces show whether Windows sends a SET_INTERFACE(0) 
request when stopping the stream for bulk-based devices ?

> 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 ?

Do you have any update on whether you would consider this to be a Linux USB 
stack issue or a uvcvideo driver issue ?

> >>> 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@nvidia.com>
> >>>> ---
> >>>> 
> >>>>   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