diff mbox series

[RESEND,v2,8/8] media: uvcvideo: Fix hw timestampt handling for slow FPS

Message ID 20220920-resend-hwtimestamp-v2-8-0d7978a817cc@chromium.org
State New
Headers
Series uvcvideo: Fixes for hw timestamping |

Commit Message

Ricardo Ribalda Dec. 2, 2022, 5:02 p.m. UTC 
  In UVC 1.5, when working with FPS under 32, there is a chance that the
circular buffer contains two dev_sof overflows, 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.

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(+)

Comments

Laurent Pinchart Dec. 30, 2022, 2:51 p.m. UTC | #1 
Hi Ricardo,

Thank you for the patch.

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.

> 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;
Ricardo Ribalda Jan. 3, 2023, 11:34 p.m. UTC | #2 
Hi Laurent

On Fri, 30 Dec 2022 at 15:51, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> 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.

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 ;).

> > 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
Laurent Pinchart Jan. 7, 2023, 12:54 a.m. UTC | #3 
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.

> 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 ? :-)

> > > 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;
Ricardo Ribalda Jan. 9, 2023, 8:13 a.m. UTC | #4 
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
diff mbox series

Patch

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;