| Message ID | 20240323-resend-hwtimestamp-v10-4-b08e590d97c7@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | uvcvideo: Fixes for hw timestamping | expand |
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
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>
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
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 :-)
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
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.
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.
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
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.
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);