Received-SPF: pass (google.com: domain of linux-kernel+bounces-193502-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 29 May 2024 15:19:30 +0900
From: Tomasz Figa <tfiga@chromium.org>
To: Ricardo Ribalda <ribalda@chromium.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, 
	Mauro Carvalho Chehab <mchehab@kernel.org>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, 
	"hn.chen" <hn.chen@sunplusit.com>, Hans Verkuil <hverkuil@xs4all.nl>, 
	Sergey Senozhatsky <senozhatsky@chromium.org>
Subject: Re: [PATCH v10 2/6] media: uvcvideo: Ignore empty TS packets
Message-ID: <fikctxwprluipwcf2ijewvyosyzzbu5n5pw2wjq6rfacwbwzkt@pi72dfa3v6zj>
References: <20240323-resend-hwtimestamp-v10-0-b08e590d97c7@chromium.org>
 <20240323-resend-hwtimestamp-v10-2-b08e590d97c7@chromium.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240323-resend-hwtimestamp-v10-2-b08e590d97c7@chromium.org>

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
> 
>