From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Ricardo Ribalda <ribalda@chromium.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
Sergey Senozhatsky <senozhatsky@chromium.org>,
linux-kernel@vger.kernel.org, "hn.chen" <hn.chen@sunplusit.com>,
linux-media@vger.kernel.org
Subject: Re: [PATCH v9 2/6] media: uvcvideo: Ignore empty TS packets
Date: Fri, 22 Mar 2024 01:26:02 +0200 [thread overview]
Message-ID: <20240321232602.GB20938@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20220920-resend-hwtimestamp-v9-2-55a89f46f6be@chromium.org>
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;
>
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2024-03-21 23:26 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-15 13:30 [PATCH v9 0/6] uvcvideo: Fixes for hw timestamping Ricardo Ribalda
2023-03-15 13:30 ` [PATCH v9 1/6] media: uvcvideo: Fix negative modulus calculation Ricardo Ribalda
2023-03-16 3:05 ` Sergey Senozhatsky
2024-02-12 22:59 ` Laurent Pinchart
2024-02-19 10:28 ` Ricardo Ribalda
2024-02-19 10:40 ` Laurent Pinchart
2024-02-19 15:07 ` Ricardo Ribalda
2024-03-21 21:50 ` Laurent Pinchart
2024-03-22 9:19 ` Laurent Pinchart
2024-03-22 9:29 ` Ricardo Ribalda
2024-03-22 9:35 ` Laurent Pinchart
2023-03-15 13:30 ` [PATCH v9 2/6] media: uvcvideo: Ignore empty TS packets Ricardo Ribalda
2024-03-21 23:26 ` Laurent Pinchart [this message]
2024-03-22 8:22 ` Ricardo Ribalda
2024-03-22 8:52 ` Laurent Pinchart
2023-03-15 13:30 ` [PATCH v9 3/6] media: uvcvideo: Quirk for invalid dev_sof in Logitech C922 Ricardo Ribalda
2024-03-21 23:44 ` Laurent Pinchart
2024-03-22 8:32 ` Ricardo Ribalda
2024-03-22 8:52 ` Laurent Pinchart
2023-03-15 13:30 ` [PATCH v9 4/6] media: uvcvideo: Allow hw clock updates with buffers not full Ricardo Ribalda
2023-03-16 3:08 ` Sergey Senozhatsky
2024-03-22 0:12 ` Laurent Pinchart
2023-03-15 13:30 ` [PATCH v9 5/6] media: uvcvideo: Refactor clock circular buffer Ricardo Ribalda
2024-03-22 0:36 ` Laurent Pinchart
2023-03-15 13:30 ` [PATCH v9 6/6] media: uvcvideo: Fix hw timestamp handling for slow FPS Ricardo Ribalda
2023-03-16 3:07 ` Sergey Senozhatsky
2024-03-22 9:35 ` Laurent Pinchart
2024-03-22 9:53 ` Ricardo Ribalda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240321232602.GB20938@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=hn.chen@sunplusit.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=ribalda@chromium.org \
--cc=senozhatsky@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).