| Message ID | 20220920-resend-hwtimestamp-v9-2-55a89f46f6be@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | uvcvideo: Fixes for hw timestamping | expand |
Hi Ricardo, Thank you for the patch. On Wed, Mar 15, 2023 at 02:30:13PM +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: "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. Because it would have been too easy otherwise of course :-) > > 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> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_video.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index 4ff4ab4471fe..1f416c494acc 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,23 @@ 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". > + * Most of the vendors, clear the `UVC_STREAM_SCR` bit when the data is > + * not valid, other vendors always set the `UVC_STREAM_SCR` bit and > + * expect that the driver only samples the stc if there is data on the > + * packet. > + * Ignore all the hardware timestamp information if there is no data > + * and stc and sof are zero. > + */ I'd like to expand this a bit (partly to make sure I understand the issue correctly): /* * 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) * 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 and the clock recovery algorithm wouldn't suffer from * this condition. */ Is this correct (and fine with you) ? If so, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + 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 +582,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; >
Hi Laurent Hi, I added some minor modifications, hope that it is fine with you. Thanks!! On Fri, 22 Mar 2024 at 00:26, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Wed, Mar 15, 2023 at 02:30:13PM +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: > > "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. > > Because it would have been too easy otherwise of course :-) > > > > > 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> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_video.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index 4ff4ab4471fe..1f416c494acc 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,23 @@ 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". > > + * Most of the vendors, clear the `UVC_STREAM_SCR` bit when the data is > > + * not valid, other vendors always set the `UVC_STREAM_SCR` bit and > > + * expect that the driver only samples the stc if there is data on the > > + * packet. > > + * Ignore all the hardware timestamp information if there is no data > > + * and stc and sof are zero. > > + */ > > I'd like to expand this a bit (partly to make sure I understand the > issue correctly): > > /* > * 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) 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 and the clock recovery algorithm wouldn't suffer from one clock sample from many > * this condition. > */ > > Is this correct (and fine with you) ? If so, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + 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 +582,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; > > > > -- > Regards, > > Laurent Pinchart
On Fri, Mar 22, 2024 at 09:22:39AM +0100, Ricardo Ribalda wrote: > Hi Laurent > > Hi, I added some minor modifications, hope that it is fine with you. They look fine to me, thanks. > On Fri, 22 Mar 2024 at 00:26, Laurent Pinchart wrote: > > On Wed, Mar 15, 2023 at 02:30:13PM +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: > > > > "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. > > > > Because it would have been too easy otherwise of course :-) > > > > > > > > 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> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > drivers/media/usb/uvc/uvc_video.c | 20 +++++++++++++++++++- > > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > > index 4ff4ab4471fe..1f416c494acc 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,23 @@ 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". > > > + * Most of the vendors, clear the `UVC_STREAM_SCR` bit when the data is > > > + * not valid, other vendors always set the `UVC_STREAM_SCR` bit and > > > + * expect that the driver only samples the stc if there is data on the > > > + * packet. > > > + * Ignore all the hardware timestamp information if there is no data > > > + * and stc and sof are zero. > > > + */ > > > > I'd like to expand this a bit (partly to make sure I understand the > > issue correctly): > > > > /* > > * 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) > 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 and the clock recovery algorithm wouldn't suffer from > one clock sample from many > > * this condition. > > */ > > > > Is this correct (and fine with you) ? If so, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > + 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 +582,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; > > >
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 4ff4ab4471fe..1f416c494acc 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,23 @@ 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". + * Most of the vendors, clear the `UVC_STREAM_SCR` bit when the data is + * not valid, other vendors always set the `UVC_STREAM_SCR` bit and + * expect that the driver only samples the stc if there is data on the + * packet. + * Ignore all the hardware timestamp information if there is no data + * and stc and sof are zero. + */ + 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 +582,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." 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. 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> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_video.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)