Message-ID: <CANiDSCuvRyAbf9YCNusR8bnRRJf9QE6Yb385oJ4J=OyZCVueGQ@mail.gmail.com>
Date:   Thu, 26 Aug 2021 20:18:22 +0200
From:   Ricardo Ribalda <ribalda@...omium.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        Tomasz Figa <tfiga@...omium.org>
Subject: Re: [PATCH v2] media: uvcvideo: Quirk for hardware with invalid sof

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@...asonboard.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@...omium.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

Powered by blists - more mailing lists