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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ricardo,

Thank you for the patch.

On Wed, Apr 15, 2026 at 03:59:58PM +0000, 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, simplify the code a bit by not getting a metadata
> buffer if there is no data buffer ready.
> 
> Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 4feb3699f520..f339d6d19a39 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1517,6 +1517,8 @@ static void uvc_video_next_buffers(struct uvc_streaming *stream,
>  						  *meta_buf);
>  	}
>  	*video_buf = uvc_queue_next_buffer(&stream->queue, *video_buf);
> +	if (*video_buf && *meta_buf)
> +		(*meta_buf)->state = UVC_BUF_STATE_ACTIVE;
>  }
>  
>  static void uvc_video_decode_isoc(struct uvc_urb *uvc_urb,
> @@ -1718,9 +1720,23 @@ static void uvc_video_complete(struct urb *urb)
>  
>  	buf = uvc_queue_get_current_buffer(queue);
>  
> -	if (vb2_qmeta)
> +	if (buf && vb2_qmeta)
>  		buf_meta = uvc_queue_get_current_buffer(qmeta);
>  
> +	/*
> +	 * Avoid partial metadata buffers, by making sure that the data buffer
> +	 * and metadata buffer state is in sync.
> +	 *
> +	 * A new (QUEUED) buffer is only allowed to become ACTIVE if we are also
> +	 * at the start of a new data buffer.
> +	 */
> +	if (buf_meta && buf_meta->state == UVC_BUF_STATE_QUEUED) {
> +		if (buf->state != UVC_BUF_STATE_QUEUED)
> +			buf_meta = NULL;
> +		else
> +			buf_meta->state = UVC_BUF_STATE_ACTIVE;
> +	}
> +

This creates a different logic for video buffers and metadata buffers.
Wouldn't it be better to use the same logic, and transition the metadata
buffer to the ACTIVE state when we detect the beginning of a new frame ?

>  	/* Re-initialise the URB async work. */
>  	uvc_urb->async_operations = 0;
>  

-- 
Regards,

Laurent Pinchart