lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Fri, 22 Mar 2024 10:53:42 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, Sergey Senozhatsky <senozhatsky@...omium.org>, 
	linux-kernel@...r.kernel.org, "hn.chen" <hn.chen@...plusit.com>, 
	linux-media@...r.kernel.org
Subject: Re: [PATCH v9 6/6] media: uvcvideo: Fix hw timestamp handling for
 slow FPS

Hi

On Fri, 22 Mar 2024 at 10:35, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Wed, Mar 15, 2023 at 02:30:17PM +0100, Ricardo Ribalda wrote:
> > In UVC 1.5 we get a single clock value per frame. With the current
> > buffer size of 32, FPS slowers than 32 might roll-over twice.
> >
> > The current code cannot handle two roll-over and provide invalid
> > timestamps.
> >
> > Revome all the samples from the circular buffer that are more than two
>
> s/Revome/Remove/
>
> > rollovers old, so the algorithm always provides good timestamps.
>
> Wouldn't it be better to support multiple rollovers instead ?

I believe one second is enough to provide a good ramp for the clock
interpolation,
with as little as 1/4 we are getting results good enough to pass CTS.

To support multiple roll-ups we would need to keep track of the
"generation" of every timestamp, and numerical overflows will start to
be an issue....

IMO it is better to fix what we have broken and If we ever need more
accuracy we could add a follow-up patch later.


>
> > Note that we are removing values that are more than one second old,
> > which means that there is enough distance between the two points that
> > we use for the interpolation to provide good values.
> >
> > Tested-by: HungNien Chen <hn.chen@...plusit.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 24 ++++++++++++++++++++++++
> >  drivers/media/usb/uvc/uvcvideo.h  |  1 +
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 1db0d1bc80e6..c58b51207be6 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -471,8 +471,31 @@ static void uvc_video_clock_add_sample(struct uvc_clock *clock,
> >  {
> >       unsigned long flags;
> >
> > +     /*
> > +      * If we write new data on the position where we had the last
> > +      * overflow, remove the overflow pointer. There is no overflow
> > +      * on the whole circular buffer.
> > +      */
> > +     if (clock->head == clock->last_sof_overflow)
> > +             clock->last_sof_overflow = -1;
> > +
> >       spin_lock_irqsave(&clock->lock, flags);
> >
> > +     /* Handle overflows */
> > +     if (clock->count > 0 && clock->last_sof > sample->dev_sof) {
> > +             /*
> > +              * Remove data from the circular buffer that is older than the
> > +              * last overflow. We only support one overflow per circular
> > +              * buffer.
> > +              */
> > +             if (clock->last_sof_overflow != -1) {
> > +                     clock->count = (clock->head - clock->last_sof_overflow
> > +                                     + clock->count) % clock->count;
> > +             }
> > +             clock->last_sof_overflow = clock->head;
> > +     }
> > +
> > +     /* Add sample */
> >       memcpy(&clock->samples[clock->head], sample, sizeof(*sample));
> >       clock->head = (clock->head + 1) % clock->size;
> >       clock->count = min(clock->count + 1, clock->size);
> > @@ -605,6 +628,7 @@ static void uvc_video_clock_reset(struct uvc_clock *clock)
> >       clock->head = 0;
> >       clock->count = 0;
> >       clock->last_sof = -1;
> > +     clock->last_sof_overflow = -1;
> >       clock->sof_offset = -1;
> >  }
> >
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 07b2fdb80adf..bf9f5162b833 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -499,6 +499,7 @@ struct uvc_streaming {
> >               unsigned int head;
> >               unsigned int count;
> >               unsigned int size;
> > +             unsigned int last_sof_overflow;
> >
> >               u16 last_sof;
> >               u16 sof_offset;
> >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ