| Message ID | 20220920-resend-hwtimestamp-v3-0-db9faee7f47d@chromium.org |
|---|---|
| Headers | show |
| Series | uvcvideo: Fixes for hw timestamping | expand |
Hi Ricardo, Thank you for the patch. On Wed, Jan 04, 2023 at 11:45:21AM +0100, Ricardo Ribalda wrote: > Some SunplusIT cameras took a borderline interpretation of the UVC 1.5 > standard, and fill the PTS and SCR fields with invalid data if the > package does not contain data. > > "STC must be captured when the first video data of a video frame is put > on the USB bus." > > Eg: > > buffer: 0xa7755c00 len 000012 header:0x8c stc 00000000 sof 0000 pts 00000000 > buffer: 0xa7755c00 len 000012 header:0x8c stc 00000000 sof 0000 pts 00000000 > buffer: 0xa7755c00 len 000668 header:0x8c stc 73779dba sof 070c pts 7376d37a > > This borderline/buggy interpretation has been implemented in a variety > of devices, from directly SunplusIT and from other OEMs that rebrand > SunplusIT products. > > Luckily we can identify the affected modules by looking at the guid of > one of the extension units: > > VideoControl Interface Descriptor: > guidExtensionCode {82066163-7050-ab49-b8cc-b3855e8d221d} > > This patch adds a new quirk to take care of this. > > lsusb of one of the affected cameras: > > Bus 001 Device 003: ID 1bcf:2a01 Sunplus Innovation Technology Inc. > Device Descriptor: > bLength 18 > bDescriptorType 1 > bcdUSB 2.01 > bDeviceClass 239 Miscellaneous Device > bDeviceSubClass 2 ? > bDeviceProtocol 1 Interface Association > bMaxPacketSize0 64 > idVendor 0x1bcf Sunplus Innovation Technology Inc. > idProduct 0x2a01 > bcdDevice 0.02 > iManufacturer 1 SunplusIT Inc > iProduct 2 HanChen Wise Camera > iSerial 3 01.00.00 > bNumConfigurations 1 > > Tested-by: HungNien Chen <hn.chen@sunplusit.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++ > drivers/media/usb/uvc/uvc_video.c | 10 ++++++++++ > drivers/media/usb/uvc/uvcvideo.h | 1 + > 3 files changed, 22 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 5448576a8413..c5ab6e2d24c3 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1199,6 +1199,17 @@ static const struct uvc_entity_quirk { > u8 guid[16]; > u32 quirks; > } uvc_entity_quirk[] = { > + /* > + * Some SunPlusIT uvc 1.5 device firmware expects that packages with > + * no frame data are ignored by the host. Therefore it does not clear > + * the PTS/SCR bits in the header, and breaks the timestamp decode > + * algorithm. > + */ > + { > + .guid = {0x82, 0x06, 0x61, 0x63, 0x70, 0x50, 0xab, 0x49, > + 0xb8, 0xcc, 0xb3, 0x85, 0x5e, 0x8d, 0x22, 0x1d}, > + .quirks = UVC_QUIRK_IGNORE_EMPTY_TS, > + }, > }; > > static void uvc_entity_quirks(struct uvc_device *dev) > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index def079c7a6fd..f469c62bedca 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -500,6 +500,16 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf, > if (len < header_size) > return; > > + /* > + * Some devices make a borderline interpreation of the UVC 1.5 standard s/interpreation/interpretation/ > + * and the packets with no data contain undefined timestamps. Ignore "and send packets with no data ..." ? > + * such packages to avoid interfering with the clock interpolation s/packages/packets/ > + * algorithm. > + */ > + if (stream->dev->quirks & UVC_QUIRK_IGNORE_EMPTY_TS && > + len == header_size) > + return; I've sent a reply to v2 which I'll copy here: Given that there may be no guarantee that the above GUID won't be used by devices that don't exhibit this problem, I'm wondering if we couldn't use a heuristics instead of a quirk. I'm thinking about something along the lines of if (buf->bytesused == 0 && len == header_size && has_scr && stc == 0 && sof == 0) This will catch suspicious SCR values (stc == 0 && sof == 0) on empty packets (len == header_size) sent before any data packet (buf->bytesused == 0), which should handle the devices you have to support, and avoid false positives (the stc == 0 && sof == 0 check is already quite restrictive, adding buf->bytesused == 0 would limit the workaround to packets before the first data packet). With this we could drop patch 2/8. > + > /* > * Extract the timestamps: > * > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index df93db259312..c3424ae29339 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -74,6 +74,7 @@ > #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT 0x00000400 > #define UVC_QUIRK_FORCE_Y8 0x00000800 > #define UVC_QUIRK_FORCE_BPP 0x00001000 > +#define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000 > > /* Format flags */ > #define UVC_FMT_FLAG_COMPRESSED 0x00000001