Hi Oleksij, On Thursday 20 February 2014 08:25:31 Oleksij Rempel wrote: > Am 20.02.2014 01:36, schrieb Laurent Pinchart: > > 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 > > ? > > May be, we should retest if we really can get better exposure. What was > your experience with it? None :-) I've never tested it. > I'll collect more information about other XUs used by win driver. Right > now i have fallowing information: > - on stream start and stream end windows set LED in automatic mode. > - clear_halt is probably workaround for some bug which i get if i stop > the stream from virtual machine. > > >>> 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 ? > > It is not working. Your patch is setting Interface to 0x80, but it > should set it to 0x81. OK, next try :-) Please see below. > > 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 > > [snip] > > > 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); Could you please replace that with unsigned int epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; unsigned int dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK; unsigned int pipe; pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir; usb_clear_halt(stream->dev->udev, pipe); and retest ? (As a side note, usb_clear_halt() should really take an endpoint address as argument, not a pipe, the code would become much simpler...) > > + } > > + > > uvc_queue_enable(&stream->queue, 0); > > uvc_video_clock_cleanup(stream); > > return 0; -- Regards, Laurent Pinchart
Attachment:
signature.asc
Description: This is a digitally signed message part.