[PATCH v6 0/5] media: uvcvideo: Implement Granular Power Saving

[PATCH v6 0/5] media: uvcvideo: Implement Granular Power Saving

[Ricardo Ribalda]

Right now we power-up the device when a user open() the device and we
power it off when the last user close() the first video node.

This behaviour affects the power consumption of the device is multiple
use cases, such as:
- Polling the privacy gpio
- udev probing the device

This patchset introduces a more granular power saving behaviour where
the camera is only awaken when needed. It is compatible with
asynchronous controls.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v6:
- Improve error handling
- Use __free instead of guards()
- Rename uvc_v4l2_unlocked_ioctl
- Link to v5: https://lore.kernel.org/r/20250303-uvc-granpower-ng-v5-0-a3dfbe29fe91@chromium.org

Changes in v5:
- Improve "media: uvcvideo: Make power management granular" commit
  message.
- Link to v4: https://lore.kernel.org/r/20250226-uvc-granpower-ng-v4-0-3ec9be906048@chromium.org

Changes in v4:
- CodeStyle
- Create uvc_pm_ functions
- Link to v3: https://lore.kernel.org/r/20250206-uvc-granpower-ng-v3-0-32d0d7b0c5d8@chromium.org

Changes in v3:
- Fix build error on sh4.
- Link to v2: https://lore.kernel.org/r/20250203-uvc-granpower-ng-v2-0-bef4b55e7b67@chromium.org

Changes in v2:
- Add missing semicolon.
- Rebase on top of media-committers/next
- Link to v1: https://lore.kernel.org/r/20241126-uvc-granpower-ng-v1-0-6312bf26549c@chromium.org

---
Ricardo Ribalda (5):
      media: uvcvideo: Keep streaming state in the file handle
      media: uvcvideo: Create uvc_pm_(get|put) functions
      media: uvcvideo: Increase/decrease the PM counter per IOCTL
      media: uvcvideo: Make power management granular
      media: uvcvideo: Do not turn on the camera for some ioctls

 drivers/media/usb/uvc/uvc_ctrl.c |  37 +++++++++----
 drivers/media/usb/uvc/uvc_v4l2.c | 115 +++++++++++++++++++++++++++++++--------
 drivers/media/usb/uvc/uvcvideo.h |   5 ++
 3 files changed, 123 insertions(+), 34 deletions(-)
---
base-commit: f2151613e040973c868d28c8b00885dfab69eb75
change-id: 20241126-uvc-granpower-ng-069185a6d474

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

[PATCH v6 1/5] media: uvcvideo: Keep streaming state in the file handle

[Ricardo Ribalda]

Add a variable in the file handle state to figure out if a camera is in
the streaming state or not. This variable will be used in the future for
power management policies.

Now that we are at it, make use of guards to simplify the code.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 18 +++++++++++++-----
 drivers/media/usb/uvc/uvcvideo.h |  1 +
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 39065db44e864b8d107659197dac9ac33b01cf46..22886b47d81c2cfd0a744f34a50d296d606e54e8 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -841,11 +841,18 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 	if (!uvc_has_privileges(handle))
 		return -EBUSY;
 
-	mutex_lock(&stream->mutex);
+	guard(mutex)(&stream->mutex);
+
+	if (handle->is_streaming)
+		return 0;
+
 	ret = uvc_queue_streamon(&stream->queue, type);
-	mutex_unlock(&stream->mutex);
+	if (ret)
+		return ret;
 
-	return ret;
+	handle->is_streaming = true;
+
+	return 0;
 }
 
 static int uvc_ioctl_streamoff(struct file *file, void *fh,
@@ -857,9 +864,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
 	if (!uvc_has_privileges(handle))
 		return -EBUSY;
 
-	mutex_lock(&stream->mutex);
+	guard(mutex)(&stream->mutex);
+
 	uvc_queue_streamoff(&stream->queue, type);
-	mutex_unlock(&stream->mutex);
+	handle->is_streaming = false;
 
 	return 0;
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b4ee701835fc016474d2cd2a0b67b2aa915c1c60..5ceb01e7831a83507550e1d3313e63da7494b2e4 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -630,6 +630,7 @@ struct uvc_fh {
 	struct uvc_streaming *stream;
 	enum uvc_handle_state state;
 	unsigned int pending_async_ctrls;
+	bool is_streaming;
 };
 
 /* ------------------------------------------------------------------------

-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v6 2/5] media: uvcvideo: Create uvc_pm_(get|put) functions

[Ricardo Ribalda]

Most of the times that we have to call uvc_status_(get|put) we need to
call the usb_autopm_ functions.

Create a new pair of functions that automate this for us. This
simplifies the current code and future PM changes in the driver.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 36 ++++++++++++++++++++++++------------
 drivers/media/usb/uvc/uvcvideo.h |  4 ++++
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 22886b47d81c2cfd0a744f34a50d296d606e54e8..1d5be045d04ecbf17e65e14b390e494a294b735f 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -26,6 +26,27 @@
 
 #include "uvcvideo.h"
 
+int uvc_pm_get(struct uvc_device *dev)
+{
+	int ret;
+
+	ret = usb_autopm_get_interface(dev->intf);
+	if (ret)
+		return ret;
+
+	ret = uvc_status_get(dev);
+	if (ret)
+		usb_autopm_put_interface(dev->intf);
+
+	return ret;
+}
+
+void uvc_pm_put(struct uvc_device *dev)
+{
+	uvc_status_put(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,
@@ -642,20 +663,13 @@ static int uvc_v4l2_open(struct file *file)
 	stream = video_drvdata(file);
 	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
 
-	ret = usb_autopm_get_interface(stream->dev->intf);
-	if (ret < 0)
-		return ret;
-
 	/* Create the device handle. */
 	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
-	if (handle == NULL) {
-		usb_autopm_put_interface(stream->dev->intf);
+	if (!handle)
 		return -ENOMEM;
-	}
 
-	ret = uvc_status_get(stream->dev);
+	ret = uvc_pm_get(stream->dev);
 	if (ret) {
-		usb_autopm_put_interface(stream->dev->intf);
 		kfree(handle);
 		return ret;
 	}
@@ -690,9 +704,7 @@ static int uvc_v4l2_release(struct file *file)
 	kfree(handle);
 	file->private_data = NULL;
 
-	uvc_status_put(stream->dev);
-
-	usb_autopm_put_interface(stream->dev->intf);
+	uvc_pm_put(stream->dev);
 	return 0;
 }
 
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 5ceb01e7831a83507550e1d3313e63da7494b2e4..b9f8eb62ba1d82ea7788cf6c10cc838a429dbc9e 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -768,6 +768,10 @@ void uvc_status_suspend(struct uvc_device *dev);
 int uvc_status_get(struct uvc_device *dev);
 void uvc_status_put(struct uvc_device *dev);
 
+/* PM */
+int uvc_pm_get(struct uvc_device *dev);
+void uvc_pm_put(struct uvc_device *dev);
+
 /* Controls */
 extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;
 

-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v6 3/5] media: uvcvideo: Increase/decrease the PM counter per IOCTL

[Ricardo Ribalda]

Now we call uvc_pm_get/put from the device open/close. This low
level of granularity might leave the camera powered on in situations
where it is not needed.

Increase the granularity by increasing and decreasing the Power
Management counter per ioctl. There are two special cases where the
power management outlives the ioctl: async controls and streamon. Handle
those cases as well.

In a future patch, we will remove the uvc_pm_get/put from open/close.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 37 +++++++++++++++++++++++++++----------
 drivers/media/usb/uvc/uvc_v4l2.c | 39 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 64 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index cbf19aa1d82374a08cf79b6a6787fa348b83523a..3fad289e41fd5a757f8dcf30a6238c694fc4250c 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1812,38 +1812,49 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
 	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
 }
 
-static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
-				struct uvc_fh *new_handle)
+static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
+			       struct uvc_fh *new_handle)
 {
 	lockdep_assert_held(&handle->chain->ctrl_mutex);
 
 	if (new_handle) {
+		int ret;
+
 		if (ctrl->handle)
 			dev_warn_ratelimited(&handle->stream->dev->udev->dev,
 					     "UVC non compliance: Setting an async control with a pending operation.");
 
 		if (new_handle == ctrl->handle)
-			return;
+			return 0;
 
 		if (ctrl->handle) {
 			WARN_ON(!ctrl->handle->pending_async_ctrls);
 			if (ctrl->handle->pending_async_ctrls)
 				ctrl->handle->pending_async_ctrls--;
+			ctrl->handle = new_handle;
+			handle->pending_async_ctrls++;
+			return 0;
 		}
 
+		ret = uvc_pm_get(handle->chain->dev);
+		if (ret)
+			return ret;
+
 		ctrl->handle = new_handle;
 		handle->pending_async_ctrls++;
-		return;
+		return 0;
 	}
 
 	/* Cannot clear the handle for a control not owned by us.*/
 	if (WARN_ON(ctrl->handle != handle))
-		return;
+		return -EINVAL;
 
 	ctrl->handle = NULL;
 	if (WARN_ON(!handle->pending_async_ctrls))
-		return;
+		return -EINVAL;
 	handle->pending_async_ctrls--;
+	uvc_pm_put(handle->chain->dev);
+	return 0;
 }
 
 void uvc_ctrl_status_event(struct uvc_video_chain *chain,
@@ -2137,15 +2148,16 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
 
 		ctrl->dirty = 0;
 
+		if (!rollback && handle && !ret &&
+		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
+			ret = uvc_ctrl_set_handle(handle, ctrl, handle);
+
 		if (ret < 0) {
 			if (err_ctrl)
 				*err_ctrl = ctrl;
 			return ret;
 		}
 
-		if (!rollback && handle &&
-		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
-			uvc_ctrl_set_handle(handle, ctrl, handle);
 	}
 
 	return 0;
@@ -3222,6 +3234,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
 {
 	struct uvc_entity *entity;
+	int i;
 
 	guard(mutex)(&handle->chain->ctrl_mutex);
 
@@ -3236,7 +3249,11 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
 		}
 	}
 
-	WARN_ON(handle->pending_async_ctrls);
+	if (!WARN_ON(handle->pending_async_ctrls))
+		return;
+
+	for (i = 0; i < handle->pending_async_ctrls; i++)
+		uvc_pm_put(handle->stream->dev);
 }
 
 /*
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 1d5be045d04ecbf17e65e14b390e494a294b735f..8bccf7e17528b62f2594c0dad99405034532973d 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -697,6 +697,9 @@ static int uvc_v4l2_release(struct file *file)
 	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);
@@ -862,6 +865,11 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 	if (ret)
 		return ret;
 
+	ret = uvc_pm_get(stream->dev);
+	if (ret) {
+		uvc_queue_streamoff(&stream->queue, type);
+		return ret;
+	}
 	handle->is_streaming = true;
 
 	return 0;
@@ -879,7 +887,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
 	guard(mutex)(&stream->mutex);
 
 	uvc_queue_streamoff(&stream->queue, type);
-	handle->is_streaming = false;
+	if (handle->is_streaming) {
+		handle->is_streaming = false;
+		uvc_pm_put(stream->dev);
+	}
 
 	return 0;
 }
@@ -1378,9 +1389,11 @@ 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;
@@ -1389,6 +1402,12 @@ 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;
+
+	uvc_device = handle->stream->dev;
+
 	switch (cmd) {
 	case UVCIOC_CTRL_MAP32:
 		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
@@ -1423,6 +1442,22 @@ 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)
+{
+	struct uvc_fh *handle = file->private_data;
+	int ret;
+
+	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 ssize_t uvc_v4l2_read(struct file *file, char __user *data,
 		    size_t count, loff_t *ppos)
 {
@@ -1507,7 +1542,7 @@ const struct v4l2_file_operations uvc_fops = {
 	.owner		= THIS_MODULE,
 	.open		= uvc_v4l2_open,
 	.release	= uvc_v4l2_release,
-	.unlocked_ioctl	= video_ioctl2,
+	.unlocked_ioctl	= uvc_v4l2_unlocked_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl32	= uvc_v4l2_compat_ioctl32,
 #endif

-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v6 4/5] media: uvcvideo: Make power management granular

[Ricardo Ribalda]

Now that every ioctl takes care of their power management we can remove
the "global" power management.

Despite its size, this is a relatively big change. We hope that there
are no size effects of it. If there are some specific devices that
miss-behave, we can add a small quirk for them.

This patch introduces a behavioral change for the uvc "trigger" button.
Before the "trigger" button would work as long as userspace has opened
/dev/videoX. Now it only works when the camera is actually streaming. We
consider that this the most common (if not the only) usecase and
therefore we do not think of this as a regression.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 8bccf7e17528b62f2594c0dad99405034532973d..0f1ed0387b2611c8d21e211afe21a35101071d93 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -658,7 +658,6 @@ static int uvc_v4l2_open(struct file *file)
 {
 	struct uvc_streaming *stream;
 	struct uvc_fh *handle;
-	int ret = 0;
 
 	stream = video_drvdata(file);
 	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
@@ -668,12 +667,6 @@ static int uvc_v4l2_open(struct file *file)
 	if (!handle)
 		return -ENOMEM;
 
-	ret = uvc_pm_get(stream->dev);
-	if (ret) {
-		kfree(handle);
-		return ret;
-	}
-
 	v4l2_fh_init(&handle->vfh, &stream->vdev);
 	v4l2_fh_add(&handle->vfh);
 	handle->chain = stream->chain;
@@ -707,7 +700,6 @@ static int uvc_v4l2_release(struct file *file)
 	kfree(handle);
 	file->private_data = NULL;
 
-	uvc_pm_put(stream->dev);
 	return 0;
 }
 

-- 
2.49.0.472.ge94155a9ec-goog

[PATCH v6 5/5] media: uvcvideo: Do not turn on the camera for some ioctls

[Ricardo Ribalda]

There are some ioctls that do not need to turn on the camera. Do not
call uvc_pm_get in those cases.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 0f1ed0387b2611c8d21e211afe21a35101071d93..668a4e9d772c6d91f045ca75e2744b3a6c69da6b 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1440,6 +1440,26 @@ static long uvc_v4l2_unlocked_ioctl(struct file *file,
 	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;

-- 
2.49.0.472.ge94155a9ec-goog

Re: [PATCH v6 0/5] media: uvcvideo: Implement Granular Power Saving

[Hans de Goede]

Hi Ricardo,

On 27-Mar-25 22:05, Ricardo Ribalda wrote:
> Right now we power-up the device when a user open() the device and we
> power it off when the last user close() the first video node.
> 
> This behaviour affects the power consumption of the device is multiple
> use cases, such as:
> - Polling the privacy gpio
> - udev probing the device
> 
> This patchset introduces a more granular power saving behaviour where
> the camera is only awaken when needed. It is compatible with
> asynchronous controls.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Changes in v6:
> - Improve error handling
> - Use __free instead of guards()
> - Rename uvc_v4l2_unlocked_ioctl
> - Link to v5: https://lore.kernel.org/r/20250303-uvc-granpower-ng-v5-0-a3dfbe29fe91@chromium.org

Thank you for the new version.

I've pushed 6.15-rc1 + the entire v6 series merged on top to:
https://gitlab.freedesktop.org/linux-media/users/uvc/ -next now.

Regards,

Hans




> Changes in v5:
> - Improve "media: uvcvideo: Make power management granular" commit
>   message.
> - Link to v4: https://lore.kernel.org/r/20250226-uvc-granpower-ng-v4-0-3ec9be906048@chromium.org
> 
> Changes in v4:
> - CodeStyle
> - Create uvc_pm_ functions
> - Link to v3: https://lore.kernel.org/r/20250206-uvc-granpower-ng-v3-0-32d0d7b0c5d8@chromium.org
> 
> Changes in v3:
> - Fix build error on sh4.
> - Link to v2: https://lore.kernel.org/r/20250203-uvc-granpower-ng-v2-0-bef4b55e7b67@chromium.org
> 
> Changes in v2:
> - Add missing semicolon.
> - Rebase on top of media-committers/next
> - Link to v1: https://lore.kernel.org/r/20241126-uvc-granpower-ng-v1-0-6312bf26549c@chromium.org
> 
> ---
> Ricardo Ribalda (5):
>       media: uvcvideo: Keep streaming state in the file handle
>       media: uvcvideo: Create uvc_pm_(get|put) functions
>       media: uvcvideo: Increase/decrease the PM counter per IOCTL
>       media: uvcvideo: Make power management granular
>       media: uvcvideo: Do not turn on the camera for some ioctls
> 
>  drivers/media/usb/uvc/uvc_ctrl.c |  37 +++++++++----
>  drivers/media/usb/uvc/uvc_v4l2.c | 115 +++++++++++++++++++++++++++++++--------
>  drivers/media/usb/uvc/uvcvideo.h |   5 ++
>  3 files changed, 123 insertions(+), 34 deletions(-)
> ---
> base-commit: f2151613e040973c868d28c8b00885dfab69eb75
> change-id: 20241126-uvc-granpower-ng-069185a6d474
> 
> Best regards,

Re: [PATCH v6 2/5] media: uvcvideo: Create uvc_pm_(get|put) functions

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Mar 27, 2025 at 09:05:28PM +0000, Ricardo Ribalda wrote:
> Most of the times that we have to call uvc_status_(get|put) we need to
> call the usb_autopm_ functions.
> 
> Create a new pair of functions that automate this for us. This
> simplifies the current code and future PM changes in the driver.
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

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

> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 36 ++++++++++++++++++++++++------------
>  drivers/media/usb/uvc/uvcvideo.h |  4 ++++
>  2 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 22886b47d81c2cfd0a744f34a50d296d606e54e8..1d5be045d04ecbf17e65e14b390e494a294b735f 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -26,6 +26,27 @@
>  
>  #include "uvcvideo.h"
>  
> +int uvc_pm_get(struct uvc_device *dev)
> +{
> +	int ret;
> +
> +	ret = usb_autopm_get_interface(dev->intf);
> +	if (ret)
> +		return ret;
> +
> +	ret = uvc_status_get(dev);
> +	if (ret)
> +		usb_autopm_put_interface(dev->intf);
> +
> +	return ret;
> +}
> +
> +void uvc_pm_put(struct uvc_device *dev)
> +{
> +	uvc_status_put(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,
> @@ -642,20 +663,13 @@ static int uvc_v4l2_open(struct file *file)
>  	stream = video_drvdata(file);
>  	uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
>  
> -	ret = usb_autopm_get_interface(stream->dev->intf);
> -	if (ret < 0)
> -		return ret;
> -
>  	/* Create the device handle. */
>  	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> -	if (handle == NULL) {
> -		usb_autopm_put_interface(stream->dev->intf);
> +	if (!handle)
>  		return -ENOMEM;
> -	}
>  
> -	ret = uvc_status_get(stream->dev);
> +	ret = uvc_pm_get(stream->dev);
>  	if (ret) {
> -		usb_autopm_put_interface(stream->dev->intf);
>  		kfree(handle);
>  		return ret;
>  	}
> @@ -690,9 +704,7 @@ static int uvc_v4l2_release(struct file *file)
>  	kfree(handle);
>  	file->private_data = NULL;
>  
> -	uvc_status_put(stream->dev);
> -
> -	usb_autopm_put_interface(stream->dev->intf);
> +	uvc_pm_put(stream->dev);
>  	return 0;
>  }
>  
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 5ceb01e7831a83507550e1d3313e63da7494b2e4..b9f8eb62ba1d82ea7788cf6c10cc838a429dbc9e 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -768,6 +768,10 @@ void uvc_status_suspend(struct uvc_device *dev);
>  int uvc_status_get(struct uvc_device *dev);
>  void uvc_status_put(struct uvc_device *dev);
>  
> +/* PM */
> +int uvc_pm_get(struct uvc_device *dev);
> +void uvc_pm_put(struct uvc_device *dev);
> +
>  /* Controls */
>  extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 3/5] media: uvcvideo: Increase/decrease the PM counter per IOCTL

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Mar 27, 2025 at 09:05:29PM +0000, Ricardo Ribalda wrote:
> Now we call uvc_pm_get/put from the device open/close. This low
> level of granularity might leave the camera powered on in situations
> where it is not needed.
> 
> Increase the granularity by increasing and decreasing the Power
> Management counter per ioctl. There are two special cases where the
> power management outlives the ioctl: async controls and streamon. Handle
> those cases as well.
> 
> In a future patch, we will remove the uvc_pm_get/put from open/close.
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 37 +++++++++++++++++++++++++++----------
>  drivers/media/usb/uvc/uvc_v4l2.c | 39 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 64 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index cbf19aa1d82374a08cf79b6a6787fa348b83523a..3fad289e41fd5a757f8dcf30a6238c694fc4250c 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1812,38 +1812,49 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>  	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
>  }
>  
> -static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> -				struct uvc_fh *new_handle)
> +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> +			       struct uvc_fh *new_handle)
>  {
>  	lockdep_assert_held(&handle->chain->ctrl_mutex);
>  
>  	if (new_handle) {
> +		int ret;
> +
>  		if (ctrl->handle)
>  			dev_warn_ratelimited(&handle->stream->dev->udev->dev,
>  					     "UVC non compliance: Setting an async control with a pending operation.");
>  
>  		if (new_handle == ctrl->handle)
> -			return;
> +			return 0;
>  
>  		if (ctrl->handle) {
>  			WARN_ON(!ctrl->handle->pending_async_ctrls);
>  			if (ctrl->handle->pending_async_ctrls)
>  				ctrl->handle->pending_async_ctrls--;
> +			ctrl->handle = new_handle;
> +			handle->pending_async_ctrls++;

As commented previously, your usage of the handle variable is confusing.

			ctrl->handle->pending_async_ctrls++;

> +			return 0;
>  		}
>  
> +		ret = uvc_pm_get(handle->chain->dev);
> +		if (ret)
> +			return ret;
> +
>  		ctrl->handle = new_handle;
>  		handle->pending_async_ctrls++;

  		ctrl->handle->pending_async_ctrls++;

> -		return;
> +		return 0;
>  	}
>  
>  	/* Cannot clear the handle for a control not owned by us.*/
>  	if (WARN_ON(ctrl->handle != handle))
> -		return;
> +		return -EINVAL;
>  
>  	ctrl->handle = NULL;
>  	if (WARN_ON(!handle->pending_async_ctrls))
> -		return;
> +		return -EINVAL;
>  	handle->pending_async_ctrls--;
> +	uvc_pm_put(handle->chain->dev);
> +	return 0;
>  }
>  
>  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> @@ -2137,15 +2148,16 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>  
>  		ctrl->dirty = 0;
>  
> +		if (!rollback && handle && !ret &&
> +		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> +			ret = uvc_ctrl_set_handle(handle, ctrl, handle);
> +
>  		if (ret < 0) {
>  			if (err_ctrl)
>  				*err_ctrl = ctrl;
>  			return ret;
>  		}
>  
> -		if (!rollback && handle &&
> -		    ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> -			uvc_ctrl_set_handle(handle, ctrl, handle);
>  	}
>  
>  	return 0;
> @@ -3222,6 +3234,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>  {
>  	struct uvc_entity *entity;
> +	int i;
>  
>  	guard(mutex)(&handle->chain->ctrl_mutex);
>  
> @@ -3236,7 +3249,11 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>  		}
>  	}
>  
> -	WARN_ON(handle->pending_async_ctrls);
> +	if (!WARN_ON(handle->pending_async_ctrls))
> +		return;
> +
> +	for (i = 0; i < handle->pending_async_ctrls; i++)
> +		uvc_pm_put(handle->stream->dev);
>  }
>  
>  /*
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 1d5be045d04ecbf17e65e14b390e494a294b735f..8bccf7e17528b62f2594c0dad99405034532973d 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -697,6 +697,9 @@ static int uvc_v4l2_release(struct file *file)
>  	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);
> @@ -862,6 +865,11 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>  	if (ret)
>  		return ret;
>  
> +	ret = uvc_pm_get(stream->dev);

Shouldn't this be done before calling uvc_queue_streamon() ? There's
another PM reference being held by the ioctl handler, but if the code is
refactored later, it would be good to make sure we resume the device
before starting streaming.

> +	if (ret) {
> +		uvc_queue_streamoff(&stream->queue, type);
> +		return ret;
> +	}
>  	handle->is_streaming = true;
>  
>  	return 0;
> @@ -879,7 +887,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
>  	guard(mutex)(&stream->mutex);
>  
>  	uvc_queue_streamoff(&stream->queue, type);
> -	handle->is_streaming = false;
> +	if (handle->is_streaming) {
> +		handle->is_streaming = false;
> +		uvc_pm_put(stream->dev);
> +	}
>  
>  	return 0;
>  }
> @@ -1378,9 +1389,11 @@ 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;
> @@ -1389,6 +1402,12 @@ 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;
> +
> +	uvc_device = handle->stream->dev;

Ouch... That's not nice very :-/

If you want to use the cleanup API, I think we could use guards with an
init function such as

struct uvc_device *__uvc_pm_get_init(struct uvc_device *dev, int *ret)
{
	*ret = uvc_pm_get(dev);
	return *ret ? NULL : dev;
}

You can use DEFINE_CLASS() instead of DEFINE_GUARD() to control the
arguments to the init function. Users of the guard could do

	int ret;

	guard(uvc_pm)(dev, &ret);
	if (ret)
		return ret;

	...

Another, simpler option would be to replace returns with breaks in
uvc_v4l2_compat_ioctl32(). I'm tempted to do that in this patch, and
switching to the cleanup API as a patch on top if desired.

> +
>  	switch (cmd) {
>  	case UVCIOC_CTRL_MAP32:
>  		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
> @@ -1423,6 +1442,22 @@ 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)
> +{
> +	struct uvc_fh *handle = file->private_data;
> +	int ret;
> +
> +	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 ssize_t uvc_v4l2_read(struct file *file, char __user *data,
>  		    size_t count, loff_t *ppos)
>  {
> @@ -1507,7 +1542,7 @@ const struct v4l2_file_operations uvc_fops = {
>  	.owner		= THIS_MODULE,
>  	.open		= uvc_v4l2_open,
>  	.release	= uvc_v4l2_release,
> -	.unlocked_ioctl	= video_ioctl2,
> +	.unlocked_ioctl	= uvc_v4l2_unlocked_ioctl,
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl32	= uvc_v4l2_compat_ioctl32,
>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 3/5] media: uvcvideo: Increase/decrease the PM counter per IOCTL

[Ricardo Ribalda]

On Wed, 23 Apr 2025 at 04:37, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Thu, Mar 27, 2025 at 09:05:29PM +0000, Ricardo Ribalda wrote:
> > Now we call uvc_pm_get/put from the device open/close. This low
> > level of granularity might leave the camera powered on in situations
> > where it is not needed.
> >
> > Increase the granularity by increasing and decreasing the Power
> > Management counter per ioctl. There are two special cases where the
> > power management outlives the ioctl: async controls and streamon. Handle
> > those cases as well.
> >
> > In a future patch, we will remove the uvc_pm_get/put from open/close.
> >
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 37 +++++++++++++++++++++++++++----------
> >  drivers/media/usb/uvc/uvc_v4l2.c | 39 +++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 64 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index cbf19aa1d82374a08cf79b6a6787fa348b83523a..3fad289e41fd5a757f8dcf30a6238c694fc4250c 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1812,38 +1812,49 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
> >       uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
> >  }
> >
> > -static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> > -                             struct uvc_fh *new_handle)
> > +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> > +                            struct uvc_fh *new_handle)
> >  {
> >       lockdep_assert_held(&handle->chain->ctrl_mutex);
> >
> >       if (new_handle) {
> > +             int ret;
> > +
> >               if (ctrl->handle)
> >                       dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> >                                            "UVC non compliance: Setting an async control with a pending operation.");
> >
> >               if (new_handle == ctrl->handle)
> > -                     return;
> > +                     return 0;
> >
> >               if (ctrl->handle) {
> >                       WARN_ON(!ctrl->handle->pending_async_ctrls);
> >                       if (ctrl->handle->pending_async_ctrls)
> >                               ctrl->handle->pending_async_ctrls--;
> > +                     ctrl->handle = new_handle;
> > +                     handle->pending_async_ctrls++;
>
> As commented previously, your usage of the handle variable is confusing.
>
>                         ctrl->handle->pending_async_ctrls++;

I believe what makes it confusing is the function arguments.

Would you mind if I send a new patch introducing:
uvc_ctrl_set_handle() and uvc_ctrl_clear_handle().



>
> > +                     return 0;
> >               }
> >
> > +             ret = uvc_pm_get(handle->chain->dev);
> > +             if (ret)
> > +                     return ret;
> > +
> >               ctrl->handle = new_handle;
> >               handle->pending_async_ctrls++;
>
>                 ctrl->handle->pending_async_ctrls++;
>
> > -             return;
> > +             return 0;
> >       }
> >
> >       /* Cannot clear the handle for a control not owned by us.*/
> >       if (WARN_ON(ctrl->handle != handle))
> > -             return;
> > +             return -EINVAL;
> >
> >       ctrl->handle = NULL;
> >       if (WARN_ON(!handle->pending_async_ctrls))
> > -             return;
> > +             return -EINVAL;
> >       handle->pending_async_ctrls--;
> > +     uvc_pm_put(handle->chain->dev);
> > +     return 0;
> >  }
> >
> >  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> > @@ -2137,15 +2148,16 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> >
> >               ctrl->dirty = 0;
> >
> > +             if (!rollback && handle && !ret &&
> > +                 ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > +                     ret = uvc_ctrl_set_handle(handle, ctrl, handle);
> > +
> >               if (ret < 0) {
> >                       if (err_ctrl)
> >                               *err_ctrl = ctrl;
> >                       return ret;
> >               }
> >
> > -             if (!rollback && handle &&
> > -                 ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > -                     uvc_ctrl_set_handle(handle, ctrl, handle);
> >       }
> >
> >       return 0;
> > @@ -3222,6 +3234,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> >  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> >  {
> >       struct uvc_entity *entity;
> > +     int i;
> >
> >       guard(mutex)(&handle->chain->ctrl_mutex);
> >
> > @@ -3236,7 +3249,11 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> >               }
> >       }
> >
> > -     WARN_ON(handle->pending_async_ctrls);
> > +     if (!WARN_ON(handle->pending_async_ctrls))
> > +             return;
> > +
> > +     for (i = 0; i < handle->pending_async_ctrls; i++)
> > +             uvc_pm_put(handle->stream->dev);
> >  }
> >
> >  /*
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 1d5be045d04ecbf17e65e14b390e494a294b735f..8bccf7e17528b62f2594c0dad99405034532973d 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -697,6 +697,9 @@ static int uvc_v4l2_release(struct file *file)
> >       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);
> > @@ -862,6 +865,11 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
> >       if (ret)
> >               return ret;
> >
> > +     ret = uvc_pm_get(stream->dev);
>
> Shouldn't this be done before calling uvc_queue_streamon() ? There's
> another PM reference being held by the ioctl handler, but if the code is
> refactored later, it would be good to make sure we resume the device
> before starting streaming.

I was trying to simplify the error handling and, as you say, the ioctl
handler already holds a reference. I do not mind sending a follow-up
patch changing the order.

>
> > +     if (ret) {
> > +             uvc_queue_streamoff(&stream->queue, type);
> > +             return ret;
> > +     }
> >       handle->is_streaming = true;
> >
> >       return 0;
> > @@ -879,7 +887,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
> >       guard(mutex)(&stream->mutex);
> >
> >       uvc_queue_streamoff(&stream->queue, type);
> > -     handle->is_streaming = false;
> > +     if (handle->is_streaming) {
> > +             handle->is_streaming = false;
> > +             uvc_pm_put(stream->dev);
> > +     }
> >
> >       return 0;
> >  }
> > @@ -1378,9 +1389,11 @@ 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;
> > @@ -1389,6 +1402,12 @@ 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;
> > +
> > +     uvc_device = handle->stream->dev;
>
> Ouch... That's not nice very :-/

IIt is nicer than changing the returns with breaks, believe me I tried
:), and it is more prone to errors.

I thought about the CLASS, but it is not worth it with a single user.
I believe the current code is a good compromise, but I might be
biased.

>
> If you want to use the cleanup API, I think we could use guards with an
> init function such as
>
> struct uvc_device *__uvc_pm_get_init(struct uvc_device *dev, int *ret)
> {
>         *ret = uvc_pm_get(dev);
>         return *ret ? NULL : dev;
> }
>
> You can use DEFINE_CLASS() instead of DEFINE_GUARD() to control the
> arguments to the init function. Users of the guard could do
>
>         int ret;
>
>         guard(uvc_pm)(dev, &ret);
>         if (ret)
>                 return ret;
>
>         ...
>
> Another, simpler option would be to replace returns with breaks in
> uvc_v4l2_compat_ioctl32(). I'm tempted to do that in this patch, and
> switching to the cleanup API as a patch on top if desired.
>
> > +
> >       switch (cmd) {
> >       case UVCIOC_CTRL_MAP32:
> >               ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
> > @@ -1423,6 +1442,22 @@ 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)
> > +{
> > +     struct uvc_fh *handle = file->private_data;
> > +     int ret;
> > +
> > +     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 ssize_t uvc_v4l2_read(struct file *file, char __user *data,
> >                   size_t count, loff_t *ppos)
> >  {
> > @@ -1507,7 +1542,7 @@ const struct v4l2_file_operations uvc_fops = {
> >       .owner          = THIS_MODULE,
> >       .open           = uvc_v4l2_open,
> >       .release        = uvc_v4l2_release,
> > -     .unlocked_ioctl = video_ioctl2,
> > +     .unlocked_ioctl = uvc_v4l2_unlocked_ioctl,
> >  #ifdef CONFIG_COMPAT
> >       .compat_ioctl32 = uvc_v4l2_compat_ioctl32,
> >  #endif
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v6 3/5] media: uvcvideo: Increase/decrease the PM counter per IOCTL

[Hans de Goede]

Hi Ricardo,

On 23-Apr-25 00:58, Ricardo Ribalda wrote:
> On Wed, 23 Apr 2025 at 04:37, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Ricardo,
>>
>> Thank you for the patch.
>>
>> On Thu, Mar 27, 2025 at 09:05:29PM +0000, Ricardo Ribalda wrote:
>>> Now we call uvc_pm_get/put from the device open/close. This low
>>> level of granularity might leave the camera powered on in situations
>>> where it is not needed.
>>>
>>> Increase the granularity by increasing and decreasing the Power
>>> Management counter per ioctl. There are two special cases where the
>>> power management outlives the ioctl: async controls and streamon. Handle
>>> those cases as well.
>>>
>>> In a future patch, we will remove the uvc_pm_get/put from open/close.
>>>
>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>>  drivers/media/usb/uvc/uvc_ctrl.c | 37 +++++++++++++++++++++++++++----------
>>>  drivers/media/usb/uvc/uvc_v4l2.c | 39 +++++++++++++++++++++++++++++++++++++--
>>>  2 files changed, 64 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>>> index cbf19aa1d82374a08cf79b6a6787fa348b83523a..3fad289e41fd5a757f8dcf30a6238c694fc4250c 100644
>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>>> @@ -1812,38 +1812,49 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>>>       uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
>>>  }
>>>
>>> -static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
>>> -                             struct uvc_fh *new_handle)
>>> +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
>>> +                            struct uvc_fh *new_handle)
>>>  {
>>>       lockdep_assert_held(&handle->chain->ctrl_mutex);
>>>
>>>       if (new_handle) {
>>> +             int ret;
>>> +
>>>               if (ctrl->handle)
>>>                       dev_warn_ratelimited(&handle->stream->dev->udev->dev,
>>>                                            "UVC non compliance: Setting an async control with a pending operation.");
>>>
>>>               if (new_handle == ctrl->handle)
>>> -                     return;
>>> +                     return 0;
>>>
>>>               if (ctrl->handle) {
>>>                       WARN_ON(!ctrl->handle->pending_async_ctrls);
>>>                       if (ctrl->handle->pending_async_ctrls)
>>>                               ctrl->handle->pending_async_ctrls--;
>>> +                     ctrl->handle = new_handle;
>>> +                     handle->pending_async_ctrls++;
>>
>> As commented previously, your usage of the handle variable is confusing.
>>
>>                         ctrl->handle->pending_async_ctrls++;
> 
> I believe what makes it confusing is the function arguments.
> 
> Would you mind if I send a new patch introducing:
> uvc_ctrl_set_handle() and uvc_ctrl_clear_handle().

Ricardo, if you do end up making this change, please do so as a follow-up
patch on top of current uvc/next so that we don't have to drop the whole
series and then rebuild uvc/next from scratch.

Regards,

Hans




>>> +                     return 0;
>>>               }
>>>
>>> +             ret = uvc_pm_get(handle->chain->dev);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>>               ctrl->handle = new_handle;
>>>               handle->pending_async_ctrls++;
>>
>>                 ctrl->handle->pending_async_ctrls++;
>>
>>> -             return;
>>> +             return 0;
>>>       }
>>>
>>>       /* Cannot clear the handle for a control not owned by us.*/
>>>       if (WARN_ON(ctrl->handle != handle))
>>> -             return;
>>> +             return -EINVAL;
>>>
>>>       ctrl->handle = NULL;
>>>       if (WARN_ON(!handle->pending_async_ctrls))
>>> -             return;
>>> +             return -EINVAL;
>>>       handle->pending_async_ctrls--;
>>> +     uvc_pm_put(handle->chain->dev);
>>> +     return 0;
>>>  }
>>>
>>>  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>>> @@ -2137,15 +2148,16 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>>>
>>>               ctrl->dirty = 0;
>>>
>>> +             if (!rollback && handle && !ret &&
>>> +                 ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
>>> +                     ret = uvc_ctrl_set_handle(handle, ctrl, handle);
>>> +
>>>               if (ret < 0) {
>>>                       if (err_ctrl)
>>>                               *err_ctrl = ctrl;
>>>                       return ret;
>>>               }
>>>
>>> -             if (!rollback && handle &&
>>> -                 ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
>>> -                     uvc_ctrl_set_handle(handle, ctrl, handle);
>>>       }
>>>
>>>       return 0;
>>> @@ -3222,6 +3234,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>>>  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>>>  {
>>>       struct uvc_entity *entity;
>>> +     int i;
>>>
>>>       guard(mutex)(&handle->chain->ctrl_mutex);
>>>
>>> @@ -3236,7 +3249,11 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>>>               }
>>>       }
>>>
>>> -     WARN_ON(handle->pending_async_ctrls);
>>> +     if (!WARN_ON(handle->pending_async_ctrls))
>>> +             return;
>>> +
>>> +     for (i = 0; i < handle->pending_async_ctrls; i++)
>>> +             uvc_pm_put(handle->stream->dev);
>>>  }
>>>
>>>  /*
>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>> index 1d5be045d04ecbf17e65e14b390e494a294b735f..8bccf7e17528b62f2594c0dad99405034532973d 100644
>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>> @@ -697,6 +697,9 @@ static int uvc_v4l2_release(struct file *file)
>>>       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);
>>> @@ -862,6 +865,11 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>>>       if (ret)
>>>               return ret;
>>>
>>> +     ret = uvc_pm_get(stream->dev);
>>
>> Shouldn't this be done before calling uvc_queue_streamon() ? There's
>> another PM reference being held by the ioctl handler, but if the code is
>> refactored later, it would be good to make sure we resume the device
>> before starting streaming.
> 
> I was trying to simplify the error handling and, as you say, the ioctl
> handler already holds a reference. I do not mind sending a follow-up
> patch changing the order.
> 
>>
>>> +     if (ret) {
>>> +             uvc_queue_streamoff(&stream->queue, type);
>>> +             return ret;
>>> +     }
>>>       handle->is_streaming = true;
>>>
>>>       return 0;
>>> @@ -879,7 +887,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
>>>       guard(mutex)(&stream->mutex);
>>>
>>>       uvc_queue_streamoff(&stream->queue, type);
>>> -     handle->is_streaming = false;
>>> +     if (handle->is_streaming) {
>>> +             handle->is_streaming = false;
>>> +             uvc_pm_put(stream->dev);
>>> +     }
>>>
>>>       return 0;
>>>  }
>>> @@ -1378,9 +1389,11 @@ 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;
>>> @@ -1389,6 +1402,12 @@ 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;
>>> +
>>> +     uvc_device = handle->stream->dev;
>>
>> Ouch... That's not nice very :-/
> 
> IIt is nicer than changing the returns with breaks, believe me I tried
> :), and it is more prone to errors.
> 
> I thought about the CLASS, but it is not worth it with a single user.
> I believe the current code is a good compromise, but I might be
> biased.
> 
>>
>> If you want to use the cleanup API, I think we could use guards with an
>> init function such as
>>
>> struct uvc_device *__uvc_pm_get_init(struct uvc_device *dev, int *ret)
>> {
>>         *ret = uvc_pm_get(dev);
>>         return *ret ? NULL : dev;
>> }
>>
>> You can use DEFINE_CLASS() instead of DEFINE_GUARD() to control the
>> arguments to the init function. Users of the guard could do
>>
>>         int ret;
>>
>>         guard(uvc_pm)(dev, &ret);
>>         if (ret)
>>                 return ret;
>>
>>         ...
>>
>> Another, simpler option would be to replace returns with breaks in
>> uvc_v4l2_compat_ioctl32(). I'm tempted to do that in this patch, and
>> switching to the cleanup API as a patch on top if desired.
>>
>>> +
>>>       switch (cmd) {
>>>       case UVCIOC_CTRL_MAP32:
>>>               ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
>>> @@ -1423,6 +1442,22 @@ 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)
>>> +{
>>> +     struct uvc_fh *handle = file->private_data;
>>> +     int ret;
>>> +
>>> +     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 ssize_t uvc_v4l2_read(struct file *file, char __user *data,
>>>                   size_t count, loff_t *ppos)
>>>  {
>>> @@ -1507,7 +1542,7 @@ const struct v4l2_file_operations uvc_fops = {
>>>       .owner          = THIS_MODULE,
>>>       .open           = uvc_v4l2_open,
>>>       .release        = uvc_v4l2_release,
>>> -     .unlocked_ioctl = video_ioctl2,
>>> +     .unlocked_ioctl = uvc_v4l2_unlocked_ioctl,
>>>  #ifdef CONFIG_COMPAT
>>>       .compat_ioctl32 = uvc_v4l2_compat_ioctl32,
>>>  #endif
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
> 
> 
> 

Re: [PATCH v6 3/5] media: uvcvideo: Increase/decrease the PM counter per IOCTL

[Laurent Pinchart]

On Mon, Apr 28, 2025 at 01:54:38PM +0200, Hans de Goede wrote:
> On 23-Apr-25 00:58, Ricardo Ribalda wrote:
> > On Wed, 23 Apr 2025 at 04:37, Laurent Pinchart wrote:
> >> On Thu, Mar 27, 2025 at 09:05:29PM +0000, Ricardo Ribalda wrote:
> >>> Now we call uvc_pm_get/put from the device open/close. This low
> >>> level of granularity might leave the camera powered on in situations
> >>> where it is not needed.
> >>>
> >>> Increase the granularity by increasing and decreasing the Power
> >>> Management counter per ioctl. There are two special cases where the
> >>> power management outlives the ioctl: async controls and streamon. Handle
> >>> those cases as well.
> >>>
> >>> In a future patch, we will remove the uvc_pm_get/put from open/close.
> >>>
> >>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>>  drivers/media/usb/uvc/uvc_ctrl.c | 37 +++++++++++++++++++++++++++----------
> >>>  drivers/media/usb/uvc/uvc_v4l2.c | 39 +++++++++++++++++++++++++++++++++++++--
> >>>  2 files changed, 64 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> >>> index cbf19aa1d82374a08cf79b6a6787fa348b83523a..3fad289e41fd5a757f8dcf30a6238c694fc4250c 100644
> >>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> >>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> >>> @@ -1812,38 +1812,49 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
> >>>       uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
> >>>  }
> >>>
> >>> -static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> >>> -                             struct uvc_fh *new_handle)
> >>> +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> >>> +                            struct uvc_fh *new_handle)
> >>>  {
> >>>       lockdep_assert_held(&handle->chain->ctrl_mutex);
> >>>
> >>>       if (new_handle) {
> >>> +             int ret;
> >>> +
> >>>               if (ctrl->handle)
> >>>                       dev_warn_ratelimited(&handle->stream->dev->udev->dev,
> >>>                                            "UVC non compliance: Setting an async control with a pending operation.");
> >>>
> >>>               if (new_handle == ctrl->handle)
> >>> -                     return;
> >>> +                     return 0;
> >>>
> >>>               if (ctrl->handle) {
> >>>                       WARN_ON(!ctrl->handle->pending_async_ctrls);
> >>>                       if (ctrl->handle->pending_async_ctrls)
> >>>                               ctrl->handle->pending_async_ctrls--;
> >>> +                     ctrl->handle = new_handle;
> >>> +                     handle->pending_async_ctrls++;
> >>
> >> As commented previously, your usage of the handle variable is confusing.
> >>
> >>                         ctrl->handle->pending_async_ctrls++;
> > 
> > I believe what makes it confusing is the function arguments.
> > 
> > Would you mind if I send a new patch introducing:
> > uvc_ctrl_set_handle() and uvc_ctrl_clear_handle().

It seems worth a try. I would still like the above change to be
addressed. Can you please send a patch for that too ?

> Ricardo, if you do end up making this change, please do so as a follow-up
> patch on top of current uvc/next so that we don't have to drop the whole
> series and then rebuild uvc/next from scratch.

Ack, that's my preference too.

> >>> +                     return 0;
> >>>               }
> >>>
> >>> +             ret = uvc_pm_get(handle->chain->dev);
> >>> +             if (ret)
> >>> +                     return ret;
> >>> +
> >>>               ctrl->handle = new_handle;
> >>>               handle->pending_async_ctrls++;
> >>
> >>                 ctrl->handle->pending_async_ctrls++;
> >>
> >>> -             return;
> >>> +             return 0;
> >>>       }
> >>>
> >>>       /* Cannot clear the handle for a control not owned by us.*/
> >>>       if (WARN_ON(ctrl->handle != handle))
> >>> -             return;
> >>> +             return -EINVAL;
> >>>
> >>>       ctrl->handle = NULL;
> >>>       if (WARN_ON(!handle->pending_async_ctrls))
> >>> -             return;
> >>> +             return -EINVAL;
> >>>       handle->pending_async_ctrls--;
> >>> +     uvc_pm_put(handle->chain->dev);
> >>> +     return 0;
> >>>  }
> >>>
> >>>  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> >>> @@ -2137,15 +2148,16 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
> >>>
> >>>               ctrl->dirty = 0;
> >>>
> >>> +             if (!rollback && handle && !ret &&
> >>> +                 ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> >>> +                     ret = uvc_ctrl_set_handle(handle, ctrl, handle);
> >>> +
> >>>               if (ret < 0) {
> >>>                       if (err_ctrl)
> >>>                               *err_ctrl = ctrl;
> >>>                       return ret;
> >>>               }
> >>>
> >>> -             if (!rollback && handle &&
> >>> -                 ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> >>> -                     uvc_ctrl_set_handle(handle, ctrl, handle);
> >>>       }
> >>>
> >>>       return 0;
> >>> @@ -3222,6 +3234,7 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> >>>  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> >>>  {
> >>>       struct uvc_entity *entity;
> >>> +     int i;
> >>>
> >>>       guard(mutex)(&handle->chain->ctrl_mutex);
> >>>
> >>> @@ -3236,7 +3249,11 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> >>>               }
> >>>       }
> >>>
> >>> -     WARN_ON(handle->pending_async_ctrls);
> >>> +     if (!WARN_ON(handle->pending_async_ctrls))
> >>> +             return;
> >>> +
> >>> +     for (i = 0; i < handle->pending_async_ctrls; i++)
> >>> +             uvc_pm_put(handle->stream->dev);
> >>>  }
> >>>
> >>>  /*
> >>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> >>> index 1d5be045d04ecbf17e65e14b390e494a294b735f..8bccf7e17528b62f2594c0dad99405034532973d 100644
> >>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >>> @@ -697,6 +697,9 @@ static int uvc_v4l2_release(struct file *file)
> >>>       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);
> >>> @@ -862,6 +865,11 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
> >>>       if (ret)
> >>>               return ret;
> >>>
> >>> +     ret = uvc_pm_get(stream->dev);
> >>
> >> Shouldn't this be done before calling uvc_queue_streamon() ? There's
> >> another PM reference being held by the ioctl handler, but if the code is
> >> refactored later, it would be good to make sure we resume the device
> >> before starting streaming.
> > 
> > I was trying to simplify the error handling and, as you say, the ioctl
> > handler already holds a reference. I do not mind sending a follow-up
> > patch changing the order.

One of the two uvc_queue_streamon() or uvc_pm_get() calls will need to
perform cleanup in case of error. As the cleanup should just be
uvc_pm_put(), it seems equally easy. I'd appreciate a follow-up patch.

> >>> +     if (ret) {
> >>> +             uvc_queue_streamoff(&stream->queue, type);
> >>> +             return ret;
> >>> +     }
> >>>       handle->is_streaming = true;
> >>>
> >>>       return 0;
> >>> @@ -879,7 +887,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
> >>>       guard(mutex)(&stream->mutex);
> >>>
> >>>       uvc_queue_streamoff(&stream->queue, type);
> >>> -     handle->is_streaming = false;
> >>> +     if (handle->is_streaming) {
> >>> +             handle->is_streaming = false;
> >>> +             uvc_pm_put(stream->dev);
> >>> +     }
> >>>
> >>>       return 0;
> >>>  }
> >>> @@ -1378,9 +1389,11 @@ 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;
> >>> @@ -1389,6 +1402,12 @@ 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;
> >>> +
> >>> +     uvc_device = handle->stream->dev;
> >>
> >> Ouch... That's not nice very :-/
> > 
> > IIt is nicer than changing the returns with breaks, believe me I tried
> > :), and it is more prone to errors.
> > 
> > I thought about the CLASS, but it is not worth it with a single user.
> > I believe the current code is a good compromise, but I might be
> > biased.

Yes, a class may be a bit of an over-engineering. I'd prefer breaks
though.

> >> If you want to use the cleanup API, I think we could use guards with an
> >> init function such as
> >>
> >> struct uvc_device *__uvc_pm_get_init(struct uvc_device *dev, int *ret)
> >> {
> >>         *ret = uvc_pm_get(dev);
> >>         return *ret ? NULL : dev;
> >> }
> >>
> >> You can use DEFINE_CLASS() instead of DEFINE_GUARD() to control the
> >> arguments to the init function. Users of the guard could do
> >>
> >>         int ret;
> >>
> >>         guard(uvc_pm)(dev, &ret);
> >>         if (ret)
> >>                 return ret;
> >>
> >>         ...
> >>
> >> Another, simpler option would be to replace returns with breaks in
> >> uvc_v4l2_compat_ioctl32(). I'm tempted to do that in this patch, and
> >> switching to the cleanup API as a patch on top if desired.
> >>
> >>> +
> >>>       switch (cmd) {
> >>>       case UVCIOC_CTRL_MAP32:
> >>>               ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
> >>> @@ -1423,6 +1442,22 @@ 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)
> >>> +{
> >>> +     struct uvc_fh *handle = file->private_data;
> >>> +     int ret;
> >>> +
> >>> +     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 ssize_t uvc_v4l2_read(struct file *file, char __user *data,
> >>>                   size_t count, loff_t *ppos)
> >>>  {
> >>> @@ -1507,7 +1542,7 @@ const struct v4l2_file_operations uvc_fops = {
> >>>       .owner          = THIS_MODULE,
> >>>       .open           = uvc_v4l2_open,
> >>>       .release        = uvc_v4l2_release,
> >>> -     .unlocked_ioctl = video_ioctl2,
> >>> +     .unlocked_ioctl = uvc_v4l2_unlocked_ioctl,
> >>>  #ifdef CONFIG_COMPAT
> >>>       .compat_ioctl32 = uvc_v4l2_compat_ioctl32,
> >>>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 5/5] media: uvcvideo: Do not turn on the camera for some ioctls

[Hans Verkuil]

On 27/03/2025 22:05, Ricardo Ribalda wrote:
> There are some ioctls that do not need to turn on the camera. Do not
> call uvc_pm_get in those cases.
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 0f1ed0387b2611c8d21e211afe21a35101071d93..668a4e9d772c6d91f045ca75e2744b3a6c69da6b 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1440,6 +1440,26 @@ static long uvc_v4l2_unlocked_ioctl(struct file *file,
>  	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:

Wouldn't it be better to check against the ioctls that DO need to turn on the camera?

That is more future proof IMHO.

If a new ioctl is created, and uvc implements it and that needs to turn on the camera,
then presumably you will realize that when you add that ioctl in uvc.

If a new ioctl is created and uvc does not need to turn on the camera, then you will
almost certainly forget to add it to this list.

I'm not blocking this patch, but I think it will be hard to keep this list up to date.
Inverting the test is probably much easier to handle in the future.

Apologies if this has been discussed before, if so, just point to that discussion so I
can read through it.

Regards,

	Hans

> +		return video_ioctl2(file, cmd, arg);
> +	}
> +
>  	ret = uvc_pm_get(handle->stream->dev);
>  	if (ret)
>  		return ret;
> 

Re: [PATCH v6 5/5] media: uvcvideo: Do not turn on the camera for some ioctls

[Ricardo Ribalda]

Hi Hans

On Fri, 9 May 2025 at 15:44, Hans Verkuil <hans@jjverkuil.nl> wrote:
>
> On 27/03/2025 22:05, Ricardo Ribalda wrote:
> > There are some ioctls that do not need to turn on the camera. Do not
> > call uvc_pm_get in those cases.
> >
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_v4l2.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 0f1ed0387b2611c8d21e211afe21a35101071d93..668a4e9d772c6d91f045ca75e2744b3a6c69da6b 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -1440,6 +1440,26 @@ static long uvc_v4l2_unlocked_ioctl(struct file *file,
> >       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:
>
> Wouldn't it be better to check against the ioctls that DO need to turn on the camera?

I thought it was safer this way. I will look into inverting the logic
in a follow-up patch.

Regards!

>
> That is more future proof IMHO.
>
> If a new ioctl is created, and uvc implements it and that needs to turn on the camera,
> then presumably you will realize that when you add that ioctl in uvc.
>
> If a new ioctl is created and uvc does not need to turn on the camera, then you will
> almost certainly forget to add it to this list.
>
> I'm not blocking this patch, but I think it will be hard to keep this list up to date.
> Inverting the test is probably much easier to handle in the future.
>
> Apologies if this has been discussed before, if so, just point to that discussion so I
> can read through it.
>
> Regards,
>
>         Hans
>
> > +             return video_ioctl2(file, cmd, arg);
> > +     }
> > +
> >       ret = uvc_pm_get(handle->stream->dev);
> >       if (ret)
> >               return ret;
> >
>


-- 
Ricardo Ribalda

Re: [PATCH v6 5/5] media: uvcvideo: Do not turn on the camera for some ioctls

[Ricardo Ribalda]

On Fri, 9 May 2025 at 15:51, Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Hans
>
> On Fri, 9 May 2025 at 15:44, Hans Verkuil <hans@jjverkuil.nl> wrote:
> >
> > On 27/03/2025 22:05, Ricardo Ribalda wrote:
> > > There are some ioctls that do not need to turn on the camera. Do not
> > > call uvc_pm_get in those cases.
> > >
> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_v4l2.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > > index 0f1ed0387b2611c8d21e211afe21a35101071d93..668a4e9d772c6d91f045ca75e2744b3a6c69da6b 100644
> > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > @@ -1440,6 +1440,26 @@ static long uvc_v4l2_unlocked_ioctl(struct file *file,
> > >       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:
> >
> > Wouldn't it be better to check against the ioctls that DO need to turn on the camera?
>
> I thought it was safer this way. I will look into inverting the logic
> in a follow-up patch.

https://patchwork.linuxtv.org/project/linux-media/list/?series=15601

>
> Regards!
>
> >
> > That is more future proof IMHO.
> >
> > If a new ioctl is created, and uvc implements it and that needs to turn on the camera,
> > then presumably you will realize that when you add that ioctl in uvc.
> >
> > If a new ioctl is created and uvc does not need to turn on the camera, then you will
> > almost certainly forget to add it to this list.
> >
> > I'm not blocking this patch, but I think it will be hard to keep this list up to date.
> > Inverting the test is probably much easier to handle in the future.
> >
> > Apologies if this has been discussed before, if so, just point to that discussion so I
> > can read through it.
> >
> > Regards,
> >
> >         Hans
> >
> > > +             return video_ioctl2(file, cmd, arg);
> > > +     }
> > > +
> > >       ret = uvc_pm_get(handle->stream->dev);
> > >       if (ret)
> > >               return ret;
> > >
> >
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda