Re: [PATCH v10 4/6] media: uvcvideo: Allow hw clock updates with buffers not full

[Laurent Pinchart]

On Wed, Jun 12, 2024 at 05:35:38PM +0900, Tomasz Figa wrote:
> On Wed, Jun 12, 2024 at 4:44 PM Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Jun 12, 2024 at 12:28:56PM +0900, Tomasz Figa wrote:
> > > On Mon, Jun 10, 2024 at 8:43 PM Laurent Pinchart wrote:
> > > > On Wed, May 29, 2024 at 05:03:08PM +0900, Tomasz Figa wrote:
> > > > > On Sat, Mar 23, 2024 at 10:48:05AM +0000, Ricardo Ribalda wrote:
> > > > > > With UVC 1.5 we get as little as one clock sample per frame. Which means
> > > > > > that it takes 32 frames to move from the software timestamp to the
> > > > > > hardware timestamp method.
> > > > > >
> > > > > > This results in abrupt changes in the timestamping after 32 frames (~1
> > > > > > second), resulting in noticeable artifacts when used for encoding.
> > > > > >
> > > > > > With this patch we modify the update algorithm to work with whatever
> > > > > > amount of values are available.
> > > > > >
> > > > > > Tested-by: HungNien Chen <hn.chen@xxxxxxxxxxxxx>
> > > > > > Reviewed-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > > > > ---
> > > > > >  drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++--
> > > > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > > index d6ca383f643e3..af25b9f1b53fe 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > > @@ -768,10 +768,10 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
> > > > > >
> > > > > >     spin_lock_irqsave(&clock->lock, flags);
> > > > > >
> > > > > > -   if (clock->count < clock->size)
> > > > > > +   if (clock->count < 2)
> > > > > >             goto done;
> > > > > >
> > > > > > -   first = &clock->samples[clock->head];
> > > > > > +   first = &clock->samples[(clock->head - clock->count + clock->size) % clock->size];
> > > > > >     last = &clock->samples[(clock->head - 1 + clock->size) % clock->size];
> > > > > >
> > > > > >     /* First step, PTS to SOF conversion. */
> > > > > > @@ -786,6 +786,18 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
> > > > > >     if (y2 < y1)
> > > > > >             y2 += 2048 << 16;
> > > > > >
> > > > > > +   /*
> > > > > > +    * Have at least 1/4 of a second of timestamps before we
> > > > > > +    * try to do any calculation. Otherwise we do not have enough
> > > > > > +    * precision. This value was determined by running Android CTS
> > > > > > +    * on different devices.
> > > > > > +    *
> > > > > > +    * dev_sof runs at 1KHz, and we have a fixed point precision of
> > > > > > +    * 16 bits.
> > > > > > +    */
> > > > > > +   if ((y2 - y1) < ((1000 / 4) << 16))
> > > > > > +           goto done;
> > > > >
> > > > > Not a comment for this patch directly, but...
> > > > >
> > > > > This kind of makes me wonder if we don't want to have some documentation
> > > > > that specifies what the userspace can expect from the timestamps, so
> > > > > that this isn't changed randomly in the future breaking what was fixed
> > > > > by this patch.
> > > >
> > > > I think timestamp handling should really be moved to userspace. It will
> > > > be easier to handle with floating-point arithmetic there. That would
> > > > have been difficult to manage for applications a while ago, but now that
> > > > we have libcamera, we could implement it there. This isn't high on my
> > > > todo list though.
> > >
> > > While indeed that would probably be a better way to handle the complex
> > > logic if we designed the driver today, we already have userspace that
> > > expects the timestamps to be handled correctly in the kernel. I
> > > suspect moving it to the userspace would require some core V4L2
> > > changes to define a new timestamp handling mode, where multiple raw
> > > hardware timestamps are exposed to the userspace, instead of the high
> > > level system monotonic one.
> >
> > The uvcvideo driver already supports exposing the packet headers to
> > userspace through a metadata capture device, so I think we have all the
> > components we need. The only missing thing would be the implementation
> > in libcamera :-)
> 
> Okay, I see. That would make it easier indeed. :)
> 
> That said, we still need to provide some valid timestamps in the
> v4l2_buffer struct returned from DQBUF, as per the current API
> contract, so we can't simply remove the timestamp handling code from
> the kernel completely.

We could pass the system timestamp, the same way the driver used to
before getting clock recovery support. That being said, I'm not calling
for this code to be dropped overnight, it can stay there.

-- 
Regards,

Laurent Pinchart