Hi Oleksij, On Monday 17 February 2014 10:40:35 Oleksij Rempel wrote: > Am 17.02.2014 10:28, schrieb Laurent Pinchart: > > On Monday 17 February 2014 10:05:41 Oleksij Rempel wrote: > >> Am 17.02.2014 01:26, schrieb Laurent Pinchart: > >>> On Sunday 16 February 2014 10:59:32 Oleksij Rempel wrote: [snip] > >>> I don't know whether Microsoft has decided to address this issue in > >>> their own way by sending a CLEAR_FEATURE(HALT) request when stopping the > >>> video stream, or if that's a standard procedure in their UVC driver that > >>> hasn't been added specifically for bulk endpoints. Neither the UVC > >>> specification nor the USB specification forbid sending a > >>> CLEAR_FEATURE(HALT) request to the streaming interface bulk endpoint in > >>> this case. > >>> > >>> It should be noted that the USB specification allows devices to stall a > >>> SET_INTERFACE(0) request for an interface that has a single alternate > >>> setting (section 9.4.10). In that case the Linux USB core sends a > >>> CLEAR_FEATURE(HALT) request to every endpoint of that interface. > >> > >> I tested this webcam with Windows Xp and 8. It was handled in same way: > >> only CLEAR_FEATURE(HALT) was send. > >> > >>>> Logitech C905: (UVC - 1.00; ISO) > >>>> • SET INTERFACE (Interface=1) > >>>> • CLEAR HALT (Interface=0x7) Halt interrupt EP > >>> > >>> The CLEAR_FEATURE(HALT) request is targeted at endpoints, not > >>> interfaces. > >>> > >>> I suppose you mean endpoint 0x87 here (EP 7 IN) ? > >> > >> yes > >> > >>> Similarly, with your bulk > >>> webcam, to which endpoint is the CLEAR_FEATURE(HALT) request sent ? > >> > >> EP 1 IN (0x81) > >> > >>>> Logitech C600: (UVC - 1.00; ISO) > >>>> • SET INTERFACE (Interface=1) > >>>> • CLEAR HALT (Interface=0x7) Halt interrupt EP > >>>> > >>>> Logitech E3500: (UVC - 1.00; ISO) > >>>> • CLEAR HALT (Interface=0x7) Halt interrupt EP > >>>> • SET INTERFACE (Interface=1) (different order then in c600 and c905 ) > >>>> > >>>> Logitech C920: (UVC 1.00; ISO) > >>>> • SET INTERFACE (Interface=1) > >>>> • Interrupt is EP 3, why it is not halted? > >>>> > >>>> Asus UX31A (UVC - 1.00; ISO) - 04f2:b330 Chicony Electronics Co., Ltd > >>>> Asus 720p CMOS webcam > >>>> • SET INTERFACE (Interface=1) > >>>> • Interrupt is EP 3, why it is not halted? > >>>> > >>>> So far it looks like Windows8 use CLEAR HALT for Bulk and SET INTERFACE > >>>> for Iso. > >>> > >>> What about Windows XP and Windows 7 ? Do they use the same scheme ? > >> > >> Windows XP uses only SET INTERFACE on this webcam. Windows 7 not tested > >> for now - should i? > > > > If Windows XP and Windows 8 behave the same way I suppose we can skip > > Windows 7. > > > >>>> But i don't understand why Windows8 trying to halt Interrupt if EP=7 > >>>> and not doing that if EP=3? > >>> > >>> No idea... I wonder if the Windows UVC driver includes device quirks. > >> > >> Yes, Windows 8 knows more about this webcam's then i would like to. For > >> example it uses intensively Logitech specific XUs (without installing > >> Logitech software): Right Light registers; some think what looks like > >> watch dog. May be it knows how to disable LED :) NSA friendly system... > > > > Nice... If you've reversed-engineered RightLight information would be > > welcome :-) > > That was easy: > https://wikidevi.com/wiki/82066163-7050-ab49-b8cc-b3855e8d2252 > See attachment with a testprogram. That's really interesting. Do you think we could integrate that in libv4l ? > >>>> diff --git a/drivers/media/usb/uvc/uvc_video.c > >>>> b/drivers/media/usb/uvc/uvc_video.c index 898c208..9d335c0 100644 > >>>> --- a/drivers/media/usb/uvc/uvc_video.c > >>>> +++ b/drivers/media/usb/uvc/uvc_video.c > >>>> @@ -1847,7 +1847,15 @@ int uvc_video_enable(struct uvc_streaming > >>>> *stream, int enable) > >>>> > >>>> if (!enable) { > >>>> uvc_uninit_video(stream, 1); > >>>> - usb_set_interface(stream->dev->udev, stream->intfnum, 0); > >>>> + if (stream->intf->num_altsetting > 1) { > >>>> + usb_set_interface(stream->dev->udev, > >>>> + stream->intfnum, 0); > >>>> + } else { > >>>> + usb_clear_halt(stream->dev->udev, > >>>> + usb_rcvctrlpipe(stream->dev->udev, > >>>> + stream->intfnum)); > >>> > >>> usb_rcvctrlpipe() expects an endpoint number as its second argument, not > >>> an interface number. The following call should do. > >>> > >>> usb_clear_halt(stream->dev->udev, > >>> stream->header.bEndpointAddress); > >>> > >>> We probably need to validate the streaming header bEndpointAddress field > >>> when probing the device to ensure that we won't call usb_clear_halt() on > >>> an unrelated endpoint. > >>> > >>>> + } > >>>> + > >>> > >>> This might cause a regression with other bulk devices that expect a > >>> SET_INTERFACE(0), or don't expect a CLEAR_FEATURE(HALT), although I > >>> suppose those devices wouldn't work with Windows then. I've tested you > >>> patch (with the above modification) with the only bulk device I own and > >>> it doesn't seem to cause any regression. > >> > >> I assume, my cam was produced after XP uvc driver. So it should be > >> default behaviour. > > > > What would you think about calling both usb_set_interface() and > > usb_clear_halt() for bulk-based devices ? Something like > > > > - usb_set_interface(stream->dev->udev, stream->intfnum, 0); > > + if (stream->intf->num_altsetting == 1) > > + usb_clear_halt(stream->dev->udev, > > + stream->header.bEndpointAddress); > > It works too. I was just afraid to be too different form MS implementation. > Manufactures usually test it against MS driver so it sort of dictate > specification. I think I agree with you. Could you please test the following patch ? From 04f3b43ba80ef5f3fbc26f81b8309dc166226b75 Mon Sep 17 00:00:00 2001 From: Oleksij Rempel <linux@xxxxxxxxxxxxxxxx> Date: Sun, 16 Feb 2014 10:59:32 +0100 Subject: [PATCH] uvcvideo: Do not use usb_set_interface on bulk EP The UVC specification uses alternate setting selection to notify devices of stream start/stop. This breaks when using bulk-based devices, as the video streaming interface has a single alternate setting in that case, making video stream start and video stream stop events to appear identical to the device. Bulk-based devices are thus not well supported by UVC. The webcam built in the Asus Zenbook UX302LA ignores the set interface request and will keep the video stream enabled when the driver tries to stop it. If USB autosuspend is enabled the device will then be suspended and will crash, requiring a cold reboot. USB trace capture showed that Windows sends a CLEAR_FEATURE(HALT) request to the bulk endpoint when stopping the stream instead of selecting alternate setting 0. The camera then behaves correctly, and thus seems to require that behaviour. Replace selection of alternate setting 0 with clearing of the endpoint halt feature at video stream stop for bulk-based devices. Let's refrain from blaming Microsoft this time, as it's not clear whether this Windows-specific but USB-compliant behaviour was specifically developed to handle bulkd-based UVC devices, or if the camera just took advantage of it. CC: stable@xxxxxxxxxxxxxxx Signed-off-by: Oleksij Rempel <linux@xxxxxxxxxxxxxxxx> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> --- drivers/media/usb/uvc/uvc_video.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 103cd4e..29147fd 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1850,7 +1850,19 @@ int uvc_video_enable(struct uvc_streaming *stream, int enable) if (!enable) { uvc_uninit_video(stream, 1); - usb_set_interface(stream->dev->udev, stream->intfnum, 0); + if (stream->intf->num_altsetting > 1) { + usb_set_interface(stream->dev->udev, + stream->intfnum, 0); + } else { + /* 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 + * bulk endpoint, mimic the same behaviour. + */ + usb_clear_halt(stream->dev->udev, + stream->header.bEndpointAddress); + } + uvc_queue_enable(&stream->queue, 0); uvc_video_clock_cleanup(stream); return 0; -- 1.8.3.2 -- Regards, Laurent Pinchart
Attachment:
signature.asc
Description: This is a digitally signed message part.