[PATCH 0/9] media: uvcvideo: Invert granular PM logic + PM fix

[PATCH 0/9] media: uvcvideo: Invert granular PM logic + PM fix

[Ricardo Ribalda]

It makes more sense to have a list of the ioctls that need power than
the other way around. This patchset takes care of this.

It also fixes one error in the PM logic introduced in a recent patchset.

To support CI I have included two patches that are in uvc/next but not
in media-committer:
media: uvcvideo: Refactor uvc_queue_streamon
media: uvcvideo: Refactor uvc_v4l2_compat_ioctl32
Do not review them again.

To avoid conflicts I am adding the fop patchset as well:
media: uvcvideo: Use vb2 ioctl and fop helpers
media: uvcvideo: Remove stream->is_streaming field
Please review them in https://patchwork.linuxtv.org/project/linux-media/list/?series=15514

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Hans Verkuil (1):
      media: uvcvideo: Use vb2 ioctl and fop helpers

Ricardo Ribalda (8):
      media: uvcvideo: Refactor uvc_queue_streamon
      media: uvcvideo: Refactor uvc_v4l2_compat_ioctl32
      media: uvcvideo: Remove stream->is_streaming field
      media: uvcvideo: Turn on the camera if V4L2_EVENT_SUB_FL_SEND_INITIAL
      media: uvcvideo: Do not enable camera during UVCIOC_CTRL_MAP32
      media: uvcvideo: uvc_v4l2_unlocked_ioctl: Invert PM logic
      media: uvcvideo: Do not turn on the camera unless is needed
      media: uvcvideo: Support granular power saving for compat syscalls

 drivers/media/usb/uvc/uvc_ctrl.c     |   8 +
 drivers/media/usb/uvc/uvc_driver.c   |  34 +---
 drivers/media/usb/uvc/uvc_metadata.c |   8 +-
 drivers/media/usb/uvc/uvc_queue.c    | 143 ---------------
 drivers/media/usb/uvc/uvc_v4l2.c     | 339 +++++++----------------------------
 drivers/media/usb/uvc/uvcvideo.h     |  38 +---
 drivers/media/v4l2-core/v4l2-ioctl.c |   3 +-
 include/media/v4l2-ioctl.h           |   1 +
 8 files changed, 87 insertions(+), 487 deletions(-)
---
base-commit: 5e1ff2314797bf53636468a97719a8222deca9ae
change-id: 20250528-uvc-grannular-invert-19ad34c59391

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

[PATCH 1/9] media: uvcvideo: Refactor uvc_queue_streamon

[Ricardo Ribalda]

Do uvc_pm_get before we call uvc_queue_streamon. Although the current
code is correct, uvc_ioctl_streamon is allways called after uvc_pm_get,
this change makes the code more resiliant to future changes.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Link: https://lore.kernel.org/r/20250509-uvc-followup-v1-2-73bcde30d2b5@chromium.org
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 668a4e9d772c6d91f045ca75e2744b3a6c69da6b..862b4e34e5b629cf324479a9bb59ebe8784ccd5d 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -853,15 +853,16 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 	if (handle->is_streaming)
 		return 0;
 
-	ret = uvc_queue_streamon(&stream->queue, type);
+	ret = uvc_pm_get(stream->dev);
 	if (ret)
 		return ret;
 
-	ret = uvc_pm_get(stream->dev);
+	ret = uvc_queue_streamon(&stream->queue, type);
 	if (ret) {
-		uvc_queue_streamoff(&stream->queue, type);
+		uvc_pm_put(stream->dev);
 		return ret;
 	}
+
 	handle->is_streaming = true;
 
 	return 0;

-- 
2.49.0.1266.g31b7d2e469-goog

[PATCH 2/9] media: uvcvideo: Refactor uvc_v4l2_compat_ioctl32

[Ricardo Ribalda]

Declaring a variable for doing automatic cleanup is not a very common
pattern. Replace the cleanup macro with manual cleanup to make the code
simpler.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Link: https://lore.kernel.org/r/20250509-uvc-followup-v1-3-73bcde30d2b5@chromium.org
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 862b4e34e5b629cf324479a9bb59ebe8784ccd5d..1abdf1ea39956bbbadd3f166f37bdac518068648 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1382,11 +1382,9 @@ static int uvc_v4l2_put_xu_query(const struct uvc_xu_control_query *kp,
 #define UVCIOC_CTRL_MAP32	_IOWR('u', 0x20, struct uvc_xu_control_mapping32)
 #define UVCIOC_CTRL_QUERY32	_IOWR('u', 0x21, struct uvc_xu_control_query32)
 
-DEFINE_FREE(uvc_pm_put, struct uvc_device *, if (_T) uvc_pm_put(_T))
 static long uvc_v4l2_compat_ioctl32(struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
-	struct uvc_device *uvc_device __free(uvc_pm_put) = NULL;
 	struct uvc_fh *handle = file->private_data;
 	union {
 		struct uvc_xu_control_mapping xmap;
@@ -1399,38 +1397,38 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 	if (ret)
 		return ret;
 
-	uvc_device = handle->stream->dev;
-
 	switch (cmd) {
 	case UVCIOC_CTRL_MAP32:
 		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_ioctl_xu_ctrl_map(handle->chain, &karg.xmap);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_v4l2_put_xu_mapping(&karg.xmap, up);
 		if (ret)
-			return ret;
-
+			break;
 		break;
 
 	case UVCIOC_CTRL_QUERY32:
 		ret = uvc_v4l2_get_xu_query(&karg.xqry, up);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_xu_ctrl_query(handle->chain, &karg.xqry);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_v4l2_put_xu_query(&karg.xqry, up);
 		if (ret)
-			return ret;
+			break;
 		break;
 
 	default:
-		return -ENOIOCTLCMD;
+		ret = -ENOIOCTLCMD;
+		break;
 	}
 
+	uvc_pm_put(handle->stream->dev);
+
 	return ret;
 }
 #endif

-- 
2.49.0.1266.g31b7d2e469-goog

[PATCH 3/9] 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
boilerplatecode and simplifies VIDIOC_G/S_PRIORITY handling 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 gran exclusive ownership of the device.

Reviewed-by: Hans de Goede <hansg@kernel.org>
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   |  34 +----
 drivers/media/usb/uvc/uvc_metadata.c |   8 +-
 drivers/media/usb/uvc/uvc_queue.c    | 143 --------------------
 drivers/media/usb/uvc/uvc_v4l2.c     | 251 +++--------------------------------
 drivers/media/usb/uvc/uvcvideo.h     |  37 +-----
 5 files changed, 30 insertions(+), 443 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index da24a655ab68cc0957762f2b67387677c22224d1..4eeedab93b90939fc4c925012a18b7d018ade39f 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1725,7 +1725,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;
 }
@@ -1958,31 +1957,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);
 
 		/*
@@ -2029,7 +2004,8 @@ 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)
 		vdev->vfl_dir = VFL_DIR_TX;
 	else
@@ -2399,8 +2375,8 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
 		if (stream->intf == intf) {
 			ret = uvc_video_resume(stream, reset);
 			if (ret < 0)
-				uvc_queue_streamoff(&stream->queue,
-						    stream->queue.queue.type);
+				vb2_streamoff(&stream->queue.queue,
+					      stream->queue.queue.type);
 			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..49cc64dd7e2e737f431b9df9df68921d9c543751 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,18 +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);
+	vb2_fop_release(file);
 	file->private_data = NULL;
 
 	return 0;
@@ -753,91 +677,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 +684,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 +691,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 +707,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 +795,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 +1302,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 +1314,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 +1347,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 b9f8eb62ba1d82ea7788cf6c10cc838a429dbc9e..3ddbf065a2cbae40ee48cb06f84ca8f0052990c4 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -326,7 +326,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;
@@ -349,7 +352,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 */
 };
@@ -619,16 +621,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;
 };
@@ -687,36 +683,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.49.0.1266.g31b7d2e469-goog

[PATCH 4/9] 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.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 12 +++++-------
 drivers/media/usb/uvc/uvcvideo.h |  1 -
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 49cc64dd7e2e737f431b9df9df68921d9c543751..65c708b3fb1066bf2e8f12ab7cdf119452ad40f9 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -617,7 +617,8 @@ static int uvc_v4l2_release(struct file *file)
 
 	uvc_ctrl_cleanup_fh(handle);
 
-	if (handle->is_streaming)
+	if (stream->queue.queue.owner == file->private_data &&
+	    uvc_queue_streaming(&stream->queue))
 		uvc_pm_put(stream->dev);
 
 	/* Release the file handle. */
@@ -684,7 +685,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 	struct uvc_streaming *stream = handle->stream;
 	int ret;
 
-	if (handle->is_streaming)
+	if (uvc_queue_streaming(&stream->queue))
 		return 0;
 
 	ret = uvc_pm_get(stream->dev);
@@ -697,8 +698,6 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 		return ret;
 	}
 
-	handle->is_streaming = true;
-
 	return 0;
 }
 
@@ -707,16 +706,15 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
+	bool was_streaming = uvc_queue_streaming(&stream->queue);
 	int ret;
 
 	ret = vb2_ioctl_streamoff(file, fh, type);
 	if (ret)
 		return ret;
 
-	if (handle->is_streaming) {
-		handle->is_streaming = false;
+	if (was_streaming)
 		uvc_pm_put(stream->dev);
-	}
 
 	return 0;
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 3ddbf065a2cbae40ee48cb06f84ca8f0052990c4..f895f690f7cdc1af942d5f3a5f10e9dd1c956a35 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -626,7 +626,6 @@ struct uvc_fh {
 	struct uvc_video_chain *chain;
 	struct uvc_streaming *stream;
 	unsigned int pending_async_ctrls;
-	bool is_streaming;
 };
 
 /* ------------------------------------------------------------------------

-- 
2.49.0.1266.g31b7d2e469-goog

[PATCH 5/9] media: uvcvideo: Turn on the camera if V4L2_EVENT_SUB_FL_SEND_INITIAL

[Ricardo Ribalda]

If we subscribe to an event with V4L2_EVENT_SUB_FL_SEND_INITIAL, the
driver needs to report back some values that require the camera to be
powered on. But VIDIOC_SUBSCRIBE_EVENT is not part of the ioctls that
turn on the camera.

We could unconditionally turn on the camera during
VIDIOC_SUBSCRIBE_EVENT, but it is more efficient to turn it on only
during V4L2_EVENT_SUB_FL_SEND_INITIAL, which we believe is not a common
usecase.

Fixes: d1b618e79548 ("media: uvcvideo: Do not turn on the camera for some ioctls")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 44b6513c526421943bb9841fb53dc5f8e9f93f02..a7b8f3ea01edd8157e0d8cc36351d511225f89d7 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2039,6 +2039,12 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
 		u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
 		s32 val = 0;
 
+		ret = uvc_pm_get(handle->chain->dev);
+		if (ret) {
+			list_del(&sev->node);
+			goto done;
+		}
+
 		if (uvc_ctrl_mapping_is_compound(mapping) ||
 		    __uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
 			changes |= V4L2_EVENT_CTRL_CH_VALUE;
@@ -2051,6 +2057,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
 		 */
 		sev->elems = elems;
 		v4l2_event_queue_fh(sev->fh, &ev);
+
+		uvc_pm_put(handle->chain->dev);
 	}
 
 done:

-- 
2.49.0.1266.g31b7d2e469-goog

Re: [PATCH 5/9] media: uvcvideo: Turn on the camera if V4L2_EVENT_SUB_FL_SEND_INITIAL

[Hans de Goede]

Hi Ricardo,

Thank you for your patch.

On 28-May-25 19:58, Ricardo Ribalda wrote:
> If we subscribe to an event with V4L2_EVENT_SUB_FL_SEND_INITIAL, the
> driver needs to report back some values that require the camera to be
> powered on. But VIDIOC_SUBSCRIBE_EVENT is not part of the ioctls that
> turn on the camera.
> 
> We could unconditionally turn on the camera during
> VIDIOC_SUBSCRIBE_EVENT, but it is more efficient to turn it on only
> during V4L2_EVENT_SUB_FL_SEND_INITIAL, which we believe is not a common
> usecase.
> 
> Fixes: d1b618e79548 ("media: uvcvideo: Do not turn on the camera for some ioctls")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 44b6513c526421943bb9841fb53dc5f8e9f93f02..a7b8f3ea01edd8157e0d8cc36351d511225f89d7 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2039,6 +2039,12 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
>  		u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
>  		s32 val = 0;
>  
> +		ret = uvc_pm_get(handle->chain->dev);
> +		if (ret) {
> +			list_del(&sev->node);

uvc_ctrl_add_event() holds chain->ctrl_mutex and the only consumer of
the ev_subs list "uvc_ctrl_send_event() also only gets called with
chain->ctrl_mutex held.

So instead of undoing the list_add_tail() here, it would be better
to just move the list_add_tail() call to below the
"if (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL) { ... }" block
(just above the done label).

This avoids the need for the list_del() here.

With that changed:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans




> +			goto done;
> +		}
> +
>  		if (uvc_ctrl_mapping_is_compound(mapping) ||
>  		    __uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
>  			changes |= V4L2_EVENT_CTRL_CH_VALUE;
> @@ -2051,6 +2057,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
>  		 */
>  		sev->elems = elems;
>  		v4l2_event_queue_fh(sev->fh, &ev);
> +
> +		uvc_pm_put(handle->chain->dev);
>  	}
>  
>  done:
> 

[PATCH 6/9] media: uvcvideo: Do not enable camera during UVCIOC_CTRL_MAP32

[Ricardo Ribalda]

The device does not need to be enabled to do this, it is merely an
internal data operation.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 65c708b3fb1066bf2e8f12ab7cdf119452ad40f9..2c6f3cf6bcc3f116bbdb3383d9af7d5be9832537 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1224,10 +1224,6 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 	void __user *up = compat_ptr(arg);
 	long ret;
 
-	ret = uvc_pm_get(handle->stream->dev);
-	if (ret)
-		return ret;
-
 	switch (cmd) {
 	case UVCIOC_CTRL_MAP32:
 		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
@@ -1245,7 +1241,13 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 		ret = uvc_v4l2_get_xu_query(&karg.xqry, up);
 		if (ret)
 			break;
+
+		ret = uvc_pm_get(handle->stream->dev);
+		if (ret)
+			return ret;
 		ret = uvc_xu_ctrl_query(handle->chain, &karg.xqry);
+		uvc_pm_put(handle->stream->dev);
+
 		if (ret)
 			break;
 		ret = uvc_v4l2_put_xu_query(&karg.xqry, up);
@@ -1258,8 +1260,6 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 		break;
 	}
 
-	uvc_pm_put(handle->stream->dev);
-
 	return ret;
 }
 #endif

-- 
2.49.0.1266.g31b7d2e469-goog

Re: [PATCH 6/9] media: uvcvideo: Do not enable camera during UVCIOC_CTRL_MAP32

[Hans de Goede]

Hi Ricardo,

On 28-May-25 19:58, Ricardo Ribalda wrote:
> The device does not need to be enabled to do this, it is merely an
> internal data operation.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 65c708b3fb1066bf2e8f12ab7cdf119452ad40f9..2c6f3cf6bcc3f116bbdb3383d9af7d5be9832537 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1224,10 +1224,6 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  	void __user *up = compat_ptr(arg);
>  	long ret;
>  
> -	ret = uvc_pm_get(handle->stream->dev);
> -	if (ret)
> -		return ret;
> -
>  	switch (cmd) {
>  	case UVCIOC_CTRL_MAP32:
>  		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
> @@ -1245,7 +1241,13 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  		ret = uvc_v4l2_get_xu_query(&karg.xqry, up);
>  		if (ret)
>  			break;
> +
> +		ret = uvc_pm_get(handle->stream->dev);
> +		if (ret)
> +			return ret;

The rest of the code here uses:

		if (ret)
			break;

as pattern, please also use that for the uvc_pm_get() error handling

Otherwise this looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans




>  		ret = uvc_xu_ctrl_query(handle->chain, &karg.xqry);
> +		uvc_pm_put(handle->stream->dev);
> +
>  		if (ret)
>  			break;
>  		ret = uvc_v4l2_put_xu_query(&karg.xqry, up);
> @@ -1258,8 +1260,6 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  		break;
>  	}
>  
> -	uvc_pm_put(handle->stream->dev);
> -
>  	return ret;
>  }
>  #endif
> 

[PATCH 7/9] media: uvcvideo: uvc_v4l2_unlocked_ioctl: Invert PM logic

[Ricardo Ribalda]

Instead of listing the IOCTLs that do not need to turn on the camera,
list the IOCTLs that need to turn it on. This makes the code more
maintainable.

This patch is designed to be a NOP. Non relevant IOCTLs will be removed
in future patches.

Suggested-by: Hans Verkuil <hans@jjverkuil.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 117 +++++++++++++++++++++++++++++++--------
 1 file changed, 93 insertions(+), 24 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 2c6f3cf6bcc3f116bbdb3383d9af7d5be9832537..e7373a2ae3c37ca02f9076773154901a603820ac 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1264,42 +1264,111 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 }
 #endif
 
-static long uvc_v4l2_unlocked_ioctl(struct file *file,
-				    unsigned int cmd, unsigned long arg)
+static long uvc_v4l2_pm_ioctl(struct file *file,
+			      unsigned int cmd, unsigned long arg)
 {
 	struct uvc_fh *handle = file->private_data;
 	int ret;
 
-	/* The following IOCTLs do not need to turn on the camera. */
-	switch (cmd) {
-	case VIDIOC_CREATE_BUFS:
-	case VIDIOC_DQBUF:
-	case VIDIOC_ENUM_FMT:
-	case VIDIOC_ENUM_FRAMEINTERVALS:
-	case VIDIOC_ENUM_FRAMESIZES:
-	case VIDIOC_ENUMINPUT:
-	case VIDIOC_EXPBUF:
-	case VIDIOC_G_FMT:
-	case VIDIOC_G_PARM:
-	case VIDIOC_G_SELECTION:
-	case VIDIOC_QBUF:
-	case VIDIOC_QUERYCAP:
-	case VIDIOC_REQBUFS:
-	case VIDIOC_SUBSCRIBE_EVENT:
-	case VIDIOC_UNSUBSCRIBE_EVENT:
-		return video_ioctl2(file, cmd, arg);
-	}
-
 	ret = uvc_pm_get(handle->stream->dev);
 	if (ret)
 		return ret;
-
 	ret = video_ioctl2(file, cmd, arg);
-
 	uvc_pm_put(handle->stream->dev);
+
 	return ret;
 }
 
+static long uvc_v4l2_unlocked_ioctl(struct file *file,
+				    unsigned int cmd, unsigned long arg)
+{
+	/*
+	 * For now, we do not support granular power saving for compat
+	 * syscalls.
+	 */
+	if (in_compat_syscall())
+		return uvc_v4l2_pm_ioctl(file, cmd, arg);
+
+	/* The following IOCTLs do need to turn on the camera. */
+	switch (cmd) {
+	case UVCIOC_CTRL_MAP:
+	case UVCIOC_CTRL_QUERY:
+	case VIDIOC_CROPCAP:
+	case VIDIOC_DBG_G_CHIP_INFO:
+	case VIDIOC_DBG_G_REGISTER:
+	case VIDIOC_DBG_S_REGISTER:
+	case VIDIOC_DECODER_CMD:
+	case VIDIOC_DQEVENT:
+	case VIDIOC_DV_TIMINGS_CAP:
+	case VIDIOC_ENCODER_CMD:
+	case VIDIOC_ENUMAUDIO:
+	case VIDIOC_ENUMAUDOUT:
+	case VIDIOC_ENUMOUTPUT:
+	case VIDIOC_ENUMSTD:
+	case VIDIOC_ENUM_DV_TIMINGS:
+	case VIDIOC_ENUM_FREQ_BANDS:
+	case VIDIOC_G_AUDIO:
+	case VIDIOC_G_AUDOUT:
+	case VIDIOC_G_CROP:
+	case VIDIOC_G_CTRL:
+	case VIDIOC_G_DV_TIMINGS:
+	case VIDIOC_G_EDID:
+	case VIDIOC_G_ENC_INDEX:
+	case VIDIOC_G_EXT_CTRLS:
+	case VIDIOC_G_FBUF:
+	case VIDIOC_G_FREQUENCY:
+	case VIDIOC_G_INPUT:
+	case VIDIOC_G_JPEGCOMP:
+	case VIDIOC_G_MODULATOR:
+	case VIDIOC_G_OUTPUT:
+	case VIDIOC_G_PRIORITY:
+	case VIDIOC_G_SLICED_VBI_CAP:
+	case VIDIOC_G_STD:
+	case VIDIOC_G_TUNER:
+	case VIDIOC_LOG_STATUS:
+	case VIDIOC_OVERLAY:
+	case VIDIOC_PREPARE_BUF:
+	case VIDIOC_QUERYBUF:
+	case VIDIOC_QUERYCAP:
+	case VIDIOC_QUERYCTRL:
+	case VIDIOC_QUERYMENU:
+	case VIDIOC_QUERYSTD:
+	case VIDIOC_QUERY_DV_TIMINGS:
+	case VIDIOC_QUERY_EXT_CTRL:
+	case VIDIOC_REMOVE_BUFS:
+	case VIDIOC_STREAMOFF:
+	case VIDIOC_STREAMON:
+	case VIDIOC_S_AUDIO:
+	case VIDIOC_S_AUDOUT:
+	case VIDIOC_S_CROP:
+	case VIDIOC_S_CTRL:
+	case VIDIOC_S_DV_TIMINGS:
+	case VIDIOC_S_EDID:
+	case VIDIOC_S_EXT_CTRLS:
+	case VIDIOC_S_FBUF:
+	case VIDIOC_S_FMT:
+	case VIDIOC_S_FREQUENCY:
+	case VIDIOC_S_HW_FREQ_SEEK:
+	case VIDIOC_S_INPUT:
+	case VIDIOC_S_JPEGCOMP:
+	case VIDIOC_S_MODULATOR:
+	case VIDIOC_S_OUTPUT:
+	case VIDIOC_S_PARM:
+	case VIDIOC_S_PRIORITY:
+	case VIDIOC_S_SELECTION:
+	case VIDIOC_S_STD:
+	case VIDIOC_S_TUNER:
+	case VIDIOC_TRY_DECODER_CMD:
+	case VIDIOC_TRY_ENCODER_CMD:
+	case VIDIOC_TRY_EXT_CTRLS:
+	case VIDIOC_TRY_FMT:
+		return uvc_v4l2_pm_ioctl(file, cmd, arg);
+	}
+
+	/* The other IOCTLs can run with the camera off. */
+	return video_ioctl2(file, cmd, arg);
+}
+
 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,

-- 
2.49.0.1266.g31b7d2e469-goog

Re: [PATCH 7/9] media: uvcvideo: uvc_v4l2_unlocked_ioctl: Invert PM logic

[Hans de Goede]

Hi Ricardo,

On 28-May-25 19:58, Ricardo Ribalda wrote:
> Instead of listing the IOCTLs that do not need to turn on the camera,
> list the IOCTLs that need to turn it on. This makes the code more
> maintainable.
> 
> This patch is designed to be a NOP. Non relevant IOCTLs will be removed
> in future patches.
> 
> Suggested-by: Hans Verkuil <hans@jjverkuil.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 117 +++++++++++++++++++++++++++++++--------
>  1 file changed, 93 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 2c6f3cf6bcc3f116bbdb3383d9af7d5be9832537..e7373a2ae3c37ca02f9076773154901a603820ac 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1264,42 +1264,111 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  }
>  #endif
>  
> -static long uvc_v4l2_unlocked_ioctl(struct file *file,
> -				    unsigned int cmd, unsigned long arg)
> +static long uvc_v4l2_pm_ioctl(struct file *file,
> +			      unsigned int cmd, unsigned long arg)
>  {
>  	struct uvc_fh *handle = file->private_data;
>  	int ret;
>  
> -	/* The following IOCTLs do not need to turn on the camera. */
> -	switch (cmd) {
> -	case VIDIOC_CREATE_BUFS:
> -	case VIDIOC_DQBUF:
> -	case VIDIOC_ENUM_FMT:
> -	case VIDIOC_ENUM_FRAMEINTERVALS:
> -	case VIDIOC_ENUM_FRAMESIZES:
> -	case VIDIOC_ENUMINPUT:
> -	case VIDIOC_EXPBUF:
> -	case VIDIOC_G_FMT:
> -	case VIDIOC_G_PARM:
> -	case VIDIOC_G_SELECTION:
> -	case VIDIOC_QBUF:
> -	case VIDIOC_QUERYCAP:
> -	case VIDIOC_REQBUFS:
> -	case VIDIOC_SUBSCRIBE_EVENT:
> -	case VIDIOC_UNSUBSCRIBE_EVENT:
> -		return video_ioctl2(file, cmd, arg);
> -	}
> -
>  	ret = uvc_pm_get(handle->stream->dev);
>  	if (ret)
>  		return ret;
> -
>  	ret = video_ioctl2(file, cmd, arg);
> -
>  	uvc_pm_put(handle->stream->dev);
> +
>  	return ret;
>  }
>  
> +static long uvc_v4l2_unlocked_ioctl(struct file *file,
> +				    unsigned int cmd, unsigned long arg)
> +{
> +	/*
> +	 * For now, we do not support granular power saving for compat
> +	 * syscalls.
> +	 */
> +	if (in_compat_syscall())
> +		return uvc_v4l2_pm_ioctl(file, cmd, arg);
> +
> +	/* The following IOCTLs do need to turn on the camera. */
> +	switch (cmd) {
> +	case UVCIOC_CTRL_MAP:
> +	case UVCIOC_CTRL_QUERY:
> +	case VIDIOC_CROPCAP:
> +	case VIDIOC_DBG_G_CHIP_INFO:
> +	case VIDIOC_DBG_G_REGISTER:
> +	case VIDIOC_DBG_S_REGISTER:
> +	case VIDIOC_DECODER_CMD:
> +	case VIDIOC_DQEVENT:
> +	case VIDIOC_DV_TIMINGS_CAP:
> +	case VIDIOC_ENCODER_CMD:
> +	case VIDIOC_ENUMAUDIO:
> +	case VIDIOC_ENUMAUDOUT:
> +	case VIDIOC_ENUMOUTPUT:
> +	case VIDIOC_ENUMSTD:
> +	case VIDIOC_ENUM_DV_TIMINGS:
> +	case VIDIOC_ENUM_FREQ_BANDS:
> +	case VIDIOC_G_AUDIO:
> +	case VIDIOC_G_AUDOUT:
> +	case VIDIOC_G_CROP:
> +	case VIDIOC_G_CTRL:
> +	case VIDIOC_G_DV_TIMINGS:
> +	case VIDIOC_G_EDID:
> +	case VIDIOC_G_ENC_INDEX:
> +	case VIDIOC_G_EXT_CTRLS:
> +	case VIDIOC_G_FBUF:
> +	case VIDIOC_G_FREQUENCY:
> +	case VIDIOC_G_INPUT:
> +	case VIDIOC_G_JPEGCOMP:
> +	case VIDIOC_G_MODULATOR:
> +	case VIDIOC_G_OUTPUT:
> +	case VIDIOC_G_PRIORITY:
> +	case VIDIOC_G_SLICED_VBI_CAP:
> +	case VIDIOC_G_STD:
> +	case VIDIOC_G_TUNER:
> +	case VIDIOC_LOG_STATUS:
> +	case VIDIOC_OVERLAY:
> +	case VIDIOC_PREPARE_BUF:
> +	case VIDIOC_QUERYBUF:
> +	case VIDIOC_QUERYCAP:
> +	case VIDIOC_QUERYCTRL:
> +	case VIDIOC_QUERYMENU:
> +	case VIDIOC_QUERYSTD:
> +	case VIDIOC_QUERY_DV_TIMINGS:
> +	case VIDIOC_QUERY_EXT_CTRL:
> +	case VIDIOC_REMOVE_BUFS:
> +	case VIDIOC_STREAMOFF:
> +	case VIDIOC_STREAMON:
> +	case VIDIOC_S_AUDIO:
> +	case VIDIOC_S_AUDOUT:
> +	case VIDIOC_S_CROP:
> +	case VIDIOC_S_CTRL:
> +	case VIDIOC_S_DV_TIMINGS:
> +	case VIDIOC_S_EDID:
> +	case VIDIOC_S_EXT_CTRLS:
> +	case VIDIOC_S_FBUF:
> +	case VIDIOC_S_FMT:
> +	case VIDIOC_S_FREQUENCY:
> +	case VIDIOC_S_HW_FREQ_SEEK:
> +	case VIDIOC_S_INPUT:
> +	case VIDIOC_S_JPEGCOMP:
> +	case VIDIOC_S_MODULATOR:
> +	case VIDIOC_S_OUTPUT:
> +	case VIDIOC_S_PARM:
> +	case VIDIOC_S_PRIORITY:
> +	case VIDIOC_S_SELECTION:
> +	case VIDIOC_S_STD:
> +	case VIDIOC_S_TUNER:
> +	case VIDIOC_TRY_DECODER_CMD:
> +	case VIDIOC_TRY_ENCODER_CMD:
> +	case VIDIOC_TRY_EXT_CTRLS:
> +	case VIDIOC_TRY_FMT:
> +		return uvc_v4l2_pm_ioctl(file, cmd, arg);
> +	}

IMHO adding a whole bunch of ioctls which are not supported by UVC at all here
and then dropping them again in patch 8/9 is not helpful.

I understand that your purpose is to have this initial patch
be a no-op, still doing the uvc_pm_get() + uvc_pm_put() for
unsupported IOCTLs but I don't really see that as something
useful to do while at the same time introducing unnecessary churn.

My preference would be to drop unsupported IOCTLs from this list
right away with a remark in the commit message that this changes
the behavior for unsupported IOCTLs.

For something like UVCIOC_CTRL_MAP I do agree that it is best
to keep it here and drop it later separately. Or maybe it would
be cleaner to add UVCIOC_CTRL_MAP to the list of IOCTLs _not_
needing pm before this patch ?

Regards,

Hans



> +
> +	/* The other IOCTLs can run with the camera off. */
> +	return video_ioctl2(file, cmd, arg);
> +}
> +
>  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,
> 

[PATCH 8/9] media: uvcvideo: Do not turn on the camera unless is needed

[Ricardo Ribalda]

There are a lot of IOCTLs that do not need to turn on the camera. Eg:
- They only operate to internal data, like UVCIOC_CTRL_MAP.
- They are not applicable to the uvc driver, like VIDIOC_G_AUDIO.
- They are handled by the uvc framework, like VIDIOC_STREAMON.

Remove them from the turn-on list.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 57 ----------------------------------------
 1 file changed, 57 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index e7373a2ae3c37ca02f9076773154901a603820ac..fcb1b79c214849ce4da96a86a688d777b32cc688 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1291,75 +1291,18 @@ static long uvc_v4l2_unlocked_ioctl(struct file *file,
 
 	/* The following IOCTLs do need to turn on the camera. */
 	switch (cmd) {
-	case UVCIOC_CTRL_MAP:
 	case UVCIOC_CTRL_QUERY:
-	case VIDIOC_CROPCAP:
-	case VIDIOC_DBG_G_CHIP_INFO:
-	case VIDIOC_DBG_G_REGISTER:
-	case VIDIOC_DBG_S_REGISTER:
-	case VIDIOC_DECODER_CMD:
-	case VIDIOC_DQEVENT:
-	case VIDIOC_DV_TIMINGS_CAP:
-	case VIDIOC_ENCODER_CMD:
-	case VIDIOC_ENUMAUDIO:
-	case VIDIOC_ENUMAUDOUT:
-	case VIDIOC_ENUMOUTPUT:
-	case VIDIOC_ENUMSTD:
-	case VIDIOC_ENUM_DV_TIMINGS:
-	case VIDIOC_ENUM_FREQ_BANDS:
-	case VIDIOC_G_AUDIO:
-	case VIDIOC_G_AUDOUT:
-	case VIDIOC_G_CROP:
 	case VIDIOC_G_CTRL:
-	case VIDIOC_G_DV_TIMINGS:
-	case VIDIOC_G_EDID:
-	case VIDIOC_G_ENC_INDEX:
 	case VIDIOC_G_EXT_CTRLS:
-	case VIDIOC_G_FBUF:
-	case VIDIOC_G_FREQUENCY:
 	case VIDIOC_G_INPUT:
-	case VIDIOC_G_JPEGCOMP:
-	case VIDIOC_G_MODULATOR:
-	case VIDIOC_G_OUTPUT:
-	case VIDIOC_G_PRIORITY:
-	case VIDIOC_G_SLICED_VBI_CAP:
-	case VIDIOC_G_STD:
-	case VIDIOC_G_TUNER:
-	case VIDIOC_LOG_STATUS:
-	case VIDIOC_OVERLAY:
-	case VIDIOC_PREPARE_BUF:
-	case VIDIOC_QUERYBUF:
-	case VIDIOC_QUERYCAP:
 	case VIDIOC_QUERYCTRL:
 	case VIDIOC_QUERYMENU:
-	case VIDIOC_QUERYSTD:
-	case VIDIOC_QUERY_DV_TIMINGS:
 	case VIDIOC_QUERY_EXT_CTRL:
-	case VIDIOC_REMOVE_BUFS:
-	case VIDIOC_STREAMOFF:
-	case VIDIOC_STREAMON:
-	case VIDIOC_S_AUDIO:
-	case VIDIOC_S_AUDOUT:
-	case VIDIOC_S_CROP:
 	case VIDIOC_S_CTRL:
-	case VIDIOC_S_DV_TIMINGS:
-	case VIDIOC_S_EDID:
 	case VIDIOC_S_EXT_CTRLS:
-	case VIDIOC_S_FBUF:
 	case VIDIOC_S_FMT:
-	case VIDIOC_S_FREQUENCY:
-	case VIDIOC_S_HW_FREQ_SEEK:
 	case VIDIOC_S_INPUT:
-	case VIDIOC_S_JPEGCOMP:
-	case VIDIOC_S_MODULATOR:
-	case VIDIOC_S_OUTPUT:
 	case VIDIOC_S_PARM:
-	case VIDIOC_S_PRIORITY:
-	case VIDIOC_S_SELECTION:
-	case VIDIOC_S_STD:
-	case VIDIOC_S_TUNER:
-	case VIDIOC_TRY_DECODER_CMD:
-	case VIDIOC_TRY_ENCODER_CMD:
 	case VIDIOC_TRY_EXT_CTRLS:
 	case VIDIOC_TRY_FMT:
 		return uvc_v4l2_pm_ioctl(file, cmd, arg);

-- 
2.49.0.1266.g31b7d2e469-goog

[PATCH 9/9] media: uvcvideo: Support granular power saving for compat syscalls

[Ricardo Ribalda]

Right now we cannot support granular power saving on compat syscalls
because the VIDIOC_*32 NRs defines are not accessible to drivers.

Use the video_translate_cmd() helper to convert the compat syscall NRs
into syscall NRs.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c     | 9 ++-------
 drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++-
 include/media/v4l2-ioctl.h           | 1 +
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index fcb1b79c214849ce4da96a86a688d777b32cc688..048ee7e01808c8944f9bd46e5df2931b9c146ad5 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1282,15 +1282,10 @@ static long uvc_v4l2_pm_ioctl(struct file *file,
 static long uvc_v4l2_unlocked_ioctl(struct file *file,
 				    unsigned int cmd, unsigned long arg)
 {
-	/*
-	 * For now, we do not support granular power saving for compat
-	 * syscalls.
-	 */
-	if (in_compat_syscall())
-		return uvc_v4l2_pm_ioctl(file, cmd, arg);
+	unsigned int converted_cmd = video_translate_cmd(cmd);
 
 	/* The following IOCTLs do need to turn on the camera. */
-	switch (cmd) {
+	switch (converted_cmd) {
 	case UVCIOC_CTRL_QUERY:
 	case VIDIOC_G_CTRL:
 	case VIDIOC_G_EXT_CTRLS:
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 650dc1956f73d2f1943b56c42140c7b8d757259f..6fbd28f911cf23eec43ef1adcf64bd46ef067c81 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3245,7 +3245,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
 	return ret;
 }
 
-static unsigned int video_translate_cmd(unsigned int cmd)
+unsigned int video_translate_cmd(unsigned int cmd)
 {
 #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
 	switch (cmd) {
@@ -3266,6 +3266,7 @@ static unsigned int video_translate_cmd(unsigned int cmd)
 
 	return cmd;
 }
+EXPORT_SYMBOL(video_translate_cmd);
 
 static int video_get_user(void __user *arg, void *parg,
 			  unsigned int real_cmd, unsigned int cmd,
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index c6ec87e88dfef9e6cfe1d1fb587c1600882fb14d..437b9f90714c62e0ba434ce47391ef64d88110aa 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -687,6 +687,7 @@ int v4l2_compat_get_array_args(struct file *file, void *mbuf,
 int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr,
 			       void *mbuf, size_t array_size,
 			       unsigned int cmd, void *arg);
+unsigned int video_translate_cmd(unsigned int cmd);
 
 /**
  * typedef v4l2_kioctl - Typedef used to pass an ioctl handler.

-- 
2.49.0.1266.g31b7d2e469-goog

Re: [PATCH 9/9] media: uvcvideo: Support granular power saving for compat syscalls

[Hans de Goede]

Hi Ricardo,

On 28-May-25 19:58, Ricardo Ribalda wrote:
> Right now we cannot support granular power saving on compat syscalls
> because the VIDIOC_*32 NRs defines are not accessible to drivers.
> 
> Use the video_translate_cmd() helper to convert the compat syscall NRs
> into syscall NRs.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c     | 9 ++-------
>  drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++-
>  include/media/v4l2-ioctl.h           | 1 +
>  3 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index fcb1b79c214849ce4da96a86a688d777b32cc688..048ee7e01808c8944f9bd46e5df2931b9c146ad5 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1282,15 +1282,10 @@ static long uvc_v4l2_pm_ioctl(struct file *file,
>  static long uvc_v4l2_unlocked_ioctl(struct file *file,
>  				    unsigned int cmd, unsigned long arg)
>  {
> -	/*
> -	 * For now, we do not support granular power saving for compat
> -	 * syscalls.
> -	 */
> -	if (in_compat_syscall())
> -		return uvc_v4l2_pm_ioctl(file, cmd, arg);
> +	unsigned int converted_cmd = video_translate_cmd(cmd);

It looks like something went wrong here and you did not test-compile this?
video_translate_cmd() is private to drivers/media/v4l2-core/v4l2-ioctl.c
so this should not compile.

You can use v4l2_compat_translate_cmd() but only when CONFIG_COMPAT is set
otherwise that symbol is not available.

Regards,

Hans



>  
>  	/* The following IOCTLs do need to turn on the camera. */
> -	switch (cmd) {
> +	switch (converted_cmd) {
>  	case UVCIOC_CTRL_QUERY:
>  	case VIDIOC_G_CTRL:
>  	case VIDIOC_G_EXT_CTRLS:
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 650dc1956f73d2f1943b56c42140c7b8d757259f..6fbd28f911cf23eec43ef1adcf64bd46ef067c81 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3245,7 +3245,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>  	return ret;
>  }
>  
> -static unsigned int video_translate_cmd(unsigned int cmd)
> +unsigned int video_translate_cmd(unsigned int cmd)
>  {
>  #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
>  	switch (cmd) {
> @@ -3266,6 +3266,7 @@ static unsigned int video_translate_cmd(unsigned int cmd)
>  
>  	return cmd;
>  }
> +EXPORT_SYMBOL(video_translate_cmd);
>  
>  static int video_get_user(void __user *arg, void *parg,
>  			  unsigned int real_cmd, unsigned int cmd,
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index c6ec87e88dfef9e6cfe1d1fb587c1600882fb14d..437b9f90714c62e0ba434ce47391ef64d88110aa 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -687,6 +687,7 @@ int v4l2_compat_get_array_args(struct file *file, void *mbuf,
>  int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr,
>  			       void *mbuf, size_t array_size,
>  			       unsigned int cmd, void *arg);
> +unsigned int video_translate_cmd(unsigned int cmd);
>  
>  /**
>   * typedef v4l2_kioctl - Typedef used to pass an ioctl handler.
> 

Re: [PATCH 9/9] media: uvcvideo: Support granular power saving for compat syscalls

[Ricardo Ribalda]

Hi Hans

On Mon, 2 Jun 2025 at 12:11, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Ricardo,
>
> On 28-May-25 19:58, Ricardo Ribalda wrote:
> > Right now we cannot support granular power saving on compat syscalls
> > because the VIDIOC_*32 NRs defines are not accessible to drivers.
> >
> > Use the video_translate_cmd() helper to convert the compat syscall NRs
> > into syscall NRs.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_v4l2.c     | 9 ++-------
> >  drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++-
> >  include/media/v4l2-ioctl.h           | 1 +
> >  3 files changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index fcb1b79c214849ce4da96a86a688d777b32cc688..048ee7e01808c8944f9bd46e5df2931b9c146ad5 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -1282,15 +1282,10 @@ static long uvc_v4l2_pm_ioctl(struct file *file,
> >  static long uvc_v4l2_unlocked_ioctl(struct file *file,
> >                                   unsigned int cmd, unsigned long arg)
> >  {
> > -     /*
> > -      * For now, we do not support granular power saving for compat
> > -      * syscalls.
> > -      */
> > -     if (in_compat_syscall())
> > -             return uvc_v4l2_pm_ioctl(file, cmd, arg);
> > +     unsigned int converted_cmd = video_translate_cmd(cmd);
>
> It looks like something went wrong here and you did not test-compile this?
> video_translate_cmd() is private to drivers/media/v4l2-core/v4l2-ioctl.c
> so this should not compile.

Hmm... Actually I am pretty sure that I tested it on real hardware.

Did you miss the EXPORT_SYMBOL() on the patch?

>
> You can use v4l2_compat_translate_cmd() but only when CONFIG_COMPAT is set
> otherwise that symbol is not available.

I tried now without CONFIG_COMPAT and it built fine.

>
> Regards,
>
> Hans
>
>
>
> >
> >       /* The following IOCTLs do need to turn on the camera. */
> > -     switch (cmd) {
> > +     switch (converted_cmd) {
> >       case UVCIOC_CTRL_QUERY:
> >       case VIDIOC_G_CTRL:
> >       case VIDIOC_G_EXT_CTRLS:
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 650dc1956f73d2f1943b56c42140c7b8d757259f..6fbd28f911cf23eec43ef1adcf64bd46ef067c81 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -3245,7 +3245,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
> >       return ret;
> >  }
> >
> > -static unsigned int video_translate_cmd(unsigned int cmd)
> > +unsigned int video_translate_cmd(unsigned int cmd)
> >  {
> >  #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> >       switch (cmd) {
> > @@ -3266,6 +3266,7 @@ static unsigned int video_translate_cmd(unsigned int cmd)
> >
> >       return cmd;
> >  }
> > +EXPORT_SYMBOL(video_translate_cmd);
> >
> >  static int video_get_user(void __user *arg, void *parg,
> >                         unsigned int real_cmd, unsigned int cmd,
> > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> > index c6ec87e88dfef9e6cfe1d1fb587c1600882fb14d..437b9f90714c62e0ba434ce47391ef64d88110aa 100644
> > --- a/include/media/v4l2-ioctl.h
> > +++ b/include/media/v4l2-ioctl.h
> > @@ -687,6 +687,7 @@ int v4l2_compat_get_array_args(struct file *file, void *mbuf,
> >  int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr,
> >                              void *mbuf, size_t array_size,
> >                              unsigned int cmd, void *arg);
> > +unsigned int video_translate_cmd(unsigned int cmd);
> >
> >  /**
> >   * typedef v4l2_kioctl - Typedef used to pass an ioctl handler.
> >
>


-- 
Ricardo Ribalda

Re: [PATCH 9/9] media: uvcvideo: Support granular power saving for compat syscalls

[Hans de Goede]

Hi Ricardo,

On 2-Jun-25 12:27, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Mon, 2 Jun 2025 at 12:11, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi Ricardo,
>>
>> On 28-May-25 19:58, Ricardo Ribalda wrote:
>>> Right now we cannot support granular power saving on compat syscalls
>>> because the VIDIOC_*32 NRs defines are not accessible to drivers.
>>>
>>> Use the video_translate_cmd() helper to convert the compat syscall NRs
>>> into syscall NRs.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>>  drivers/media/usb/uvc/uvc_v4l2.c     | 9 ++-------
>>>  drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++-
>>>  include/media/v4l2-ioctl.h           | 1 +
>>>  3 files changed, 5 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>> index fcb1b79c214849ce4da96a86a688d777b32cc688..048ee7e01808c8944f9bd46e5df2931b9c146ad5 100644
>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>> @@ -1282,15 +1282,10 @@ static long uvc_v4l2_pm_ioctl(struct file *file,
>>>  static long uvc_v4l2_unlocked_ioctl(struct file *file,
>>>                                   unsigned int cmd, unsigned long arg)
>>>  {
>>> -     /*
>>> -      * For now, we do not support granular power saving for compat
>>> -      * syscalls.
>>> -      */
>>> -     if (in_compat_syscall())
>>> -             return uvc_v4l2_pm_ioctl(file, cmd, arg);
>>> +     unsigned int converted_cmd = video_translate_cmd(cmd);
>>
>> It looks like something went wrong here and you did not test-compile this?
>> video_translate_cmd() is private to drivers/media/v4l2-core/v4l2-ioctl.c
>> so this should not compile.
> 
> Hmm... Actually I am pretty sure that I tested it on real hardware.
> 
> Did you miss the EXPORT_SYMBOL() on the patch?

Ah yes I did miss that, sorry.

For the next time please split core changes out into their own
separate patches.

In this case I think the core changes are not necessary instead
you can just do:

	unsigned int converted_cmd = cmd;

#ifdef CONFIG_COMPAT
	converted_cmd = v4l2_compat_translate_cmd(cmd);
#endif

Regards,

Hans




> 
>>
>> You can use v4l2_compat_translate_cmd() but only when CONFIG_COMPAT is set
>> otherwise that symbol is not available.
> 
> I tried now without CONFIG_COMPAT and it built fine.
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>
>>>       /* The following IOCTLs do need to turn on the camera. */
>>> -     switch (cmd) {
>>> +     switch (converted_cmd) {
>>>       case UVCIOC_CTRL_QUERY:
>>>       case VIDIOC_G_CTRL:
>>>       case VIDIOC_G_EXT_CTRLS:
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 650dc1956f73d2f1943b56c42140c7b8d757259f..6fbd28f911cf23eec43ef1adcf64bd46ef067c81 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -3245,7 +3245,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>>>       return ret;
>>>  }
>>>
>>> -static unsigned int video_translate_cmd(unsigned int cmd)
>>> +unsigned int video_translate_cmd(unsigned int cmd)
>>>  {
>>>  #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
>>>       switch (cmd) {
>>> @@ -3266,6 +3266,7 @@ static unsigned int video_translate_cmd(unsigned int cmd)
>>>
>>>       return cmd;
>>>  }
>>> +EXPORT_SYMBOL(video_translate_cmd);
>>>
>>>  static int video_get_user(void __user *arg, void *parg,
>>>                         unsigned int real_cmd, unsigned int cmd,
>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>> index c6ec87e88dfef9e6cfe1d1fb587c1600882fb14d..437b9f90714c62e0ba434ce47391ef64d88110aa 100644
>>> --- a/include/media/v4l2-ioctl.h
>>> +++ b/include/media/v4l2-ioctl.h
>>> @@ -687,6 +687,7 @@ int v4l2_compat_get_array_args(struct file *file, void *mbuf,
>>>  int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr,
>>>                              void *mbuf, size_t array_size,
>>>                              unsigned int cmd, void *arg);
>>> +unsigned int video_translate_cmd(unsigned int cmd);
>>>
>>>  /**
>>>   * typedef v4l2_kioctl - Typedef used to pass an ioctl handler.
>>>
>>
> 
> 

Re: [PATCH 9/9] media: uvcvideo: Support granular power saving for compat syscalls

[Ricardo Ribalda]

On Mon, 2 Jun 2025 at 13:07, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Ricardo,
>
> On 2-Jun-25 12:27, Ricardo Ribalda wrote:
> > Hi Hans
> >
> > On Mon, 2 Jun 2025 at 12:11, Hans de Goede <hansg@kernel.org> wrote:
> >>
> >> Hi Ricardo,
> >>
> >> On 28-May-25 19:58, Ricardo Ribalda wrote:
> >>> Right now we cannot support granular power saving on compat syscalls
> >>> because the VIDIOC_*32 NRs defines are not accessible to drivers.
> >>>
> >>> Use the video_translate_cmd() helper to convert the compat syscall NRs
> >>> into syscall NRs.
> >>>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>>  drivers/media/usb/uvc/uvc_v4l2.c     | 9 ++-------
> >>>  drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++-
> >>>  include/media/v4l2-ioctl.h           | 1 +
> >>>  3 files changed, 5 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> >>> index fcb1b79c214849ce4da96a86a688d777b32cc688..048ee7e01808c8944f9bd46e5df2931b9c146ad5 100644
> >>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >>> @@ -1282,15 +1282,10 @@ static long uvc_v4l2_pm_ioctl(struct file *file,
> >>>  static long uvc_v4l2_unlocked_ioctl(struct file *file,
> >>>                                   unsigned int cmd, unsigned long arg)
> >>>  {
> >>> -     /*
> >>> -      * For now, we do not support granular power saving for compat
> >>> -      * syscalls.
> >>> -      */
> >>> -     if (in_compat_syscall())
> >>> -             return uvc_v4l2_pm_ioctl(file, cmd, arg);
> >>> +     unsigned int converted_cmd = video_translate_cmd(cmd);
> >>
> >> It looks like something went wrong here and you did not test-compile this?
> >> video_translate_cmd() is private to drivers/media/v4l2-core/v4l2-ioctl.c
> >> so this should not compile.
> >
> > Hmm... Actually I am pretty sure that I tested it on real hardware.
> >
> > Did you miss the EXPORT_SYMBOL() on the patch?
>
> Ah yes I did miss that, sorry.

My bad, I doubt it till the last second if I should split it or not :)

>
> For the next time please split core changes out into their own
> separate patches.
>
> In this case I think the core changes are not necessary instead
> you can just do:
>
>         unsigned int converted_cmd = cmd;
>
> #ifdef CONFIG_COMPAT
>         converted_cmd = v4l2_compat_translate_cmd(cmd);
> #endif

I believe this should work as well:

unsigned int converted_cmd = cmd;
if (in_compat_syscall())
  converted_cmd = v4l2_compat_translate_cmd(cmd);

the compiler knows that CONFIG_COMPAT=n means in_compat_syscall() will
be always fails.

If it is ok with you (and it actually works :) ) I will use this version.

Regards

>
> Regards,
>
> Hans
>
>
>
>
> >
> >>
> >> You can use v4l2_compat_translate_cmd() but only when CONFIG_COMPAT is set
> >> otherwise that symbol is not available.
> >
> > I tried now without CONFIG_COMPAT and it built fine.
> >
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> >>>
> >>>       /* The following IOCTLs do need to turn on the camera. */
> >>> -     switch (cmd) {
> >>> +     switch (converted_cmd) {
> >>>       case UVCIOC_CTRL_QUERY:
> >>>       case VIDIOC_G_CTRL:
> >>>       case VIDIOC_G_EXT_CTRLS:
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>> index 650dc1956f73d2f1943b56c42140c7b8d757259f..6fbd28f911cf23eec43ef1adcf64bd46ef067c81 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>> @@ -3245,7 +3245,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
> >>>       return ret;
> >>>  }
> >>>
> >>> -static unsigned int video_translate_cmd(unsigned int cmd)
> >>> +unsigned int video_translate_cmd(unsigned int cmd)
> >>>  {
> >>>  #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> >>>       switch (cmd) {
> >>> @@ -3266,6 +3266,7 @@ static unsigned int video_translate_cmd(unsigned int cmd)
> >>>
> >>>       return cmd;
> >>>  }
> >>> +EXPORT_SYMBOL(video_translate_cmd);
> >>>
> >>>  static int video_get_user(void __user *arg, void *parg,
> >>>                         unsigned int real_cmd, unsigned int cmd,
> >>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> >>> index c6ec87e88dfef9e6cfe1d1fb587c1600882fb14d..437b9f90714c62e0ba434ce47391ef64d88110aa 100644
> >>> --- a/include/media/v4l2-ioctl.h
> >>> +++ b/include/media/v4l2-ioctl.h
> >>> @@ -687,6 +687,7 @@ int v4l2_compat_get_array_args(struct file *file, void *mbuf,
> >>>  int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr,
> >>>                              void *mbuf, size_t array_size,
> >>>                              unsigned int cmd, void *arg);
> >>> +unsigned int video_translate_cmd(unsigned int cmd);
> >>>
> >>>  /**
> >>>   * typedef v4l2_kioctl - Typedef used to pass an ioctl handler.
> >>>
> >>
> >
> >
>


-- 
Ricardo Ribalda

Re: [PATCH 9/9] media: uvcvideo: Support granular power saving for compat syscalls

[Hans de Goede]

On 2-Jun-25 13:11, Ricardo Ribalda wrote:
> On Mon, 2 Jun 2025 at 13:07, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi Ricardo,
>>
>> On 2-Jun-25 12:27, Ricardo Ribalda wrote:
>>> Hi Hans
>>>
>>> On Mon, 2 Jun 2025 at 12:11, Hans de Goede <hansg@kernel.org> wrote:
>>>>
>>>> Hi Ricardo,
>>>>
>>>> On 28-May-25 19:58, Ricardo Ribalda wrote:
>>>>> Right now we cannot support granular power saving on compat syscalls
>>>>> because the VIDIOC_*32 NRs defines are not accessible to drivers.
>>>>>
>>>>> Use the video_translate_cmd() helper to convert the compat syscall NRs
>>>>> into syscall NRs.
>>>>>
>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>> ---
>>>>>  drivers/media/usb/uvc/uvc_v4l2.c     | 9 ++-------
>>>>>  drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++-
>>>>>  include/media/v4l2-ioctl.h           | 1 +
>>>>>  3 files changed, 5 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>> index fcb1b79c214849ce4da96a86a688d777b32cc688..048ee7e01808c8944f9bd46e5df2931b9c146ad5 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>> @@ -1282,15 +1282,10 @@ static long uvc_v4l2_pm_ioctl(struct file *file,
>>>>>  static long uvc_v4l2_unlocked_ioctl(struct file *file,
>>>>>                                   unsigned int cmd, unsigned long arg)
>>>>>  {
>>>>> -     /*
>>>>> -      * For now, we do not support granular power saving for compat
>>>>> -      * syscalls.
>>>>> -      */
>>>>> -     if (in_compat_syscall())
>>>>> -             return uvc_v4l2_pm_ioctl(file, cmd, arg);
>>>>> +     unsigned int converted_cmd = video_translate_cmd(cmd);
>>>>
>>>> It looks like something went wrong here and you did not test-compile this?
>>>> video_translate_cmd() is private to drivers/media/v4l2-core/v4l2-ioctl.c
>>>> so this should not compile.
>>>
>>> Hmm... Actually I am pretty sure that I tested it on real hardware.
>>>
>>> Did you miss the EXPORT_SYMBOL() on the patch?
>>
>> Ah yes I did miss that, sorry.
> 
> My bad, I doubt it till the last second if I should split it or not :)
> 
>>
>> For the next time please split core changes out into their own
>> separate patches.
>>
>> In this case I think the core changes are not necessary instead
>> you can just do:
>>
>>         unsigned int converted_cmd = cmd;
>>
>> #ifdef CONFIG_COMPAT
>>         converted_cmd = v4l2_compat_translate_cmd(cmd);
>> #endif
> 
> I believe this should work as well:
> 
> unsigned int converted_cmd = cmd;
> if (in_compat_syscall())
>   converted_cmd = v4l2_compat_translate_cmd(cmd);
> 
> the compiler knows that CONFIG_COMPAT=n means in_compat_syscall() will
> be always fails.
> 
> If it is ok with you (and it actually works :) ) I will use this version.

I agree that that is cleaner/better and I also think it should work,
so lets go with that.

Regards,

Hans



>>>> You can use v4l2_compat_translate_cmd() but only when CONFIG_COMPAT is set
>>>> otherwise that symbol is not available.
>>>
>>> I tried now without CONFIG_COMPAT and it built fine.
>>>
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>
>>>>>
>>>>>       /* The following IOCTLs do need to turn on the camera. */
>>>>> -     switch (cmd) {
>>>>> +     switch (converted_cmd) {
>>>>>       case UVCIOC_CTRL_QUERY:
>>>>>       case VIDIOC_G_CTRL:
>>>>>       case VIDIOC_G_EXT_CTRLS:
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> index 650dc1956f73d2f1943b56c42140c7b8d757259f..6fbd28f911cf23eec43ef1adcf64bd46ef067c81 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>>> @@ -3245,7 +3245,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>>>>>       return ret;
>>>>>  }
>>>>>
>>>>> -static unsigned int video_translate_cmd(unsigned int cmd)
>>>>> +unsigned int video_translate_cmd(unsigned int cmd)
>>>>>  {
>>>>>  #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
>>>>>       switch (cmd) {
>>>>> @@ -3266,6 +3266,7 @@ static unsigned int video_translate_cmd(unsigned int cmd)
>>>>>
>>>>>       return cmd;
>>>>>  }
>>>>> +EXPORT_SYMBOL(video_translate_cmd);
>>>>>
>>>>>  static int video_get_user(void __user *arg, void *parg,
>>>>>                         unsigned int real_cmd, unsigned int cmd,
>>>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>>>>> index c6ec87e88dfef9e6cfe1d1fb587c1600882fb14d..437b9f90714c62e0ba434ce47391ef64d88110aa 100644
>>>>> --- a/include/media/v4l2-ioctl.h
>>>>> +++ b/include/media/v4l2-ioctl.h
>>>>> @@ -687,6 +687,7 @@ int v4l2_compat_get_array_args(struct file *file, void *mbuf,
>>>>>  int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr,
>>>>>                              void *mbuf, size_t array_size,
>>>>>                              unsigned int cmd, void *arg);
>>>>> +unsigned int video_translate_cmd(unsigned int cmd);
>>>>>
>>>>>  /**
>>>>>   * typedef v4l2_kioctl - Typedef used to pass an ioctl handler.
>>>>>
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH 9/9] media: uvcvideo: Support granular power saving for compat syscalls

[Ricardo Ribalda]

Hi Hans

On Mon, 2 Jun 2025 at 13:24, Hans de Goede <hansg@kernel.org> wrote:
>
> On 2-Jun-25 13:11, Ricardo Ribalda wrote:
> > On Mon, 2 Jun 2025 at 13:07, Hans de Goede <hansg@kernel.org> wrote:
> >>
> >> Hi Ricardo,
> >>
> >> On 2-Jun-25 12:27, Ricardo Ribalda wrote:
> >>> Hi Hans
> >>>
> >>> On Mon, 2 Jun 2025 at 12:11, Hans de Goede <hansg@kernel.org> wrote:
> >>>>
> >>>> Hi Ricardo,
> >>>>
> >>>> On 28-May-25 19:58, Ricardo Ribalda wrote:
> >>>>> Right now we cannot support granular power saving on compat syscalls
> >>>>> because the VIDIOC_*32 NRs defines are not accessible to drivers.
> >>>>>
> >>>>> Use the video_translate_cmd() helper to convert the compat syscall NRs
> >>>>> into syscall NRs.
> >>>>>
> >>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>>>> ---
> >>>>>  drivers/media/usb/uvc/uvc_v4l2.c     | 9 ++-------
> >>>>>  drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++-
> >>>>>  include/media/v4l2-ioctl.h           | 1 +
> >>>>>  3 files changed, 5 insertions(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> >>>>> index fcb1b79c214849ce4da96a86a688d777b32cc688..048ee7e01808c8944f9bd46e5df2931b9c146ad5 100644
> >>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >>>>> @@ -1282,15 +1282,10 @@ static long uvc_v4l2_pm_ioctl(struct file *file,
> >>>>>  static long uvc_v4l2_unlocked_ioctl(struct file *file,
> >>>>>                                   unsigned int cmd, unsigned long arg)
> >>>>>  {
> >>>>> -     /*
> >>>>> -      * For now, we do not support granular power saving for compat
> >>>>> -      * syscalls.
> >>>>> -      */
> >>>>> -     if (in_compat_syscall())
> >>>>> -             return uvc_v4l2_pm_ioctl(file, cmd, arg);
> >>>>> +     unsigned int converted_cmd = video_translate_cmd(cmd);
> >>>>
> >>>> It looks like something went wrong here and you did not test-compile this?
> >>>> video_translate_cmd() is private to drivers/media/v4l2-core/v4l2-ioctl.c
> >>>> so this should not compile.
> >>>
> >>> Hmm... Actually I am pretty sure that I tested it on real hardware.
> >>>
> >>> Did you miss the EXPORT_SYMBOL() on the patch?
> >>
> >> Ah yes I did miss that, sorry.
> >
> > My bad, I doubt it till the last second if I should split it or not :)
> >
> >>
> >> For the next time please split core changes out into their own
> >> separate patches.
> >>
> >> In this case I think the core changes are not necessary instead
> >> you can just do:
> >>
> >>         unsigned int converted_cmd = cmd;
> >>
> >> #ifdef CONFIG_COMPAT
> >>         converted_cmd = v4l2_compat_translate_cmd(cmd);
> >> #endif
> >
> > I believe this should work as well:
> >
> > unsigned int converted_cmd = cmd;
> > if (in_compat_syscall())
> >   converted_cmd = v4l2_compat_translate_cmd(cmd);
> >
> > the compiler knows that CONFIG_COMPAT=n means in_compat_syscall() will
> > be always fails.
> >
> > If it is ok with you (and it actually works :) ) I will use this version.
>
> I agree that that is cleaner/better and I also think it should work,
> so lets go with that.

Actually, v4l2_compat_translate_cmd() does not seem to be EXPORT_SYMBOL()ed

So I still need to do some changes in the core.
(It also does not handle COMPAT_32BIT_TIME... but in this case it
seems to be the same).


Any preference between what to use: v4l2_compat_translate_cmd() vs
video_translate_cmd()?

Thanks!
>
> Regards,
>
> Hans
>
>
>
> >>>> You can use v4l2_compat_translate_cmd() but only when CONFIG_COMPAT is set
> >>>> otherwise that symbol is not available.
> >>>
> >>> I tried now without CONFIG_COMPAT and it built fine.
> >>>
> >>>>
> >>>> Regards,
> >>>>
> >>>> Hans
> >>>>
> >>>>
> >>>>
> >>>>>
> >>>>>       /* The following IOCTLs do need to turn on the camera. */
> >>>>> -     switch (cmd) {
> >>>>> +     switch (converted_cmd) {
> >>>>>       case UVCIOC_CTRL_QUERY:
> >>>>>       case VIDIOC_G_CTRL:
> >>>>>       case VIDIOC_G_EXT_CTRLS:
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>>> index 650dc1956f73d2f1943b56c42140c7b8d757259f..6fbd28f911cf23eec43ef1adcf64bd46ef067c81 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>>> @@ -3245,7 +3245,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
> >>>>>       return ret;
> >>>>>  }
> >>>>>
> >>>>> -static unsigned int video_translate_cmd(unsigned int cmd)
> >>>>> +unsigned int video_translate_cmd(unsigned int cmd)
> >>>>>  {
> >>>>>  #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> >>>>>       switch (cmd) {
> >>>>> @@ -3266,6 +3266,7 @@ static unsigned int video_translate_cmd(unsigned int cmd)
> >>>>>
> >>>>>       return cmd;
> >>>>>  }
> >>>>> +EXPORT_SYMBOL(video_translate_cmd);
> >>>>>
> >>>>>  static int video_get_user(void __user *arg, void *parg,
> >>>>>                         unsigned int real_cmd, unsigned int cmd,
> >>>>> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> >>>>> index c6ec87e88dfef9e6cfe1d1fb587c1600882fb14d..437b9f90714c62e0ba434ce47391ef64d88110aa 100644
> >>>>> --- a/include/media/v4l2-ioctl.h
> >>>>> +++ b/include/media/v4l2-ioctl.h
> >>>>> @@ -687,6 +687,7 @@ int v4l2_compat_get_array_args(struct file *file, void *mbuf,
> >>>>>  int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr,
> >>>>>                              void *mbuf, size_t array_size,
> >>>>>                              unsigned int cmd, void *arg);
> >>>>> +unsigned int video_translate_cmd(unsigned int cmd);
> >>>>>
> >>>>>  /**
> >>>>>   * typedef v4l2_kioctl - Typedef used to pass an ioctl handler.
> >>>>>
> >>>>
> >>>
> >>>
> >>
> >
> >
>


-- 
Ricardo Ribalda

Re: [PATCH 9/9] media: uvcvideo: Support granular power saving for compat syscalls

[Hans de Goede]

Hi,

On 2-Jun-25 13:40, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Mon, 2 Jun 2025 at 13:24, Hans de Goede <hansg@kernel.org> wrote:
>>
>> On 2-Jun-25 13:11, Ricardo Ribalda wrote:
>>> On Mon, 2 Jun 2025 at 13:07, Hans de Goede <hansg@kernel.org> wrote:
>>>>
>>>> Hi Ricardo,
>>>>
>>>> On 2-Jun-25 12:27, Ricardo Ribalda wrote:
>>>>> Hi Hans
>>>>>
>>>>> On Mon, 2 Jun 2025 at 12:11, Hans de Goede <hansg@kernel.org> wrote:
>>>>>>
>>>>>> Hi Ricardo,
>>>>>>
>>>>>> On 28-May-25 19:58, Ricardo Ribalda wrote:
>>>>>>> Right now we cannot support granular power saving on compat syscalls
>>>>>>> because the VIDIOC_*32 NRs defines are not accessible to drivers.
>>>>>>>
>>>>>>> Use the video_translate_cmd() helper to convert the compat syscall NRs
>>>>>>> into syscall NRs.
>>>>>>>
>>>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>>>> ---
>>>>>>>  drivers/media/usb/uvc/uvc_v4l2.c     | 9 ++-------
>>>>>>>  drivers/media/v4l2-core/v4l2-ioctl.c | 3 ++-
>>>>>>>  include/media/v4l2-ioctl.h           | 1 +
>>>>>>>  3 files changed, 5 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>>> index fcb1b79c214849ce4da96a86a688d777b32cc688..048ee7e01808c8944f9bd46e5df2931b9c146ad5 100644
>>>>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>>>> @@ -1282,15 +1282,10 @@ static long uvc_v4l2_pm_ioctl(struct file *file,
>>>>>>>  static long uvc_v4l2_unlocked_ioctl(struct file *file,
>>>>>>>                                   unsigned int cmd, unsigned long arg)
>>>>>>>  {
>>>>>>> -     /*
>>>>>>> -      * For now, we do not support granular power saving for compat
>>>>>>> -      * syscalls.
>>>>>>> -      */
>>>>>>> -     if (in_compat_syscall())
>>>>>>> -             return uvc_v4l2_pm_ioctl(file, cmd, arg);
>>>>>>> +     unsigned int converted_cmd = video_translate_cmd(cmd);
>>>>>>
>>>>>> It looks like something went wrong here and you did not test-compile this?
>>>>>> video_translate_cmd() is private to drivers/media/v4l2-core/v4l2-ioctl.c
>>>>>> so this should not compile.
>>>>>
>>>>> Hmm... Actually I am pretty sure that I tested it on real hardware.
>>>>>
>>>>> Did you miss the EXPORT_SYMBOL() on the patch?
>>>>
>>>> Ah yes I did miss that, sorry.
>>>
>>> My bad, I doubt it till the last second if I should split it or not :)
>>>
>>>>
>>>> For the next time please split core changes out into their own
>>>> separate patches.
>>>>
>>>> In this case I think the core changes are not necessary instead
>>>> you can just do:
>>>>
>>>>         unsigned int converted_cmd = cmd;
>>>>
>>>> #ifdef CONFIG_COMPAT
>>>>         converted_cmd = v4l2_compat_translate_cmd(cmd);
>>>> #endif
>>>
>>> I believe this should work as well:
>>>
>>> unsigned int converted_cmd = cmd;
>>> if (in_compat_syscall())
>>>   converted_cmd = v4l2_compat_translate_cmd(cmd);
>>>
>>> the compiler knows that CONFIG_COMPAT=n means in_compat_syscall() will
>>> be always fails.
>>>
>>> If it is ok with you (and it actually works :) ) I will use this version.
>>
>> I agree that that is cleaner/better and I also think it should work,
>> so lets go with that.
> 
> Actually, v4l2_compat_translate_cmd() does not seem to be EXPORT_SYMBOL()ed
> 
> So I still need to do some changes in the core.
> (It also does not handle COMPAT_32BIT_TIME... but in this case it
> seems to be the same).
> 
> 
> Any preference between what to use: v4l2_compat_translate_cmd() vs
> video_translate_cmd()?

v4l2_compat_translate_cmd() is already exposed in include/media/v4l2-ioctl.h
so I think it is best to go with that function.

Regards,

Hans