Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220920-resend-hwtimestamp-v2-0-0d7978a817cc@chromium.org>
 <20220920-resend-hwtimestamp-v2-8-0d7978a817cc@chromium.org>
 <Y6761ScB9FgMC6zb@pendragon.ideasonboard.com> <CANiDSCtsk+oejXSDGo_DStocHOEsk4=oYOu6KccQdckfBXbj-g@mail.gmail.com>
 <Y7jCydc6mFvZRQof@pendragon.ideasonboard.com>
In-Reply-To: <Y7jCydc6mFvZRQof@pendragon.ideasonboard.com>
From:   Ricardo Ribalda <ribalda@chromium.org>
Date:   Mon, 9 Jan 2023 09:13:38 +0100
Message-ID: <CANiDSCtyAbqyrE6+R16DeAqghSbu=inkYLR_4VsOCJqUO-B5jw@mail.gmail.com>
Subject: Re: [PATCH RESEND v2 8/8] media: uvcvideo: Fix hw timestampt handling
 for slow FPS
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        "hn.chen" <hn.chen@sunplusit.com>, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Laurent

On Sat, 7 Jan 2023 at 01:54, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Wed, Jan 04, 2023 at 12:34:34AM +0100, Ricardo Ribalda wrote:
> > On Fri, 30 Dec 2022 at 15:51, Laurent Pinchart wrote:
> > > On Fri, Dec 02, 2022 at 06:02:48PM +0100, Ricardo Ribalda wrote:
> > > > In UVC 1.5, when working with FPS under 32, there is a chance that the
> > > > circular buffer contains two dev_sof overflows,
> > >
> > > An explanation of why this is the case would be good in the commit
> > > message. Also, by overflow, are you talking about the SOF counter
> > > rolling over ?
> > >
> > > > but the clock interpolator
> > > > is only capable of handle a single overflow.
> > > >
> > > > Remove all the samples from the circular buffer that are two overflows
> > > > old.
> > >
> > > I would rather support multiple roll-over in the clock interpolator.
> > > Dropping older sampls will lead to less predictable behaviour and harder
> > > to debug issues.
> >
> > Multiple roll-over would not necessarily mean better data here. We are
> > deleting data that is more than 1 second away, and our resolution is
> > 1kHz, which means that we have enough data to provide good results, we
> > have measured that 250msec already provides good data.
>
> Do we ? We may get one SCR per frame only. With low frame rates (say,
> 5fps for instance, which is fairly common, I have 2092 out of 16475
> frame descriptors supporting that format in my database of UVC
> descriptors), we'll have 4 to 5 data points.

In the current algorithm, the accuracy is not given by the number of
samples, but the time between the first and the last sample.
Every sample has an average error error of (1/clkrate)/2, but the
errors do not add up.
This is: 2 samples at 2 fps is as accurate as 4 samples at 4 fps.

>
> > From a logical point of view, this patch is fixing a bug, not adding a
> > new feature, and it has been validated. I would rather add multi
> > roll-over as a follow-up patch, or maybe suggest implementing it in
> > userspace ;).
>
> Would you give the latter a try ? :-)

Aren't these two orthogonal problems? The kernel today provides an API
and that API is broken for fps < 32, which is a common fps.
Even if we re-implement the hwtimestamp in user space we need to fix
it in the kernel.


>
> > > > Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_video.c | 15 +++++++++++++++
> > > >  drivers/media/usb/uvc/uvcvideo.h  |  1 +
> > > >  2 files changed, 16 insertions(+)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index c81a8362d582..6b6bd521d6c2 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -471,6 +471,20 @@ static void uvc_video_clock_add_sample(struct uvc_clock *clock,
> > > >
> > > >       spin_lock_irqsave(&clock->lock, flags);
> > > >
> > > > +     /* Delete last overflows */
> > > > +     if (clock->head == clock->last_sof_overflow)
> > > > +             clock->last_sof_overflow = -1;
> > > > +
> > > > +     /* Handle overflows */
> > > > +     if (clock->count > 0 && clock->last_sof > sample->dev_sof) {
> > > > +             /* Remove data from the last^2 overflows */
> > > > +             if (clock->last_sof_overflow != -1)
> > > > +                     clock->count = (clock->head - clock->last_sof_overflow)
> > > > +                                                             % clock->count;
> > > > +             clock->last_sof_overflow = clock->head;
> > > > +     }
> > > > +
> > > > +     /* Add sample */
> > > >       memcpy(&clock->samples[clock->head], sample, sizeof(*sample));
> > > >       clock->last_sof = sample->dev_sof;
> > > >       clock->head = (clock->head + 1) % clock->size;
> > > > @@ -594,6 +608,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 14daa7111953..d8c520ce5a86 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -647,6 +647,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