| Message ID | 20240323-resend-hwtimestamp-v10-2-b08e590d97c7@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uvcvideo: Fixes for hw timestamping | expand |
On Sat, Mar 23, 2024 at 10:48:03AM +0000, 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." > > Some SunplusIT devices send, e.g., > > 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 > > While the UVC specification meant that the first two packets shouldn't > have had the SCR bit set in the header. > > This borderline/buggy interpretation has been implemented in a variety > of devices, from directly SunplusIT and from other OEMs that rebrand > SunplusIT products. So quirking based on VID:PID will be problematic. > > All the affected modules have the following extension unit: > VideoControl Interface Descriptor: > guidExtensionCode {82066163-7050-ab49-b8cc-b3855e8d221d} > > But the vendor plans to use that GUID in the future and fix the bug, > this means that we should use heuristic to figure out the broken > packets. > > This patch takes 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> > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_video.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > Reviewed-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index 659c9e9880a99..b2e70fcf4eb4c 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -478,6 +478,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf, > ktime_t time; > u16 host_sof; > u16 dev_sof; > + u32 dev_stc; > > switch (data[1] & (UVC_STREAM_PTS | UVC_STREAM_SCR)) { > case UVC_STREAM_PTS | UVC_STREAM_SCR: > @@ -526,6 +527,34 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf, > if (dev_sof == stream->clock.last_sof) > return; > > + dev_stc = get_unaligned_le32(&data[header_size - 6]); > + > + /* > + * STC (Source Time Clock) is the clock used by the camera. The UVC 1.5 > + * standard states that it "must be captured when the first video data > + * of a video frame is put on the USB bus". This is generally understood > + * as requiring devices to clear the payload header's SCR bit before > + * the first packet containing video data. > + * > + * Most vendors follow that interpretation, but some (namely SunplusIT > + * on some devices) always set the `UVC_STREAM_SCR` bit, fill the SCR > + * field with 0's,and expect that the driver only processes the SCR if > + * there is data in the packet. > + * > + * Ignore all the hardware timestamp information if we haven't received > + * any data for this frame yet, the packet contains no data, and both > + * STC and SOF are zero. This heuristics should be safe on compliant > + * devices. This should be safe with compliant devices, as in the very > + * unlikely case where a UVC 1.1 device would send timing information > + * only before the first packet containing data, and both STC and SOF > + * happen to be zero for a particular frame, we would only miss one > + * clock sample from many and the clock recovery algorithm wouldn't > + * suffer from this condition. > + */ > + if (buf && buf->bytesused == 0 && len == header_size && > + dev_stc == 0 && dev_sof == 0) > + return; > + > stream->clock.last_sof = dev_sof; > > host_sof = usb_get_current_frame_number(stream->dev->udev); > @@ -564,7 +593,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf, > spin_lock_irqsave(&stream->clock.lock, flags); > > sample = &stream->clock.samples[stream->clock.head]; > - sample->dev_stc = get_unaligned_le32(&data[header_size - 6]); > + sample->dev_stc = dev_stc; > sample->dev_sof = dev_sof; > sample->host_sof = host_sof; > sample->host_time = time; > > -- > 2.44.0.396.g6e790dbe36-goog > >
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 659c9e9880a99..b2e70fcf4eb4c 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -478,6 +478,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf, ktime_t time; u16 host_sof; u16 dev_sof; + u32 dev_stc; switch (data[1] & (UVC_STREAM_PTS | UVC_STREAM_SCR)) { case UVC_STREAM_PTS | UVC_STREAM_SCR: @@ -526,6 +527,34 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf, if (dev_sof == stream->clock.last_sof) return; + dev_stc = get_unaligned_le32(&data[header_size - 6]); + + /* + * STC (Source Time Clock) is the clock used by the camera. The UVC 1.5 + * standard states that it "must be captured when the first video data + * of a video frame is put on the USB bus". This is generally understood + * as requiring devices to clear the payload header's SCR bit before + * the first packet containing video data. + * + * Most vendors follow that interpretation, but some (namely SunplusIT + * on some devices) always set the `UVC_STREAM_SCR` bit, fill the SCR + * field with 0's,and expect that the driver only processes the SCR if + * there is data in the packet. + * + * Ignore all the hardware timestamp information if we haven't received + * any data for this frame yet, the packet contains no data, and both + * STC and SOF are zero. This heuristics should be safe on compliant + * devices. This should be safe with compliant devices, as in the very + * unlikely case where a UVC 1.1 device would send timing information + * only before the first packet containing data, and both STC and SOF + * happen to be zero for a particular frame, we would only miss one + * clock sample from many and the clock recovery algorithm wouldn't + * suffer from this condition. + */ + if (buf && buf->bytesused == 0 && len == header_size && + dev_stc == 0 && dev_sof == 0) + return; + stream->clock.last_sof = dev_sof; host_sof = usb_get_current_frame_number(stream->dev->udev); @@ -564,7 +593,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf, spin_lock_irqsave(&stream->clock.lock, flags); sample = &stream->clock.samples[stream->clock.head]; - sample->dev_stc = get_unaligned_le32(&data[header_size - 6]); + sample->dev_stc = dev_stc; sample->dev_sof = dev_sof; sample->host_sof = host_sof; sample->host_time = time;
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." Some SunplusIT devices send, e.g., 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 While the UVC specification meant that the first two packets shouldn't have had the SCR bit set in the header. This borderline/buggy interpretation has been implemented in a variety of devices, from directly SunplusIT and from other OEMs that rebrand SunplusIT products. So quirking based on VID:PID will be problematic. All the affected modules have the following extension unit: VideoControl Interface Descriptor: guidExtensionCode {82066163-7050-ab49-b8cc-b3855e8d221d} But the vendor plans to use that GUID in the future and fix the bug, this means that we should use heuristic to figure out the broken packets. This patch takes 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> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_video.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)