[PATCH v4 0/5] media: uvcvideo: use vb2 ioctl and fop helpers

[PATCH v4 0/5] media: uvcvideo: use vb2 ioctl and fop helpers

[Ricardo Ribalda]

This is a rebump of a 4 years old patch from Hans.
https://lore.kernel.org/linux-media/20210618122923.385938-21-ribalda@chromium.org/

It brings "new" helpers to the uvcdriver and removes tons of code.

The patch:
media: uvcvideo: Refactor uvc_queue_streamon
Is already in the uvc tree. It is here just for CI purposes, do not
review.

I have uploaded my working tree at:
https://gitlab.freedesktop.org/linux-media/users/ribalda/-/commits/b4/uvc-fop
which shows the differences from the original patch, this is mainly for
helping the review to people familiar with the previous patch.

The patch:
"media: uvcvideo: Use prio state from v4l2_device"
is just for RFC, the set can land without it.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v4:
- Improve commit messages
- Move prio changes to a different patch
- Add missing lock
- Remove redundant file->private_data = NULL;
- Link to v3: https://lore.kernel.org/r/20250602-uvc-fop-v3-0-a99e18f65640@chromium.org

Changes in v3:
- Refactor start/stop_streaming(): make meta and video versions
- Link to v2: https://lore.kernel.org/r/20250602-uvc-fop-v2-0-508a293eae81@chromium.org

Changes in v2, Thanks HansV:
- Fix typos
- Use start_streaming and stop_streaming for managing pm
- Link to v1: https://lore.kernel.org/r/20250522-uvc-fop-v1-0-3bfe7a00f31d@chromium.org

---
Hans Verkuil (1):
      media: uvcvideo: Use vb2 ioctl and fop helpers

Ricardo Ribalda (4):
      media: uvcvideo: Handle locks in uvc_queue_return_buffers
      media: uvcvideo: Split uvc_stop_streaming()
      media: uvcvideo: Remove stream->is_streaming field
      media: uvcvideo: Use prio state from v4l2_device

 drivers/media/usb/uvc/uvc_driver.c   |  39 ++---
 drivers/media/usb/uvc/uvc_metadata.c |   8 +-
 drivers/media/usb/uvc/uvc_queue.c    | 194 +++++------------------
 drivers/media/usb/uvc/uvc_v4l2.c     | 293 ++---------------------------------
 drivers/media/usb/uvc/uvcvideo.h     |  38 +----
 5 files changed, 68 insertions(+), 504 deletions(-)
---
base-commit: def55d9b22d294e47a2b4c9eb09a0e9faade7ae7
change-id: 20250521-uvc-fop-b74e9007dd51

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

[PATCH v4 1/5] media: uvcvideo: Use vb2 ioctl and fop helpers

[Ricardo Ribalda]

From: Hans Verkuil <hans@jjverkuil.nl>

When uvc was written the vb2 ioctl and file operation helpers didn't exist.

This patch switches uvc over to those helpers, which removes a lot of
boilerplate code and allows us to drop the 'privileges' scheme, since
that's now handled inside the vb2 helpers.

This makes it possible for uvc to fix the v4l2-compliance streaming tests:
 warn: v4l2-test-formats.cpp(1075): Could not set fmt2

This patch introduces a change on behavior on the uvcdriver to be
aligned with the rest of the subsystem. Now S_INPUT, S_PARM and
S_FORMAT do no grant exclusive ownership of the device.

There are other side effects, some better than others:
- Locking is now more coarse than before, the queue is locked for almost
  every ioctl.
- vidioc_querybuf() can now work when the queue is busy.

Future patches should look into the locking architecture of UVC to
remove one of stream->mutex or queue->mutex.

Reviewed-by: Hans de Goede <hansg@kernel.org>
Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Hans Verkuil <hans@jjverkuil.nl>
Co-developed-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c   |  37 ++---
 drivers/media/usb/uvc/uvc_metadata.c |   8 +-
 drivers/media/usb/uvc/uvc_queue.c    | 143 --------------------
 drivers/media/usb/uvc/uvc_v4l2.c     | 252 +++--------------------------------
 drivers/media/usb/uvc/uvcvideo.h     |  36 +----
 5 files changed, 34 insertions(+), 442 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 62eb45435d8bec5c955720ecb46fb8936871e6cc..accfb4ca3c72cb899185ddc8ecf4e29143d58fc6 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1961,31 +1961,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
 		if (!video_is_registered(&stream->vdev))
 			continue;
 
-		/*
-		 * For stream->vdev we follow the same logic as:
-		 * vb2_video_unregister_device().
-		 */
-
-		/* 1. Take a reference to vdev */
-		get_device(&stream->vdev.dev);
-
-		/* 2. Ensure that no new ioctls can be called. */
-		video_unregister_device(&stream->vdev);
-
-		/* 3. Wait for old ioctls to finish. */
-		mutex_lock(&stream->mutex);
-
-		/* 4. Stop streaming. */
-		uvc_queue_release(&stream->queue);
-
-		mutex_unlock(&stream->mutex);
-
-		put_device(&stream->vdev.dev);
-
-		/*
-		 * For stream->meta.vdev we can directly call:
-		 * vb2_video_unregister_device().
-		 */
+		vb2_video_unregister_device(&stream->vdev);
 		vb2_video_unregister_device(&stream->meta.vdev);
 
 		/*
@@ -2033,6 +2009,8 @@ int uvc_register_video_device(struct uvc_device *dev,
 	vdev->ioctl_ops = ioctl_ops;
 	vdev->release = uvc_release;
 	vdev->prio = &stream->chain->prio;
+	vdev->queue = &queue->queue;
+	vdev->lock = &queue->mutex;
 	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
 		vdev->vfl_dir = VFL_DIR_TX;
 	else
@@ -2397,9 +2375,12 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
 	list_for_each_entry(stream, &dev->streams, list) {
 		if (stream->intf == intf) {
 			ret = uvc_video_resume(stream, reset);
-			if (ret < 0)
-				uvc_queue_streamoff(&stream->queue,
-						    stream->queue.queue.type);
+			if (ret < 0) {
+				mutex_lock(&stream->queue.mutex);
+				vb2_streamoff(&stream->queue.queue,
+					      stream->queue.queue.type);
+				mutex_unlock(&stream->queue.mutex);
+			}
 			return ret;
 		}
 	}
diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
index 82de7781f5b6b70c5ba16bcba9e0741231231904..d3aab22f91cea21aefc56409924dfa1451aec914 100644
--- a/drivers/media/usb/uvc/uvc_metadata.c
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -96,7 +96,7 @@ static int uvc_meta_v4l2_set_format(struct file *file, void *fh,
 	 */
 	mutex_lock(&stream->mutex);
 
-	if (uvc_queue_allocated(&stream->queue))
+	if (vb2_is_busy(&stream->meta.queue.queue))
 		ret = -EBUSY;
 	else
 		stream->meta.format = fmt->dataformat;
@@ -164,12 +164,6 @@ int uvc_meta_register(struct uvc_streaming *stream)
 
 	stream->meta.format = V4L2_META_FMT_UVC;
 
-	/*
-	 * The video interface queue uses manual locking and thus does not set
-	 * the queue pointer. Set it manually here.
-	 */
-	vdev->queue = &queue->queue;
-
 	return uvc_register_video_device(dev, stream, vdev, queue,
 					 V4L2_BUF_TYPE_META_CAPTURE,
 					 &uvc_meta_fops, &uvc_meta_ioctl_ops);
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 2ee142621042167c2587b6a6fdd51c1a46d31c11..72c5494dee9f46ff61072e7d293bfaddda40e615 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -242,153 +242,10 @@ int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type)
 	return 0;
 }
 
-void uvc_queue_release(struct uvc_video_queue *queue)
-{
-	mutex_lock(&queue->mutex);
-	vb2_queue_release(&queue->queue);
-	mutex_unlock(&queue->mutex);
-}
-
-/* -----------------------------------------------------------------------------
- * V4L2 queue operations
- */
-
-int uvc_request_buffers(struct uvc_video_queue *queue,
-			struct v4l2_requestbuffers *rb)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_reqbufs(&queue->queue, rb);
-	mutex_unlock(&queue->mutex);
-
-	return ret ? ret : rb->count;
-}
-
-int uvc_query_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_querybuf(&queue->queue, buf);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_create_buffers(struct uvc_video_queue *queue,
-		       struct v4l2_create_buffers *cb)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_create_bufs(&queue->queue, cb);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_queue_buffer(struct uvc_video_queue *queue,
-		     struct media_device *mdev, struct v4l2_buffer *buf)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_qbuf(&queue->queue, mdev, buf);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_export_buffer(struct uvc_video_queue *queue,
-		      struct v4l2_exportbuffer *exp)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_expbuf(&queue->queue, exp);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_dequeue_buffer(struct uvc_video_queue *queue, struct v4l2_buffer *buf,
-		       int nonblocking)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_dqbuf(&queue->queue, buf, nonblocking);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_streamon(&queue->queue, type);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_queue_streamoff(struct uvc_video_queue *queue, enum v4l2_buf_type type)
-{
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_streamoff(&queue->queue, type);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
-int uvc_queue_mmap(struct uvc_video_queue *queue, struct vm_area_struct *vma)
-{
-	return vb2_mmap(&queue->queue, vma);
-}
-
-#ifndef CONFIG_MMU
-unsigned long uvc_queue_get_unmapped_area(struct uvc_video_queue *queue,
-		unsigned long pgoff)
-{
-	return vb2_get_unmapped_area(&queue->queue, 0, 0, pgoff, 0);
-}
-#endif
-
-__poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
-			    poll_table *wait)
-{
-	__poll_t ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_poll(&queue->queue, file, wait);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
-}
-
 /* -----------------------------------------------------------------------------
  *
  */
 
-/*
- * Check if buffers have been allocated.
- */
-int uvc_queue_allocated(struct uvc_video_queue *queue)
-{
-	int allocated;
-
-	mutex_lock(&queue->mutex);
-	allocated = vb2_is_busy(&queue->queue);
-	mutex_unlock(&queue->mutex);
-
-	return allocated;
-}
-
 /*
  * Cancel the video buffers queue.
  *
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 1abdf1ea39956bbbadd3f166f37bdac518068648..d06ecf3418a988152c6c413568ce32e60040fd87 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -47,8 +47,6 @@ void uvc_pm_put(struct uvc_device *dev)
 	usb_autopm_put_interface(dev->intf);
 }
 
-static int uvc_acquire_privileges(struct uvc_fh *handle);
-
 static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain,
 				      struct uvc_control_mapping *map,
 				      const struct uvc_xu_control_mapping *xmap)
@@ -436,10 +434,6 @@ static int uvc_ioctl_s_fmt(struct file *file, void *fh,
 	const struct uvc_frame *frame;
 	int ret;
 
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
-
 	if (fmt->type != stream->type)
 		return -EINVAL;
 
@@ -448,8 +442,7 @@ static int uvc_ioctl_s_fmt(struct file *file, void *fh,
 		return ret;
 
 	mutex_lock(&stream->mutex);
-
-	if (uvc_queue_allocated(&stream->queue)) {
+	if (vb2_is_busy(&stream->queue.queue)) {
 		ret = -EBUSY;
 		goto done;
 	}
@@ -513,10 +506,6 @@ static int uvc_ioctl_s_parm(struct file *file, void *fh,
 	unsigned int i;
 	int ret;
 
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
-
 	if (parm->type != stream->type)
 		return -EINVAL;
 
@@ -593,63 +582,6 @@ static int uvc_ioctl_s_parm(struct file *file, void *fh,
 	return 0;
 }
 
-/* ------------------------------------------------------------------------
- * Privilege management
- */
-
-/*
- * Privilege management is the multiple-open implementation basis. The current
- * implementation is completely transparent for the end-user and doesn't
- * require explicit use of the VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY ioctls.
- * Those ioctls enable finer control on the device (by making possible for a
- * user to request exclusive access to a device), but are not mature yet.
- * Switching to the V4L2 priority mechanism might be considered in the future
- * if this situation changes.
- *
- * Each open instance of a UVC device can either be in a privileged or
- * unprivileged state. Only a single instance can be in a privileged state at
- * a given time. Trying to perform an operation that requires privileges will
- * automatically acquire the required privileges if possible, or return -EBUSY
- * otherwise. Privileges are dismissed when closing the instance or when
- * freeing the video buffers using VIDIOC_REQBUFS.
- *
- * Operations that require privileges are:
- *
- * - VIDIOC_S_INPUT
- * - VIDIOC_S_PARM
- * - VIDIOC_S_FMT
- * - VIDIOC_CREATE_BUFS
- * - VIDIOC_REQBUFS
- */
-static int uvc_acquire_privileges(struct uvc_fh *handle)
-{
-	/* Always succeed if the handle is already privileged. */
-	if (handle->state == UVC_HANDLE_ACTIVE)
-		return 0;
-
-	/* Check if the device already has a privileged handle. */
-	if (atomic_inc_return(&handle->stream->active) != 1) {
-		atomic_dec(&handle->stream->active);
-		return -EBUSY;
-	}
-
-	handle->state = UVC_HANDLE_ACTIVE;
-	return 0;
-}
-
-static void uvc_dismiss_privileges(struct uvc_fh *handle)
-{
-	if (handle->state == UVC_HANDLE_ACTIVE)
-		atomic_dec(&handle->stream->active);
-
-	handle->state = UVC_HANDLE_PASSIVE;
-}
-
-static int uvc_has_privileges(struct uvc_fh *handle)
-{
-	return handle->state == UVC_HANDLE_ACTIVE;
-}
-
 /* ------------------------------------------------------------------------
  * V4L2 file operations
  */
@@ -671,7 +603,6 @@ static int uvc_v4l2_open(struct file *file)
 	v4l2_fh_add(&handle->vfh);
 	handle->chain = stream->chain;
 	handle->stream = stream;
-	handle->state = UVC_HANDLE_PASSIVE;
 	file->private_data = handle;
 
 	return 0;
@@ -686,19 +617,11 @@ static int uvc_v4l2_release(struct file *file)
 
 	uvc_ctrl_cleanup_fh(handle);
 
-	/* Only free resources if this is a privileged handle. */
-	if (uvc_has_privileges(handle))
-		uvc_queue_release(&stream->queue);
-
 	if (handle->is_streaming)
 		uvc_pm_put(stream->dev);
 
 	/* Release the file handle. */
-	uvc_dismiss_privileges(handle);
-	v4l2_fh_del(&handle->vfh);
-	v4l2_fh_exit(&handle->vfh);
-	kfree(handle);
-	file->private_data = NULL;
+	vb2_fop_release(file);
 
 	return 0;
 }
@@ -753,91 +676,6 @@ static int uvc_ioctl_try_fmt(struct file *file, void *fh,
 	return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
 }
 
-static int uvc_ioctl_reqbufs(struct file *file, void *fh,
-			     struct v4l2_requestbuffers *rb)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-	int ret;
-
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
-
-	mutex_lock(&stream->mutex);
-	ret = uvc_request_buffers(&stream->queue, rb);
-	mutex_unlock(&stream->mutex);
-	if (ret < 0)
-		return ret;
-
-	if (ret == 0)
-		uvc_dismiss_privileges(handle);
-
-	return 0;
-}
-
-static int uvc_ioctl_querybuf(struct file *file, void *fh,
-			      struct v4l2_buffer *buf)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	return uvc_query_buffer(&stream->queue, buf);
-}
-
-static int uvc_ioctl_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	return uvc_queue_buffer(&stream->queue,
-				stream->vdev.v4l2_dev->mdev, buf);
-}
-
-static int uvc_ioctl_expbuf(struct file *file, void *fh,
-			    struct v4l2_exportbuffer *exp)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	return uvc_export_buffer(&stream->queue, exp);
-}
-
-static int uvc_ioctl_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	return uvc_dequeue_buffer(&stream->queue, buf,
-				  file->f_flags & O_NONBLOCK);
-}
-
-static int uvc_ioctl_create_bufs(struct file *file, void *fh,
-				  struct v4l2_create_buffers *cb)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-	int ret;
-
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
-
-	return uvc_create_buffers(&stream->queue, cb);
-}
-
 static int uvc_ioctl_streamon(struct file *file, void *fh,
 			      enum v4l2_buf_type type)
 {
@@ -845,11 +683,6 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 	struct uvc_streaming *stream = handle->stream;
 	int ret;
 
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	guard(mutex)(&stream->mutex);
-
 	if (handle->is_streaming)
 		return 0;
 
@@ -857,7 +690,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 	if (ret)
 		return ret;
 
-	ret = uvc_queue_streamon(&stream->queue, type);
+	ret = vb2_ioctl_streamon(file, fh, type);
 	if (ret) {
 		uvc_pm_put(stream->dev);
 		return ret;
@@ -873,13 +706,12 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
+	int ret;
 
-	if (!uvc_has_privileges(handle))
-		return -EBUSY;
-
-	guard(mutex)(&stream->mutex);
+	ret = vb2_ioctl_streamoff(file, fh, type);
+	if (ret)
+		return ret;
 
-	uvc_queue_streamoff(&stream->queue, type);
 	if (handle->is_streaming) {
 		handle->is_streaming = false;
 		uvc_pm_put(stream->dev);
@@ -962,13 +794,13 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
 static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
 {
 	struct uvc_fh *handle = fh;
+	struct uvc_streaming *stream = handle->stream;
 	struct uvc_video_chain *chain = handle->chain;
 	u8 *buf;
 	int ret;
 
-	ret = uvc_acquire_privileges(handle);
-	if (ret < 0)
-		return ret;
+	if (vb2_is_busy(&stream->queue.queue))
+		return -EBUSY;
 
 	if (chain->selector == NULL ||
 	    (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
@@ -1469,50 +1301,6 @@ static long uvc_v4l2_unlocked_ioctl(struct file *file,
 	return ret;
 }
 
-static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
-		    size_t count, loff_t *ppos)
-{
-	struct uvc_fh *handle = file->private_data;
-	struct uvc_streaming *stream = handle->stream;
-
-	uvc_dbg(stream->dev, CALLS, "%s: not implemented\n", __func__);
-	return -EINVAL;
-}
-
-static int uvc_v4l2_mmap(struct file *file, struct vm_area_struct *vma)
-{
-	struct uvc_fh *handle = file->private_data;
-	struct uvc_streaming *stream = handle->stream;
-
-	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
-
-	return uvc_queue_mmap(&stream->queue, vma);
-}
-
-static __poll_t uvc_v4l2_poll(struct file *file, poll_table *wait)
-{
-	struct uvc_fh *handle = file->private_data;
-	struct uvc_streaming *stream = handle->stream;
-
-	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
-
-	return uvc_queue_poll(&stream->queue, file, wait);
-}
-
-#ifndef CONFIG_MMU
-static unsigned long uvc_v4l2_get_unmapped_area(struct file *file,
-		unsigned long addr, unsigned long len, unsigned long pgoff,
-		unsigned long flags)
-{
-	struct uvc_fh *handle = file->private_data;
-	struct uvc_streaming *stream = handle->stream;
-
-	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
-
-	return uvc_queue_get_unmapped_area(&stream->queue, pgoff);
-}
-#endif
-
 const struct v4l2_ioctl_ops uvc_ioctl_ops = {
 	.vidioc_g_fmt_vid_cap = uvc_ioctl_g_fmt,
 	.vidioc_g_fmt_vid_out = uvc_ioctl_g_fmt,
@@ -1525,12 +1313,13 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
 	.vidioc_enum_fmt_vid_out = uvc_ioctl_enum_fmt,
 	.vidioc_try_fmt_vid_cap = uvc_ioctl_try_fmt,
 	.vidioc_try_fmt_vid_out = uvc_ioctl_try_fmt,
-	.vidioc_reqbufs = uvc_ioctl_reqbufs,
-	.vidioc_querybuf = uvc_ioctl_querybuf,
-	.vidioc_qbuf = uvc_ioctl_qbuf,
-	.vidioc_expbuf = uvc_ioctl_expbuf,
-	.vidioc_dqbuf = uvc_ioctl_dqbuf,
-	.vidioc_create_bufs = uvc_ioctl_create_bufs,
+	.vidioc_reqbufs = vb2_ioctl_reqbufs,
+	.vidioc_querybuf = vb2_ioctl_querybuf,
+	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
+	.vidioc_qbuf = vb2_ioctl_qbuf,
+	.vidioc_expbuf = vb2_ioctl_expbuf,
+	.vidioc_dqbuf = vb2_ioctl_dqbuf,
+	.vidioc_create_bufs = vb2_ioctl_create_bufs,
 	.vidioc_streamon = uvc_ioctl_streamon,
 	.vidioc_streamoff = uvc_ioctl_streamoff,
 	.vidioc_enum_input = uvc_ioctl_enum_input,
@@ -1557,11 +1346,10 @@ const struct v4l2_file_operations uvc_fops = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl32	= uvc_v4l2_compat_ioctl32,
 #endif
-	.read		= uvc_v4l2_read,
-	.mmap		= uvc_v4l2_mmap,
-	.poll		= uvc_v4l2_poll,
+	.mmap		= vb2_fop_mmap,
+	.poll		= vb2_fop_poll,
 #ifndef CONFIG_MMU
-	.get_unmapped_area = uvc_v4l2_get_unmapped_area,
+	.get_unmapped_area = vb2_fop_get_unmapped_area,
 #endif
 };
 
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 11d6e3c2ebdfbabd7bbe5722f88ff85f406d9bb6..b300487e6ec9163ac8236803b9e819814233f419 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -328,7 +328,10 @@ struct uvc_buffer {
 
 struct uvc_video_queue {
 	struct vb2_queue queue;
-	struct mutex mutex;			/* Protects queue */
+	struct mutex mutex;			/*
+						 * Serializes vb2_queue and
+						 * fops
+						 */
 
 	unsigned int flags;
 	unsigned int buf_used;
@@ -621,16 +624,10 @@ struct uvc_device {
 	struct uvc_entity *gpio_unit;
 };
 
-enum uvc_handle_state {
-	UVC_HANDLE_PASSIVE	= 0,
-	UVC_HANDLE_ACTIVE	= 1,
-};
-
 struct uvc_fh {
 	struct v4l2_fh vfh;
 	struct uvc_video_chain *chain;
 	struct uvc_streaming *stream;
-	enum uvc_handle_state state;
 	unsigned int pending_async_ctrls;
 	bool is_streaming;
 };
@@ -689,36 +686,11 @@ struct uvc_entity *uvc_entity_by_id(struct uvc_device *dev, int id);
 
 /* Video buffers queue management. */
 int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type);
-void uvc_queue_release(struct uvc_video_queue *queue);
-int uvc_request_buffers(struct uvc_video_queue *queue,
-			struct v4l2_requestbuffers *rb);
-int uvc_query_buffer(struct uvc_video_queue *queue,
-		     struct v4l2_buffer *v4l2_buf);
-int uvc_create_buffers(struct uvc_video_queue *queue,
-		       struct v4l2_create_buffers *v4l2_cb);
-int uvc_queue_buffer(struct uvc_video_queue *queue,
-		     struct media_device *mdev,
-		     struct v4l2_buffer *v4l2_buf);
-int uvc_export_buffer(struct uvc_video_queue *queue,
-		      struct v4l2_exportbuffer *exp);
-int uvc_dequeue_buffer(struct uvc_video_queue *queue,
-		       struct v4l2_buffer *v4l2_buf, int nonblocking);
-int uvc_queue_streamon(struct uvc_video_queue *queue, enum v4l2_buf_type type);
-int uvc_queue_streamoff(struct uvc_video_queue *queue, enum v4l2_buf_type type);
 void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect);
 struct uvc_buffer *uvc_queue_next_buffer(struct uvc_video_queue *queue,
 					 struct uvc_buffer *buf);
 struct uvc_buffer *uvc_queue_get_current_buffer(struct uvc_video_queue *queue);
 void uvc_queue_buffer_release(struct uvc_buffer *buf);
-int uvc_queue_mmap(struct uvc_video_queue *queue,
-		   struct vm_area_struct *vma);
-__poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
-			poll_table *wait);
-#ifndef CONFIG_MMU
-unsigned long uvc_queue_get_unmapped_area(struct uvc_video_queue *queue,
-					  unsigned long pgoff);
-#endif
-int uvc_queue_allocated(struct uvc_video_queue *queue);
 static inline int uvc_queue_streaming(struct uvc_video_queue *queue)
 {
 	return vb2_is_streaming(&queue->queue);

-- 
2.50.0.rc1.591.g9c95f17f64-goog

[PATCH v4 2/5] media: uvcvideo: Handle locks in uvc_queue_return_buffers

[Ricardo Ribalda]

Most of the calls to uvc_queue_return_buffers() wrap the call with
spin_lock_irq()/spin_unlock_irq().

Rename uvc_queue_return_buffers to __uvc_queue_return_buffers to
indicate that this is the version that does not handle locks and create
a new version of the function that handles the lock.

Reviewed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_queue.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 72c5494dee9f46ff61072e7d293bfaddda40e615..8f9737ac729546683ca48f5e71ce3dfacbae2926 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -42,13 +42,15 @@ static inline struct uvc_buffer *uvc_vbuf_to_buffer(struct vb2_v4l2_buffer *buf)
  *
  * This function must be called with the queue spinlock held.
  */
-static void uvc_queue_return_buffers(struct uvc_video_queue *queue,
-			       enum uvc_buffer_state state)
+static void __uvc_queue_return_buffers(struct uvc_video_queue *queue,
+				       enum uvc_buffer_state state)
 {
 	enum vb2_buffer_state vb2_state = state == UVC_BUF_STATE_ERROR
 					? VB2_BUF_STATE_ERROR
 					: VB2_BUF_STATE_QUEUED;
 
+	lockdep_assert_held(&queue->irqlock);
+
 	while (!list_empty(&queue->irqqueue)) {
 		struct uvc_buffer *buf = list_first_entry(&queue->irqqueue,
 							  struct uvc_buffer,
@@ -59,6 +61,14 @@ static void uvc_queue_return_buffers(struct uvc_video_queue *queue,
 	}
 }
 
+static void uvc_queue_return_buffers(struct uvc_video_queue *queue,
+				     enum uvc_buffer_state state)
+{
+	spin_lock_irq(&queue->irqlock);
+	__uvc_queue_return_buffers(queue, state);
+	spin_unlock_irq(&queue->irqlock);
+}
+
 /* -----------------------------------------------------------------------------
  * videobuf2 queue operations
  */
@@ -171,9 +181,7 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (ret == 0)
 		return 0;
 
-	spin_lock_irq(&queue->irqlock);
 	uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
-	spin_unlock_irq(&queue->irqlock);
 
 	return ret;
 }
@@ -187,9 +195,7 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
 	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
 		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
 
-	spin_lock_irq(&queue->irqlock);
 	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
-	spin_unlock_irq(&queue->irqlock);
 }
 
 static const struct vb2_ops uvc_queue_qops = {
@@ -263,7 +269,7 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
 	unsigned long flags;
 
 	spin_lock_irqsave(&queue->irqlock, flags);
-	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
+	__uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
 	/*
 	 * This must be protected by the irqlock spinlock to avoid race
 	 * conditions between uvc_buffer_queue and the disconnection event that

-- 
2.50.0.rc1.591.g9c95f17f64-goog

[PATCH v4 3/5] media: uvcvideo: Split uvc_stop_streaming()

[Ricardo Ribalda]

uvc_stop_streaming() is used for meta and video nodes. Split the function
in two to avoid confusion.

Use this opportunity to rename uvc_start_streaming() to
uvc_start_streaming_video(), as it is only called by the video nodes.

Reviewed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_queue.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 8f9737ac729546683ca48f5e71ce3dfacbae2926..3f357c2d48cfd258c26f0342007d1d12f1e01007 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -167,7 +167,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
 		uvc_video_clock_update(stream, vbuf, buf);
 }
 
-static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
+static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
 	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
@@ -186,14 +186,22 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return ret;
 }
 
-static void uvc_stop_streaming(struct vb2_queue *vq)
+static void uvc_stop_streaming_video(struct vb2_queue *vq)
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
 
 	lockdep_assert_irqs_enabled();
 
-	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
-		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
+	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
+
+	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
+}
+
+static void uvc_stop_streaming_meta(struct vb2_queue *vq)
+{
+	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
+
+	lockdep_assert_irqs_enabled();
 
 	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
 }
@@ -203,15 +211,15 @@ static const struct vb2_ops uvc_queue_qops = {
 	.buf_prepare = uvc_buffer_prepare,
 	.buf_queue = uvc_buffer_queue,
 	.buf_finish = uvc_buffer_finish,
-	.start_streaming = uvc_start_streaming,
-	.stop_streaming = uvc_stop_streaming,
+	.start_streaming = uvc_start_streaming_video,
+	.stop_streaming = uvc_stop_streaming_video,
 };
 
 static const struct vb2_ops uvc_meta_queue_qops = {
 	.queue_setup = uvc_queue_setup,
 	.buf_prepare = uvc_buffer_prepare,
 	.buf_queue = uvc_buffer_queue,
-	.stop_streaming = uvc_stop_streaming,
+	.stop_streaming = uvc_stop_streaming_meta,
 };
 
 int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type)

-- 
2.50.0.rc1.591.g9c95f17f64-goog

[PATCH v4 4/5] media: uvcvideo: Remove stream->is_streaming field

[Ricardo Ribalda]

The is_streaming field is used by modular PM to know if the device is
currently streaming or not.

With the transition to vb2 and fop helpers, we can use vb2 functions for
the same functionality. The great benefit is that vb2 already tracks the
streaming state for us.

Reviewed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_queue.c |  9 +++++++
 drivers/media/usb/uvc/uvc_v4l2.c  | 51 ++-------------------------------------
 drivers/media/usb/uvc/uvcvideo.h  |  1 -
 3 files changed, 11 insertions(+), 50 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 3f357c2d48cfd258c26f0342007d1d12f1e01007..6e845705b3286348a60650eb262e620dc6039d60 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -175,12 +175,18 @@ static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
 
 	lockdep_assert_irqs_enabled();
 
+	ret = uvc_pm_get(stream->dev);
+	if (ret)
+		return ret;
+
 	queue->buf_used = 0;
 
 	ret = uvc_video_start_streaming(stream);
 	if (ret == 0)
 		return 0;
 
+	uvc_pm_put(stream->dev);
+
 	uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
 
 	return ret;
@@ -189,11 +195,14 @@ static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
 static void uvc_stop_streaming_video(struct vb2_queue *vq)
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
+	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
 
 	lockdep_assert_irqs_enabled();
 
 	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
 
+	uvc_pm_put(stream->dev);
+
 	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
 }
 
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index d06ecf3418a988152c6c413568ce32e60040fd87..7ab1bdcfb493fe9f47dbdc86da23cba98d7d10ff 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -617,9 +617,6 @@ static int uvc_v4l2_release(struct file *file)
 
 	uvc_ctrl_cleanup_fh(handle);
 
-	if (handle->is_streaming)
-		uvc_pm_put(stream->dev);
-
 	/* Release the file handle. */
 	vb2_fop_release(file);
 
@@ -676,50 +673,6 @@ static int uvc_ioctl_try_fmt(struct file *file, void *fh,
 	return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
 }
 
-static int uvc_ioctl_streamon(struct file *file, void *fh,
-			      enum v4l2_buf_type type)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-	int ret;
-
-	if (handle->is_streaming)
-		return 0;
-
-	ret = uvc_pm_get(stream->dev);
-	if (ret)
-		return ret;
-
-	ret = vb2_ioctl_streamon(file, fh, type);
-	if (ret) {
-		uvc_pm_put(stream->dev);
-		return ret;
-	}
-
-	handle->is_streaming = true;
-
-	return 0;
-}
-
-static int uvc_ioctl_streamoff(struct file *file, void *fh,
-			       enum v4l2_buf_type type)
-{
-	struct uvc_fh *handle = fh;
-	struct uvc_streaming *stream = handle->stream;
-	int ret;
-
-	ret = vb2_ioctl_streamoff(file, fh, type);
-	if (ret)
-		return ret;
-
-	if (handle->is_streaming) {
-		handle->is_streaming = false;
-		uvc_pm_put(stream->dev);
-	}
-
-	return 0;
-}
-
 static int uvc_ioctl_enum_input(struct file *file, void *fh,
 				struct v4l2_input *input)
 {
@@ -1320,8 +1273,8 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
 	.vidioc_expbuf = vb2_ioctl_expbuf,
 	.vidioc_dqbuf = vb2_ioctl_dqbuf,
 	.vidioc_create_bufs = vb2_ioctl_create_bufs,
-	.vidioc_streamon = uvc_ioctl_streamon,
-	.vidioc_streamoff = uvc_ioctl_streamoff,
+	.vidioc_streamon = vb2_ioctl_streamon,
+	.vidioc_streamoff = vb2_ioctl_streamoff,
 	.vidioc_enum_input = uvc_ioctl_enum_input,
 	.vidioc_g_input = uvc_ioctl_g_input,
 	.vidioc_s_input = uvc_ioctl_s_input,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b300487e6ec9163ac8236803b9e819814233f419..3e6d2d912f3a1cfcf63b2bc8edd3f86f3da305db 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -629,7 +629,6 @@ struct uvc_fh {
 	struct uvc_video_chain *chain;
 	struct uvc_streaming *stream;
 	unsigned int pending_async_ctrls;
-	bool is_streaming;
 };
 
 /* ------------------------------------------------------------------------

-- 
2.50.0.rc1.591.g9c95f17f64-goog

[PATCH v4 5/5] media: uvcvideo: Use prio state from v4l2_device

[Ricardo Ribalda]

Currently, a UVC device can have multiple chains, and each chain maintains
its own priority state. While this behavior is technically correct for UVC,
uvcvideo is the *only* V4L2 driver that does not utilize the priority state
defined within `v4l2_device`.

This patch modifies uvcvideo to use the `v4l2_device` priority state. While
this might not be strictly "correct" for uvcvideo's multi-chain design, it
aligns uvcvideo with the rest of the V4L2 drivers, providing "correct enough"
behavior and enabling code cleanup in v4l2-core. Also, multi-chain
devices are extremely rare, they are typically implemented as two
independent usb devices.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 2 --
 drivers/media/usb/uvc/uvcvideo.h   | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index accfb4ca3c72cb899185ddc8ecf4e29143d58fc6..e3795e40f14dc325e5bd120f5f45b60937841641 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1728,7 +1728,6 @@ static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
 	INIT_LIST_HEAD(&chain->entities);
 	mutex_init(&chain->ctrl_mutex);
 	chain->dev = dev;
-	v4l2_prio_init(&chain->prio);
 
 	return chain;
 }
@@ -2008,7 +2007,6 @@ int uvc_register_video_device(struct uvc_device *dev,
 	vdev->fops = fops;
 	vdev->ioctl_ops = ioctl_ops;
 	vdev->release = uvc_release;
-	vdev->prio = &stream->chain->prio;
 	vdev->queue = &queue->queue;
 	vdev->lock = &queue->mutex;
 	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 3e6d2d912f3a1cfcf63b2bc8edd3f86f3da305db..5ed9785d59c698cc7e0ac69955b892f932961617 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -354,7 +354,6 @@ struct uvc_video_chain {
 						 * uvc_fh.pending_async_ctrls
 						 */
 
-	struct v4l2_prio_state prio;		/* V4L2 priority state */
 	u32 caps;				/* V4L2 chain-wide caps */
 	u8 ctrl_class_bitmap;			/* Bitmap of valid classes */
 };

-- 
2.50.0.rc1.591.g9c95f17f64-goog

Re: [PATCH v4 5/5] media: uvcvideo: Use prio state from v4l2_device

[Ricardo Ribalda]

Hello All

On Mon, 16 Jun 2025 at 17:24, Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Currently, a UVC device can have multiple chains, and each chain maintains
> its own priority state. While this behavior is technically correct for UVC,
> uvcvideo is the *only* V4L2 driver that does not utilize the priority state
> defined within `v4l2_device`.
>
> This patch modifies uvcvideo to use the `v4l2_device` priority state. While
> this might not be strictly "correct" for uvcvideo's multi-chain design, it
> aligns uvcvideo with the rest of the V4L2 drivers, providing "correct enough"
> behavior and enabling code cleanup in v4l2-core. Also, multi-chain
> devices are extremely rare, they are typically implemented as two
> independent usb devices.

As the cover letter says, this last patch 5/5 is a RFC. We can decide
if it is worth to keep it or not.

The pros is that we can do some cleanup in the core, the cons is that
it might break kAPI.

I checked in the debian sourcecode and I could only find a user of
PRIORITY for dvb and was optional.




>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 2 --
>  drivers/media/usb/uvc/uvcvideo.h   | 1 -
>  2 files changed, 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index accfb4ca3c72cb899185ddc8ecf4e29143d58fc6..e3795e40f14dc325e5bd120f5f45b60937841641 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1728,7 +1728,6 @@ static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
>         INIT_LIST_HEAD(&chain->entities);
>         mutex_init(&chain->ctrl_mutex);
>         chain->dev = dev;
> -       v4l2_prio_init(&chain->prio);
>
>         return chain;
>  }
> @@ -2008,7 +2007,6 @@ int uvc_register_video_device(struct uvc_device *dev,
>         vdev->fops = fops;
>         vdev->ioctl_ops = ioctl_ops;
>         vdev->release = uvc_release;
> -       vdev->prio = &stream->chain->prio;
>         vdev->queue = &queue->queue;
>         vdev->lock = &queue->mutex;
>         if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 3e6d2d912f3a1cfcf63b2bc8edd3f86f3da305db..5ed9785d59c698cc7e0ac69955b892f932961617 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -354,7 +354,6 @@ struct uvc_video_chain {
>                                                  * uvc_fh.pending_async_ctrls
>                                                  */
>
> -       struct v4l2_prio_state prio;            /* V4L2 priority state */
>         u32 caps;                               /* V4L2 chain-wide caps */
>         u8 ctrl_class_bitmap;                   /* Bitmap of valid classes */
>  };
>
> --
> 2.50.0.rc1.591.g9c95f17f64-goog
>


-- 
Ricardo Ribalda

Re: [PATCH v4 5/5] media: uvcvideo: Use prio state from v4l2_device

[Hans de Goede]

Hi Ricardo,

On 16-Jun-25 8:30 PM, Ricardo Ribalda wrote:
> Hello All
> 
> On Mon, 16 Jun 2025 at 17:24, Ricardo Ribalda <ribalda@chromium.org> wrote:
>>
>> Currently, a UVC device can have multiple chains, and each chain maintains
>> its own priority state. While this behavior is technically correct for UVC,
>> uvcvideo is the *only* V4L2 driver that does not utilize the priority state
>> defined within `v4l2_device`.
>>
>> This patch modifies uvcvideo to use the `v4l2_device` priority state. While
>> this might not be strictly "correct" for uvcvideo's multi-chain design, it
>> aligns uvcvideo with the rest of the V4L2 drivers, providing "correct enough"
>> behavior and enabling code cleanup in v4l2-core. Also, multi-chain
>> devices are extremely rare, they are typically implemented as two
>> independent usb devices.
> 
> As the cover letter says, this last patch 5/5 is a RFC. We can decide
> if it is worth to keep it or not.
> 
> The pros is that we can do some cleanup in the core, the cons is that
> it might break kAPI.

I've no objections against this change, but lets wait and see what
Laurent has to say.

Regards,

Hans




>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>> ---
>>  drivers/media/usb/uvc/uvc_driver.c | 2 --
>>  drivers/media/usb/uvc/uvcvideo.h   | 1 -
>>  2 files changed, 3 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>> index accfb4ca3c72cb899185ddc8ecf4e29143d58fc6..e3795e40f14dc325e5bd120f5f45b60937841641 100644
>> --- a/drivers/media/usb/uvc/uvc_driver.c
>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>> @@ -1728,7 +1728,6 @@ static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
>>         INIT_LIST_HEAD(&chain->entities);
>>         mutex_init(&chain->ctrl_mutex);
>>         chain->dev = dev;
>> -       v4l2_prio_init(&chain->prio);
>>
>>         return chain;
>>  }
>> @@ -2008,7 +2007,6 @@ int uvc_register_video_device(struct uvc_device *dev,
>>         vdev->fops = fops;
>>         vdev->ioctl_ops = ioctl_ops;
>>         vdev->release = uvc_release;
>> -       vdev->prio = &stream->chain->prio;
>>         vdev->queue = &queue->queue;
>>         vdev->lock = &queue->mutex;
>>         if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>> index 3e6d2d912f3a1cfcf63b2bc8edd3f86f3da305db..5ed9785d59c698cc7e0ac69955b892f932961617 100644
>> --- a/drivers/media/usb/uvc/uvcvideo.h
>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>> @@ -354,7 +354,6 @@ struct uvc_video_chain {
>>                                                  * uvc_fh.pending_async_ctrls
>>                                                  */
>>
>> -       struct v4l2_prio_state prio;            /* V4L2 priority state */
>>         u32 caps;                               /* V4L2 chain-wide caps */
>>         u8 ctrl_class_bitmap;                   /* Bitmap of valid classes */
>>  };
>>
>> --
>> 2.50.0.rc1.591.g9c95f17f64-goog
>>
> 
> 

Re: [PATCH v4 2/5] media: uvcvideo: Handle locks in uvc_queue_return_buffers

[Hans Verkuil]

On 16/06/2025 17:24, Ricardo Ribalda wrote:
> Most of the calls to uvc_queue_return_buffers() wrap the call with
> spin_lock_irq()/spin_unlock_irq().
> 
> Rename uvc_queue_return_buffers to __uvc_queue_return_buffers to
> indicate that this is the version that does not handle locks and create
> a new version of the function that handles the lock.
> 
> Reviewed-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Hans erkuil <hverkuil@xs4all.nl>

Regards,

	Hans

> ---
>  drivers/media/usb/uvc/uvc_queue.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index 72c5494dee9f46ff61072e7d293bfaddda40e615..8f9737ac729546683ca48f5e71ce3dfacbae2926 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -42,13 +42,15 @@ static inline struct uvc_buffer *uvc_vbuf_to_buffer(struct vb2_v4l2_buffer *buf)
>   *
>   * This function must be called with the queue spinlock held.
>   */
> -static void uvc_queue_return_buffers(struct uvc_video_queue *queue,
> -			       enum uvc_buffer_state state)
> +static void __uvc_queue_return_buffers(struct uvc_video_queue *queue,
> +				       enum uvc_buffer_state state)
>  {
>  	enum vb2_buffer_state vb2_state = state == UVC_BUF_STATE_ERROR
>  					? VB2_BUF_STATE_ERROR
>  					: VB2_BUF_STATE_QUEUED;
>  
> +	lockdep_assert_held(&queue->irqlock);
> +
>  	while (!list_empty(&queue->irqqueue)) {
>  		struct uvc_buffer *buf = list_first_entry(&queue->irqqueue,
>  							  struct uvc_buffer,
> @@ -59,6 +61,14 @@ static void uvc_queue_return_buffers(struct uvc_video_queue *queue,
>  	}
>  }
>  
> +static void uvc_queue_return_buffers(struct uvc_video_queue *queue,
> +				     enum uvc_buffer_state state)
> +{
> +	spin_lock_irq(&queue->irqlock);
> +	__uvc_queue_return_buffers(queue, state);
> +	spin_unlock_irq(&queue->irqlock);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * videobuf2 queue operations
>   */
> @@ -171,9 +181,7 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	if (ret == 0)
>  		return 0;
>  
> -	spin_lock_irq(&queue->irqlock);
>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
> -	spin_unlock_irq(&queue->irqlock);
>  
>  	return ret;
>  }
> @@ -187,9 +195,7 @@ static void uvc_stop_streaming(struct vb2_queue *vq)
>  	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
>  		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>  
> -	spin_lock_irq(&queue->irqlock);
>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> -	spin_unlock_irq(&queue->irqlock);
>  }
>  
>  static const struct vb2_ops uvc_queue_qops = {
> @@ -263,7 +269,7 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&queue->irqlock, flags);
> -	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> +	__uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>  	/*
>  	 * This must be protected by the irqlock spinlock to avoid race
>  	 * conditions between uvc_buffer_queue and the disconnection event that
> 

Re: [PATCH v4 3/5] media: uvcvideo: Split uvc_stop_streaming()

[Hans Verkuil]

On 16/06/2025 17:24, Ricardo Ribalda wrote:
> uvc_stop_streaming() is used for meta and video nodes. Split the function
> in two to avoid confusion.
> 
> Use this opportunity to rename uvc_start_streaming() to
> uvc_start_streaming_video(), as it is only called by the video nodes.
> 
> Reviewed-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_queue.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index 8f9737ac729546683ca48f5e71ce3dfacbae2926..3f357c2d48cfd258c26f0342007d1d12f1e01007 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -167,7 +167,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
>  		uvc_video_clock_update(stream, vbuf, buf);
>  }
>  
> -static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> +static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>  	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> @@ -186,14 +186,22 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	return ret;
>  }
>  
> -static void uvc_stop_streaming(struct vb2_queue *vq)
> +static void uvc_stop_streaming_video(struct vb2_queue *vq)
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>  
>  	lockdep_assert_irqs_enabled();
>  
> -	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> -		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> +	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> +
> +	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> +}
> +
> +static void uvc_stop_streaming_meta(struct vb2_queue *vq)
> +{
> +	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> +
> +	lockdep_assert_irqs_enabled();
>  
>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>  }
> @@ -203,15 +211,15 @@ static const struct vb2_ops uvc_queue_qops = {
>  	.buf_prepare = uvc_buffer_prepare,
>  	.buf_queue = uvc_buffer_queue,
>  	.buf_finish = uvc_buffer_finish,
> -	.start_streaming = uvc_start_streaming,
> -	.stop_streaming = uvc_stop_streaming,
> +	.start_streaming = uvc_start_streaming_video,
> +	.stop_streaming = uvc_stop_streaming_video,
>  };
>  
>  static const struct vb2_ops uvc_meta_queue_qops = {
>  	.queue_setup = uvc_queue_setup,
>  	.buf_prepare = uvc_buffer_prepare,
>  	.buf_queue = uvc_buffer_queue,
> -	.stop_streaming = uvc_stop_streaming,
> +	.stop_streaming = uvc_stop_streaming_meta,
>  };

I think there should be a comment stating that the metadata stream
expects that video is streaming, it does not start streaming by itself.

Something like:

	/*
	 * .start_streaming is not provided here. Metadata relies on
	 * video streaming being active. If video isn't streaming, then
	 * no metadata will arrive either.
	 */

It's unexpected that there is no start_streaming for metadata, so a
comment wouldn't hurt.

Otherwise this looks fine, so:

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Regards,

	Hans

>  
>  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type)
> 

Re: [PATCH v4 4/5] media: uvcvideo: Remove stream->is_streaming field

[Hans Verkuil]

On 16/06/2025 17:24, Ricardo Ribalda wrote:
> The is_streaming field is used by modular PM to know if the device is
> currently streaming or not.
> 
> With the transition to vb2 and fop helpers, we can use vb2 functions for
> the same functionality. The great benefit is that vb2 already tracks the
> streaming state for us.
> 
> Reviewed-by: Hans de Goede <hansg@kernel.org>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Regards,

	Hans

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_queue.c |  9 +++++++
>  drivers/media/usb/uvc/uvc_v4l2.c  | 51 ++-------------------------------------
>  drivers/media/usb/uvc/uvcvideo.h  |  1 -
>  3 files changed, 11 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index 3f357c2d48cfd258c26f0342007d1d12f1e01007..6e845705b3286348a60650eb262e620dc6039d60 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -175,12 +175,18 @@ static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
>  
>  	lockdep_assert_irqs_enabled();
>  
> +	ret = uvc_pm_get(stream->dev);
> +	if (ret)
> +		return ret;
> +
>  	queue->buf_used = 0;
>  
>  	ret = uvc_video_start_streaming(stream);
>  	if (ret == 0)
>  		return 0;
>  
> +	uvc_pm_put(stream->dev);
> +
>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_QUEUED);
>  
>  	return ret;
> @@ -189,11 +195,14 @@ static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
>  static void uvc_stop_streaming_video(struct vb2_queue *vq)
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> +	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>  
>  	lockdep_assert_irqs_enabled();
>  
>  	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>  
> +	uvc_pm_put(stream->dev);
> +
>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>  }
>  
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index d06ecf3418a988152c6c413568ce32e60040fd87..7ab1bdcfb493fe9f47dbdc86da23cba98d7d10ff 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -617,9 +617,6 @@ static int uvc_v4l2_release(struct file *file)
>  
>  	uvc_ctrl_cleanup_fh(handle);
>  
> -	if (handle->is_streaming)
> -		uvc_pm_put(stream->dev);
> -
>  	/* Release the file handle. */
>  	vb2_fop_release(file);
>  
> @@ -676,50 +673,6 @@ static int uvc_ioctl_try_fmt(struct file *file, void *fh,
>  	return uvc_v4l2_try_format(stream, fmt, &probe, NULL, NULL);
>  }
>  
> -static int uvc_ioctl_streamon(struct file *file, void *fh,
> -			      enum v4l2_buf_type type)
> -{
> -	struct uvc_fh *handle = fh;
> -	struct uvc_streaming *stream = handle->stream;
> -	int ret;
> -
> -	if (handle->is_streaming)
> -		return 0;
> -
> -	ret = uvc_pm_get(stream->dev);
> -	if (ret)
> -		return ret;
> -
> -	ret = vb2_ioctl_streamon(file, fh, type);
> -	if (ret) {
> -		uvc_pm_put(stream->dev);
> -		return ret;
> -	}
> -
> -	handle->is_streaming = true;
> -
> -	return 0;
> -}
> -
> -static int uvc_ioctl_streamoff(struct file *file, void *fh,
> -			       enum v4l2_buf_type type)
> -{
> -	struct uvc_fh *handle = fh;
> -	struct uvc_streaming *stream = handle->stream;
> -	int ret;
> -
> -	ret = vb2_ioctl_streamoff(file, fh, type);
> -	if (ret)
> -		return ret;
> -
> -	if (handle->is_streaming) {
> -		handle->is_streaming = false;
> -		uvc_pm_put(stream->dev);
> -	}
> -
> -	return 0;
> -}
> -
>  static int uvc_ioctl_enum_input(struct file *file, void *fh,
>  				struct v4l2_input *input)
>  {
> @@ -1320,8 +1273,8 @@ const struct v4l2_ioctl_ops uvc_ioctl_ops = {
>  	.vidioc_expbuf = vb2_ioctl_expbuf,
>  	.vidioc_dqbuf = vb2_ioctl_dqbuf,
>  	.vidioc_create_bufs = vb2_ioctl_create_bufs,
> -	.vidioc_streamon = uvc_ioctl_streamon,
> -	.vidioc_streamoff = uvc_ioctl_streamoff,
> +	.vidioc_streamon = vb2_ioctl_streamon,
> +	.vidioc_streamoff = vb2_ioctl_streamoff,
>  	.vidioc_enum_input = uvc_ioctl_enum_input,
>  	.vidioc_g_input = uvc_ioctl_g_input,
>  	.vidioc_s_input = uvc_ioctl_s_input,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index b300487e6ec9163ac8236803b9e819814233f419..3e6d2d912f3a1cfcf63b2bc8edd3f86f3da305db 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -629,7 +629,6 @@ struct uvc_fh {
>  	struct uvc_video_chain *chain;
>  	struct uvc_streaming *stream;
>  	unsigned int pending_async_ctrls;
> -	bool is_streaming;
>  };
>  
>  /* ------------------------------------------------------------------------
> 

Re: [PATCH v4 5/5] media: uvcvideo: Use prio state from v4l2_device

[Hans Verkuil]

On 16/06/2025 17:24, Ricardo Ribalda wrote:
> Currently, a UVC device can have multiple chains, and each chain maintains
> its own priority state. While this behavior is technically correct for UVC,
> uvcvideo is the *only* V4L2 driver that does not utilize the priority state
> defined within `v4l2_device`.
> 
> This patch modifies uvcvideo to use the `v4l2_device` priority state. While
> this might not be strictly "correct" for uvcvideo's multi-chain design, it
> aligns uvcvideo with the rest of the V4L2 drivers, providing "correct enough"
> behavior and enabling code cleanup in v4l2-core. Also, multi-chain
> devices are extremely rare, they are typically implemented as two
> independent usb devices.

I don't think this change is needed. I'm fine with the current code.

If someone has a multi-chain USB device, then it would be nice to see if
there are any v4l2-compliance problems. I don't think so, but it would be
good to have confirmation of that.

Regards,

	Hans

> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 2 --
>  drivers/media/usb/uvc/uvcvideo.h   | 1 -
>  2 files changed, 3 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index accfb4ca3c72cb899185ddc8ecf4e29143d58fc6..e3795e40f14dc325e5bd120f5f45b60937841641 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1728,7 +1728,6 @@ static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
>  	INIT_LIST_HEAD(&chain->entities);
>  	mutex_init(&chain->ctrl_mutex);
>  	chain->dev = dev;
> -	v4l2_prio_init(&chain->prio);
>  
>  	return chain;
>  }
> @@ -2008,7 +2007,6 @@ int uvc_register_video_device(struct uvc_device *dev,
>  	vdev->fops = fops;
>  	vdev->ioctl_ops = ioctl_ops;
>  	vdev->release = uvc_release;
> -	vdev->prio = &stream->chain->prio;
>  	vdev->queue = &queue->queue;
>  	vdev->lock = &queue->mutex;
>  	if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 3e6d2d912f3a1cfcf63b2bc8edd3f86f3da305db..5ed9785d59c698cc7e0ac69955b892f932961617 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -354,7 +354,6 @@ struct uvc_video_chain {
>  						 * uvc_fh.pending_async_ctrls
>  						 */
>  
> -	struct v4l2_prio_state prio;		/* V4L2 priority state */
>  	u32 caps;				/* V4L2 chain-wide caps */
>  	u8 ctrl_class_bitmap;			/* Bitmap of valid classes */
>  };
> 

Re: [PATCH v4 5/5] media: uvcvideo: Use prio state from v4l2_device

[Laurent Pinchart]

On Mon, Jun 16, 2025 at 08:30:08PM +0200, Ricardo Ribalda wrote:
> On Mon, 16 Jun 2025 at 17:24, Ricardo Ribalda <ribalda@chromium.org> wrote:
> >
> > Currently, a UVC device can have multiple chains, and each chain maintains
> > its own priority state. While this behavior is technically correct for UVC,
> > uvcvideo is the *only* V4L2 driver that does not utilize the priority state
> > defined within `v4l2_device`.
> >
> > This patch modifies uvcvideo to use the `v4l2_device` priority state. While
> > this might not be strictly "correct" for uvcvideo's multi-chain design, it
> > aligns uvcvideo with the rest of the V4L2 drivers, providing "correct enough"
> > behavior and enabling code cleanup in v4l2-core. Also, multi-chain
> > devices are extremely rare, they are typically implemented as two
> > independent usb devices.
> 
> As the cover letter says, this last patch 5/5 is a RFC. We can decide
> if it is worth to keep it or not.
> 
> The pros is that we can do some cleanup in the core,

What cleanups would that be ?

> the cons is that it might break kAPI.

Multi-chain devices are essentially multiple video devices inside a
single USB function. They are exposed as completely separate devices to
userspace, having the priority ioctls on one chain impact the other
chain wouldn't make much sense to me. I think we should drop this patch.

> I checked in the debian sourcecode and I could only find a user of
> PRIORITY for dvb and was optional.

We could discuss deprecating the priority ioctls overall if we think
they're not useful (and used) by userspace. I was however considering
using them in libcamera though, to prevent other applications from
modifying the camera configuration behind the library's back.

> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 2 --
> >  drivers/media/usb/uvc/uvcvideo.h   | 1 -
> >  2 files changed, 3 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index accfb4ca3c72cb899185ddc8ecf4e29143d58fc6..e3795e40f14dc325e5bd120f5f45b60937841641 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1728,7 +1728,6 @@ static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
> >         INIT_LIST_HEAD(&chain->entities);
> >         mutex_init(&chain->ctrl_mutex);
> >         chain->dev = dev;
> > -       v4l2_prio_init(&chain->prio);
> >
> >         return chain;
> >  }
> > @@ -2008,7 +2007,6 @@ int uvc_register_video_device(struct uvc_device *dev,
> >         vdev->fops = fops;
> >         vdev->ioctl_ops = ioctl_ops;
> >         vdev->release = uvc_release;
> > -       vdev->prio = &stream->chain->prio;
> >         vdev->queue = &queue->queue;
> >         vdev->lock = &queue->mutex;
> >         if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 3e6d2d912f3a1cfcf63b2bc8edd3f86f3da305db..5ed9785d59c698cc7e0ac69955b892f932961617 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -354,7 +354,6 @@ struct uvc_video_chain {
> >                                                  * uvc_fh.pending_async_ctrls
> >                                                  */
> >
> > -       struct v4l2_prio_state prio;            /* V4L2 priority state */
> >         u32 caps;                               /* V4L2 chain-wide caps */
> >         u8 ctrl_class_bitmap;                   /* Bitmap of valid classes */
> >  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/5] media: uvcvideo: Use prio state from v4l2_device

[Ricardo Ribalda]

On Tue, 17 Jun 2025 at 12:07, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Jun 16, 2025 at 08:30:08PM +0200, Ricardo Ribalda wrote:
> > On Mon, 16 Jun 2025 at 17:24, Ricardo Ribalda <ribalda@chromium.org> wrote:
> > >
> > > Currently, a UVC device can have multiple chains, and each chain maintains
> > > its own priority state. While this behavior is technically correct for UVC,
> > > uvcvideo is the *only* V4L2 driver that does not utilize the priority state
> > > defined within `v4l2_device`.
> > >
> > > This patch modifies uvcvideo to use the `v4l2_device` priority state. While
> > > this might not be strictly "correct" for uvcvideo's multi-chain design, it
> > > aligns uvcvideo with the rest of the V4L2 drivers, providing "correct enough"
> > > behavior and enabling code cleanup in v4l2-core. Also, multi-chain
> > > devices are extremely rare, they are typically implemented as two
> > > independent usb devices.
> >
> > As the cover letter says, this last patch 5/5 is a RFC. We can decide
> > if it is worth to keep it or not.
> >
> > The pros is that we can do some cleanup in the core,
>
> What cleanups would that be ?
>
> > the cons is that it might break kAPI.
>
> Multi-chain devices are essentially multiple video devices inside a
> single USB function. They are exposed as completely separate devices to
> userspace, having the priority ioctls on one chain impact the other
> chain wouldn't make much sense to me. I think we should drop this patch.

Ack, let's drop it.

PS: Do you know about a multi chain device that is commercially
available? I would love to buy one for testing.
Also do you know any "output" device that I can buy?

>
> > I checked in the debian sourcecode and I could only find a user of
> > PRIORITY for dvb and was optional.
>
> We could discuss deprecating the priority ioctls overall if we think
> they're not useful (and used) by userspace. I was however considering
> using them in libcamera though, to prevent other applications from
> modifying the camera configuration behind the library's back.

For the record:
From: https://codesearch.debian.net/search?q=VIDIOC_S_PRIORITY
If I am not wrong, this is the only relevant usecase:
https://sources.debian.org/src/zvbi/0.2.44-1/daemon/proxyd.c/?hl=1523#L1523

O_EXCL does not work for you?


>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c | 2 --
> > >  drivers/media/usb/uvc/uvcvideo.h   | 1 -
> > >  2 files changed, 3 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index accfb4ca3c72cb899185ddc8ecf4e29143d58fc6..e3795e40f14dc325e5bd120f5f45b60937841641 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1728,7 +1728,6 @@ static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
> > >         INIT_LIST_HEAD(&chain->entities);
> > >         mutex_init(&chain->ctrl_mutex);
> > >         chain->dev = dev;
> > > -       v4l2_prio_init(&chain->prio);
> > >
> > >         return chain;
> > >  }
> > > @@ -2008,7 +2007,6 @@ int uvc_register_video_device(struct uvc_device *dev,
> > >         vdev->fops = fops;
> > >         vdev->ioctl_ops = ioctl_ops;
> > >         vdev->release = uvc_release;
> > > -       vdev->prio = &stream->chain->prio;
> > >         vdev->queue = &queue->queue;
> > >         vdev->lock = &queue->mutex;
> > >         if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index 3e6d2d912f3a1cfcf63b2bc8edd3f86f3da305db..5ed9785d59c698cc7e0ac69955b892f932961617 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -354,7 +354,6 @@ struct uvc_video_chain {
> > >                                                  * uvc_fh.pending_async_ctrls
> > >                                                  */
> > >
> > > -       struct v4l2_prio_state prio;            /* V4L2 priority state */
> > >         u32 caps;                               /* V4L2 chain-wide caps */
> > >         u8 ctrl_class_bitmap;                   /* Bitmap of valid classes */
> > >  };
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v4 5/5] media: uvcvideo: Use prio state from v4l2_device

[Laurent Pinchart]

On Tue, Jun 17, 2025 at 01:09:38PM +0200, Ricardo Ribalda wrote:
> On Tue, 17 Jun 2025 at 12:07, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > On Mon, Jun 16, 2025 at 08:30:08PM +0200, Ricardo Ribalda wrote:
> > > On Mon, 16 Jun 2025 at 17:24, Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > >
> > > > Currently, a UVC device can have multiple chains, and each chain maintains
> > > > its own priority state. While this behavior is technically correct for UVC,
> > > > uvcvideo is the *only* V4L2 driver that does not utilize the priority state
> > > > defined within `v4l2_device`.
> > > >
> > > > This patch modifies uvcvideo to use the `v4l2_device` priority state. While
> > > > this might not be strictly "correct" for uvcvideo's multi-chain design, it
> > > > aligns uvcvideo with the rest of the V4L2 drivers, providing "correct enough"
> > > > behavior and enabling code cleanup in v4l2-core. Also, multi-chain
> > > > devices are extremely rare, they are typically implemented as two
> > > > independent usb devices.
> > >
> > > As the cover letter says, this last patch 5/5 is a RFC. We can decide
> > > if it is worth to keep it or not.
> > >
> > > The pros is that we can do some cleanup in the core,
> >
> > What cleanups would that be ?
> >
> > > the cons is that it might break kAPI.
> >
> > Multi-chain devices are essentially multiple video devices inside a
> > single USB function. They are exposed as completely separate devices to
> > userspace, having the priority ioctls on one chain impact the other
> > chain wouldn't make much sense to me. I think we should drop this patch.
> 
> Ack, let's drop it.
> 
> PS: Do you know about a multi chain device that is commercially
> available? I would love to buy one for testing.
> Also do you know any "output" device that I can buy?

The only output device I've worked with was a custom camera developed by
a customer that had a "UVC to VGA" output path. It was a loooooong time
ago, I don't have a device to test UVC output anymore.

> > > I checked in the debian sourcecode and I could only find a user of
> > > PRIORITY for dvb and was optional.
> >
> > We could discuss deprecating the priority ioctls overall if we think
> > they're not useful (and used) by userspace. I was however considering
> > using them in libcamera though, to prevent other applications from
> > modifying the camera configuration behind the library's back.
> 
> For the record:
> From: https://codesearch.debian.net/search?q=VIDIOC_S_PRIORITY
> If I am not wrong, this is the only relevant usecase:
> https://sources.debian.org/src/zvbi/0.2.44-1/daemon/proxyd.c/?hl=1523#L1523
> 
> O_EXCL does not work for you?

I haven't tried it, but tt's defined as

    O_EXCL Ensure that this call creates the file: if this flag is
	   specified in conjunction with O_CREAT, and pathname already
	   exists, then open() fails with the error EEXIST.

	   When these two flags are specified, symbolic links are not
	   followed: if pathname is a symbolic link, then open() fails
	   regardless of where the symbolic link points.

	   In general, the behavior of O_EXCL is undefined if it is used
	   without O_CREAT.  There is one exception: on Linux 2.6 and
	   later, O_EXCL can be used without O_CREAT  if  pathname
	   refers to a block device.  If the block device is in use by
	   the system (e.g., mounted), open() fails with the error
	   EBUSY.

so I don't expect it would work.

It's not a big deal, libcamera doesn't get exclusive access to video
devices today and the world doesn't collapse (at least not because of
this specific issue). And we don't have priority support on subdevs, so
we couldn't solve the whole problem anyway.

> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_driver.c | 2 --
> > > >  drivers/media/usb/uvc/uvcvideo.h   | 1 -
> > > >  2 files changed, 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index accfb4ca3c72cb899185ddc8ecf4e29143d58fc6..e3795e40f14dc325e5bd120f5f45b60937841641 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -1728,7 +1728,6 @@ static struct uvc_video_chain *uvc_alloc_chain(struct uvc_device *dev)
> > > >         INIT_LIST_HEAD(&chain->entities);
> > > >         mutex_init(&chain->ctrl_mutex);
> > > >         chain->dev = dev;
> > > > -       v4l2_prio_init(&chain->prio);
> > > >
> > > >         return chain;
> > > >  }
> > > > @@ -2008,7 +2007,6 @@ int uvc_register_video_device(struct uvc_device *dev,
> > > >         vdev->fops = fops;
> > > >         vdev->ioctl_ops = ioctl_ops;
> > > >         vdev->release = uvc_release;
> > > > -       vdev->prio = &stream->chain->prio;
> > > >         vdev->queue = &queue->queue;
> > > >         vdev->lock = &queue->mutex;
> > > >         if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT)
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index 3e6d2d912f3a1cfcf63b2bc8edd3f86f3da305db..5ed9785d59c698cc7e0ac69955b892f932961617 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -354,7 +354,6 @@ struct uvc_video_chain {
> > > >                                                  * uvc_fh.pending_async_ctrls
> > > >                                                  */
> > > >
> > > > -       struct v4l2_prio_state prio;            /* V4L2 priority state */
> > > >         u32 caps;                               /* V4L2 chain-wide caps */
> > > >         u8 ctrl_class_bitmap;                   /* Bitmap of valid classes */
> > > >  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/5] media: uvcvideo: Split uvc_stop_streaming()

[Hans de Goede]

Hi all,

On 17-Jun-25 11:27 AM, Hans Verkuil wrote:
> On 16/06/2025 17:24, Ricardo Ribalda wrote:
>> uvc_stop_streaming() is used for meta and video nodes. Split the function
>> in two to avoid confusion.
>>
>> Use this opportunity to rename uvc_start_streaming() to
>> uvc_start_streaming_video(), as it is only called by the video nodes.
>>
>> Reviewed-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>> ---
>>  drivers/media/usb/uvc/uvc_queue.c | 22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
>> index 8f9737ac729546683ca48f5e71ce3dfacbae2926..3f357c2d48cfd258c26f0342007d1d12f1e01007 100644
>> --- a/drivers/media/usb/uvc/uvc_queue.c
>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>> @@ -167,7 +167,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
>>  		uvc_video_clock_update(stream, vbuf, buf);
>>  }
>>  
>> -static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>> +static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
>>  {
>>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>  	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>> @@ -186,14 +186,22 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>  	return ret;
>>  }
>>  
>> -static void uvc_stop_streaming(struct vb2_queue *vq)
>> +static void uvc_stop_streaming_video(struct vb2_queue *vq)
>>  {
>>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>  
>>  	lockdep_assert_irqs_enabled();
>>  
>> -	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
>> -		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>> +	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>> +
>> +	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>> +}
>> +
>> +static void uvc_stop_streaming_meta(struct vb2_queue *vq)
>> +{
>> +	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>> +
>> +	lockdep_assert_irqs_enabled();
>>  
>>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>>  }
>> @@ -203,15 +211,15 @@ static const struct vb2_ops uvc_queue_qops = {
>>  	.buf_prepare = uvc_buffer_prepare,
>>  	.buf_queue = uvc_buffer_queue,
>>  	.buf_finish = uvc_buffer_finish,
>> -	.start_streaming = uvc_start_streaming,
>> -	.stop_streaming = uvc_stop_streaming,
>> +	.start_streaming = uvc_start_streaming_video,
>> +	.stop_streaming = uvc_stop_streaming_video,
>>  };
>>  
>>  static const struct vb2_ops uvc_meta_queue_qops = {
>>  	.queue_setup = uvc_queue_setup,
>>  	.buf_prepare = uvc_buffer_prepare,
>>  	.buf_queue = uvc_buffer_queue,
>> -	.stop_streaming = uvc_stop_streaming,
>> +	.stop_streaming = uvc_stop_streaming_meta,
>>  };
> 
> I think there should be a comment stating that the metadata stream
> expects that video is streaming, it does not start streaming by itself.
> 
> Something like:
> 
> 	/*
> 	 * .start_streaming is not provided here. Metadata relies on
> 	 * video streaming being active. If video isn't streaming, then
> 	 * no metadata will arrive either.
> 	 */
> 
> It's unexpected that there is no start_streaming for metadata, so a
> comment wouldn't hurt.

I've added this comment while merging this series and I've now pushed
the entire series to uvc.git/for-next .

BTW it seems that both uvc.git/next and uvc.git/for-next are in
use now?  With uvc.git/next seemingly following media-commiters/next ?

I thought we had agreed to only use uvc.git/for-next ?

Regards,

Hans

Re: [PATCH v4 3/5] media: uvcvideo: Split uvc_stop_streaming()

[Laurent Pinchart]

On Mon, Jun 30, 2025 at 02:59:05PM +0200, Hans de Goede wrote:
> On 17-Jun-25 11:27 AM, Hans Verkuil wrote:
> > On 16/06/2025 17:24, Ricardo Ribalda wrote:
> >> uvc_stop_streaming() is used for meta and video nodes. Split the function
> >> in two to avoid confusion.
> >>
> >> Use this opportunity to rename uvc_start_streaming() to
> >> uvc_start_streaming_video(), as it is only called by the video nodes.
> >>
> >> Reviewed-by: Hans de Goede <hansg@kernel.org>
> >> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >> ---
> >>  drivers/media/usb/uvc/uvc_queue.c | 22 +++++++++++++++-------
> >>  1 file changed, 15 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> >> index 8f9737ac729546683ca48f5e71ce3dfacbae2926..3f357c2d48cfd258c26f0342007d1d12f1e01007 100644
> >> --- a/drivers/media/usb/uvc/uvc_queue.c
> >> +++ b/drivers/media/usb/uvc/uvc_queue.c
> >> @@ -167,7 +167,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
> >>  		uvc_video_clock_update(stream, vbuf, buf);
> >>  }
> >>  
> >> -static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> >> +static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
> >>  {
> >>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >>  	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> >> @@ -186,14 +186,22 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> >>  	return ret;
> >>  }
> >>  
> >> -static void uvc_stop_streaming(struct vb2_queue *vq)
> >> +static void uvc_stop_streaming_video(struct vb2_queue *vq)
> >>  {
> >>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >>  
> >>  	lockdep_assert_irqs_enabled();
> >>  
> >> -	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> >> -		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> >> +	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> >> +
> >> +	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> >> +}
> >> +
> >> +static void uvc_stop_streaming_meta(struct vb2_queue *vq)
> >> +{
> >> +	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >> +
> >> +	lockdep_assert_irqs_enabled();
> >>  
> >>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> >>  }
> >> @@ -203,15 +211,15 @@ static const struct vb2_ops uvc_queue_qops = {
> >>  	.buf_prepare = uvc_buffer_prepare,
> >>  	.buf_queue = uvc_buffer_queue,
> >>  	.buf_finish = uvc_buffer_finish,
> >> -	.start_streaming = uvc_start_streaming,
> >> -	.stop_streaming = uvc_stop_streaming,
> >> +	.start_streaming = uvc_start_streaming_video,
> >> +	.stop_streaming = uvc_stop_streaming_video,
> >>  };
> >>  
> >>  static const struct vb2_ops uvc_meta_queue_qops = {
> >>  	.queue_setup = uvc_queue_setup,
> >>  	.buf_prepare = uvc_buffer_prepare,
> >>  	.buf_queue = uvc_buffer_queue,
> >> -	.stop_streaming = uvc_stop_streaming,
> >> +	.stop_streaming = uvc_stop_streaming_meta,
> >>  };
> > 
> > I think there should be a comment stating that the metadata stream
> > expects that video is streaming, it does not start streaming by itself.
> > 
> > Something like:
> > 
> > 	/*
> > 	 * .start_streaming is not provided here. Metadata relies on
> > 	 * video streaming being active. If video isn't streaming, then
> > 	 * no metadata will arrive either.
> > 	 */
> > 
> > It's unexpected that there is no start_streaming for metadata, so a
> > comment wouldn't hurt.
> 
> I've added this comment while merging this series and I've now pushed
> the entire series to uvc.git/for-next .
> 
> BTW it seems that both uvc.git/next and uvc.git/for-next are in
> use now?  With uvc.git/next seemingly following media-commiters/next ?

As far as I understand, some jobs in the media CI use the next branch,
for instance the bisect job that tries to compile every commit uses the
next branch as a base. We therefore need to keep the next branch
up-to-date, mirroring upstream.

> I thought we had agreed to only use uvc.git/for-next ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/5] media: uvcvideo: Split uvc_stop_streaming()

[Hans de Goede]

Hi,

On 30-Jun-25 3:10 PM, Laurent Pinchart wrote:
> On Mon, Jun 30, 2025 at 02:59:05PM +0200, Hans de Goede wrote:
>> On 17-Jun-25 11:27 AM, Hans Verkuil wrote:
>>> On 16/06/2025 17:24, Ricardo Ribalda wrote:
>>>> uvc_stop_streaming() is used for meta and video nodes. Split the function
>>>> in two to avoid confusion.
>>>>
>>>> Use this opportunity to rename uvc_start_streaming() to
>>>> uvc_start_streaming_video(), as it is only called by the video nodes.
>>>>
>>>> Reviewed-by: Hans de Goede <hansg@kernel.org>
>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>> ---
>>>>  drivers/media/usb/uvc/uvc_queue.c | 22 +++++++++++++++-------
>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
>>>> index 8f9737ac729546683ca48f5e71ce3dfacbae2926..3f357c2d48cfd258c26f0342007d1d12f1e01007 100644
>>>> --- a/drivers/media/usb/uvc/uvc_queue.c
>>>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>>>> @@ -167,7 +167,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
>>>>  		uvc_video_clock_update(stream, vbuf, buf);
>>>>  }
>>>>  
>>>> -static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>>> +static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
>>>>  {
>>>>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>>>  	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>>>> @@ -186,14 +186,22 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>>>  	return ret;
>>>>  }
>>>>  
>>>> -static void uvc_stop_streaming(struct vb2_queue *vq)
>>>> +static void uvc_stop_streaming_video(struct vb2_queue *vq)
>>>>  {
>>>>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>>>  
>>>>  	lockdep_assert_irqs_enabled();
>>>>  
>>>> -	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
>>>> -		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>>>> +	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>>>> +
>>>> +	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>>>> +}
>>>> +
>>>> +static void uvc_stop_streaming_meta(struct vb2_queue *vq)
>>>> +{
>>>> +	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>>> +
>>>> +	lockdep_assert_irqs_enabled();
>>>>  
>>>>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>>>>  }
>>>> @@ -203,15 +211,15 @@ static const struct vb2_ops uvc_queue_qops = {
>>>>  	.buf_prepare = uvc_buffer_prepare,
>>>>  	.buf_queue = uvc_buffer_queue,
>>>>  	.buf_finish = uvc_buffer_finish,
>>>> -	.start_streaming = uvc_start_streaming,
>>>> -	.stop_streaming = uvc_stop_streaming,
>>>> +	.start_streaming = uvc_start_streaming_video,
>>>> +	.stop_streaming = uvc_stop_streaming_video,
>>>>  };
>>>>  
>>>>  static const struct vb2_ops uvc_meta_queue_qops = {
>>>>  	.queue_setup = uvc_queue_setup,
>>>>  	.buf_prepare = uvc_buffer_prepare,
>>>>  	.buf_queue = uvc_buffer_queue,
>>>> -	.stop_streaming = uvc_stop_streaming,
>>>> +	.stop_streaming = uvc_stop_streaming_meta,
>>>>  };
>>>
>>> I think there should be a comment stating that the metadata stream
>>> expects that video is streaming, it does not start streaming by itself.
>>>
>>> Something like:
>>>
>>> 	/*
>>> 	 * .start_streaming is not provided here. Metadata relies on
>>> 	 * video streaming being active. If video isn't streaming, then
>>> 	 * no metadata will arrive either.
>>> 	 */
>>>
>>> It's unexpected that there is no start_streaming for metadata, so a
>>> comment wouldn't hurt.
>>
>> I've added this comment while merging this series and I've now pushed
>> the entire series to uvc.git/for-next .
>>
>> BTW it seems that both uvc.git/next and uvc.git/for-next are in
>> use now?  With uvc.git/next seemingly following media-commiters/next ?
> 
> As far as I understand, some jobs in the media CI use the next branch,
> for instance the bisect job that tries to compile every commit uses the
> next branch as a base. We therefore need to keep the next branch
> up-to-date, mirroring upstream.

Ok, so we have the next branch mirroring upstream and then we
use for-next to merge new patches as I've just done ?

Regards,

Hans

Re: [PATCH v4 3/5] media: uvcvideo: Split uvc_stop_streaming()

[Laurent Pinchart]

On Mon, Jun 30, 2025 at 03:12:38PM +0200, Hans de Goede wrote:
> On 30-Jun-25 3:10 PM, Laurent Pinchart wrote:
> > On Mon, Jun 30, 2025 at 02:59:05PM +0200, Hans de Goede wrote:
> >> On 17-Jun-25 11:27 AM, Hans Verkuil wrote:
> >>> On 16/06/2025 17:24, Ricardo Ribalda wrote:
> >>>> uvc_stop_streaming() is used for meta and video nodes. Split the function
> >>>> in two to avoid confusion.
> >>>>
> >>>> Use this opportunity to rename uvc_start_streaming() to
> >>>> uvc_start_streaming_video(), as it is only called by the video nodes.
> >>>>
> >>>> Reviewed-by: Hans de Goede <hansg@kernel.org>
> >>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>>> ---
> >>>>  drivers/media/usb/uvc/uvc_queue.c | 22 +++++++++++++++-------
> >>>>  1 file changed, 15 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> >>>> index 8f9737ac729546683ca48f5e71ce3dfacbae2926..3f357c2d48cfd258c26f0342007d1d12f1e01007 100644
> >>>> --- a/drivers/media/usb/uvc/uvc_queue.c
> >>>> +++ b/drivers/media/usb/uvc/uvc_queue.c
> >>>> @@ -167,7 +167,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
> >>>>  		uvc_video_clock_update(stream, vbuf, buf);
> >>>>  }
> >>>>  
> >>>> -static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> >>>> +static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
> >>>>  {
> >>>>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >>>>  	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> >>>> @@ -186,14 +186,22 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> >>>>  	return ret;
> >>>>  }
> >>>>  
> >>>> -static void uvc_stop_streaming(struct vb2_queue *vq)
> >>>> +static void uvc_stop_streaming_video(struct vb2_queue *vq)
> >>>>  {
> >>>>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >>>>  
> >>>>  	lockdep_assert_irqs_enabled();
> >>>>  
> >>>> -	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> >>>> -		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> >>>> +	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> >>>> +
> >>>> +	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> >>>> +}
> >>>> +
> >>>> +static void uvc_stop_streaming_meta(struct vb2_queue *vq)
> >>>> +{
> >>>> +	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >>>> +
> >>>> +	lockdep_assert_irqs_enabled();
> >>>>  
> >>>>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> >>>>  }
> >>>> @@ -203,15 +211,15 @@ static const struct vb2_ops uvc_queue_qops = {
> >>>>  	.buf_prepare = uvc_buffer_prepare,
> >>>>  	.buf_queue = uvc_buffer_queue,
> >>>>  	.buf_finish = uvc_buffer_finish,
> >>>> -	.start_streaming = uvc_start_streaming,
> >>>> -	.stop_streaming = uvc_stop_streaming,
> >>>> +	.start_streaming = uvc_start_streaming_video,
> >>>> +	.stop_streaming = uvc_stop_streaming_video,
> >>>>  };
> >>>>  
> >>>>  static const struct vb2_ops uvc_meta_queue_qops = {
> >>>>  	.queue_setup = uvc_queue_setup,
> >>>>  	.buf_prepare = uvc_buffer_prepare,
> >>>>  	.buf_queue = uvc_buffer_queue,
> >>>> -	.stop_streaming = uvc_stop_streaming,
> >>>> +	.stop_streaming = uvc_stop_streaming_meta,
> >>>>  };
> >>>
> >>> I think there should be a comment stating that the metadata stream
> >>> expects that video is streaming, it does not start streaming by itself.
> >>>
> >>> Something like:
> >>>
> >>> 	/*
> >>> 	 * .start_streaming is not provided here. Metadata relies on
> >>> 	 * video streaming being active. If video isn't streaming, then
> >>> 	 * no metadata will arrive either.
> >>> 	 */
> >>>
> >>> It's unexpected that there is no start_streaming for metadata, so a
> >>> comment wouldn't hurt.
> >>
> >> I've added this comment while merging this series and I've now pushed
> >> the entire series to uvc.git/for-next .
> >>
> >> BTW it seems that both uvc.git/next and uvc.git/for-next are in
> >> use now?  With uvc.git/next seemingly following media-commiters/next ?
> > 
> > As far as I understand, some jobs in the media CI use the next branch,
> > for instance the bisect job that tries to compile every commit uses the
> > next branch as a base. We therefore need to keep the next branch
> > up-to-date, mirroring upstream.
> 
> Ok, so we have the next branch mirroring upstream and then we
> use for-next to merge new patches as I've just done ?

Sounds good.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/5] media: uvcvideo: Split uvc_stop_streaming()

[Ricardo Ribalda]

On Mon, 30 Jun 2025 at 15:25, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Jun 30, 2025 at 03:12:38PM +0200, Hans de Goede wrote:
> > On 30-Jun-25 3:10 PM, Laurent Pinchart wrote:
> > > On Mon, Jun 30, 2025 at 02:59:05PM +0200, Hans de Goede wrote:
> > >> On 17-Jun-25 11:27 AM, Hans Verkuil wrote:
> > >>> On 16/06/2025 17:24, Ricardo Ribalda wrote:
> > >>>> uvc_stop_streaming() is used for meta and video nodes. Split the function
> > >>>> in two to avoid confusion.
> > >>>>
> > >>>> Use this opportunity to rename uvc_start_streaming() to
> > >>>> uvc_start_streaming_video(), as it is only called by the video nodes.
> > >>>>
> > >>>> Reviewed-by: Hans de Goede <hansg@kernel.org>
> > >>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > >>>> ---
> > >>>>  drivers/media/usb/uvc/uvc_queue.c | 22 +++++++++++++++-------
> > >>>>  1 file changed, 15 insertions(+), 7 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> > >>>> index 8f9737ac729546683ca48f5e71ce3dfacbae2926..3f357c2d48cfd258c26f0342007d1d12f1e01007 100644
> > >>>> --- a/drivers/media/usb/uvc/uvc_queue.c
> > >>>> +++ b/drivers/media/usb/uvc/uvc_queue.c
> > >>>> @@ -167,7 +167,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
> > >>>>                  uvc_video_clock_update(stream, vbuf, buf);
> > >>>>  }
> > >>>>
> > >>>> -static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> > >>>> +static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
> > >>>>  {
> > >>>>          struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> > >>>>          struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> > >>>> @@ -186,14 +186,22 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> > >>>>          return ret;
> > >>>>  }
> > >>>>
> > >>>> -static void uvc_stop_streaming(struct vb2_queue *vq)
> > >>>> +static void uvc_stop_streaming_video(struct vb2_queue *vq)
> > >>>>  {
> > >>>>          struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> > >>>>
> > >>>>          lockdep_assert_irqs_enabled();
> > >>>>
> > >>>> -        if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> > >>>> -                uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> > >>>> +        uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> > >>>> +
> > >>>> +        uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> > >>>> +}
> > >>>> +
> > >>>> +static void uvc_stop_streaming_meta(struct vb2_queue *vq)
> > >>>> +{
> > >>>> +        struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> > >>>> +
> > >>>> +        lockdep_assert_irqs_enabled();
> > >>>>
> > >>>>          uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> > >>>>  }
> > >>>> @@ -203,15 +211,15 @@ static const struct vb2_ops uvc_queue_qops = {
> > >>>>          .buf_prepare = uvc_buffer_prepare,
> > >>>>          .buf_queue = uvc_buffer_queue,
> > >>>>          .buf_finish = uvc_buffer_finish,
> > >>>> -        .start_streaming = uvc_start_streaming,
> > >>>> -        .stop_streaming = uvc_stop_streaming,
> > >>>> +        .start_streaming = uvc_start_streaming_video,
> > >>>> +        .stop_streaming = uvc_stop_streaming_video,
> > >>>>  };
> > >>>>
> > >>>>  static const struct vb2_ops uvc_meta_queue_qops = {
> > >>>>          .queue_setup = uvc_queue_setup,
> > >>>>          .buf_prepare = uvc_buffer_prepare,
> > >>>>          .buf_queue = uvc_buffer_queue,
> > >>>> -        .stop_streaming = uvc_stop_streaming,
> > >>>> +        .stop_streaming = uvc_stop_streaming_meta,
> > >>>>  };
> > >>>
> > >>> I think there should be a comment stating that the metadata stream
> > >>> expects that video is streaming, it does not start streaming by itself.
> > >>>
> > >>> Something like:
> > >>>
> > >>>   /*
> > >>>    * .start_streaming is not provided here. Metadata relies on
> > >>>    * video streaming being active. If video isn't streaming, then
> > >>>    * no metadata will arrive either.
> > >>>    */
> > >>>
> > >>> It's unexpected that there is no start_streaming for metadata, so a
> > >>> comment wouldn't hurt.
> > >>
> > >> I've added this comment while merging this series and I've now pushed
> > >> the entire series to uvc.git/for-next .
> > >>
> > >> BTW it seems that both uvc.git/next and uvc.git/for-next are in
> > >> use now?  With uvc.git/next seemingly following media-commiters/next ?
> > >
> > > As far as I understand, some jobs in the media CI use the next branch,
> > > for instance the bisect job that tries to compile every commit uses the
> > > next branch as a base. We therefore need to keep the next branch
> > > up-to-date, mirroring upstream.
> >
> > Ok, so we have the next branch mirroring upstream and then we
> > use for-next to merge new patches as I've just done ?
>
> Sounds good.

BTW the auto-mirroring from media-committers has not been enabled. Do
you want it?

>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v4 3/5] media: uvcvideo: Split uvc_stop_streaming()

[Laurent Pinchart]

On Mon, Jun 16, 2025 at 03:24:40PM +0000, Ricardo Ribalda wrote:
> uvc_stop_streaming() is used for meta and video nodes. Split the function
> in two to avoid confusion.
> 
> Use this opportunity to rename uvc_start_streaming() to
> uvc_start_streaming_video(), as it is only called by the video nodes.
> 
> Reviewed-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_queue.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index 8f9737ac729546683ca48f5e71ce3dfacbae2926..3f357c2d48cfd258c26f0342007d1d12f1e01007 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -167,7 +167,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
>  		uvc_video_clock_update(stream, vbuf, buf);
>  }
>  
> -static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> +static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>  	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> @@ -186,14 +186,22 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	return ret;
>  }
>  
> -static void uvc_stop_streaming(struct vb2_queue *vq)
> +static void uvc_stop_streaming_video(struct vb2_queue *vq)
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>  
>  	lockdep_assert_irqs_enabled();
>  
> -	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> -		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> +	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> +
> +	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> +}
> +
> +static void uvc_stop_streaming_meta(struct vb2_queue *vq)
> +{
> +	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> +
> +	lockdep_assert_irqs_enabled();
>  
>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);

I haven't checked where it was introduced, but I think you have a race
here. uvc_queue_return_buffers() will return all buffers currently
sitting in the queue->irqqueue. This can race with a bunch of places in
uvc_video.c that call uvc_queue_get_current_buffer() or
uvc_queue_get_next_buffer(), as those functions return a buffer without
removing it from the list.

>  }
> @@ -203,15 +211,15 @@ static const struct vb2_ops uvc_queue_qops = {
>  	.buf_prepare = uvc_buffer_prepare,
>  	.buf_queue = uvc_buffer_queue,
>  	.buf_finish = uvc_buffer_finish,
> -	.start_streaming = uvc_start_streaming,
> -	.stop_streaming = uvc_stop_streaming,
> +	.start_streaming = uvc_start_streaming_video,
> +	.stop_streaming = uvc_stop_streaming_video,
>  };
>  
>  static const struct vb2_ops uvc_meta_queue_qops = {
>  	.queue_setup = uvc_queue_setup,
>  	.buf_prepare = uvc_buffer_prepare,
>  	.buf_queue = uvc_buffer_queue,
> -	.stop_streaming = uvc_stop_streaming,
> +	.stop_streaming = uvc_stop_streaming_meta,
>  };
>  
>  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/5] media: uvcvideo: Split uvc_stop_streaming()

[Hans de Goede]

Hi Laurent

On 30-Jun-25 4:17 PM, Laurent Pinchart wrote:
> On Mon, Jun 16, 2025 at 03:24:40PM +0000, Ricardo Ribalda wrote:
>> uvc_stop_streaming() is used for meta and video nodes. Split the function
>> in two to avoid confusion.
>>
>> Use this opportunity to rename uvc_start_streaming() to
>> uvc_start_streaming_video(), as it is only called by the video nodes.
>>
>> Reviewed-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>> ---
>>  drivers/media/usb/uvc/uvc_queue.c | 22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
>> index 8f9737ac729546683ca48f5e71ce3dfacbae2926..3f357c2d48cfd258c26f0342007d1d12f1e01007 100644
>> --- a/drivers/media/usb/uvc/uvc_queue.c
>> +++ b/drivers/media/usb/uvc/uvc_queue.c
>> @@ -167,7 +167,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
>>  		uvc_video_clock_update(stream, vbuf, buf);
>>  }
>>  
>> -static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>> +static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
>>  {
>>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>  	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>> @@ -186,14 +186,22 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>  	return ret;
>>  }
>>  
>> -static void uvc_stop_streaming(struct vb2_queue *vq)
>> +static void uvc_stop_streaming_video(struct vb2_queue *vq)
>>  {
>>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>>  
>>  	lockdep_assert_irqs_enabled();
>>  
>> -	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
>> -		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>> +	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
>> +
>> +	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>> +}
>> +
>> +static void uvc_stop_streaming_meta(struct vb2_queue *vq)
>> +{
>> +	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>> +
>> +	lockdep_assert_irqs_enabled();
>>  
>>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> 
> I haven't checked where it was introduced, but I think you have a race
> here. uvc_queue_return_buffers() will return all buffers currently
> sitting in the queue->irqqueue. This can race with a bunch of places in
> uvc_video.c that call uvc_queue_get_current_buffer() or
> uvc_queue_get_next_buffer(), as those functions return a buffer without
> removing it from the list.

This change just splits uvc_stop_streaming() into 2 separate
functions for uvc_queue_qops + uvc_meta_queue_qops to remove
the weird looking "if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)"
check done in the shared uvc_stop_streaming().

This patch does not make any functional changes. So if such
a race exists then that is a pre-existing problem and not
caused by this patch.

Regards,

Hans



> 
>>  }
>> @@ -203,15 +211,15 @@ static const struct vb2_ops uvc_queue_qops = {
>>  	.buf_prepare = uvc_buffer_prepare,
>>  	.buf_queue = uvc_buffer_queue,
>>  	.buf_finish = uvc_buffer_finish,
>> -	.start_streaming = uvc_start_streaming,
>> -	.stop_streaming = uvc_stop_streaming,
>> +	.start_streaming = uvc_start_streaming_video,
>> +	.stop_streaming = uvc_stop_streaming_video,
>>  };
>>  
>>  static const struct vb2_ops uvc_meta_queue_qops = {
>>  	.queue_setup = uvc_queue_setup,
>>  	.buf_prepare = uvc_buffer_prepare,
>>  	.buf_queue = uvc_buffer_queue,
>> -	.stop_streaming = uvc_stop_streaming,
>> +	.stop_streaming = uvc_stop_streaming_meta,
>>  };
>>  
>>  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type)
> 

Re: [PATCH v4 3/5] media: uvcvideo: Split uvc_stop_streaming()

[Laurent Pinchart]

On Mon, Jun 30, 2025 at 05:12:29PM +0200, Hans de Goede wrote:
> On 30-Jun-25 4:17 PM, Laurent Pinchart wrote:
> > On Mon, Jun 16, 2025 at 03:24:40PM +0000, Ricardo Ribalda wrote:
> >> uvc_stop_streaming() is used for meta and video nodes. Split the function
> >> in two to avoid confusion.
> >>
> >> Use this opportunity to rename uvc_start_streaming() to
> >> uvc_start_streaming_video(), as it is only called by the video nodes.
> >>
> >> Reviewed-by: Hans de Goede <hansg@kernel.org>
> >> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >> ---
> >>  drivers/media/usb/uvc/uvc_queue.c | 22 +++++++++++++++-------
> >>  1 file changed, 15 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> >> index 8f9737ac729546683ca48f5e71ce3dfacbae2926..3f357c2d48cfd258c26f0342007d1d12f1e01007 100644
> >> --- a/drivers/media/usb/uvc/uvc_queue.c
> >> +++ b/drivers/media/usb/uvc/uvc_queue.c
> >> @@ -167,7 +167,7 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
> >>  		uvc_video_clock_update(stream, vbuf, buf);
> >>  }
> >>  
> >> -static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> >> +static int uvc_start_streaming_video(struct vb2_queue *vq, unsigned int count)
> >>  {
> >>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >>  	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
> >> @@ -186,14 +186,22 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count)
> >>  	return ret;
> >>  }
> >>  
> >> -static void uvc_stop_streaming(struct vb2_queue *vq)
> >> +static void uvc_stop_streaming_video(struct vb2_queue *vq)
> >>  {
> >>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >>  
> >>  	lockdep_assert_irqs_enabled();
> >>  
> >> -	if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)
> >> -		uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> >> +	uvc_video_stop_streaming(uvc_queue_to_stream(queue));
> >> +
> >> +	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> >> +}
> >> +
> >> +static void uvc_stop_streaming_meta(struct vb2_queue *vq)
> >> +{
> >> +	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> >> +
> >> +	lockdep_assert_irqs_enabled();
> >>  
> >>  	uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> > 
> > I haven't checked where it was introduced, but I think you have a race
> > here. uvc_queue_return_buffers() will return all buffers currently
> > sitting in the queue->irqqueue. This can race with a bunch of places in
> > uvc_video.c that call uvc_queue_get_current_buffer() or
> > uvc_queue_get_next_buffer(), as those functions return a buffer without
> > removing it from the list.
> 
> This change just splits uvc_stop_streaming() into 2 separate
> functions for uvc_queue_qops + uvc_meta_queue_qops to remove
> the weird looking "if (vq->type != V4L2_BUF_TYPE_META_CAPTURE)"
> check done in the shared uvc_stop_streaming().
> 
> This patch does not make any functional changes. So if such
> a race exists then that is a pre-existing problem and not
> caused by this patch.

Yes, that's why I said I'm not sure where it was introduced. I only
noticed the issue here, it comes from before this patch.

> >>  }
> >> @@ -203,15 +211,15 @@ static const struct vb2_ops uvc_queue_qops = {
> >>  	.buf_prepare = uvc_buffer_prepare,
> >>  	.buf_queue = uvc_buffer_queue,
> >>  	.buf_finish = uvc_buffer_finish,
> >> -	.start_streaming = uvc_start_streaming,
> >> -	.stop_streaming = uvc_stop_streaming,
> >> +	.start_streaming = uvc_start_streaming_video,
> >> +	.stop_streaming = uvc_stop_streaming_video,
> >>  };
> >>  
> >>  static const struct vb2_ops uvc_meta_queue_qops = {
> >>  	.queue_setup = uvc_queue_setup,
> >>  	.buf_prepare = uvc_buffer_prepare,
> >>  	.buf_queue = uvc_buffer_queue,
> >> -	.stop_streaming = uvc_stop_streaming,
> >> +	.stop_streaming = uvc_stop_streaming_meta,
> >>  };
> >>  
> >>  int uvc_queue_init(struct uvc_video_queue *queue, enum v4l2_buf_type type)

-- 
Regards,

Laurent Pinchart