Re: [PATCH v3 3/8] media: uvc: Create UVC_QUIRK_IGNORE_EMPTY_TS quirk

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Ricardo Ribalda <ribalda@chromium.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-kernel@vger.kernel.org, "hn.chen" <hn.chen@sunplusit.com>,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v3 3/8] media: uvc: Create UVC_QUIRK_IGNORE_EMPTY_TS quirk
Date: Sat, 7 Jan 2023 03:20:48 +0200	[thread overview]
Message-ID: <Y7jI8Joa5fTc1yRT@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20220920-resend-hwtimestamp-v3-3-db9faee7f47d@chromium.org>

Hi Ricardo,

Thank you for the patch.

On Wed, Jan 04, 2023 at 11:45:21AM +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:
> 
> 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.
> 
> Luckily we can identify the affected modules by looking at the guid of
> one of the extension units:
> 
> VideoControl Interface Descriptor:
>   guidExtensionCode         {82066163-7050-ab49-b8cc-b3855e8d221d}
> 
> This patch adds a new quirk to take 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>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>  drivers/media/usb/uvc/uvc_video.c  | 10 ++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |  1 +
>  3 files changed, 22 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 5448576a8413..c5ab6e2d24c3 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1199,6 +1199,17 @@ static const struct uvc_entity_quirk {
>  	u8 guid[16];
>  	u32 quirks;
>  } uvc_entity_quirk[] = {
> +	/*
> +	 * Some SunPlusIT uvc 1.5 device firmware expects that packages with
> +	 * no frame data are ignored by the host. Therefore it does not clear
> +	 * the PTS/SCR bits in the header, and breaks the timestamp decode
> +	 * algorithm.
> +	 */
> +	{
> +		.guid = {0x82, 0x06, 0x61, 0x63, 0x70, 0x50, 0xab, 0x49,
> +			 0xb8, 0xcc, 0xb3, 0x85, 0x5e, 0x8d, 0x22, 0x1d},
> +		.quirks = UVC_QUIRK_IGNORE_EMPTY_TS,
> +	},
>  };
>  
>  static void uvc_entity_quirks(struct uvc_device *dev)
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index def079c7a6fd..f469c62bedca 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -500,6 +500,16 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
>  	if (len < header_size)
>  		return;
>  
> +	/*
> +	 * Some devices make a borderline interpreation of the UVC 1.5 standard

s/interpreation/interpretation/

> +	 * and the packets with no data contain undefined timestamps. Ignore

"and send packets with no data ..." ?

> +	 * such packages to avoid interfering with the clock interpolation

s/packages/packets/

> +	 * algorithm.
> +	 */
> +	if (stream->dev->quirks & UVC_QUIRK_IGNORE_EMPTY_TS &&
> +	    len == header_size)
> +		return;

I've sent a reply to v2 which I'll copy here:

Given that there may be no guarantee that the above GUID won't be used
by devices that don't exhibit this problem, I'm wondering if we couldn't
use a heuristics instead of a quirk. I'm thinking about something along
the lines of

        if (buf->bytesused == 0 && len == header_size && has_scr &&
            stc == 0 && sof == 0)

This will catch suspicious SCR values (stc == 0 && sof == 0) on empty
packets (len == header_size) sent before any data packet (buf->bytesused
== 0), which should handle the devices you have to support, and avoid
false positives (the stc == 0 && sof == 0 check is already quite
restrictive, adding buf->bytesused == 0 would limit the workaround to
packets before the first data packet).

With this we could drop patch 2/8.

> +
>  	/*
>  	 * Extract the timestamps:
>  	 *
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df93db259312..c3424ae29339 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -74,6 +74,7 @@
>  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
>  #define UVC_QUIRK_FORCE_Y8		0x00000800
>  #define UVC_QUIRK_FORCE_BPP		0x00001000
> +#define UVC_QUIRK_IGNORE_EMPTY_TS	0x00002000
>  
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001

-- 
Regards,

Laurent Pinchart