[PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Ricardo Ribalda]

The hardware timestamping code has the assumption than the device_sof
and the host_sof run at the same frequency (1 KHz).

Unfortunately, this is not the case for all the hardware. Add a quirk to
support such hardware.

Note on how to identify such hardware:
When running with "yavta -c /dev/videoX" Look for periodic jumps of the
fps. Eg:

30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
...
99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE

They happen because the delta_sof calculated at
uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
v2: Fix typo in frequency

 drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
 drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
 drivers/media/usb/uvc/uvcvideo.h   |  2 ++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 9a791d8ef200..d1e6cba10b15 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
+	/* Logitech HD Pro Webcam C922 */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x046d,
+	  .idProduct		= 0x085c,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
 	/* Chicony CNF7129 (Asus EEE 100HE) */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_INT_INFO,
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 6d0e474671a2..760ab015cf9c 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
 	/* To limit the amount of data, drop SCRs with an SOF identical to the
 	 * previous one.
 	 */
-	dev_sof = get_unaligned_le16(&data[header_size - 2]);
+	if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
+		dev_sof = usb_get_current_frame_number(stream->dev->udev);
+	else
+		dev_sof = get_unaligned_le16(&data[header_size - 2]);
+
 	if (dev_sof == stream->clock.last_sof)
 		return;
 
 	stream->clock.last_sof = dev_sof;
 
-	host_sof = usb_get_current_frame_number(stream->dev->udev);
+	if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
+		host_sof = dev_sof;
+	else
+		host_sof = usb_get_current_frame_number(stream->dev->udev);
 	time = uvc_video_get_time();
 
 	/* The UVC specification allows device implementations that can't obtain
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index cce5e38133cd..89d909661915 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -209,6 +209,8 @@
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
 #define UVC_QUIRK_FORCE_Y8		0x00000800
 #define UVC_QUIRK_FORCE_BPP		0x00001000
+#define UVC_QUIRK_INVALID_DEVICE_SOF	0x00002000
+
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001
-- 
2.33.0.rc2.250.ged5fa647cd-goog

Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Wed, Aug 18, 2021 at 10:35:02PM +0200, Ricardo Ribalda wrote:
> The hardware timestamping code has the assumption than the device_sof
> and the host_sof run at the same frequency (1 KHz).
> 
> Unfortunately, this is not the case for all the hardware. Add a quirk to
> support such hardware.
> 
> Note on how to identify such hardware:
> When running with "yavta -c /dev/videoX" Look for periodic jumps of the
> fps. Eg:
> 
> 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> ...
> 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
> 
> They happen because the delta_sof calculated at
> uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.

That looks plain wrong. First of all, the whole purpose of the SOF clock
is to have a shared clock between the host and the device. It makes no
sense for a device to have a free-running "SOF" clock. Given the log
above, the issue occurs so quickly that it doesn't seem to be a mere
drift of a free running clock. Could you investigate this more carefully
?

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> v2: Fix typo in frequency
> 
>  drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
>  drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
>  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 9a791d8ef200..d1e6cba10b15 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0,
>  	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> +	/* Logitech HD Pro Webcam C922 */
> +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> +				| USB_DEVICE_ID_MATCH_INT_INFO,
> +	  .idVendor		= 0x046d,
> +	  .idProduct		= 0x085c,
> +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> +	  .bInterfaceSubClass	= 1,
> +	  .bInterfaceProtocol	= 0,
> +	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
>  	/* Chicony CNF7129 (Asus EEE 100HE) */
>  	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
>  				| USB_DEVICE_ID_MATCH_INT_INFO,
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 6d0e474671a2..760ab015cf9c 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
>  	/* To limit the amount of data, drop SCRs with an SOF identical to the
>  	 * previous one.
>  	 */
> -	dev_sof = get_unaligned_le16(&data[header_size - 2]);
> +	if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> +		dev_sof = usb_get_current_frame_number(stream->dev->udev);
> +	else
> +		dev_sof = get_unaligned_le16(&data[header_size - 2]);
> +
>  	if (dev_sof == stream->clock.last_sof)
>  		return;
>  
>  	stream->clock.last_sof = dev_sof;
>  
> -	host_sof = usb_get_current_frame_number(stream->dev->udev);
> +	if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> +		host_sof = dev_sof;
> +	else
> +		host_sof = usb_get_current_frame_number(stream->dev->udev);
>  	time = uvc_video_get_time();
>  
>  	/* The UVC specification allows device implementations that can't obtain
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index cce5e38133cd..89d909661915 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -209,6 +209,8 @@
>  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
>  #define UVC_QUIRK_FORCE_Y8		0x00000800
>  #define UVC_QUIRK_FORCE_BPP		0x00001000
> +#define UVC_QUIRK_INVALID_DEVICE_SOF	0x00002000
> +
>  
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Ricardo Ribalda]

Hi Laurent

Thanks for your prompt reply

On Thu, 19 Aug 2021 at 00:22, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Wed, Aug 18, 2021 at 10:35:02PM +0200, Ricardo Ribalda wrote:
> > The hardware timestamping code has the assumption than the device_sof
> > and the host_sof run at the same frequency (1 KHz).
> >
> > Unfortunately, this is not the case for all the hardware. Add a quirk to
> > support such hardware.
> >
> > Note on how to identify such hardware:
> > When running with "yavta -c /dev/videoX" Look for periodic jumps of the
> > fps. Eg:
> >
> > 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> > 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> > 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> > 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> > 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> > 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> > 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> > ...
> > 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> > 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> > 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> > 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> > 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> > 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
> >
> > They happen because the delta_sof calculated at
> > uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.
>
> That looks plain wrong. First of all, the whole purpose of the SOF clock
> is to have a shared clock between the host and the device. It makes no
> sense for a device to have a free-running "SOF" clock. Given the log
> above, the issue occurs so quickly that it doesn't seem to be a mere
> drift of a free running clock. Could you investigate this more carefully
> ?

In my test the dev_sof runs at 887.91Hz and the dev_sof at 1000.35Hz.
If I plot the difference of both clocks host_sof - (dev_sof % 2048), I
get this nice graph
https://imgur.com/a/5fQnKa7


I agree that it makes not sense to have a free-running "SOF", but the
manufacturer thinks otherwise :)

Best regards

>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > v2: Fix typo in frequency
> >
> >  drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
> >  drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
> >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> >  3 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 9a791d8ef200..d1e6cba10b15 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
> >         .bInterfaceSubClass   = 1,
> >         .bInterfaceProtocol   = 0,
> >         .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> > +     /* Logitech HD Pro Webcam C922 */
> > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > +       .idVendor             = 0x046d,
> > +       .idProduct            = 0x085c,
> > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > +       .bInterfaceSubClass   = 1,
> > +       .bInterfaceProtocol   = 0,
> > +       .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
> >       /* Chicony CNF7129 (Asus EEE 100HE) */
> >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 6d0e474671a2..760ab015cf9c 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> >       /* To limit the amount of data, drop SCRs with an SOF identical to the
> >        * previous one.
> >        */
> > -     dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > +             dev_sof = usb_get_current_frame_number(stream->dev->udev);
> > +     else
> > +             dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > +
> >       if (dev_sof == stream->clock.last_sof)
> >               return;
> >
> >       stream->clock.last_sof = dev_sof;
> >
> > -     host_sof = usb_get_current_frame_number(stream->dev->udev);
> > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > +             host_sof = dev_sof;
> > +     else
> > +             host_sof = usb_get_current_frame_number(stream->dev->udev);
> >       time = uvc_video_get_time();
> >
> >       /* The UVC specification allows device implementations that can't obtain
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index cce5e38133cd..89d909661915 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -209,6 +209,8 @@
> >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > +#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00002000
> > +
> >
> >  /* Format flags */
> >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Laurent Pinchart]

Hi Ricardo,

On Thu, Aug 19, 2021 at 08:27:00AM +0200, Ricardo Ribalda wrote:
> On Thu, 19 Aug 2021 at 00:22, Laurent Pinchart wrote:
> > On Wed, Aug 18, 2021 at 10:35:02PM +0200, Ricardo Ribalda wrote:
> > > The hardware timestamping code has the assumption than the device_sof
> > > and the host_sof run at the same frequency (1 KHz).
> > >
> > > Unfortunately, this is not the case for all the hardware. Add a quirk to
> > > support such hardware.
> > >
> > > Note on how to identify such hardware:
> > > When running with "yavta -c /dev/videoX" Look for periodic jumps of the
> > > fps. Eg:
> > >
> > > 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> > > 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> > > 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> > > 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> > > 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> > > 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> > > 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> > > ...
> > > 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> > > 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> > > 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> > > 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> > > 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> > > 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
> > >
> > > They happen because the delta_sof calculated at
> > > uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.
> >
> > That looks plain wrong. First of all, the whole purpose of the SOF clock
> > is to have a shared clock between the host and the device. It makes no
> > sense for a device to have a free-running "SOF" clock. Given the log
> > above, the issue occurs so quickly that it doesn't seem to be a mere
> > drift of a free running clock. Could you investigate this more carefully
> > ?
> 
> In my test the dev_sof runs at 887.91Hz and the dev_sof at 1000.35Hz.
> If I plot the difference of both clocks host_sof - (dev_sof % 2048), I
> get this nice graph https://imgur.com/a/5fQnKa7
> 
> 
> I agree that it makes not sense to have a free-running "SOF", but the
> manufacturer thinks otherwise :)

In that case there's no common clock between the device and the host,
which means that clock recovery is impossible. The whole timestamp
computation should be bypassed, and the driver should use the system
timestamp instead.

I still find it hard to believe that a Logitech camera would get this
wrong.

> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > > v2: Fix typo in frequency
> > >
> > >  drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
> > >  drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
> > >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> > >  3 files changed, 20 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 9a791d8ef200..d1e6cba10b15 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
> > >         .bInterfaceSubClass   = 1,
> > >         .bInterfaceProtocol   = 0,
> > >         .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> > > +     /* Logitech HD Pro Webcam C922 */
> > > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > > +       .idVendor             = 0x046d,
> > > +       .idProduct            = 0x085c,
> > > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > > +       .bInterfaceSubClass   = 1,
> > > +       .bInterfaceProtocol   = 0,
> > > +       .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
> > >       /* Chicony CNF7129 (Asus EEE 100HE) */
> > >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > index 6d0e474671a2..760ab015cf9c 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > @@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> > >       /* To limit the amount of data, drop SCRs with an SOF identical to the
> > >        * previous one.
> > >        */
> > > -     dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > +             dev_sof = usb_get_current_frame_number(stream->dev->udev);
> > > +     else
> > > +             dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > +
> > >       if (dev_sof == stream->clock.last_sof)
> > >               return;
> > >
> > >       stream->clock.last_sof = dev_sof;
> > >
> > > -     host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > +             host_sof = dev_sof;
> > > +     else
> > > +             host_sof = usb_get_current_frame_number(stream->dev->udev);
> > >       time = uvc_video_get_time();
> > >
> > >       /* The UVC specification allows device implementations that can't obtain
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index cce5e38133cd..89d909661915 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -209,6 +209,8 @@
> > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > +#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00002000
> > > +
> > >
> > >  /* Format flags */
> > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Ricardo Ribalda]

On Thu, 19 Aug 2021 at 12:28, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Thu, Aug 19, 2021 at 08:27:00AM +0200, Ricardo Ribalda wrote:
> > On Thu, 19 Aug 2021 at 00:22, Laurent Pinchart wrote:
> > > On Wed, Aug 18, 2021 at 10:35:02PM +0200, Ricardo Ribalda wrote:
> > > > The hardware timestamping code has the assumption than the device_sof
> > > > and the host_sof run at the same frequency (1 KHz).
> > > >
> > > > Unfortunately, this is not the case for all the hardware. Add a quirk to
> > > > support such hardware.
> > > >
> > > > Note on how to identify such hardware:
> > > > When running with "yavta -c /dev/videoX" Look for periodic jumps of the
> > > > fps. Eg:
> > > >
> > > > 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> > > > 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> > > > 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> > > > 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> > > > 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> > > > 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> > > > 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> > > > ...
> > > > 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> > > > 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> > > > 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> > > > 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> > > > 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> > > > 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
> > > >
> > > > They happen because the delta_sof calculated at
> > > > uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.
> > >
> > > That looks plain wrong. First of all, the whole purpose of the SOF clock
> > > is to have a shared clock between the host and the device. It makes no
> > > sense for a device to have a free-running "SOF" clock. Given the log
> > > above, the issue occurs so quickly that it doesn't seem to be a mere
> > > drift of a free running clock. Could you investigate this more carefully
> > > ?
> >
> > In my test the dev_sof runs at 887.91Hz and the dev_sof at 1000.35Hz.
> > If I plot the difference of both clocks host_sof - (dev_sof % 2048), I
> > get this nice graph https://imgur.com/a/5fQnKa7
> >
> >
> > I agree that it makes not sense to have a free-running "SOF", but the
> > manufacturer thinks otherwise :)
>
> In that case there's no common clock between the device and the host,
> which means that clock recovery is impossible. The whole timestamp
> computation should be bypassed, and the driver should use the system
> timestamp instead.

Or said differently. The clock recovery is susceptible to the jitter
in the frame acquisition.

If you have no jitter, the clock recovered will match the reality, and
if you have bad jitter, it will be as bad as system timestamp.

So this patch will still be better than nothing.

>
> I still find it hard to believe that a Logitech camera would get this
> wrong.

I guess I can send you a device, or give you access to mine remotely
if you do not believe me :)


>
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > > v2: Fix typo in frequency
> > > >
> > > >  drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
> > > >  drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
> > > >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> > > >  3 files changed, 20 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index 9a791d8ef200..d1e6cba10b15 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
> > > >         .bInterfaceSubClass   = 1,
> > > >         .bInterfaceProtocol   = 0,
> > > >         .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> > > > +     /* Logitech HD Pro Webcam C922 */
> > > > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > +       .idVendor             = 0x046d,
> > > > +       .idProduct            = 0x085c,
> > > > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > > > +       .bInterfaceSubClass   = 1,
> > > > +       .bInterfaceProtocol   = 0,
> > > > +       .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
> > > >       /* Chicony CNF7129 (Asus EEE 100HE) */
> > > >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index 6d0e474671a2..760ab015cf9c 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> > > >       /* To limit the amount of data, drop SCRs with an SOF identical to the
> > > >        * previous one.
> > > >        */
> > > > -     dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > +             dev_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > +     else
> > > > +             dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > +
> > > >       if (dev_sof == stream->clock.last_sof)
> > > >               return;
> > > >
> > > >       stream->clock.last_sof = dev_sof;
> > > >
> > > > -     host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > +             host_sof = dev_sof;
> > > > +     else
> > > > +             host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > >       time = uvc_video_get_time();
> > > >
> > > >       /* The UVC specification allows device implementations that can't obtain
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index cce5e38133cd..89d909661915 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -209,6 +209,8 @@
> > > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > > +#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00002000
> > > > +
> > > >
> > > >  /* Format flags */
> > > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Laurent Pinchart]

Hi Ricardo,

On Thu, Aug 19, 2021 at 01:31:32PM +0200, Ricardo Ribalda wrote:
> On Thu, 19 Aug 2021 at 12:28, Laurent Pinchart wrote:
> > On Thu, Aug 19, 2021 at 08:27:00AM +0200, Ricardo Ribalda wrote:
> > > On Thu, 19 Aug 2021 at 00:22, Laurent Pinchart wrote:
> > > > On Wed, Aug 18, 2021 at 10:35:02PM +0200, Ricardo Ribalda wrote:
> > > > > The hardware timestamping code has the assumption than the device_sof
> > > > > and the host_sof run at the same frequency (1 KHz).
> > > > >
> > > > > Unfortunately, this is not the case for all the hardware. Add a quirk to
> > > > > support such hardware.
> > > > >
> > > > > Note on how to identify such hardware:
> > > > > When running with "yavta -c /dev/videoX" Look for periodic jumps of the
> > > > > fps. Eg:
> > > > >
> > > > > 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> > > > > 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> > > > > 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> > > > > 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> > > > > 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> > > > > 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> > > > > 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> > > > > ...
> > > > > 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> > > > > 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> > > > > 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> > > > > 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> > > > > 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> > > > > 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
> > > > >
> > > > > They happen because the delta_sof calculated at
> > > > > uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.
> > > >
> > > > That looks plain wrong. First of all, the whole purpose of the SOF clock
> > > > is to have a shared clock between the host and the device. It makes no
> > > > sense for a device to have a free-running "SOF" clock. Given the log
> > > > above, the issue occurs so quickly that it doesn't seem to be a mere
> > > > drift of a free running clock. Could you investigate this more carefully
> > > > ?
> > >
> > > In my test the dev_sof runs at 887.91Hz and the dev_sof at 1000.35Hz.
> > > If I plot the difference of both clocks host_sof - (dev_sof % 2048), I
> > > get this nice graph https://imgur.com/a/5fQnKa7
> > >
> > >
> > > I agree that it makes not sense to have a free-running "SOF", but the
> > > manufacturer thinks otherwise :)
> >
> > In that case there's no common clock between the device and the host,
> > which means that clock recovery is impossible. The whole timestamp
> > computation should be bypassed, and the driver should use the system
> > timestamp instead.
> 
> Or said differently. The clock recovery is susceptible to the jitter
> in the frame acquisition.
> 
> If you have no jitter, the clock recovered will match the reality, and
> if you have bad jitter, it will be as bad as system timestamp.

The whole point of the clock recovery code is to convert a precise
timestamp, expressed using a device clock that the host has no access
to, to a system clock. This can only be done if the relationship between
the two clocks can be inferred, and the UVC specifies a mechanism to
allow this by using a common clock, in the form of the SOF counter. If
we don't have that, we're essentially screwed, and can't use the
algorithm implemented in the driver at all. I'd much rather skip is
completely in that case, instead of trying to hack the algorithm itself.

On a side note, I think the whole clock recovery implementation should
move from the uvcvideo driver to userspace, where we'll have the ability
to perform floating point computation. The kernel implementation is
crude, it should be replaced with a linear regression.

> So this patch will still be better than nothing.
> 
> > I still find it hard to believe that a Logitech camera would get this
> > wrong.
> 
> I guess I can send you a device, or give you access to mine remotely
> if you do not believe me :)
> 
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > > v2: Fix typo in frequency
> > > > >
> > > > >  drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
> > > > >  drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
> > > > >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> > > > >  3 files changed, 20 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > index 9a791d8ef200..d1e6cba10b15 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > @@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
> > > > >         .bInterfaceSubClass   = 1,
> > > > >         .bInterfaceProtocol   = 0,
> > > > >         .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> > > > > +     /* Logitech HD Pro Webcam C922 */
> > > > > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > +       .idVendor             = 0x046d,
> > > > > +       .idProduct            = 0x085c,
> > > > > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > > > > +       .bInterfaceSubClass   = 1,
> > > > > +       .bInterfaceProtocol   = 0,
> > > > > +       .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
> > > > >       /* Chicony CNF7129 (Asus EEE 100HE) */
> > > > >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > index 6d0e474671a2..760ab015cf9c 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > @@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> > > > >       /* To limit the amount of data, drop SCRs with an SOF identical to the
> > > > >        * previous one.
> > > > >        */
> > > > > -     dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > +             dev_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > +     else
> > > > > +             dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > +
> > > > >       if (dev_sof == stream->clock.last_sof)
> > > > >               return;
> > > > >
> > > > >       stream->clock.last_sof = dev_sof;
> > > > >
> > > > > -     host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > +             host_sof = dev_sof;
> > > > > +     else
> > > > > +             host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > >       time = uvc_video_get_time();
> > > > >
> > > > >       /* The UVC specification allows device implementations that can't obtain
> > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > > index cce5e38133cd..89d909661915 100644
> > > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > > @@ -209,6 +209,8 @@
> > > > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > > > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > > > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > > > +#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00002000
> > > > > +
> > > > >
> > > > >  /* Format flags */
> > > > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Ricardo Ribalda]

On Thu, 19 Aug 2021 at 16:15, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Thu, Aug 19, 2021 at 01:31:32PM +0200, Ricardo Ribalda wrote:
> > On Thu, 19 Aug 2021 at 12:28, Laurent Pinchart wrote:
> > > On Thu, Aug 19, 2021 at 08:27:00AM +0200, Ricardo Ribalda wrote:
> > > > On Thu, 19 Aug 2021 at 00:22, Laurent Pinchart wrote:
> > > > > On Wed, Aug 18, 2021 at 10:35:02PM +0200, Ricardo Ribalda wrote:
> > > > > > The hardware timestamping code has the assumption than the device_sof
> > > > > > and the host_sof run at the same frequency (1 KHz).
> > > > > >
> > > > > > Unfortunately, this is not the case for all the hardware. Add a quirk to
> > > > > > support such hardware.
> > > > > >
> > > > > > Note on how to identify such hardware:
> > > > > > When running with "yavta -c /dev/videoX" Look for periodic jumps of the
> > > > > > fps. Eg:
> > > > > >
> > > > > > 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> > > > > > 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> > > > > > 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> > > > > > 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> > > > > > 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> > > > > > 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> > > > > > 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> > > > > > ...
> > > > > > 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> > > > > > 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> > > > > > 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> > > > > > 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> > > > > > 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> > > > > > 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
> > > > > >
> > > > > > They happen because the delta_sof calculated at
> > > > > > uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.
> > > > >
> > > > > That looks plain wrong. First of all, the whole purpose of the SOF clock
> > > > > is to have a shared clock between the host and the device. It makes no
> > > > > sense for a device to have a free-running "SOF" clock. Given the log
> > > > > above, the issue occurs so quickly that it doesn't seem to be a mere
> > > > > drift of a free running clock. Could you investigate this more carefully
> > > > > ?
> > > >
> > > > In my test the dev_sof runs at 887.91Hz and the dev_sof at 1000.35Hz.
> > > > If I plot the difference of both clocks host_sof - (dev_sof % 2048), I
> > > > get this nice graph https://imgur.com/a/5fQnKa7
> > > >
> > > >
> > > > I agree that it makes not sense to have a free-running "SOF", but the
> > > > manufacturer thinks otherwise :)
> > >
> > > In that case there's no common clock between the device and the host,
> > > which means that clock recovery is impossible. The whole timestamp
> > > computation should be bypassed, and the driver should use the system
> > > timestamp instead.
> >
> > Or said differently. The clock recovery is susceptible to the jitter
> > in the frame acquisition.
> >
> > If you have no jitter, the clock recovered will match the reality, and
> > if you have bad jitter, it will be as bad as system timestamp.
>
> The whole point of the clock recovery code is to convert a precise
> timestamp, expressed using a device clock that the host has no access
> to, to a system clock. This can only be done if the relationship between
> the two clocks can be inferred, and the UVC specifies a mechanism to
> allow this by using a common clock, in the form of the SOF counter. If
> we don't have that, we're essentially screwed, and can't use the
> algorithm implemented in the driver at all. I'd much rather skip is
> completely in that case, instead of trying to hack the algorithm itself.

Considering T(f) as the time between the usb package (f) is received
and uvc_video_clock_decode()
If the jitter between the different T(f)s is under one unit of our
clock (1 msec) the accuracy of the "hacked" algorithm and the real
algorithm is exactly the same.

We can agree that 1 msec is a "lot" of time. And if our system has a
worse latency than that, the hacked algorithm will not be worse than
system timestamping.

So in most of the situations this patch will produce better timestamps
than the current code and never worse than now...

Anyway, I have tried to ping the vendor to see if there is something
that I could be doing wrong, lets see what they reply.

>
> On a side note, I think the whole clock recovery implementation should
> move from the uvcvideo driver to userspace, where we'll have the ability
> to perform floating point computation. The kernel implementation is
> crude, it should be replaced with a linear regression.

Agree, but instead of a linear regression, a resampling algorithm.

>
> > So this patch will still be better than nothing.
> >
> > > I still find it hard to believe that a Logitech camera would get this
> > > wrong.
> >
> > I guess I can send you a device, or give you access to mine remotely
> > if you do not believe me :)
> >
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > ---
> > > > > > v2: Fix typo in frequency
> > > > > >
> > > > > >  drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
> > > > > >  drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
> > > > > >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> > > > > >  3 files changed, 20 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > index 9a791d8ef200..d1e6cba10b15 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > @@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
> > > > > >         .bInterfaceSubClass   = 1,
> > > > > >         .bInterfaceProtocol   = 0,
> > > > > >         .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> > > > > > +     /* Logitech HD Pro Webcam C922 */
> > > > > > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > > +       .idVendor             = 0x046d,
> > > > > > +       .idProduct            = 0x085c,
> > > > > > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > > > > > +       .bInterfaceSubClass   = 1,
> > > > > > +       .bInterfaceProtocol   = 0,
> > > > > > +       .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
> > > > > >       /* Chicony CNF7129 (Asus EEE 100HE) */
> > > > > >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > > index 6d0e474671a2..760ab015cf9c 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > > @@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> > > > > >       /* To limit the amount of data, drop SCRs with an SOF identical to the
> > > > > >        * previous one.
> > > > > >        */
> > > > > > -     dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > > +             dev_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > +     else
> > > > > > +             dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > > +
> > > > > >       if (dev_sof == stream->clock.last_sof)
> > > > > >               return;
> > > > > >
> > > > > >       stream->clock.last_sof = dev_sof;
> > > > > >
> > > > > > -     host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > > +             host_sof = dev_sof;
> > > > > > +     else
> > > > > > +             host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > >       time = uvc_video_get_time();
> > > > > >
> > > > > >       /* The UVC specification allows device implementations that can't obtain
> > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > > > index cce5e38133cd..89d909661915 100644
> > > > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > > > @@ -209,6 +209,8 @@
> > > > > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > > > > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > > > > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > > > > +#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00002000
> > > > > > +
> > > > > >
> > > > > >  /* Format flags */
> > > > > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Laurent Pinchart]

Hi Ricardo,

On Thu, Aug 19, 2021 at 04:46:38PM +0200, Ricardo Ribalda wrote:
> On Thu, 19 Aug 2021 at 16:15, Laurent Pinchart wrote:
> > On Thu, Aug 19, 2021 at 01:31:32PM +0200, Ricardo Ribalda wrote:
> > > On Thu, 19 Aug 2021 at 12:28, Laurent Pinchart wrote:
> > > > On Thu, Aug 19, 2021 at 08:27:00AM +0200, Ricardo Ribalda wrote:
> > > > > On Thu, 19 Aug 2021 at 00:22, Laurent Pinchart wrote:
> > > > > > On Wed, Aug 18, 2021 at 10:35:02PM +0200, Ricardo Ribalda wrote:
> > > > > > > The hardware timestamping code has the assumption than the device_sof
> > > > > > > and the host_sof run at the same frequency (1 KHz).
> > > > > > >
> > > > > > > Unfortunately, this is not the case for all the hardware. Add a quirk to
> > > > > > > support such hardware.
> > > > > > >
> > > > > > > Note on how to identify such hardware:
> > > > > > > When running with "yavta -c /dev/videoX" Look for periodic jumps of the
> > > > > > > fps. Eg:
> > > > > > >
> > > > > > > 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> > > > > > > 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> > > > > > > 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> > > > > > > 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> > > > > > > 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> > > > > > > 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> > > > > > > 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> > > > > > > ...
> > > > > > > 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> > > > > > > 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> > > > > > > 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> > > > > > > 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> > > > > > > 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> > > > > > > 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
> > > > > > >
> > > > > > > They happen because the delta_sof calculated at
> > > > > > > uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.
> > > > > >
> > > > > > That looks plain wrong. First of all, the whole purpose of the SOF clock
> > > > > > is to have a shared clock between the host and the device. It makes no
> > > > > > sense for a device to have a free-running "SOF" clock. Given the log
> > > > > > above, the issue occurs so quickly that it doesn't seem to be a mere
> > > > > > drift of a free running clock. Could you investigate this more carefully
> > > > > > ?
> > > > >
> > > > > In my test the dev_sof runs at 887.91Hz and the dev_sof at 1000.35Hz.
> > > > > If I plot the difference of both clocks host_sof - (dev_sof % 2048), I
> > > > > get this nice graph https://imgur.com/a/5fQnKa7
> > > > >
> > > > > I agree that it makes not sense to have a free-running "SOF", but the
> > > > > manufacturer thinks otherwise :)
> > > >
> > > > In that case there's no common clock between the device and the host,
> > > > which means that clock recovery is impossible. The whole timestamp
> > > > computation should be bypassed, and the driver should use the system
> > > > timestamp instead.
> > >
> > > Or said differently. The clock recovery is susceptible to the jitter
> > > in the frame acquisition.
> > >
> > > If you have no jitter, the clock recovered will match the reality, and
> > > if you have bad jitter, it will be as bad as system timestamp.
> >
> > The whole point of the clock recovery code is to convert a precise
> > timestamp, expressed using a device clock that the host has no access
> > to, to a system clock. This can only be done if the relationship between
> > the two clocks can be inferred, and the UVC specifies a mechanism to
> > allow this by using a common clock, in the form of the SOF counter. If
> > we don't have that, we're essentially screwed, and can't use the
> > algorithm implemented in the driver at all. I'd much rather skip is
> > completely in that case, instead of trying to hack the algorithm itself.
> 
> Considering T(f) as the time between the usb package (f) is received
> and uvc_video_clock_decode()
> If the jitter between the different T(f)s is under one unit of our
> clock (1 msec) the accuracy of the "hacked" algorithm and the real
> algorithm is exactly the same.
> 
> We can agree that 1 msec is a "lot" of time. And if our system has a
> worse latency than that, the hacked algorithm will not be worse than
> system timestamping.
> 
> So in most of the situations this patch will produce better timestamps
> than the current code and never worse than now...

How can it produce better timestamps if it's missing the crucial
information that provides the correlation of timestamps between the
device and host side ?

> Anyway, I have tried to ping the vendor to see if there is something
> that I could be doing wrong, lets see what they reply.
> 
> > On a side note, I think the whole clock recovery implementation should
> > move from the uvcvideo driver to userspace, where we'll have the ability
> > to perform floating point computation. The kernel implementation is
> > crude, it should be replaced with a linear regression.
> 
> Agree, but instead of a linear regression, a resampling algorithm.

A linear regression is likely a good enough resampling algorithm in this
case, but I'd be curious to see if someone could do better.

> > > So this patch will still be better than nothing.
> > >
> > > > I still find it hard to believe that a Logitech camera would get this
> > > > wrong.
> > >
> > > I guess I can send you a device, or give you access to mine remotely
> > > if you do not believe me :)
> > >
> > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > ---
> > > > > > > v2: Fix typo in frequency
> > > > > > >
> > > > > > >  drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
> > > > > > >  drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
> > > > > > >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> > > > > > >  3 files changed, 20 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > index 9a791d8ef200..d1e6cba10b15 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > @@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
> > > > > > >         .bInterfaceSubClass   = 1,
> > > > > > >         .bInterfaceProtocol   = 0,
> > > > > > >         .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> > > > > > > +     /* Logitech HD Pro Webcam C922 */
> > > > > > > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > > > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > > > +       .idVendor             = 0x046d,
> > > > > > > +       .idProduct            = 0x085c,
> > > > > > > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > > > > > > +       .bInterfaceSubClass   = 1,
> > > > > > > +       .bInterfaceProtocol   = 0,
> > > > > > > +       .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
> > > > > > >       /* Chicony CNF7129 (Asus EEE 100HE) */
> > > > > > >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > > >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > > > index 6d0e474671a2..760ab015cf9c 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > > > @@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> > > > > > >       /* To limit the amount of data, drop SCRs with an SOF identical to the
> > > > > > >        * previous one.
> > > > > > >        */
> > > > > > > -     dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > > > +             dev_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > +     else
> > > > > > > +             dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > > > +
> > > > > > >       if (dev_sof == stream->clock.last_sof)
> > > > > > >               return;
> > > > > > >
> > > > > > >       stream->clock.last_sof = dev_sof;
> > > > > > >
> > > > > > > -     host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > > > +             host_sof = dev_sof;
> > > > > > > +     else
> > > > > > > +             host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > >       time = uvc_video_get_time();
> > > > > > >
> > > > > > >       /* The UVC specification allows device implementations that can't obtain
> > > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > index cce5e38133cd..89d909661915 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > @@ -209,6 +209,8 @@
> > > > > > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > > > > > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > > > > > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > > > > > +#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00002000
> > > > > > > +
> > > > > > >
> > > > > > >  /* Format flags */
> > > > > > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Ricardo Ribalda]

On Mon, 23 Aug 2021 at 03:54, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Thu, Aug 19, 2021 at 04:46:38PM +0200, Ricardo Ribalda wrote:
> > On Thu, 19 Aug 2021 at 16:15, Laurent Pinchart wrote:
> > > On Thu, Aug 19, 2021 at 01:31:32PM +0200, Ricardo Ribalda wrote:
> > > > On Thu, 19 Aug 2021 at 12:28, Laurent Pinchart wrote:
> > > > > On Thu, Aug 19, 2021 at 08:27:00AM +0200, Ricardo Ribalda wrote:
> > > > > > On Thu, 19 Aug 2021 at 00:22, Laurent Pinchart wrote:
> > > > > > > On Wed, Aug 18, 2021 at 10:35:02PM +0200, Ricardo Ribalda wrote:
> > > > > > > > The hardware timestamping code has the assumption than the device_sof
> > > > > > > > and the host_sof run at the same frequency (1 KHz).
> > > > > > > >
> > > > > > > > Unfortunately, this is not the case for all the hardware. Add a quirk to
> > > > > > > > support such hardware.
> > > > > > > >
> > > > > > > > Note on how to identify such hardware:
> > > > > > > > When running with "yavta -c /dev/videoX" Look for periodic jumps of the
> > > > > > > > fps. Eg:
> > > > > > > >
> > > > > > > > 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> > > > > > > > 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> > > > > > > > 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> > > > > > > > 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> > > > > > > > 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> > > > > > > > 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> > > > > > > > 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> > > > > > > > ...
> > > > > > > > 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> > > > > > > > 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> > > > > > > > 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> > > > > > > > 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> > > > > > > > 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> > > > > > > > 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
> > > > > > > >
> > > > > > > > They happen because the delta_sof calculated at
> > > > > > > > uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.
> > > > > > >
> > > > > > > That looks plain wrong. First of all, the whole purpose of the SOF clock
> > > > > > > is to have a shared clock between the host and the device. It makes no
> > > > > > > sense for a device to have a free-running "SOF" clock. Given the log
> > > > > > > above, the issue occurs so quickly that it doesn't seem to be a mere
> > > > > > > drift of a free running clock. Could you investigate this more carefully
> > > > > > > ?
> > > > > >
> > > > > > In my test the dev_sof runs at 887.91Hz and the dev_sof at 1000.35Hz.
> > > > > > If I plot the difference of both clocks host_sof - (dev_sof % 2048), I
> > > > > > get this nice graph https://imgur.com/a/5fQnKa7
> > > > > >
> > > > > > I agree that it makes not sense to have a free-running "SOF", but the
> > > > > > manufacturer thinks otherwise :)
> > > > >
> > > > > In that case there's no common clock between the device and the host,
> > > > > which means that clock recovery is impossible. The whole timestamp
> > > > > computation should be bypassed, and the driver should use the system
> > > > > timestamp instead.
> > > >
> > > > Or said differently. The clock recovery is susceptible to the jitter
> > > > in the frame acquisition.
> > > >
> > > > If you have no jitter, the clock recovered will match the reality, and
> > > > if you have bad jitter, it will be as bad as system timestamp.
> > >
> > > The whole point of the clock recovery code is to convert a precise
> > > timestamp, expressed using a device clock that the host has no access
> > > to, to a system clock. This can only be done if the relationship between
> > > the two clocks can be inferred, and the UVC specifies a mechanism to
> > > allow this by using a common clock, in the form of the SOF counter. If
> > > we don't have that, we're essentially screwed, and can't use the
> > > algorithm implemented in the driver at all. I'd much rather skip is
> > > completely in that case, instead of trying to hack the algorithm itself.
> >
> > Considering T(f) as the time between the usb package (f) is received
> > and uvc_video_clock_decode()
> > If the jitter between the different T(f)s is under one unit of our
> > clock (1 msec) the accuracy of the "hacked" algorithm and the real
> > algorithm is exactly the same.
> >
> > We can agree that 1 msec is a "lot" of time. And if our system has a
> > worse latency than that, the hacked algorithm will not be worse than
> > system timestamping.
> >
> > So in most of the situations this patch will produce better timestamps
> > than the current code and never worse than now...
>
> How can it produce better timestamps if it's missing the crucial
> information that provides the correlation of timestamps between the
> device and host side ?

Because in a system with a latency jitter under 1msec sof_device and
sof_host you already know that information: sof_host = sof_device

It is a special case of the general problem.

>
> > Anyway, I have tried to ping the vendor to see if there is something
> > that I could be doing wrong, lets see what they reply.
> >
> > > On a side note, I think the whole clock recovery implementation should
> > > move from the uvcvideo driver to userspace, where we'll have the ability
> > > to perform floating point computation. The kernel implementation is
> > > crude, it should be replaced with a linear regression.
> >
> > Agree, but instead of a linear regression, a resampling algorithm.
>
> A linear regression is likely a good enough resampling algorithm in this
> case, but I'd be curious to see if someone could do better.
>
> > > > So this patch will still be better than nothing.
> > > >
> > > > > I still find it hard to believe that a Logitech camera would get this
> > > > > wrong.
> > > >
> > > > I guess I can send you a device, or give you access to mine remotely
> > > > if you do not believe me :)
> > > >
> > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > > ---
> > > > > > > > v2: Fix typo in frequency
> > > > > > > >
> > > > > > > >  drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
> > > > > > > >  drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
> > > > > > > >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> > > > > > > >  3 files changed, 20 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > index 9a791d8ef200..d1e6cba10b15 100644
> > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > @@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
> > > > > > > >         .bInterfaceSubClass   = 1,
> > > > > > > >         .bInterfaceProtocol   = 0,
> > > > > > > >         .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> > > > > > > > +     /* Logitech HD Pro Webcam C922 */
> > > > > > > > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > > > > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > > > > +       .idVendor             = 0x046d,
> > > > > > > > +       .idProduct            = 0x085c,
> > > > > > > > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > > > > > > > +       .bInterfaceSubClass   = 1,
> > > > > > > > +       .bInterfaceProtocol   = 0,
> > > > > > > > +       .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
> > > > > > > >       /* Chicony CNF7129 (Asus EEE 100HE) */
> > > > > > > >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > > > >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > > > > index 6d0e474671a2..760ab015cf9c 100644
> > > > > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > > > > @@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> > > > > > > >       /* To limit the amount of data, drop SCRs with an SOF identical to the
> > > > > > > >        * previous one.
> > > > > > > >        */
> > > > > > > > -     dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > > > > +             dev_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > > +     else
> > > > > > > > +             dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > > > > +
> > > > > > > >       if (dev_sof == stream->clock.last_sof)
> > > > > > > >               return;
> > > > > > > >
> > > > > > > >       stream->clock.last_sof = dev_sof;
> > > > > > > >
> > > > > > > > -     host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > > > > +             host_sof = dev_sof;
> > > > > > > > +     else
> > > > > > > > +             host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > >       time = uvc_video_get_time();
> > > > > > > >
> > > > > > > >       /* The UVC specification allows device implementations that can't obtain
> > > > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > > index cce5e38133cd..89d909661915 100644
> > > > > > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > > @@ -209,6 +209,8 @@
> > > > > > > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > > > > > > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > > > > > > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > > > > > > +#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00002000
> > > > > > > > +
> > > > > > > >
> > > > > > > >  /* Format flags */
> > > > > > > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Laurent Pinchart]

On Mon, Aug 23, 2021 at 11:56:43AM +0200, Ricardo Ribalda wrote:
> On Mon, 23 Aug 2021 at 03:54, Laurent Pinchart wrote:
> > On Thu, Aug 19, 2021 at 04:46:38PM +0200, Ricardo Ribalda wrote:
> > > On Thu, 19 Aug 2021 at 16:15, Laurent Pinchart wrote:
> > > > On Thu, Aug 19, 2021 at 01:31:32PM +0200, Ricardo Ribalda wrote:
> > > > > On Thu, 19 Aug 2021 at 12:28, Laurent Pinchart wrote:
> > > > > > On Thu, Aug 19, 2021 at 08:27:00AM +0200, Ricardo Ribalda wrote:
> > > > > > > On Thu, 19 Aug 2021 at 00:22, Laurent Pinchart wrote:
> > > > > > > > On Wed, Aug 18, 2021 at 10:35:02PM +0200, Ricardo Ribalda wrote:
> > > > > > > > > The hardware timestamping code has the assumption than the device_sof
> > > > > > > > > and the host_sof run at the same frequency (1 KHz).
> > > > > > > > >
> > > > > > > > > Unfortunately, this is not the case for all the hardware. Add a quirk to
> > > > > > > > > support such hardware.
> > > > > > > > >
> > > > > > > > > Note on how to identify such hardware:
> > > > > > > > > When running with "yavta -c /dev/videoX" Look for periodic jumps of the
> > > > > > > > > fps. Eg:
> > > > > > > > >
> > > > > > > > > 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> > > > > > > > > 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> > > > > > > > > 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> > > > > > > > > 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> > > > > > > > > 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> > > > > > > > > 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> > > > > > > > > 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> > > > > > > > > ...
> > > > > > > > > 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> > > > > > > > > 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> > > > > > > > > 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> > > > > > > > > 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> > > > > > > > > 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> > > > > > > > > 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
> > > > > > > > >
> > > > > > > > > They happen because the delta_sof calculated at
> > > > > > > > > uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.
> > > > > > > >
> > > > > > > > That looks plain wrong. First of all, the whole purpose of the SOF clock
> > > > > > > > is to have a shared clock between the host and the device. It makes no
> > > > > > > > sense for a device to have a free-running "SOF" clock. Given the log
> > > > > > > > above, the issue occurs so quickly that it doesn't seem to be a mere
> > > > > > > > drift of a free running clock. Could you investigate this more carefully
> > > > > > > > ?
> > > > > > >
> > > > > > > In my test the dev_sof runs at 887.91Hz and the dev_sof at 1000.35Hz.
> > > > > > > If I plot the difference of both clocks host_sof - (dev_sof % 2048), I
> > > > > > > get this nice graph https://imgur.com/a/5fQnKa7
> > > > > > >
> > > > > > > I agree that it makes not sense to have a free-running "SOF", but the
> > > > > > > manufacturer thinks otherwise :)
> > > > > >
> > > > > > In that case there's no common clock between the device and the host,
> > > > > > which means that clock recovery is impossible. The whole timestamp
> > > > > > computation should be bypassed, and the driver should use the system
> > > > > > timestamp instead.
> > > > >
> > > > > Or said differently. The clock recovery is susceptible to the jitter
> > > > > in the frame acquisition.
> > > > >
> > > > > If you have no jitter, the clock recovered will match the reality, and
> > > > > if you have bad jitter, it will be as bad as system timestamp.
> > > >
> > > > The whole point of the clock recovery code is to convert a precise
> > > > timestamp, expressed using a device clock that the host has no access
> > > > to, to a system clock. This can only be done if the relationship between
> > > > the two clocks can be inferred, and the UVC specifies a mechanism to
> > > > allow this by using a common clock, in the form of the SOF counter. If
> > > > we don't have that, we're essentially screwed, and can't use the
> > > > algorithm implemented in the driver at all. I'd much rather skip is
> > > > completely in that case, instead of trying to hack the algorithm itself.
> > >
> > > Considering T(f) as the time between the usb package (f) is received
> > > and uvc_video_clock_decode()
> > > If the jitter between the different T(f)s is under one unit of our
> > > clock (1 msec) the accuracy of the "hacked" algorithm and the real
> > > algorithm is exactly the same.
> > >
> > > We can agree that 1 msec is a "lot" of time. And if our system has a
> > > worse latency than that, the hacked algorithm will not be worse than
> > > system timestamping.
> > >
> > > So in most of the situations this patch will produce better timestamps
> > > than the current code and never worse than now...
> >
> > How can it produce better timestamps if it's missing the crucial
> > information that provides the correlation of timestamps between the
> > device and host side ?
> 
> Because in a system with a latency jitter under 1msec sof_device and
> sof_host you already know that information: sof_host = sof_device

Only (100 - jitter/1ms) % of the time.

Given that the kernel implementation of the clock recovery is known to
cause timestamps to jump back in time once in a while (with devices that
behave properly), and that this should be implemented in userspace, I'd
rather bypass the kernel-side clock recovery completely when the device
doesn't behave. Then, we'll discuss whether it shuold be bypassed in
userspace too for this device, based on mathematical evidence :-)

> It is a special case of the general problem.
> 
> > > Anyway, I have tried to ping the vendor to see if there is something
> > > that I could be doing wrong, lets see what they reply.
> > >
> > > > On a side note, I think the whole clock recovery implementation should
> > > > move from the uvcvideo driver to userspace, where we'll have the ability
> > > > to perform floating point computation. The kernel implementation is
> > > > crude, it should be replaced with a linear regression.
> > >
> > > Agree, but instead of a linear regression, a resampling algorithm.
> >
> > A linear regression is likely a good enough resampling algorithm in this
> > case, but I'd be curious to see if someone could do better.
> >
> > > > > So this patch will still be better than nothing.
> > > > >
> > > > > > I still find it hard to believe that a Logitech camera would get this
> > > > > > wrong.
> > > > >
> > > > > I guess I can send you a device, or give you access to mine remotely
> > > > > if you do not believe me :)
> > > > >
> > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > > > ---
> > > > > > > > > v2: Fix typo in frequency
> > > > > > > > >
> > > > > > > > >  drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
> > > > > > > > >  drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
> > > > > > > > >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> > > > > > > > >  3 files changed, 20 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > index 9a791d8ef200..d1e6cba10b15 100644
> > > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > @@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
> > > > > > > > >         .bInterfaceSubClass   = 1,
> > > > > > > > >         .bInterfaceProtocol   = 0,
> > > > > > > > >         .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> > > > > > > > > +     /* Logitech HD Pro Webcam C922 */
> > > > > > > > > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > > > > > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > > > > > +       .idVendor             = 0x046d,
> > > > > > > > > +       .idProduct            = 0x085c,
> > > > > > > > > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > > > > > > > > +       .bInterfaceSubClass   = 1,
> > > > > > > > > +       .bInterfaceProtocol   = 0,
> > > > > > > > > +       .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
> > > > > > > > >       /* Chicony CNF7129 (Asus EEE 100HE) */
> > > > > > > > >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > > > > >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > > > > > index 6d0e474671a2..760ab015cf9c 100644
> > > > > > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > > > > > @@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> > > > > > > > >       /* To limit the amount of data, drop SCRs with an SOF identical to the
> > > > > > > > >        * previous one.
> > > > > > > > >        */
> > > > > > > > > -     dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > > > > > +             dev_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > > > +     else
> > > > > > > > > +             dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > > > > > +
> > > > > > > > >       if (dev_sof == stream->clock.last_sof)
> > > > > > > > >               return;
> > > > > > > > >
> > > > > > > > >       stream->clock.last_sof = dev_sof;
> > > > > > > > >
> > > > > > > > > -     host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > > > > > +             host_sof = dev_sof;
> > > > > > > > > +     else
> > > > > > > > > +             host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > > >       time = uvc_video_get_time();
> > > > > > > > >
> > > > > > > > >       /* The UVC specification allows device implementations that can't obtain
> > > > > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > > > index cce5e38133cd..89d909661915 100644
> > > > > > > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > > > @@ -209,6 +209,8 @@
> > > > > > > > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > > > > > > > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > > > > > > > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > > > > > > > +#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00002000
> > > > > > > > > +
> > > > > > > > >
> > > > > > > > >  /* Format flags */
> > > > > > > > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

[Ricardo Ribalda]

Hi Laurent

I have taken a data analysis approach to solve this discussion (I
always wanted to say that phrase sorry :P)

I have run a logitech c9222 camera for 5 minutes with yavta. And then
I have subtracted the frame time to its previous frame time using
three different ways to measure the frametime:
- unchanged (the driver as it is today)
- hacked (applying this patch)
- software (uvc_hw_timestamps_param=0)

And I get the following histograms (please note the logarithmic scale
and the different range in the horizontal axis):
https://ibb.co/D1HJJ4x
https://ibb.co/8s9dBdk
https://ibb.co/QC9MgVK
(can share a google sheets with the raw data)

Eventhough anyone can torture the numbers enough to say almost
anything, I think in this case it is clear how, for this specific
device, we should consider applying this patch. Because even a pure
software solution is clearly inferior.

This goes without saying that the vendor shall be pinged. (I am
already working on that).

Best regards and sorry for the stubbornness  ;)

On Mon, 23 Aug 2021 at 12:09, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Aug 23, 2021 at 11:56:43AM +0200, Ricardo Ribalda wrote:
> > On Mon, 23 Aug 2021 at 03:54, Laurent Pinchart wrote:
> > > On Thu, Aug 19, 2021 at 04:46:38PM +0200, Ricardo Ribalda wrote:
> > > > On Thu, 19 Aug 2021 at 16:15, Laurent Pinchart wrote:
> > > > > On Thu, Aug 19, 2021 at 01:31:32PM +0200, Ricardo Ribalda wrote:
> > > > > > On Thu, 19 Aug 2021 at 12:28, Laurent Pinchart wrote:
> > > > > > > On Thu, Aug 19, 2021 at 08:27:00AM +0200, Ricardo Ribalda wrote:
> > > > > > > > On Thu, 19 Aug 2021 at 00:22, Laurent Pinchart wrote:
> > > > > > > > > On Wed, Aug 18, 2021 at 10:35:02PM +0200, Ricardo Ribalda wrote:
> > > > > > > > > > The hardware timestamping code has the assumption than the device_sof
> > > > > > > > > > and the host_sof run at the same frequency (1 KHz).
> > > > > > > > > >
> > > > > > > > > > Unfortunately, this is not the case for all the hardware. Add a quirk to
> > > > > > > > > > support such hardware.
> > > > > > > > > >
> > > > > > > > > > Note on how to identify such hardware:
> > > > > > > > > > When running with "yavta -c /dev/videoX" Look for periodic jumps of the
> > > > > > > > > > fps. Eg:
> > > > > > > > > >
> > > > > > > > > > 30 (6) [-] none 30 614400 B 21.245557 21.395214 34.133 fps ts mono/SoE
> > > > > > > > > > 31 (7) [-] none 31 614400 B 21.275327 21.427246 33.591 fps ts mono/SoE
> > > > > > > > > > 32 (0) [-] none 32 614400 B 21.304739 21.459256 34.000 fps ts mono/SoE
> > > > > > > > > > 33 (1) [-] none 33 614400 B 21.334324 21.495274 33.801 fps ts mono/SoE
> > > > > > > > > > 34 (2) [-] none 34 614400 B 21.529237 21.527297 5.130 fps ts mono/SoE
> > > > > > > > > > 35 (3) [-] none 35 614400 B 21.649416 21.559306 8.321 fps ts mono/SoE
> > > > > > > > > > 36 (4) [-] none 36 614400 B 21.678789 21.595320 34.045 fps ts mono/SoE
> > > > > > > > > > ...
> > > > > > > > > > 99 (3) [-] none 99 614400 B 23.542226 23.696352 33.541 fps ts mono/SoE
> > > > > > > > > > 100 (4) [-] none 100 614400 B 23.571578 23.728404 34.069 fps ts mono/SoE
> > > > > > > > > > 101 (5) [-] none 101 614400 B 23.601425 23.760420 33.504 fps ts mono/SoE
> > > > > > > > > > 102 (6) [-] none 102 614400 B 23.798324 23.796428 5.079 fps ts mono/SoE
> > > > > > > > > > 103 (7) [-] none 103 614400 B 23.916271 23.828450 8.478 fps ts mono/SoE
> > > > > > > > > > 104 (0) [-] none 104 614400 B 23.945720 23.860479 33.957 fps ts mono/SoE
> > > > > > > > > >
> > > > > > > > > > They happen because the delta_sof calculated at
> > > > > > > > > > uvc_video_clock_host_sof(), wraps periodically, as both clocks drift.
> > > > > > > > >
> > > > > > > > > That looks plain wrong. First of all, the whole purpose of the SOF clock
> > > > > > > > > is to have a shared clock between the host and the device. It makes no
> > > > > > > > > sense for a device to have a free-running "SOF" clock. Given the log
> > > > > > > > > above, the issue occurs so quickly that it doesn't seem to be a mere
> > > > > > > > > drift of a free running clock. Could you investigate this more carefully
> > > > > > > > > ?
> > > > > > > >
> > > > > > > > In my test the dev_sof runs at 887.91Hz and the dev_sof at 1000.35Hz.
> > > > > > > > If I plot the difference of both clocks host_sof - (dev_sof % 2048), I
> > > > > > > > get this nice graph https://imgur.com/a/5fQnKa7
> > > > > > > >
> > > > > > > > I agree that it makes not sense to have a free-running "SOF", but the
> > > > > > > > manufacturer thinks otherwise :)
> > > > > > >
> > > > > > > In that case there's no common clock between the device and the host,
> > > > > > > which means that clock recovery is impossible. The whole timestamp
> > > > > > > computation should be bypassed, and the driver should use the system
> > > > > > > timestamp instead.
> > > > > >
> > > > > > Or said differently. The clock recovery is susceptible to the jitter
> > > > > > in the frame acquisition.
> > > > > >
> > > > > > If you have no jitter, the clock recovered will match the reality, and
> > > > > > if you have bad jitter, it will be as bad as system timestamp.
> > > > >
> > > > > The whole point of the clock recovery code is to convert a precise
> > > > > timestamp, expressed using a device clock that the host has no access
> > > > > to, to a system clock. This can only be done if the relationship between
> > > > > the two clocks can be inferred, and the UVC specifies a mechanism to
> > > > > allow this by using a common clock, in the form of the SOF counter. If
> > > > > we don't have that, we're essentially screwed, and can't use the
> > > > > algorithm implemented in the driver at all. I'd much rather skip is
> > > > > completely in that case, instead of trying to hack the algorithm itself.
> > > >
> > > > Considering T(f) as the time between the usb package (f) is received
> > > > and uvc_video_clock_decode()
> > > > If the jitter between the different T(f)s is under one unit of our
> > > > clock (1 msec) the accuracy of the "hacked" algorithm and the real
> > > > algorithm is exactly the same.
> > > >
> > > > We can agree that 1 msec is a "lot" of time. And if our system has a
> > > > worse latency than that, the hacked algorithm will not be worse than
> > > > system timestamping.
> > > >
> > > > So in most of the situations this patch will produce better timestamps
> > > > than the current code and never worse than now...
> > >
> > > How can it produce better timestamps if it's missing the crucial
> > > information that provides the correlation of timestamps between the
> > > device and host side ?
> >
> > Because in a system with a latency jitter under 1msec sof_device and
> > sof_host you already know that information: sof_host = sof_device
>
> Only (100 - jitter/1ms) % of the time.
>
> Given that the kernel implementation of the clock recovery is known to
> cause timestamps to jump back in time once in a while (with devices that
> behave properly), and that this should be implemented in userspace, I'd
> rather bypass the kernel-side clock recovery completely when the device
> doesn't behave. Then, we'll discuss whether it shuold be bypassed in
> userspace too for this device, based on mathematical evidence :-)
>
> > It is a special case of the general problem.
> >
> > > > Anyway, I have tried to ping the vendor to see if there is something
> > > > that I could be doing wrong, lets see what they reply.
> > > >
> > > > > On a side note, I think the whole clock recovery implementation should
> > > > > move from the uvcvideo driver to userspace, where we'll have the ability
> > > > > to perform floating point computation. The kernel implementation is
> > > > > crude, it should be replaced with a linear regression.
> > > >
> > > > Agree, but instead of a linear regression, a resampling algorithm.
> > >
> > > A linear regression is likely a good enough resampling algorithm in this
> > > case, but I'd be curious to see if someone could do better.
> > >
> > > > > > So this patch will still be better than nothing.
> > > > > >
> > > > > > > I still find it hard to believe that a Logitech camera would get this
> > > > > > > wrong.
> > > > > >
> > > > > > I guess I can send you a device, or give you access to mine remotely
> > > > > > if you do not believe me :)
> > > > > >
> > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > > > > ---
> > > > > > > > > > v2: Fix typo in frequency
> > > > > > > > > >
> > > > > > > > > >  drivers/media/usb/uvc/uvc_driver.c |  9 +++++++++
> > > > > > > > > >  drivers/media/usb/uvc/uvc_video.c  | 11 +++++++++--
> > > > > > > > > >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> > > > > > > > > >  3 files changed, 20 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > index 9a791d8ef200..d1e6cba10b15 100644
> > > > > > > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > > > > > > @@ -2771,6 +2771,15 @@ static const struct usb_device_id uvc_ids[] = {
> > > > > > > > > >         .bInterfaceSubClass   = 1,
> > > > > > > > > >         .bInterfaceProtocol   = 0,
> > > > > > > > > >         .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_RESTORE_CTRLS_ON_INIT) },
> > > > > > > > > > +     /* Logitech HD Pro Webcam C922 */
> > > > > > > > > > +     { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > > > > > > +                             | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > > > > > > +       .idVendor             = 0x046d,
> > > > > > > > > > +       .idProduct            = 0x085c,
> > > > > > > > > > +       .bInterfaceClass      = USB_CLASS_VIDEO,
> > > > > > > > > > +       .bInterfaceSubClass   = 1,
> > > > > > > > > > +       .bInterfaceProtocol   = 0,
> > > > > > > > > > +       .driver_info          = UVC_INFO_QUIRK(UVC_QUIRK_INVALID_DEVICE_SOF) },
> > > > > > > > > >       /* Chicony CNF7129 (Asus EEE 100HE) */
> > > > > > > > > >       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > > > > > > > > >                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > > > > > > index 6d0e474671a2..760ab015cf9c 100644
> > > > > > > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > > > > > > @@ -518,13 +518,20 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> > > > > > > > > >       /* To limit the amount of data, drop SCRs with an SOF identical to the
> > > > > > > > > >        * previous one.
> > > > > > > > > >        */
> > > > > > > > > > -     dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > > > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > > > > > > +             dev_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > > > > +     else
> > > > > > > > > > +             dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > > > > > > > > > +
> > > > > > > > > >       if (dev_sof == stream->clock.last_sof)
> > > > > > > > > >               return;
> > > > > > > > > >
> > > > > > > > > >       stream->clock.last_sof = dev_sof;
> > > > > > > > > >
> > > > > > > > > > -     host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > > > > +     if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> > > > > > > > > > +             host_sof = dev_sof;
> > > > > > > > > > +     else
> > > > > > > > > > +             host_sof = usb_get_current_frame_number(stream->dev->udev);
> > > > > > > > > >       time = uvc_video_get_time();
> > > > > > > > > >
> > > > > > > > > >       /* The UVC specification allows device implementations that can't obtain
> > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > > > > index cce5e38133cd..89d909661915 100644
> > > > > > > > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > > > > > > > @@ -209,6 +209,8 @@
> > > > > > > > > >  #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT      0x00000400
> > > > > > > > > >  #define UVC_QUIRK_FORCE_Y8           0x00000800
> > > > > > > > > >  #define UVC_QUIRK_FORCE_BPP          0x00001000
> > > > > > > > > > +#define UVC_QUIRK_INVALID_DEVICE_SOF 0x00002000
> > > > > > > > > > +
> > > > > > > > > >
> > > > > > > > > >  /* Format flags */
> > > > > > > > > >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda