Message ID | 20240323-resend-hwtimestamp-v10-6-b08e590d97c7@chromium.org (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | uvcvideo: Fixes for hw timestamping | expand |
On Sat, Mar 23, 2024 at 10:48:07AM +0000, 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. > > Remove all the samples from the circular buffer that are more than two > rollovers old, so the algorithm always provides good timestamps. > > 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@sunplusit.com> > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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 5df8f61d39cd1..900b57afac93a 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 */ > clock->samples[clock->head] = *sample; > clock->head = (clock->head + 1) % clock->size; > clock->count = min(clock->count + 1, clock->size); > @@ -616,6 +639,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 cb9dd50bba8ac..fb9f9771131ac 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; > > -- > 2.44.0.396.g6e790dbe36-goog > > Given that majority of cameras kind of run ~30 fps, this should improve the timestamps for quite a lot of the users. Thanks. Reviewed-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz
Hi Ricardo, Thank you for the patch. On Sat, Mar 23, 2024 at 10:48:07AM +0000, 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. > > Remove all the samples from the circular buffer that are more than two > rollovers old, so the algorithm always provides good timestamps. > > 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@sunplusit.com> > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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 5df8f61d39cd1..900b57afac93a 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 s/overflow/SOF overflow/ otherwise it sounds like a circular buffer overflow. Same in the other comments below. > + * 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 */ s/overflows/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; If I'm following you correctly here, you want to set count to the distance between last_sof_overflow and head. Shouldn't it be clock->count = (clock->head - clock->last_sof_overflow + clock->size) % clock->size; > + } No need for curly braces. > + clock->last_sof_overflow = clock->head; > + } > + > + /* Add sample */ s/sample/sample./ I still think it would be nicer to handle multiple rollovers correctly, but that probably better handled by moving all the clock handling to userspace. With the above issues addressed, I think this patch can go in. I could address all these issues when applying, but I'd like the count -> size change to be tested first. You can submit a new version of this patch only, I've applied the rest of the series to my tree already. > clock->samples[clock->head] = *sample; > clock->head = (clock->head + 1) % clock->size; > clock->count = min(clock->count + 1, clock->size); > @@ -616,6 +639,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 cb9dd50bba8ac..fb9f9771131ac 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;
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 5df8f61d39cd1..900b57afac93a 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 */ clock->samples[clock->head] = *sample; clock->head = (clock->head + 1) % clock->size; clock->count = min(clock->count + 1, clock->size); @@ -616,6 +639,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 cb9dd50bba8ac..fb9f9771131ac 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;