From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Ricardo Ribalda <ribalda@chromium.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
Sergey Senozhatsky <senozhatsky@chromium.org>,
linux-kernel@vger.kernel.org, "hn.chen" <hn.chen@sunplusit.com>,
linux-media@vger.kernel.org
Subject: Re: [PATCH v9 5/6] media: uvcvideo: Refactor clock circular buffer
Date: Fri, 22 Mar 2024 02:36:31 +0200 [thread overview]
Message-ID: <20240322003631.GE20938@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20220920-resend-hwtimestamp-v9-5-55a89f46f6be@chromium.org>
Hi Ricardo,
Thank you for the patch.
On Wed, Mar 15, 2023 at 02:30:16PM +0100, Ricardo Ribalda wrote:
> Isolate all the changes related to the clock circular buffer to its own
> function, that way we can make changes easier to the buffer logic.
>
> Also simplify the lock, by removing the circular buffer clock handling
> from uvc_video_clock_decode().
>
> And now that we are at it, unify the API of the clock functions.
>
> 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 | 84 ++++++++++++++++++---------------------
> 1 file changed, 38 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 6d0243ea0e07..1db0d1bc80e6 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -466,19 +466,30 @@ static inline ktime_t uvc_video_get_time(void)
> return ktime_get_real();
> }
>
> +static void uvc_video_clock_add_sample(struct uvc_clock *clock,
> + const struct uvc_clock_sample *sample)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&clock->lock, flags);
> +
> + memcpy(&clock->samples[clock->head], sample, sizeof(*sample));
How about
clock->samples[clock->head] = *sample;
?
> + clock->head = (clock->head + 1) % clock->size;
> + clock->count = min(clock->count + 1, clock->size);
> +
> + spin_unlock_irqrestore(&clock->lock, flags);
> +
> + clock->last_sof = sample->dev_sof;
Strictly speaking I wonder if this counts as "adding a sample". Any
objection against keeping it in the caller ?
> +}
> +
> static void
> uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> const u8 *data, int len)
> {
> - struct uvc_clock_sample *sample;
> + struct uvc_clock_sample sample;
> unsigned int header_size;
> bool has_pts = false;
> bool has_scr = false;
> - unsigned long flags;
> - ktime_t time;
> - u16 host_sof;
> - u16 dev_sof;
> - u32 dev_stc;
>
> switch (data[1] & (UVC_STREAM_PTS | UVC_STREAM_SCR)) {
> case UVC_STREAM_PTS | UVC_STREAM_SCR:
> @@ -523,11 +534,11 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> * all the data packets of the same frame contains the same SOF. In that
> * case only the first one will match the host_sof.
> */
> - dev_sof = get_unaligned_le16(&data[header_size - 2]);
> - if (dev_sof == stream->clock.last_sof)
> + sample.dev_sof = get_unaligned_le16(&data[header_size - 2]);
> + if (sample.dev_sof == stream->clock.last_sof)
> return;
>
> - dev_stc = get_unaligned_le32(&data[header_size - 6]);
> + sample.dev_stc = get_unaligned_le32(&data[header_size - 6]);
>
> /*
> * STC (Source Time Clock) is the clock used by the camera. The UVC 1.5
> @@ -541,21 +552,19 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> * and stc and sof are zero.
> */
> if (buf && buf->bytesused == 0 && len == header_size &&
> - dev_stc == 0 && dev_sof == 0)
> + sample.dev_stc == 0 && sample.dev_sof == 0)
> return;
>
> - stream->clock.last_sof = dev_sof;
> -
> - host_sof = usb_get_current_frame_number(stream->dev->udev);
> + sample.host_sof = usb_get_current_frame_number(stream->dev->udev);
>
> /*
> * On some devices, like the Logitech C922, the device SOF does not run
> * at a stable rate of 1kHz. For those devices use the host SOF instead.
> */
> if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> - dev_sof = host_sof;
> + sample.dev_sof = sample.host_sof;
>
> - time = uvc_video_get_time();
> + sample.host_time = uvc_video_get_time();
>
> /*
> * The UVC specification allows device implementations that can't obtain
> @@ -578,46 +587,29 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> * the 8 LSBs of the delta are kept.
> */
> if (stream->clock.sof_offset == (u16)-1) {
> - u16 delta_sof = (host_sof - dev_sof) & 255;
> + u16 delta_sof = (sample.host_sof - sample.dev_sof) & 255;
> if (delta_sof >= 10)
> stream->clock.sof_offset = delta_sof;
> else
> stream->clock.sof_offset = 0;
> }
>
> - dev_sof = (dev_sof + stream->clock.sof_offset) & 2047;
> -
> - spin_lock_irqsave(&stream->clock.lock, flags);
> -
> - sample = &stream->clock.samples[stream->clock.head];
> - sample->dev_stc = dev_stc;
> - sample->dev_sof = dev_sof;
> - sample->host_sof = host_sof;
> - sample->host_time = time;
> -
> - /* Update the sliding window head and count. */
> - stream->clock.head = (stream->clock.head + 1) % stream->clock.size;
> + sample.dev_sof = (sample.dev_sof + stream->clock.sof_offset) & 2047;
> + sample.dev_stc = get_unaligned_le32(&data[header_size - 6]);
This is already done above.
With these small issues addressed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> - if (stream->clock.count < stream->clock.size)
> - stream->clock.count++;
> -
> - spin_unlock_irqrestore(&stream->clock.lock, flags);
> + uvc_video_clock_add_sample(&stream->clock, &sample);
> }
>
> -static void uvc_video_clock_reset(struct uvc_streaming *stream)
> +static void uvc_video_clock_reset(struct uvc_clock *clock)
> {
> - struct uvc_clock *clock = &stream->clock;
> -
> clock->head = 0;
> clock->count = 0;
> clock->last_sof = -1;
> clock->sof_offset = -1;
> }
>
> -static int uvc_video_clock_init(struct uvc_streaming *stream)
> +static int uvc_video_clock_init(struct uvc_clock *clock)
> {
> - struct uvc_clock *clock = &stream->clock;
> -
> spin_lock_init(&clock->lock);
> clock->size = 32;
>
> @@ -626,15 +618,15 @@ static int uvc_video_clock_init(struct uvc_streaming *stream)
> if (clock->samples == NULL)
> return -ENOMEM;
>
> - uvc_video_clock_reset(stream);
> + uvc_video_clock_reset(clock);
>
> return 0;
> }
>
> -static void uvc_video_clock_cleanup(struct uvc_streaming *stream)
> +static void uvc_video_clock_cleanup(struct uvc_clock *clock)
> {
> - kfree(stream->clock.samples);
> - stream->clock.samples = NULL;
> + kfree(clock->samples);
> + clock->samples = NULL;
> }
>
> /*
> @@ -2108,7 +2100,7 @@ int uvc_video_resume(struct uvc_streaming *stream, int reset)
>
> stream->frozen = 0;
>
> - uvc_video_clock_reset(stream);
> + uvc_video_clock_reset(&stream->clock);
>
> if (!uvc_queue_streaming(&stream->queue))
> return 0;
> @@ -2257,7 +2249,7 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> {
> int ret;
>
> - ret = uvc_video_clock_init(stream);
> + ret = uvc_video_clock_init(&stream->clock);
> if (ret < 0)
> return ret;
>
> @@ -2275,7 +2267,7 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> error_video:
> usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> error_commit:
> - uvc_video_clock_cleanup(stream);
> + uvc_video_clock_cleanup(&stream->clock);
>
> return ret;
> }
> @@ -2303,5 +2295,5 @@ void uvc_video_stop_streaming(struct uvc_streaming *stream)
> usb_clear_halt(stream->dev->udev, pipe);
> }
>
> - uvc_video_clock_cleanup(stream);
> + uvc_video_clock_cleanup(&stream->clock);
> }
>
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2024-03-22 0:36 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-15 13:30 [PATCH v9 0/6] uvcvideo: Fixes for hw timestamping Ricardo Ribalda
2023-03-15 13:30 ` [PATCH v9 1/6] media: uvcvideo: Fix negative modulus calculation Ricardo Ribalda
2023-03-16 3:05 ` Sergey Senozhatsky
2024-02-12 22:59 ` Laurent Pinchart
2024-02-19 10:28 ` Ricardo Ribalda
2024-02-19 10:40 ` Laurent Pinchart
2024-02-19 15:07 ` Ricardo Ribalda
2024-03-21 21:50 ` Laurent Pinchart
2024-03-22 9:19 ` Laurent Pinchart
2024-03-22 9:29 ` Ricardo Ribalda
2024-03-22 9:35 ` Laurent Pinchart
2023-03-15 13:30 ` [PATCH v9 2/6] media: uvcvideo: Ignore empty TS packets Ricardo Ribalda
2024-03-21 23:26 ` Laurent Pinchart
2024-03-22 8:22 ` Ricardo Ribalda
2024-03-22 8:52 ` Laurent Pinchart
2023-03-15 13:30 ` [PATCH v9 3/6] media: uvcvideo: Quirk for invalid dev_sof in Logitech C922 Ricardo Ribalda
2024-03-21 23:44 ` Laurent Pinchart
2024-03-22 8:32 ` Ricardo Ribalda
2024-03-22 8:52 ` Laurent Pinchart
2023-03-15 13:30 ` [PATCH v9 4/6] media: uvcvideo: Allow hw clock updates with buffers not full Ricardo Ribalda
2023-03-16 3:08 ` Sergey Senozhatsky
2024-03-22 0:12 ` Laurent Pinchart
2023-03-15 13:30 ` [PATCH v9 5/6] media: uvcvideo: Refactor clock circular buffer Ricardo Ribalda
2024-03-22 0:36 ` Laurent Pinchart [this message]
2023-03-15 13:30 ` [PATCH v9 6/6] media: uvcvideo: Fix hw timestamp handling for slow FPS Ricardo Ribalda
2023-03-16 3:07 ` Sergey Senozhatsky
2024-03-22 9:35 ` Laurent Pinchart
2024-03-22 9:53 ` Ricardo Ribalda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240322003631.GE20938@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=hn.chen@sunplusit.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=ribalda@chromium.org \
--cc=senozhatsky@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).