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

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

While developing this patchset, two bugs were found. The patchset has
been developed so these fixes can be taken independently.

Signed-off-by: Ricardo Ribalda <ribalda@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 | 13 +++++-
 drivers/media/usb/uvc/uvc_v4l2.c | 99 ++++++++++++++++++++++++++++++----------
 drivers/media/usb/uvc/uvcvideo.h |  6 +++
 3 files changed, 92 insertions(+), 26 deletions(-)
---
base-commit: d98e9213a768a3cc3a99f5e1abe09ad3baff2104
change-id: 20241126-uvc-granpower-ng-069185a6d474

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

[PATCH v5 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 93c6cdb23881..f9cd6db759c5 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -835,11 +835,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,
@@ -851,9 +858,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 5e388f05f3fc..bc87e1f2c669 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -618,6 +618,7 @@ struct uvc_fh {
 	struct uvc_streaming *stream;
 	enum uvc_handle_state state;
 	unsigned int pending_async_ctrls;
+	bool is_streaming;
 };
 
 struct uvc_driver {

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v5 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 f9cd6db759c5..de1e105f7263 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,
@@ -636,20 +657,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;
 	}
@@ -684,9 +698,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 bc87e1f2c669..fbe3649c7cd6 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -763,6 +763,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.48.1.711.g2feabab25a-goog

[PATCH v5 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 | 13 +++++++++++--
 drivers/media/usb/uvc/uvc_v4l2.c | 23 +++++++++++++++++++++--
 drivers/media/usb/uvc/uvcvideo.h |  1 +
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 4e58476d305e..47188c7f96c7 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1594,12 +1594,15 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
 
 		if (ctrl->handle) {
 			WARN_ON(!ctrl->handle->pending_async_ctrls);
-			if (ctrl->handle->pending_async_ctrls)
+			if (ctrl->handle->pending_async_ctrls) {
 				ctrl->handle->pending_async_ctrls--;
+				uvc_pm_put(handle->chain->dev);
+			}
 		}
 
 		ctrl->handle = new_handle;
 		handle->pending_async_ctrls++;
+		uvc_pm_get(handle->chain->dev);
 		return;
 	}
 
@@ -1611,6 +1614,7 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
 	if (WARN_ON(!handle->pending_async_ctrls))
 		return;
 	handle->pending_async_ctrls--;
+	uvc_pm_put(handle->chain->dev);
 }
 
 void uvc_ctrl_status_event(struct uvc_video_chain *chain,
@@ -2815,6 +2819,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);
 
@@ -2829,7 +2834,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 de1e105f7263..1c9ac72be58a 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -691,6 +691,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);
@@ -857,6 +860,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 		return ret;
 
 	handle->is_streaming = true;
+	uvc_pm_get(stream->dev);
 
 	return 0;
 }
@@ -873,7 +877,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;
 }
@@ -1410,6 +1417,8 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 	void __user *up = compat_ptr(arg);
 	long ret;
 
+	guard(uvc_pm)(handle->stream->dev);
+
 	switch (cmd) {
 	case UVCIOC_CTRL_MAP32:
 		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
@@ -1444,6 +1453,16 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 }
 #endif
 
+static long uvc_v4l2_video_ioctl2(struct file *file,
+				  unsigned int cmd, unsigned long arg)
+{
+	struct uvc_fh *handle = file->private_data;
+
+	guard(uvc_pm)(handle->stream->dev);
+
+	return video_ioctl2(file, cmd, arg);
+}
+
 static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
 		    size_t count, loff_t *ppos)
 {
@@ -1529,7 +1548,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_video_ioctl2,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl32	= uvc_v4l2_compat_ioctl32,
 #endif
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index fbe3649c7cd6..eb8e374fa4c5 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -766,6 +766,7 @@ 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);
+DEFINE_GUARD(uvc_pm, struct uvc_device *, uvc_pm_get(_T), uvc_pm_put(_T))
 
 /* Controls */
 extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;

-- 
2.48.1.711.g2feabab25a-goog

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

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Mar 03, 2025 at 07:13:40PM +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

You're decreasing the granularity, not increasing it.

> 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 | 13 +++++++++++--
>  drivers/media/usb/uvc/uvc_v4l2.c | 23 +++++++++++++++++++++--
>  drivers/media/usb/uvc/uvcvideo.h |  1 +
>  3 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 4e58476d305e..47188c7f96c7 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1594,12 +1594,15 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
>  
>  		if (ctrl->handle) {
>  			WARN_ON(!ctrl->handle->pending_async_ctrls);
> -			if (ctrl->handle->pending_async_ctrls)
> +			if (ctrl->handle->pending_async_ctrls) {
>  				ctrl->handle->pending_async_ctrls--;
> +				uvc_pm_put(handle->chain->dev);

Shouldn't this be

				uvc_pm_put(ctrl->handle->chain->dev);

? In practice it won't make a difference as dev will be the same for
both, but it seems clearer.

> +			}
>  		}
>  
>  		ctrl->handle = new_handle;
>  		handle->pending_async_ctrls++;
> +		uvc_pm_get(handle->chain->dev);

Similarly, we should use ctrl->handle here too (including for the
pending_async_ctrls++).

>  		return;
>  	}
>  
> @@ -1611,6 +1614,7 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
>  	if (WARN_ON(!handle->pending_async_ctrls))
>  		return;
>  	handle->pending_async_ctrls--;
> +	uvc_pm_put(handle->chain->dev);
>  }
>  
>  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> @@ -2815,6 +2819,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);
>  
> @@ -2829,7 +2834,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 de1e105f7263..1c9ac72be58a 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -691,6 +691,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);
> @@ -857,6 +860,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>  		return ret;
>  
>  	handle->is_streaming = true;
> +	uvc_pm_get(stream->dev);
>  
>  	return 0;
>  }
> @@ -873,7 +877,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;
>  }
> @@ -1410,6 +1417,8 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  	void __user *up = compat_ptr(arg);
>  	long ret;
>  
> +	guard(uvc_pm)(handle->stream->dev);
> +
>  	switch (cmd) {
>  	case UVCIOC_CTRL_MAP32:
>  		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
> @@ -1444,6 +1453,16 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  }
>  #endif
>  
> +static long uvc_v4l2_video_ioctl2(struct file *file,
> +				  unsigned int cmd, unsigned long arg)
> +{
> +	struct uvc_fh *handle = file->private_data;
> +
> +	guard(uvc_pm)(handle->stream->dev);
> +
> +	return video_ioctl2(file, cmd, arg);
> +}
> +
>  static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
>  		    size_t count, loff_t *ppos)
>  {
> @@ -1529,7 +1548,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_video_ioctl2,

I'd have named this uvc_v4l2_unlocked_ioctl.

>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl32	= uvc_v4l2_compat_ioctl32,
>  #endif
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index fbe3649c7cd6..eb8e374fa4c5 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -766,6 +766,7 @@ 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);
> +DEFINE_GUARD(uvc_pm, struct uvc_device *, uvc_pm_get(_T), uvc_pm_put(_T))
>  
>  /* Controls */
>  extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Thu, Mar 27, 2025 at 07:52:27PM +0200, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> Thank you for the patch.
> 
> On Mon, Mar 03, 2025 at 07:13:40PM +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
> 
> You're decreasing the granularity, not increasing it.
> 
> > 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 | 13 +++++++++++--
> >  drivers/media/usb/uvc/uvc_v4l2.c | 23 +++++++++++++++++++++--
> >  drivers/media/usb/uvc/uvcvideo.h |  1 +
> >  3 files changed, 33 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 4e58476d305e..47188c7f96c7 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1594,12 +1594,15 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> >  
> >  		if (ctrl->handle) {
> >  			WARN_ON(!ctrl->handle->pending_async_ctrls);
> > -			if (ctrl->handle->pending_async_ctrls)
> > +			if (ctrl->handle->pending_async_ctrls) {
> >  				ctrl->handle->pending_async_ctrls--;
> > +				uvc_pm_put(handle->chain->dev);
> 
> Shouldn't this be
> 
> 				uvc_pm_put(ctrl->handle->chain->dev);
> 
> ? In practice it won't make a difference as dev will be the same for
> both, but it seems clearer.
> 
> > +			}
> >  		}
> >  
> >  		ctrl->handle = new_handle;
> >  		handle->pending_async_ctrls++;
> > +		uvc_pm_get(handle->chain->dev);
> 
> Similarly, we should use ctrl->handle here too (including for the
> pending_async_ctrls++).
> 
> >  		return;
> >  	}
> >  
> > @@ -1611,6 +1614,7 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> >  	if (WARN_ON(!handle->pending_async_ctrls))
> >  		return;
> >  	handle->pending_async_ctrls--;
> > +	uvc_pm_put(handle->chain->dev);
> >  }
> >  
> >  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> > @@ -2815,6 +2819,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);
> >  
> > @@ -2829,7 +2834,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 de1e105f7263..1c9ac72be58a 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -691,6 +691,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);
> > @@ -857,6 +860,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
> >  		return ret;
> >  
> >  	handle->is_streaming = true;
> > +	uvc_pm_get(stream->dev);

Another comment: shouldn't you handle the return value (here and
elsewhere, including where you use guards) ?

> >  
> >  	return 0;
> >  }
> > @@ -873,7 +877,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;
> >  }
> > @@ -1410,6 +1417,8 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
> >  	void __user *up = compat_ptr(arg);
> >  	long ret;
> >  
> > +	guard(uvc_pm)(handle->stream->dev);
> > +
> >  	switch (cmd) {
> >  	case UVCIOC_CTRL_MAP32:
> >  		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
> > @@ -1444,6 +1453,16 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
> >  }
> >  #endif
> >  
> > +static long uvc_v4l2_video_ioctl2(struct file *file,
> > +				  unsigned int cmd, unsigned long arg)
> > +{
> > +	struct uvc_fh *handle = file->private_data;
> > +
> > +	guard(uvc_pm)(handle->stream->dev);
> > +
> > +	return video_ioctl2(file, cmd, arg);
> > +}
> > +
> >  static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
> >  		    size_t count, loff_t *ppos)
> >  {
> > @@ -1529,7 +1548,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_video_ioctl2,
> 
> I'd have named this uvc_v4l2_unlocked_ioctl.
> 
> >  #ifdef CONFIG_COMPAT
> >  	.compat_ioctl32	= uvc_v4l2_compat_ioctl32,
> >  #endif
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index fbe3649c7cd6..eb8e374fa4c5 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -766,6 +766,7 @@ 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);
> > +DEFINE_GUARD(uvc_pm, struct uvc_device *, uvc_pm_get(_T), uvc_pm_put(_T))
> >  
> >  /* Controls */
> >  extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

On Thu, 27 Mar 2025 at 18:57, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Thu, Mar 27, 2025 at 07:52:27PM +0200, Laurent Pinchart wrote:
> > Hi Ricardo,
> >
> > Thank you for the patch.
> >
> > On Mon, Mar 03, 2025 at 07:13:40PM +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
> >
> > You're decreasing the granularity, not increasing it.
> >
> > > 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 | 13 +++++++++++--
> > >  drivers/media/usb/uvc/uvc_v4l2.c | 23 +++++++++++++++++++++--
> > >  drivers/media/usb/uvc/uvcvideo.h |  1 +
> > >  3 files changed, 33 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 4e58476d305e..47188c7f96c7 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1594,12 +1594,15 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> > >
> > >             if (ctrl->handle) {
> > >                     WARN_ON(!ctrl->handle->pending_async_ctrls);
> > > -                   if (ctrl->handle->pending_async_ctrls)
> > > +                   if (ctrl->handle->pending_async_ctrls) {
> > >                             ctrl->handle->pending_async_ctrls--;
> > > +                           uvc_pm_put(handle->chain->dev);
> >
> > Shouldn't this be
> >
> >                               uvc_pm_put(ctrl->handle->chain->dev);
> >
> > ? In practice it won't make a difference as dev will be the same for
> > both, but it seems clearer.
> >
> > > +                   }
> > >             }
> > >
> > >             ctrl->handle = new_handle;
> > >             handle->pending_async_ctrls++;
> > > +           uvc_pm_get(handle->chain->dev);
> >
> > Similarly, we should use ctrl->handle here too (including for the
> > pending_async_ctrls++).
> >
> > >             return;
> > >     }
> > >
> > > @@ -1611,6 +1614,7 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> > >     if (WARN_ON(!handle->pending_async_ctrls))
> > >             return;
> > >     handle->pending_async_ctrls--;
> > > +   uvc_pm_put(handle->chain->dev);
> > >  }
> > >
> > >  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> > > @@ -2815,6 +2819,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);
> > >
> > > @@ -2829,7 +2834,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 de1e105f7263..1c9ac72be58a 100644
> > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > @@ -691,6 +691,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);
> > > @@ -857,6 +860,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
> > >             return ret;
> > >
> > >     handle->is_streaming = true;
> > > +   uvc_pm_get(stream->dev);
>
> Another comment: shouldn't you handle the return value (here and
> elsewhere, including where you use guards) ?

Good point... I guess I got excited trying to use the guards :)
>
> > >
> > >     return 0;
> > >  }
> > > @@ -873,7 +877,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;
> > >  }
> > > @@ -1410,6 +1417,8 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
> > >     void __user *up = compat_ptr(arg);
> > >     long ret;
> > >
> > > +   guard(uvc_pm)(handle->stream->dev);
> > > +
> > >     switch (cmd) {
> > >     case UVCIOC_CTRL_MAP32:
> > >             ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
> > > @@ -1444,6 +1453,16 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
> > >  }
> > >  #endif
> > >
> > > +static long uvc_v4l2_video_ioctl2(struct file *file,
> > > +                             unsigned int cmd, unsigned long arg)
> > > +{
> > > +   struct uvc_fh *handle = file->private_data;
> > > +
> > > +   guard(uvc_pm)(handle->stream->dev);
> > > +
> > > +   return video_ioctl2(file, cmd, arg);
> > > +}
> > > +
> > >  static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
> > >                 size_t count, loff_t *ppos)
> > >  {
> > > @@ -1529,7 +1548,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_video_ioctl2,
> >
> > I'd have named this uvc_v4l2_unlocked_ioctl.
> >
> > >  #ifdef CONFIG_COMPAT
> > >     .compat_ioctl32 = uvc_v4l2_compat_ioctl32,
> > >  #endif
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index fbe3649c7cd6..eb8e374fa4c5 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -766,6 +766,7 @@ 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);
> > > +DEFINE_GUARD(uvc_pm, struct uvc_device *, uvc_pm_get(_T), uvc_pm_put(_T))
> > >
> > >  /* Controls */
> > >  extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

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

[Laurent Pinchart]

On Thu, Mar 27, 2025 at 10:05:19PM +0100, Ricardo Ribalda wrote:
> On Thu, 27 Mar 2025 at 18:57, Laurent Pinchart wrote:
> > On Thu, Mar 27, 2025 at 07:52:27PM +0200, Laurent Pinchart wrote:
> > > On Mon, Mar 03, 2025 at 07:13:40PM +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
> > >
> > > You're decreasing the granularity, not increasing it.
> > >
> > > > 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 | 13 +++++++++++--
> > > >  drivers/media/usb/uvc/uvc_v4l2.c | 23 +++++++++++++++++++++--
> > > >  drivers/media/usb/uvc/uvcvideo.h |  1 +
> > > >  3 files changed, 33 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 4e58476d305e..47188c7f96c7 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -1594,12 +1594,15 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> > > >
> > > >             if (ctrl->handle) {
> > > >                     WARN_ON(!ctrl->handle->pending_async_ctrls);
> > > > -                   if (ctrl->handle->pending_async_ctrls)
> > > > +                   if (ctrl->handle->pending_async_ctrls) {
> > > >                             ctrl->handle->pending_async_ctrls--;
> > > > +                           uvc_pm_put(handle->chain->dev);
> > >
> > > Shouldn't this be
> > >
> > >                               uvc_pm_put(ctrl->handle->chain->dev);
> > >
> > > ? In practice it won't make a difference as dev will be the same for
> > > both, but it seems clearer.
> > >
> > > > +                   }
> > > >             }
> > > >
> > > >             ctrl->handle = new_handle;
> > > >             handle->pending_async_ctrls++;
> > > > +           uvc_pm_get(handle->chain->dev);
> > >
> > > Similarly, we should use ctrl->handle here too (including for the
> > > pending_async_ctrls++).
> > >
> > > >             return;
> > > >     }
> > > >
> > > > @@ -1611,6 +1614,7 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> > > >     if (WARN_ON(!handle->pending_async_ctrls))
> > > >             return;
> > > >     handle->pending_async_ctrls--;
> > > > +   uvc_pm_put(handle->chain->dev);
> > > >  }
> > > >
> > > >  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> > > > @@ -2815,6 +2819,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);
> > > >
> > > > @@ -2829,7 +2834,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 de1e105f7263..1c9ac72be58a 100644
> > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > > > @@ -691,6 +691,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);
> > > > @@ -857,6 +860,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
> > > >             return ret;
> > > >
> > > >     handle->is_streaming = true;
> > > > +   uvc_pm_get(stream->dev);
> >
> > Another comment: shouldn't you handle the return value (here and
> > elsewhere, including where you use guards) ?
> 
> Good point... I guess I got excited trying to use the guards :)

I like them too :-)

> > > >
> > > >     return 0;
> > > >  }
> > > > @@ -873,7 +877,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;
> > > >  }
> > > > @@ -1410,6 +1417,8 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
> > > >     void __user *up = compat_ptr(arg);
> > > >     long ret;
> > > >
> > > > +   guard(uvc_pm)(handle->stream->dev);
> > > > +
> > > >     switch (cmd) {
> > > >     case UVCIOC_CTRL_MAP32:
> > > >             ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
> > > > @@ -1444,6 +1453,16 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
> > > >  }
> > > >  #endif
> > > >
> > > > +static long uvc_v4l2_video_ioctl2(struct file *file,
> > > > +                             unsigned int cmd, unsigned long arg)
> > > > +{
> > > > +   struct uvc_fh *handle = file->private_data;
> > > > +
> > > > +   guard(uvc_pm)(handle->stream->dev);
> > > > +
> > > > +   return video_ioctl2(file, cmd, arg);
> > > > +}
> > > > +
> > > >  static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
> > > >                 size_t count, loff_t *ppos)
> > > >  {
> > > > @@ -1529,7 +1548,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_video_ioctl2,
> > >
> > > I'd have named this uvc_v4l2_unlocked_ioctl.
> > >
> > > >  #ifdef CONFIG_COMPAT
> > > >     .compat_ioctl32 = uvc_v4l2_compat_ioctl32,
> > > >  #endif
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index fbe3649c7cd6..eb8e374fa4c5 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -766,6 +766,7 @@ 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);
> > > > +DEFINE_GUARD(uvc_pm, struct uvc_device *, uvc_pm_get(_T), uvc_pm_put(_T))
> > > >
> > > >  /* Controls */
> > > >  extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

Hi Laurent

On Thu, 27 Mar 2025 at 18:52, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Mon, Mar 03, 2025 at 07:13:40PM +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
>
> You're decreasing the granularity, not increasing it.

I believe that this patch increases the level of detail. So it is
increasing the granularity... But I might be wrong

>
> > 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 | 13 +++++++++++--
> >  drivers/media/usb/uvc/uvc_v4l2.c | 23 +++++++++++++++++++++--
> >  drivers/media/usb/uvc/uvcvideo.h |  1 +
> >  3 files changed, 33 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 4e58476d305e..47188c7f96c7 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1594,12 +1594,15 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> >
> >               if (ctrl->handle) {
> >                       WARN_ON(!ctrl->handle->pending_async_ctrls);
> > -                     if (ctrl->handle->pending_async_ctrls)
> > +                     if (ctrl->handle->pending_async_ctrls) {
> >                               ctrl->handle->pending_async_ctrls--;
> > +                             uvc_pm_put(handle->chain->dev);
>
> Shouldn't this be
>
>                                 uvc_pm_put(ctrl->handle->chain->dev);
>
> ? In practice it won't make a difference as dev will be the same for
> both, but it seems clearer.

The last line of the function needs to be
uvc_pm_put(handle->chain->dev);

So I'd rather not mix handle-> and ctrl->handle. As you say, both
should be identical.

>
> > +                     }
> >               }
> >
> >               ctrl->handle = new_handle;
> >               handle->pending_async_ctrls++;
> > +             uvc_pm_get(handle->chain->dev);
>
> Similarly, we should use ctrl->handle here too (including for the
> pending_async_ctrls++).
>
> >               return;
> >       }
> >
> > @@ -1611,6 +1614,7 @@ static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl,
> >       if (WARN_ON(!handle->pending_async_ctrls))
> >               return;
> >       handle->pending_async_ctrls--;
> > +     uvc_pm_put(handle->chain->dev);
> >  }
> >
> >  void uvc_ctrl_status_event(struct uvc_video_chain *chain,
> > @@ -2815,6 +2819,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);
> >
> > @@ -2829,7 +2834,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 de1e105f7263..1c9ac72be58a 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -691,6 +691,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);
> > @@ -857,6 +860,7 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
> >               return ret;
> >
> >       handle->is_streaming = true;
> > +     uvc_pm_get(stream->dev);
> >
> >       return 0;
> >  }
> > @@ -873,7 +877,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;
> >  }
> > @@ -1410,6 +1417,8 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
> >       void __user *up = compat_ptr(arg);
> >       long ret;
> >
> > +     guard(uvc_pm)(handle->stream->dev);
> > +
> >       switch (cmd) {
> >       case UVCIOC_CTRL_MAP32:
> >               ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
> > @@ -1444,6 +1453,16 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
> >  }
> >  #endif
> >
> > +static long uvc_v4l2_video_ioctl2(struct file *file,
> > +                               unsigned int cmd, unsigned long arg)
> > +{
> > +     struct uvc_fh *handle = file->private_data;
> > +
> > +     guard(uvc_pm)(handle->stream->dev);
> > +
> > +     return video_ioctl2(file, cmd, arg);
> > +}
> > +
> >  static ssize_t uvc_v4l2_read(struct file *file, char __user *data,
> >                   size_t count, loff_t *ppos)
> >  {
> > @@ -1529,7 +1548,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_video_ioctl2,
>
> I'd have named this uvc_v4l2_unlocked_ioctl.
Changed in the next version
>
> >  #ifdef CONFIG_COMPAT
> >       .compat_ioctl32 = uvc_v4l2_compat_ioctl32,
> >  #endif
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index fbe3649c7cd6..eb8e374fa4c5 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -766,6 +766,7 @@ 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);
> > +DEFINE_GUARD(uvc_pm, struct uvc_device *, uvc_pm_get(_T), uvc_pm_put(_T))
> >
> >  /* Controls */
> >  extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

[PATCH v5 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>
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 1c9ac72be58a..6af93e00b304 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -652,7 +652,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__);
@@ -662,12 +661,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;
@@ -701,7 +694,6 @@ static int uvc_v4l2_release(struct file *file)
 	kfree(handle);
 	file->private_data = NULL;
 
-	uvc_pm_put(stream->dev);
 	return 0;
 }
 

-- 
2.48.1.711.g2feabab25a-goog

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

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Mar 03, 2025 at 07:13:41PM +0000, Ricardo Ribalda wrote:
> 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.

We'll see :-)

> 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 | 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 1c9ac72be58a..6af93e00b304 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -652,7 +652,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__);
> @@ -662,12 +661,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;
> @@ -701,7 +694,6 @@ static int uvc_v4l2_release(struct file *file)
>  	kfree(handle);
>  	file->private_data = NULL;
>  
> -	uvc_pm_put(stream->dev);
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

[PATCH v5 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>
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 6af93e00b304..de8d26164996 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1450,6 +1450,26 @@ static long uvc_v4l2_video_ioctl2(struct file *file,
 {
 	struct uvc_fh *handle = file->private_data;
 
+	/* 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);
+	}
+
 	guard(uvc_pm)(handle->stream->dev);
 
 	return video_ioctl2(file, cmd, arg);

-- 
2.48.1.711.g2feabab25a-goog

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

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Mar 03, 2025 at 07:13:42PM +0000, 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>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

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

> ---
>  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 6af93e00b304..de8d26164996 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -1450,6 +1450,26 @@ static long uvc_v4l2_video_ioctl2(struct file *file,
>  {
>  	struct uvc_fh *handle = file->private_data;
>  
> +	/* 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);
> +	}
> +
>  	guard(uvc_pm)(handle->stream->dev);
>  
>  	return video_ioctl2(file, cmd, arg);

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Ricardo,

On 3-Mar-25 20:13, 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.
> 
> While developing this patchset, two bugs were found. The patchset has
> been developed so these fixes can be taken independently.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@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

Thank you for the new version.

I've merge the entire series into:
https://gitlab.freedesktop.org/linux-media/users/uvc/ -next now.

Regards,

Hans





> 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 | 13 +++++-
>  drivers/media/usb/uvc/uvc_v4l2.c | 99 ++++++++++++++++++++++++++++++----------
>  drivers/media/usb/uvc/uvcvideo.h |  6 +++
>  3 files changed, 92 insertions(+), 26 deletions(-)
> ---
> base-commit: d98e9213a768a3cc3a99f5e1abe09ad3baff2104
> change-id: 20241126-uvc-granpower-ng-069185a6d474
> 
> Best regards,