Received-SPF: pass (google.com: domain of linux-kernel+bounces-193704-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 29 May 2024 17:13:21 +0900
From: Tomasz Figa <tfiga@chromium.org>
To: Ricardo Ribalda <ribalda@chromium.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, 
	Mauro Carvalho Chehab <mchehab@kernel.org>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, 
	"hn.chen" <hn.chen@sunplusit.com>, Hans Verkuil <hverkuil@xs4all.nl>, 
	Sergey Senozhatsky <senozhatsky@chromium.org>
Subject: Re: [PATCH v10 5/6] media: uvcvideo: Refactor clock circular buffer
Message-ID: <manyf44d4ijttnnz2ml52zygg7hvyuhtligivcslx6hjbp7y2u@jqb6fnysnn2a>
References: <20240323-resend-hwtimestamp-v10-0-b08e590d97c7@chromium.org>
 <20240323-resend-hwtimestamp-v10-5-b08e590d97c7@chromium.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240323-resend-hwtimestamp-v10-5-b08e590d97c7@chromium.org>

On Sat, Mar 23, 2024 at 10:48:06AM +0000, 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 83 +++++++++++++++++----------------------
>  1 file changed, 36 insertions(+), 47 deletions(-)
> 

Very nice clean-up. Thanks!

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

Best regards,
Tomasz

> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index af25b9f1b53fe..5df8f61d39cd1 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -466,19 +466,28 @@ 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);
> +
> +	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);
> +}
> +
>  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 +532,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
> @@ -552,12 +561,10 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
>  	 * suffer from this condition.
>  	 */
>  	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
> @@ -567,9 +574,9 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
>  	 * host, but the exact reason hasn't been fully determined.
>  	 */
>  	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
> @@ -592,46 +599,28 @@ 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;
> -
> -	if (stream->clock.count < stream->clock.size)
> -		stream->clock.count++;
> -
> -	spin_unlock_irqrestore(&stream->clock.lock, flags);
> +	sample.dev_sof = (sample.dev_sof + stream->clock.sof_offset) & 2047;
> +	uvc_video_clock_add_sample(&stream->clock, &sample);
> +	stream->clock.last_sof = sample.dev_sof;
>  }
>  
> -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;
>  
> @@ -640,15 +629,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;
>  }
>  
>  /*
> @@ -2123,7 +2112,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;
> @@ -2272,7 +2261,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;
>  
> @@ -2290,7 +2279,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;
>  }
> @@ -2318,5 +2307,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);
>  }
> 
> -- 
> 2.44.0.396.g6e790dbe36-goog
> 
>