Re: [PATCH v2 1/2] media: uvcvideo: Do not open code uvc_queue_get_current_buffer

Re: [PATCH v2 1/2] media: uvcvideo: Do not open code uvc_queue_get_current_buffer

[Hans de Goede]

Hi,

On 17-Apr-26 07:19, Ricardo Ribalda wrote:
> Do not re-implement uvc_queue_get_current_buffer() logic inside
> uvc_video_complete(), just call the function.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> ---
>  drivers/media/usb/uvc/uvc_video.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 271c246a02ea..5d45c74c6041 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1704,7 +1704,6 @@ static void uvc_video_complete(struct urb *urb)
>  	struct vb2_queue *vb2_qmeta = stream->meta.queue.vdev.queue;
>  	struct uvc_buffer *buf = NULL;
>  	struct uvc_buffer *buf_meta = NULL;
> -	unsigned long flags;
>  	int ret;
>  
>  	switch (urb->status) {
> @@ -1730,13 +1729,8 @@ static void uvc_video_complete(struct urb *urb)
>  
>  	buf = uvc_queue_get_current_buffer(queue);
>  
> -	if (vb2_qmeta) {
> -		spin_lock_irqsave(&qmeta->irqlock, flags);
> -		if (!list_empty(&qmeta->irqqueue))
> -			buf_meta = list_first_entry(&qmeta->irqqueue,
> -						    struct uvc_buffer, queue);
> -		spin_unlock_irqrestore(&qmeta->irqlock, flags);
> -	}
> +	if (vb2_qmeta)
> +		buf_meta = uvc_queue_get_current_buffer(qmeta);
>  
>  	/* Re-initialise the URB async work. */
>  	uvc_urb->async_operations = 0;
> 

Re: [PATCH v2 2/2] media: uvcvideo: Avoid partial metadata buffers

[Hans de Goede]

Hi,

On 17-Apr-26 07:19, Ricardo Ribalda wrote:
> If the metadata queue that is empty receives a new buffer while we are
> in the middle of processing a frame, the first metadata buffer will
> contain partial information.
> 
> Avoid this by tracking the state of the metadata buffer and making sure
> that it is in sync with the data buffer.
> 
> Now that we are at it, make sure that we skip buffers of size 1 or 0.
> They are not allowed by the spec... but it is a simple check to add and
> better be safe than sorry.
> 
> Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans




> ---
>  drivers/media/usb/uvc/uvc_video.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 5d45c74c6041..cd77ca6f8136 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1149,7 +1149,9 @@ static void uvc_video_stats_stop(struct uvc_streaming *stream)
>   * uvc_video_decode_end will never be called with a NULL buffer.
>   */
>  static int uvc_video_decode_start(struct uvc_streaming *stream,
> -		struct uvc_buffer *buf, const u8 *data, int len)
> +				  struct uvc_buffer *buf,
> +				  struct uvc_buffer *meta_buf,
> +				  const u8 *data, int len)
>  {
>  	u8 header_len;
>  	u8 fid;
> @@ -1278,6 +1280,8 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>  
>  		/* TODO: Handle PTS and SCR. */
>  		buf->state = UVC_BUF_STATE_ACTIVE;
> +		if (meta_buf)
> +			meta_buf->state = UVC_BUF_STATE_ACTIVE;
>  	}
>  
>  	stream->last_fid = fid;
> @@ -1435,7 +1439,7 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
>  	ktime_t time;
>  	const u8 *scr;
>  
> -	if (!meta_buf || length == 2)
> +	if (length <= 2 || !meta_buf || meta_buf->state != UVC_BUF_STATE_ACTIVE)
>  		return;
>  
>  	has_pts = mem[1] & UVC_STREAM_PTS;
> @@ -1552,7 +1556,7 @@ static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
>  		/* Decode the payload header. */
>  		mem = urb->transfer_buffer + urb->iso_frame_desc[i].offset;
>  		do {
> -			ret = uvc_video_decode_start(stream, buf, mem,
> +			ret = uvc_video_decode_start(stream, buf, meta_buf, mem,
>  				urb->iso_frame_desc[i].actual_length);
>  			if (ret == -EAGAIN)
>  				uvc_video_next_buffers(stream, &buf, &meta_buf);
> @@ -1601,7 +1605,8 @@ static void uvc_video_decode_bulk(struct uvc_urb *uvc_urb,
>  	 */
>  	if (stream->bulk.header_size == 0 && !stream->bulk.skip_payload) {
>  		do {
> -			ret = uvc_video_decode_start(stream, buf, mem, len);
> +			ret = uvc_video_decode_start(stream, buf, meta_buf, mem,
> +						     len);
>  			if (ret == -EAGAIN)
>  				uvc_video_next_buffers(stream, &buf, &meta_buf);
>  		} while (ret == -EAGAIN);
> 

Re: [PATCH v2 0/2] media: uvcvideo: Avoid partial metadata buffers

[Hans de Goede]

Hi,

On 17-Apr-26 07:19, Ricardo Ribalda wrote:
> The current code can lead to partial metadata buffers when the metadata
> queue transitions from empty to ready. Fix that.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

I've merged this in my local uvc/for-next branch.

I'll push this out to gitlab for CI later today.

Regards,

Hans



> ---
> Changes in v2: (Thanks Laurent)
> - Transition to UVC_BUF_STATE_ACTIVE with the data buffer
> - Link to v1: https://lore.kernel.org/r/20260415-uvc-meta-partial-v1-0-a0acc79a6300@chromium.org
> 
> ---
> Ricardo Ribalda (2):
>       media: uvcvideo: Do not open code uvc_queue_get_current_buffer
>       media: uvcvideo: Avoid partial metadata buffers
> 
>  drivers/media/usb/uvc/uvc_video.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> ---
> base-commit: 2e9a8a967f836cf879f35c7434025de265826cc1
> change-id: 20260415-uvc-meta-partial-a5767866d0e0
> 
> Best regards,