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@xxxxxxxxxxxxx>
> > > Reviewed-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
> > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxx>
> >
> > > + 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
