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

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

[Ricardo Ribalda]

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>
---
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 | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)
---
base-commit: 4fbeef21f5387234111b5d52924e77757626faa5
change-id: 20260415-uvc-meta-partial-a5767866d0e0

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

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

[Ricardo Ribalda]

Do not re-implement uvc_queue_get_current_buffer() logic inside
uvc_video_complete(), just call the function.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 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 40c76c051da2..4feb3699f520 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1693,7 +1693,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) {
@@ -1719,13 +1718,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;

-- 
2.54.0.rc1.513.gad8abe7a5a-goog

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

[Ricardo Ribalda]

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;
+	}
+
 	/* Re-initialise the URB async work. */
 	uvc_urb->async_operations = 0;
 

-- 
2.54.0.rc1.513.gad8abe7a5a-goog

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

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Wed, Apr 15, 2026 at 03:59:57PM +0000, Ricardo Ribalda wrote:
> Do not re-implement uvc_queue_get_current_buffer() logic inside
> uvc_video_complete(), just call the function.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  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 40c76c051da2..4feb3699f520 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1693,7 +1693,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) {
> @@ -1719,13 +1718,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;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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