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

[PATCH v2 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>
---
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,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v2 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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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 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;

-- 
2.54.0.rc1.513.gad8abe7a5a-goog

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;
> 

[PATCH v2 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, 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>
---
 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);

-- 
2.54.0.rc1.513.gad8abe7a5a-goog

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,