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

Message ID 20240323-resend-hwtimestamp-v10-4-b08e590d97c7@chromium.org (mailing list archive)
State New
Delegated to: Laurent Pinchart
Headers
Series uvcvideo: Fixes for hw timestamping |

Commit Message

Ricardo Ribalda March 23, 2024, 10:48 a.m. UTC
  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@sunplusit.com>
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
  

Comments

Tomasz Figa May 29, 2024, 8:03 a.m. UTC | #1
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@sunplusit.com>
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  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.

Anyway:

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz
  
Laurent Pinchart June 10, 2024, 11:43 a.m. UTC | #2
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@sunplusit.com>
> > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  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.

> Anyway:
> 
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
  
Tomasz Figa June 12, 2024, 3:28 a.m. UTC | #3
On Mon, Jun 10, 2024 at 8:43 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@sunplusit.com>
> > > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  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.

Best regards,
Tomasz
  
Laurent Pinchart June 12, 2024, 7:43 a.m. UTC | #4
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@sunplusit.com>
> > > > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  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 :-)
  
Ricardo Ribalda June 12, 2024, 7:47 a.m. UTC | #5
On Wed, 12 Jun 2024 at 09:44, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@sunplusit.com>
> > > > > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > >  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 :-)

We would still have to duplicate the quirk information in libcamera
and the kernel.



>
> --
> Regards,
>
> Laurent Pinchart



--
Ricardo Ribalda
  
Laurent Pinchart June 12, 2024, 8:20 a.m. UTC | #6
On Wed, Jun 12, 2024 at 09:47:26AM +0200, Ricardo Ribalda wrote:
> On Wed, 12 Jun 2024 at 09:44, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> 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@sunplusit.com>
> > > > > > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > ---
> > > > > >  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 :-)
> 
> We would still have to duplicate the quirk information in libcamera
> and the kernel.

Sure, and there will be some level of code duplication. My point is that
I believe it can be done in userspace, and while small changes to the
clock handling on the kernel side are fine, if we wanted to change the
code significantly I think it would be better moved to userspace. I
don't have plans to work on this, and I'm not requesting anyone to do so
either at this point.
  
Tomasz Figa June 12, 2024, 8:35 a.m. UTC | #7
On Wed, Jun 12, 2024 at 4:44 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@sunplusit.com>
> > > > > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > >  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.
  
Tomasz Figa June 12, 2024, 8:47 a.m. UTC | #8
On Wed, Jun 12, 2024 at 5:21 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Jun 12, 2024 at 09:47:26AM +0200, Ricardo Ribalda wrote:
> > On Wed, 12 Jun 2024 at 09:44, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> 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@sunplusit.com>
> > > > > > > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > ---
> > > > > > >  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 :-)
> >
> > We would still have to duplicate the quirk information in libcamera
> > and the kernel.
>
> Sure, and there will be some level of code duplication. My point is that
> I believe it can be done in userspace, and while small changes to the
> clock handling on the kernel side are fine, if we wanted to change the
> code significantly I think it would be better moved to userspace.

Okay, that sounds much more reasonable. I guess I misunderstood your
original reply, sorry.

Best regards,
Tomasz

> I
> don't have plans to work on this, and I'm not requesting anyone to do so
> either at this point.
>
> --
> Regards,
>
> Laurent Pinchart
  
Laurent Pinchart June 12, 2024, 9:21 a.m. UTC | #9
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@ideasonboard.com> 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@sunplusit.com>
> > > > > > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > ---
> > > > > >  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.
  

Patch

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;
+
 	y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
 	  - (u64)y2 * (u64)x1;
 	y = div_u64(y, x2 - x1);