[PATCH v5 0/2] media: uvcvideo: Implement granular power management

[PATCH v5 0/2] media: uvcvideo: Implement granular power management

[Ricardo Ribalda]

Instead of suspending/resume the USB device at open()/close(), do it
when the device is actually used.

This way we can reduce the power consumption when a service is holding
the video device and leaving it in an idle state.

To: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Max Staudt <mstaudt@chromium.org>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Yunke Cao <yunkec@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v5:
- Improve uvc_fd padding (Thanks Sergey)
- Take the mutex always in the same order.
- Link to v4: https://lore.kernel.org/r/20220920-resend-powersave-v4-0-47484ae40761@chromium.org

Changes in v4:
- Remove patches to avoid crashes during device removal
- Link to v3: https://lore.kernel.org/r/20220920-resend-powersave-v3-0-c47856d8757e@chromium.org

Changes in v3:
- Rebase on top of uvc/next
- Reorder series, and put "controversial" patches at the end.
- Fix "use-before-set" bug. Thanks Max!
- Link to v2: https://lore.kernel.org/r/20220920-resend-powersave-v2-0-5135d1bb1c38@chromium.org

Changes in v2:
- Make access to uvc_status contitional
- Merge with Guenter race condition patchset: https://lore.kernel.org/lkml/20200917022547.198090-1-linux@roeck-us.net/
- Link to v1: https://lore.kernel.org/r/20220920-resend-powersave-v1-0-123aa2ba3836@chromium.org

---
Ricardo Ribalda (2):
      media: uvcvideo: Refactor streamon/streamoff
      media: uvcvideo: Do power management granularly

 drivers/media/usb/uvc/uvc_v4l2.c | 217 +++++++++++++++++++++++++++++++--------
 drivers/media/usb/uvc/uvcvideo.h |   1 +
 2 files changed, 178 insertions(+), 40 deletions(-)
---
base-commit: f599c56de2bfcf5ff791b0f4155e997e08e364f0
change-id: 20220920-resend-powersave-5981719ed267

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

[PATCH v5 1/2] media: uvcvideo: Refactor streamon/streamoff

[Ricardo Ribalda]

Add a new variable to handle the streaming state and handle the
streamoff errors, that were not handled before.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Max Staudt <mstaudt@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++++++---
 drivers/media/usb/uvc/uvcvideo.h |  1 +
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index f4d4c33b6dfb..1389a87b8ae1 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -840,13 +840,19 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
-	int ret;
+	int ret = -EBUSY;
 
 	if (!uvc_has_privileges(handle))
 		return -EBUSY;
 
 	mutex_lock(&stream->mutex);
+
+	if (handle->is_streaming)
+		goto unlock;
 	ret = uvc_queue_streamon(&stream->queue, type);
+	handle->is_streaming = !ret;
+
+unlock:
 	mutex_unlock(&stream->mutex);
 
 	return ret;
@@ -857,15 +863,22 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_streaming *stream = handle->stream;
+	int ret = 0;
 
 	if (!uvc_has_privileges(handle))
 		return -EBUSY;
 
 	mutex_lock(&stream->mutex);
-	uvc_queue_streamoff(&stream->queue, type);
+
+	if (!handle->is_streaming)
+		goto unlock;
+	ret = uvc_queue_streamoff(&stream->queue, type);
+	handle->is_streaming = !!ret;
+
+unlock:
 	mutex_unlock(&stream->mutex);
 
-	return 0;
+	return ret;
 }
 
 static int uvc_ioctl_enum_input(struct file *file, void *fh,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index df93db259312..0d9f335053b8 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -584,6 +584,7 @@ struct uvc_fh {
 	struct uvc_video_chain *chain;
 	struct uvc_streaming *stream;
 	enum uvc_handle_state state;
+	bool is_streaming;
 };
 
 struct uvc_driver {

-- 
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae

[PATCH v5 2/2] media: uvcvideo: Do power management granularly

[Ricardo Ribalda]

Instead of suspending/resume the USB device at open()/close(), do it
when the device is actually used.

This way we can reduce the power consumption when a service is holding
the video device and leaving it in an idle state.

Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-by: Max Staudt <mstaudt@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 198 +++++++++++++++++++++++++++++++--------
 1 file changed, 161 insertions(+), 37 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 1389a87b8ae1..2e8f78bd1e4e 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -25,6 +25,46 @@
 
 #include "uvcvideo.h"
 
+/* ------------------------------------------------------------------------
+ * UVC power management
+ */
+
+static int uvc_pm_get(struct uvc_streaming *stream)
+{
+	int ret;
+
+	ret = usb_autopm_get_interface(stream->dev->intf);
+	if (ret)
+		return ret;
+
+	mutex_lock(&stream->dev->lock);
+	if (!stream->dev->users)
+		ret = uvc_status_start(stream->dev, GFP_KERNEL);
+	if (!ret)
+		stream->dev->users++;
+	mutex_unlock(&stream->dev->lock);
+
+	if (ret)
+		usb_autopm_put_interface(stream->dev->intf);
+
+	return ret;
+}
+
+static void uvc_pm_put(struct uvc_streaming *stream)
+{
+	mutex_lock(&stream->dev->lock);
+	if (WARN_ON(!stream->dev->users)) {
+		mutex_unlock(&stream->dev->lock);
+		return;
+	}
+	stream->dev->users--;
+	if (!stream->dev->users)
+		uvc_status_stop(stream->dev);
+	mutex_unlock(&stream->dev->lock);
+
+	usb_autopm_put_interface(stream->dev->intf);
+}
+
 /* ------------------------------------------------------------------------
  * UVC ioctls
  */
@@ -249,6 +289,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
 	 * developers test their webcams with the Linux driver as well as with
 	 * the Windows driver).
 	 */
+	ret = uvc_pm_get(stream);
+	if (ret)
+		return ret;
 	mutex_lock(&stream->mutex);
 	if (stream->dev->quirks & UVC_QUIRK_PROBE_EXTRAFIELDS)
 		probe->dwMaxVideoFrameSize =
@@ -257,6 +300,7 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
 	/* Probe the device. */
 	ret = uvc_probe_video(stream, probe);
 	mutex_unlock(&stream->mutex);
+	uvc_pm_put(stream);
 	if (ret < 0)
 		return ret;
 
@@ -408,8 +452,8 @@ static int uvc_v4l2_get_streamparm(struct uvc_streaming *stream,
 	return 0;
 }
 
-static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
-		struct v4l2_streamparm *parm)
+static int __uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
+				     struct v4l2_streamparm *parm)
 {
 	struct uvc_streaming_control probe;
 	struct v4l2_fract timeperframe;
@@ -419,9 +463,6 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
 	unsigned int i;
 	int ret;
 
-	if (parm->type != stream->type)
-		return -EINVAL;
-
 	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		timeperframe = parm->parm.capture.timeperframe;
 	else
@@ -495,6 +536,25 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
 	return 0;
 }
 
+static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
+				   struct v4l2_streamparm *parm)
+{
+	int ret;
+
+	if (parm->type != stream->type)
+		return -EINVAL;
+
+	ret = uvc_pm_get(stream);
+	if (ret)
+		return ret;
+
+	ret = __uvc_v4l2_set_streamparm(stream, parm);
+
+	uvc_pm_put(stream);
+
+	return ret;
+}
+
 /* ------------------------------------------------------------------------
  * Privilege management
  */
@@ -559,36 +619,29 @@ 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__);
 
-	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;
-	}
 
-	mutex_lock(&stream->dev->lock);
-	if (stream->dev->users == 0) {
-		ret = uvc_status_start(stream->dev, GFP_KERNEL);
-		if (ret < 0) {
-			mutex_unlock(&stream->dev->lock);
-			usb_autopm_put_interface(stream->dev->intf);
+	/*
+	 * If the uvc evdev exists we cannot suspend when the device
+	 * is idle. Otherwise we will miss button actions.
+	 */
+	if (stream->dev->input) {
+		int ret;
+
+		ret = uvc_pm_get(stream);
+		if (ret) {
 			kfree(handle);
 			return ret;
 		}
 	}
 
-	stream->dev->users++;
-	mutex_unlock(&stream->dev->lock);
-
 	v4l2_fh_init(&handle->vfh, &stream->vdev);
 	v4l2_fh_add(&handle->vfh);
 	handle->chain = stream->chain;
@@ -610,6 +663,16 @@ static int uvc_v4l2_release(struct file *file)
 	if (uvc_has_privileges(handle))
 		uvc_queue_release(&stream->queue);
 
+	/*
+	 * Release cannot happen at the same time as streamon/streamoff
+	 * no need to take the stream->mutex.
+	 */
+	if (handle->is_streaming)
+		uvc_pm_put(stream);
+
+	if (stream->dev->input)
+		uvc_pm_put(stream);
+
 	/* Release the file handle. */
 	uvc_dismiss_privileges(handle);
 	v4l2_fh_del(&handle->vfh);
@@ -617,12 +680,6 @@ static int uvc_v4l2_release(struct file *file)
 	kfree(handle);
 	file->private_data = NULL;
 
-	mutex_lock(&stream->dev->lock);
-	if (--stream->dev->users == 0)
-		uvc_status_stop(stream->dev);
-	mutex_unlock(&stream->dev->lock);
-
-	usb_autopm_put_interface(stream->dev->intf);
 	return 0;
 }
 
@@ -849,9 +906,17 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
 
 	if (handle->is_streaming)
 		goto unlock;
+
+	ret = uvc_pm_get(stream);
+	if (ret)
+		goto unlock;
+
 	ret = uvc_queue_streamon(&stream->queue, type);
 	handle->is_streaming = !ret;
 
+	if (!handle->is_streaming)
+		uvc_pm_put(stream);
+
 unlock:
 	mutex_unlock(&stream->mutex);
 
@@ -875,6 +940,9 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
 	ret = uvc_queue_streamoff(&stream->queue, type);
 	handle->is_streaming = !!ret;
 
+	if (!handle->is_streaming)
+		uvc_pm_put(stream);
+
 unlock:
 	mutex_unlock(&stream->mutex);
 
@@ -928,6 +996,7 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_video_chain *chain = handle->chain;
+	struct uvc_streaming *stream = handle->stream;
 	u8 *buf;
 	int ret;
 
@@ -941,9 +1010,16 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
 	if (!buf)
 		return -ENOMEM;
 
+	ret = uvc_pm_get(stream);
+	if (ret) {
+		kfree(buf);
+		return ret;
+	}
+
 	ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id,
 			     chain->dev->intfnum,  UVC_SU_INPUT_SELECT_CONTROL,
 			     buf, 1);
+	uvc_pm_put(stream);
 	if (!ret)
 		*input = *buf - 1;
 
@@ -956,6 +1032,7 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_video_chain *chain = handle->chain;
+	struct uvc_streaming *stream = handle->stream;
 	u8 *buf;
 	int ret;
 
@@ -977,10 +1054,17 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
 	if (!buf)
 		return -ENOMEM;
 
+	ret = uvc_pm_get(stream);
+	if (ret) {
+		kfree(buf);
+		return ret;
+	}
+
 	*buf = input + 1;
 	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id,
 			     chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
 			     buf, 1);
+	uvc_pm_put(stream);
 	kfree(buf);
 
 	return ret;
@@ -991,8 +1075,15 @@ static int uvc_ioctl_queryctrl(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_video_chain *chain = handle->chain;
+	struct uvc_streaming *stream = handle->stream;
+	int ret;
 
-	return uvc_query_v4l2_ctrl(chain, qc);
+	ret = uvc_pm_get(stream);
+	if (ret)
+		return ret;
+	ret = uvc_query_v4l2_ctrl(chain, qc);
+	uvc_pm_put(stream);
+	return ret;
 }
 
 static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
@@ -1000,10 +1091,15 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_video_chain *chain = handle->chain;
+	struct uvc_streaming *stream = handle->stream;
 	struct v4l2_queryctrl qc = { qec->id };
 	int ret;
 
+	ret = uvc_pm_get(stream);
+	if (ret)
+		return ret;
 	ret = uvc_query_v4l2_ctrl(chain, &qc);
+	uvc_pm_put(stream);
 	if (ret)
 		return ret;
 
@@ -1049,6 +1145,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_video_chain *chain = handle->chain;
+	struct uvc_streaming *stream = handle->stream;
 	struct v4l2_ext_control *ctrl = ctrls->controls;
 	unsigned int i;
 	int ret;
@@ -1073,22 +1170,30 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
 		return 0;
 	}
 
+	ret = uvc_pm_get(stream);
+	if (ret)
+		return ret;
 	ret = uvc_ctrl_begin(chain);
-	if (ret < 0)
+	if (ret < 0) {
+		uvc_pm_put(stream);
 		return ret;
+	}
 
 	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
 		ret = uvc_ctrl_get(chain, ctrl);
 		if (ret < 0) {
 			uvc_ctrl_rollback(handle);
 			ctrls->error_idx = i;
-			return ret;
+			goto done;
 		}
 	}
 
 	ctrls->error_idx = 0;
 
-	return uvc_ctrl_rollback(handle);
+	ret = uvc_ctrl_rollback(handle);
+done:
+	uvc_pm_put(stream);
+	return ret;
 }
 
 static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
@@ -1097,6 +1202,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
 {
 	struct v4l2_ext_control *ctrl = ctrls->controls;
 	struct uvc_video_chain *chain = handle->chain;
+	struct uvc_streaming *stream = handle->stream;
 	unsigned int i;
 	int ret;
 
@@ -1104,9 +1210,15 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
 	if (ret < 0)
 		return ret;
 
+	ret = uvc_pm_get(stream);
+	if (ret)
+		return ret;
+
 	ret = uvc_ctrl_begin(chain);
-	if (ret < 0)
+	if (ret < 0) {
+		uvc_pm_put(stream);
 		return ret;
+	}
 
 	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
 		ret = uvc_ctrl_set(handle, ctrl);
@@ -1114,16 +1226,20 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
 			uvc_ctrl_rollback(handle);
 			ctrls->error_idx = ioctl == VIDIOC_S_EXT_CTRLS ?
 						    ctrls->count : i;
-			return ret;
+			goto done;
 		}
 	}
 
 	ctrls->error_idx = 0;
 
 	if (ioctl == VIDIOC_S_EXT_CTRLS)
-		return uvc_ctrl_commit(handle, ctrls);
+		ret = uvc_ctrl_commit(handle, ctrls);
 	else
-		return uvc_ctrl_rollback(handle);
+		ret = uvc_ctrl_rollback(handle);
+
+done:
+	uvc_pm_put(stream);
+	return ret;
 }
 
 static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh,
@@ -1147,8 +1263,16 @@ static int uvc_ioctl_querymenu(struct file *file, void *fh,
 {
 	struct uvc_fh *handle = fh;
 	struct uvc_video_chain *chain = handle->chain;
+	struct uvc_streaming *stream = handle->stream;
+	int ret;
 
-	return uvc_query_v4l2_menu(chain, qm);
+	ret = uvc_pm_get(stream);
+	if (ret)
+		return ret;
+	ret = uvc_query_v4l2_menu(chain, qm);
+	uvc_pm_put(stream);
+
+	return ret;
 }
 
 static int uvc_ioctl_g_selection(struct file *file, void *fh,

-- 
2.39.0.rc0.267.gcb52ba06e7-goog-b4-0.11.0-dev-696ae

Re: [PATCH v5 1/2] media: uvcvideo: Refactor streamon/streamoff

[Sergey Senozhatsky]

On (22/12/06 15:06), Ricardo Ribalda wrote:
> 
> Add a new variable to handle the streaming state and handle the
> streamoff errors, that were not handled before.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Max Staudt <mstaudt@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH v5 2/2] media: uvcvideo: Do power management granularly

[Sergey Senozhatsky]

On (22/12/06 15:06), Ricardo Ribalda wrote:
> 
> Instead of suspending/resume the USB device at open()/close(), do it
> when the device is actually used.
> 
> This way we can reduce the power consumption when a service is holding
> the video device and leaving it in an idle state.
> 
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> Reviewed-by: Max Staudt <mstaudt@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH v5 1/2] media: uvcvideo: Refactor streamon/streamoff

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 06, 2022 at 03:06:55PM +0100, Ricardo Ribalda wrote:
> Add a new variable to handle the streaming state and handle the
> streamoff errors, that were not handled before.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Max Staudt <mstaudt@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++++++---
>  drivers/media/usb/uvc/uvcvideo.h |  1 +
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index f4d4c33b6dfb..1389a87b8ae1 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -840,13 +840,19 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>  {
>  	struct uvc_fh *handle = fh;
>  	struct uvc_streaming *stream = handle->stream;
> -	int ret;
> +	int ret = -EBUSY;
>  
>  	if (!uvc_has_privileges(handle))
>  		return -EBUSY;
>  
>  	mutex_lock(&stream->mutex);
> +
> +	if (handle->is_streaming)
> +		goto unlock;

This isn't needed, uvc_queue_streamon() calls vb2_streamon(), which
returns an error if the queue is already streaming.

>  	ret = uvc_queue_streamon(&stream->queue, type);
> +	handle->is_streaming = !ret;

You can just turn this into

	if (!ret)
		handle->is_streaming = true;

and drop all other changes to this function.

> +
> +unlock:
>  	mutex_unlock(&stream->mutex);
>  
>  	return ret;
> @@ -857,15 +863,22 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
>  {
>  	struct uvc_fh *handle = fh;
>  	struct uvc_streaming *stream = handle->stream;
> +	int ret = 0;
>  
>  	if (!uvc_has_privileges(handle))
>  		return -EBUSY;
>  
>  	mutex_lock(&stream->mutex);
> -	uvc_queue_streamoff(&stream->queue, type);
> +
> +	if (!handle->is_streaming)
> +		goto unlock;

More than unneeded, this is wrong. Calling VIDIOC_STREAMOFF on a queue
that is not streaming is a valid use case, it's the only way to release
buffers that have been queued to the device. vb2_core_streamoff()
handles this correctly. You should drop this check.

> +	ret = uvc_queue_streamoff(&stream->queue, type);
> +	handle->is_streaming = !!ret;

And turn this into

	if (!ret)
		handle->is_streaming = false;

and drop all other changes to this function.

> +
> +unlock:
>  	mutex_unlock(&stream->mutex);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int uvc_ioctl_enum_input(struct file *file, void *fh,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df93db259312..0d9f335053b8 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -584,6 +584,7 @@ struct uvc_fh {
>  	struct uvc_video_chain *chain;
>  	struct uvc_streaming *stream;
>  	enum uvc_handle_state state;
> +	bool is_streaming;
>  };
>  
>  struct uvc_driver {
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/2] media: uvcvideo: Do power management granularly

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tue, Dec 06, 2022 at 03:06:56PM +0100, Ricardo Ribalda wrote:
> Instead of suspending/resume the USB device at open()/close(), do it
> when the device is actually used.
> 
> This way we can reduce the power consumption when a service is holding
> the video device and leaving it in an idle state.
> 
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> Reviewed-by: Max Staudt <mstaudt@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 198 +++++++++++++++++++++++++++++++--------
>  1 file changed, 161 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 1389a87b8ae1..2e8f78bd1e4e 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -25,6 +25,46 @@
>  
>  #include "uvcvideo.h"
>  
> +/* ------------------------------------------------------------------------
> + * UVC power management
> + */
> +
> +static int uvc_pm_get(struct uvc_streaming *stream)
> +{
> +	int ret;
> +
> +	ret = usb_autopm_get_interface(stream->dev->intf);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&stream->dev->lock);
> +	if (!stream->dev->users)
> +		ret = uvc_status_start(stream->dev, GFP_KERNEL);
> +	if (!ret)
> +		stream->dev->users++;
> +	mutex_unlock(&stream->dev->lock);
> +
> +	if (ret)
> +		usb_autopm_put_interface(stream->dev->intf);
> +
> +	return ret;
> +}
> +
> +static void uvc_pm_put(struct uvc_streaming *stream)
> +{
> +	mutex_lock(&stream->dev->lock);
> +	if (WARN_ON(!stream->dev->users)) {
> +		mutex_unlock(&stream->dev->lock);
> +		return;
> +	}
> +	stream->dev->users--;
> +	if (!stream->dev->users)
> +		uvc_status_stop(stream->dev);
> +	mutex_unlock(&stream->dev->lock);
> +
> +	usb_autopm_put_interface(stream->dev->intf);
> +}
> +
>  /* ------------------------------------------------------------------------
>   * UVC ioctls
>   */
> @@ -249,6 +289,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
>  	 * developers test their webcams with the Linux driver as well as with
>  	 * the Windows driver).
>  	 */
> +	ret = uvc_pm_get(stream);
> +	if (ret)
> +		return ret;
>  	mutex_lock(&stream->mutex);
>  	if (stream->dev->quirks & UVC_QUIRK_PROBE_EXTRAFIELDS)
>  		probe->dwMaxVideoFrameSize =
> @@ -257,6 +300,7 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
>  	/* Probe the device. */
>  	ret = uvc_probe_video(stream, probe);
>  	mutex_unlock(&stream->mutex);
> +	uvc_pm_put(stream);

Sprinkling get/put calls around ioctls individually is error-prone. For
instance, I think you're missing uvc_xu_ctrl_query() already, and
possibly other ioctls. It would be better to wrap the ioctl calls only
level up, essentially doing something like

@@ -1432,6 +1490,7 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
 	struct uvc_fh *handle = file->private_data;
+	struct uvc_streaming *stream = handle->stream;
 	union {
 		struct uvc_xu_control_mapping xmap;
 		struct uvc_xu_control_query xqry;
@@ -1439,36 +1498,41 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
 	void __user *up = compat_ptr(arg);
 	long ret;

+	ret = uvc_pm_get(stream);
+	if (ret)
+		return ret;
+
 	switch (cmd) {
 	case UVCIOC_CTRL_MAP32:
 		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_ioctl_ctrl_map(handle->chain, &karg.xmap);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_v4l2_put_xu_mapping(&karg.xmap, up);
 		if (ret)
-			return ret;
-
+			break;
 		break;

 	case UVCIOC_CTRL_QUERY32:
 		ret = uvc_v4l2_get_xu_query(&karg.xqry, up);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_xu_ctrl_query(handle->chain, &karg.xqry);
 		if (ret)
-			return ret;
+			break;
 		ret = uvc_v4l2_put_xu_query(&karg.xqry, up);
 		if (ret)
-			return ret;
+			break;
 		break;

 	default:
-		return -ENOIOCTLCMD;
+		ret = -ENOIOCTLCMD;
+		break;
 	}

+	uvc_pm_put(stream);
 	return ret;
 }
 #endif
@@ -1558,7 +1622,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_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl32	= uvc_v4l2_compat_ioctl32,
 #endif

This means, however, that we'll call uvc_pm_get() and uvc_pm_put()
around more ioctls. As we're using usb_autopm_put_interface(), I assume
that the USB core delays device suspend, so it's not much of an issue,
we won't actually suspend the device synchronously every time, and if
userspace issues ioctls with short intervals (less than 2s with the
default kernel configuration), it should be fine.

(Side note: I've looked at the USB autosuspend implementation, and it
seems to use pm_runtime_put_sync(), with manual autosuspend handling in
the RPM suspend handler. I got scared and ran away without making sure
it actually has a low cost as stated above :-S)

However, I'm more bother by the fact that the status endpoint polling is
started and stopped synchronously for each ioctl call, for two reasons.
One of them is the overhead of doing so for each ioctl call. It would be
made worse by wrapping the top-level ioctl handlers with get/put calls,
but it's already an issue without that.

The other reason is that I think it introduces a regression. UVC devices
can implement asynchronous controls, to report the new value of a
control at a later time. This is used for controls that take time to be
set, typically all controls related to physical motors (pan/tilt for
instance). The uvcvideo driver sends control change events to userspace
in that case. With this patch, and with commit "media: uvcvideo: Only
create input devs if hw supports it" merged, if an aplication opens a
video device but doesn't start streaming, the status URB will be killed
as soon as a S_CTRL ioctl returns, which will prevent asynchronous event
reporting from working.

It seems that one way to fix this would be to start/stop the status
endpoint polling in the resume/suspend handlers. I haven't investigated
though. I may give it a try, so please ping me on IRC before you resume
work on this. Feel free to comment by e-mail on the above though, and
tell me where I got it wrong :-)

>  	if (ret < 0)
>  		return ret;
>  
> @@ -408,8 +452,8 @@ static int uvc_v4l2_get_streamparm(struct uvc_streaming *stream,
>  	return 0;
>  }
>  
> -static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> -		struct v4l2_streamparm *parm)
> +static int __uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> +				     struct v4l2_streamparm *parm)
>  {
>  	struct uvc_streaming_control probe;
>  	struct v4l2_fract timeperframe;
> @@ -419,9 +463,6 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
>  	unsigned int i;
>  	int ret;
>  
> -	if (parm->type != stream->type)
> -		return -EINVAL;
> -
>  	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>  		timeperframe = parm->parm.capture.timeperframe;
>  	else
> @@ -495,6 +536,25 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
>  	return 0;
>  }
>  
> +static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> +				   struct v4l2_streamparm *parm)
> +{
> +	int ret;
> +
> +	if (parm->type != stream->type)
> +		return -EINVAL;
> +
> +	ret = uvc_pm_get(stream);
> +	if (ret)
> +		return ret;
> +
> +	ret = __uvc_v4l2_set_streamparm(stream, parm);
> +
> +	uvc_pm_put(stream);
> +
> +	return ret;
> +}
> +
>  /* ------------------------------------------------------------------------
>   * Privilege management
>   */
> @@ -559,36 +619,29 @@ 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__);
>  
> -	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;
> -	}
>  
> -	mutex_lock(&stream->dev->lock);
> -	if (stream->dev->users == 0) {
> -		ret = uvc_status_start(stream->dev, GFP_KERNEL);
> -		if (ret < 0) {
> -			mutex_unlock(&stream->dev->lock);
> -			usb_autopm_put_interface(stream->dev->intf);
> +	/*
> +	 * If the uvc evdev exists we cannot suspend when the device

s/uvc evdev/UVC input device/

> +	 * is idle. Otherwise we will miss button actions.
> +	 */
> +	if (stream->dev->input) {
> +		int ret;
> +
> +		ret = uvc_pm_get(stream);
> +		if (ret) {
>  			kfree(handle);
>  			return ret;
>  		}
>  	}
>  
> -	stream->dev->users++;
> -	mutex_unlock(&stream->dev->lock);
> -
>  	v4l2_fh_init(&handle->vfh, &stream->vdev);
>  	v4l2_fh_add(&handle->vfh);
>  	handle->chain = stream->chain;
> @@ -610,6 +663,16 @@ static int uvc_v4l2_release(struct file *file)
>  	if (uvc_has_privileges(handle))
>  		uvc_queue_release(&stream->queue);
>  
> +	/*
> +	 * Release cannot happen at the same time as streamon/streamoff
> +	 * no need to take the stream->mutex.

There seems to be something missing here, maybe

	 * Release cannot happen at the same time as streamon/streamoff, so
	 * there is no need to take the stream->mutex.

> +	 */
> +	if (handle->is_streaming)
> +		uvc_pm_put(stream);
> +
> +	if (stream->dev->input)
> +		uvc_pm_put(stream);
> +
>  	/* Release the file handle. */
>  	uvc_dismiss_privileges(handle);
>  	v4l2_fh_del(&handle->vfh);
> @@ -617,12 +680,6 @@ static int uvc_v4l2_release(struct file *file)
>  	kfree(handle);
>  	file->private_data = NULL;
>  
> -	mutex_lock(&stream->dev->lock);
> -	if (--stream->dev->users == 0)
> -		uvc_status_stop(stream->dev);
> -	mutex_unlock(&stream->dev->lock);
> -
> -	usb_autopm_put_interface(stream->dev->intf);
>  	return 0;
>  }
>  
> @@ -849,9 +906,17 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
>  
>  	if (handle->is_streaming)
>  		goto unlock;
> +
> +	ret = uvc_pm_get(stream);
> +	if (ret)
> +		goto unlock;
> +
>  	ret = uvc_queue_streamon(&stream->queue, type);
>  	handle->is_streaming = !ret;
>  
> +	if (!handle->is_streaming)
> +		uvc_pm_put(stream);
> +
>  unlock:
>  	mutex_unlock(&stream->mutex);
>  
> @@ -875,6 +940,9 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
>  	ret = uvc_queue_streamoff(&stream->queue, type);
>  	handle->is_streaming = !!ret;
>  
> +	if (!handle->is_streaming)
> +		uvc_pm_put(stream);
> +
>  unlock:
>  	mutex_unlock(&stream->mutex);
>  
> @@ -928,6 +996,7 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
>  {
>  	struct uvc_fh *handle = fh;
>  	struct uvc_video_chain *chain = handle->chain;
> +	struct uvc_streaming *stream = handle->stream;
>  	u8 *buf;
>  	int ret;
>  
> @@ -941,9 +1010,16 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
>  	if (!buf)
>  		return -ENOMEM;
>  
> +	ret = uvc_pm_get(stream);
> +	if (ret) {
> +		kfree(buf);
> +		return ret;
> +	}
> +
>  	ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id,
>  			     chain->dev->intfnum,  UVC_SU_INPUT_SELECT_CONTROL,
>  			     buf, 1);
> +	uvc_pm_put(stream);
>  	if (!ret)
>  		*input = *buf - 1;
>  
> @@ -956,6 +1032,7 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
>  {
>  	struct uvc_fh *handle = fh;
>  	struct uvc_video_chain *chain = handle->chain;
> +	struct uvc_streaming *stream = handle->stream;
>  	u8 *buf;
>  	int ret;
>  
> @@ -977,10 +1054,17 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
>  	if (!buf)
>  		return -ENOMEM;
>  
> +	ret = uvc_pm_get(stream);
> +	if (ret) {
> +		kfree(buf);
> +		return ret;
> +	}
> +
>  	*buf = input + 1;
>  	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id,
>  			     chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
>  			     buf, 1);
> +	uvc_pm_put(stream);
>  	kfree(buf);
>  
>  	return ret;
> @@ -991,8 +1075,15 @@ static int uvc_ioctl_queryctrl(struct file *file, void *fh,
>  {
>  	struct uvc_fh *handle = fh;
>  	struct uvc_video_chain *chain = handle->chain;
> +	struct uvc_streaming *stream = handle->stream;
> +	int ret;
>  
> -	return uvc_query_v4l2_ctrl(chain, qc);
> +	ret = uvc_pm_get(stream);
> +	if (ret)
> +		return ret;
> +	ret = uvc_query_v4l2_ctrl(chain, qc);
> +	uvc_pm_put(stream);
> +	return ret;
>  }
>  
>  static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
> @@ -1000,10 +1091,15 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
>  {
>  	struct uvc_fh *handle = fh;
>  	struct uvc_video_chain *chain = handle->chain;
> +	struct uvc_streaming *stream = handle->stream;
>  	struct v4l2_queryctrl qc = { qec->id };
>  	int ret;
>  
> +	ret = uvc_pm_get(stream);
> +	if (ret)
> +		return ret;
>  	ret = uvc_query_v4l2_ctrl(chain, &qc);
> +	uvc_pm_put(stream);
>  	if (ret)
>  		return ret;
>  
> @@ -1049,6 +1145,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>  {
>  	struct uvc_fh *handle = fh;
>  	struct uvc_video_chain *chain = handle->chain;
> +	struct uvc_streaming *stream = handle->stream;
>  	struct v4l2_ext_control *ctrl = ctrls->controls;
>  	unsigned int i;
>  	int ret;
> @@ -1073,22 +1170,30 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
>  		return 0;
>  	}
>  
> +	ret = uvc_pm_get(stream);
> +	if (ret)
> +		return ret;
>  	ret = uvc_ctrl_begin(chain);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		uvc_pm_put(stream);
>  		return ret;
> +	}
>  
>  	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
>  		ret = uvc_ctrl_get(chain, ctrl);
>  		if (ret < 0) {
>  			uvc_ctrl_rollback(handle);
>  			ctrls->error_idx = i;
> -			return ret;
> +			goto done;
>  		}
>  	}
>  
>  	ctrls->error_idx = 0;
>  
> -	return uvc_ctrl_rollback(handle);
> +	ret = uvc_ctrl_rollback(handle);
> +done:
> +	uvc_pm_put(stream);
> +	return ret;
>  }
>  
>  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
> @@ -1097,6 +1202,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  {
>  	struct v4l2_ext_control *ctrl = ctrls->controls;
>  	struct uvc_video_chain *chain = handle->chain;
> +	struct uvc_streaming *stream = handle->stream;
>  	unsigned int i;
>  	int ret;
>  
> @@ -1104,9 +1210,15 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = uvc_pm_get(stream);
> +	if (ret)
> +		return ret;
> +
>  	ret = uvc_ctrl_begin(chain);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		uvc_pm_put(stream);
>  		return ret;
> +	}
>  
>  	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
>  		ret = uvc_ctrl_set(handle, ctrl);
> @@ -1114,16 +1226,20 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
>  			uvc_ctrl_rollback(handle);
>  			ctrls->error_idx = ioctl == VIDIOC_S_EXT_CTRLS ?
>  						    ctrls->count : i;
> -			return ret;
> +			goto done;
>  		}
>  	}
>  
>  	ctrls->error_idx = 0;
>  
>  	if (ioctl == VIDIOC_S_EXT_CTRLS)
> -		return uvc_ctrl_commit(handle, ctrls);
> +		ret = uvc_ctrl_commit(handle, ctrls);
>  	else
> -		return uvc_ctrl_rollback(handle);
> +		ret = uvc_ctrl_rollback(handle);
> +
> +done:
> +	uvc_pm_put(stream);
> +	return ret;
>  }
>  
>  static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh,
> @@ -1147,8 +1263,16 @@ static int uvc_ioctl_querymenu(struct file *file, void *fh,
>  {
>  	struct uvc_fh *handle = fh;
>  	struct uvc_video_chain *chain = handle->chain;
> +	struct uvc_streaming *stream = handle->stream;
> +	int ret;
>  
> -	return uvc_query_v4l2_menu(chain, qm);
> +	ret = uvc_pm_get(stream);
> +	if (ret)
> +		return ret;
> +	ret = uvc_query_v4l2_menu(chain, qm);
> +	uvc_pm_put(stream);
> +
> +	return ret;
>  }
>  
>  static int uvc_ioctl_g_selection(struct file *file, void *fh,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/2] media: uvcvideo: Do power management granularly

[Laurent Pinchart]

On Wed, Dec 28, 2022 at 01:34:05AM +0200, Laurent Pinchart wrote:
> Hi Ricardo,
> 
> Thank you for the patch.
> 
> On Tue, Dec 06, 2022 at 03:06:56PM +0100, Ricardo Ribalda wrote:
> > Instead of suspending/resume the USB device at open()/close(), do it
> > when the device is actually used.
> > 
> > This way we can reduce the power consumption when a service is holding
> > the video device and leaving it in an idle state.
> > 
> > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > Reviewed-by: Max Staudt <mstaudt@chromium.org>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_v4l2.c | 198 +++++++++++++++++++++++++++++++--------
> >  1 file changed, 161 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 1389a87b8ae1..2e8f78bd1e4e 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -25,6 +25,46 @@
> >  
> >  #include "uvcvideo.h"
> >  
> > +/* ------------------------------------------------------------------------
> > + * UVC power management
> > + */
> > +
> > +static int uvc_pm_get(struct uvc_streaming *stream)
> > +{
> > +	int ret;
> > +
> > +	ret = usb_autopm_get_interface(stream->dev->intf);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&stream->dev->lock);
> > +	if (!stream->dev->users)
> > +		ret = uvc_status_start(stream->dev, GFP_KERNEL);
> > +	if (!ret)
> > +		stream->dev->users++;
> > +	mutex_unlock(&stream->dev->lock);
> > +
> > +	if (ret)
> > +		usb_autopm_put_interface(stream->dev->intf);
> > +
> > +	return ret;
> > +}
> > +
> > +static void uvc_pm_put(struct uvc_streaming *stream)
> > +{
> > +	mutex_lock(&stream->dev->lock);
> > +	if (WARN_ON(!stream->dev->users)) {
> > +		mutex_unlock(&stream->dev->lock);
> > +		return;
> > +	}
> > +	stream->dev->users--;
> > +	if (!stream->dev->users)
> > +		uvc_status_stop(stream->dev);
> > +	mutex_unlock(&stream->dev->lock);
> > +
> > +	usb_autopm_put_interface(stream->dev->intf);
> > +}
> > +
> >  /* ------------------------------------------------------------------------
> >   * UVC ioctls
> >   */
> > @@ -249,6 +289,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
> >  	 * developers test their webcams with the Linux driver as well as with
> >  	 * the Windows driver).
> >  	 */
> > +	ret = uvc_pm_get(stream);
> > +	if (ret)
> > +		return ret;
> >  	mutex_lock(&stream->mutex);
> >  	if (stream->dev->quirks & UVC_QUIRK_PROBE_EXTRAFIELDS)
> >  		probe->dwMaxVideoFrameSize =
> > @@ -257,6 +300,7 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
> >  	/* Probe the device. */
> >  	ret = uvc_probe_video(stream, probe);
> >  	mutex_unlock(&stream->mutex);
> > +	uvc_pm_put(stream);
> 
> Sprinkling get/put calls around ioctls individually is error-prone. For
> instance, I think you're missing uvc_xu_ctrl_query() already, and
> possibly other ioctls. It would be better to wrap the ioctl calls only
> level up, essentially doing something like
> 
> @@ -1432,6 +1490,7 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  		     unsigned int cmd, unsigned long arg)
>  {
>  	struct uvc_fh *handle = file->private_data;
> +	struct uvc_streaming *stream = handle->stream;
>  	union {
>  		struct uvc_xu_control_mapping xmap;
>  		struct uvc_xu_control_query xqry;
> @@ -1439,36 +1498,41 @@ static long uvc_v4l2_compat_ioctl32(struct file *file,
>  	void __user *up = compat_ptr(arg);
>  	long ret;
> 
> +	ret = uvc_pm_get(stream);
> +	if (ret)
> +		return ret;
> +
>  	switch (cmd) {
>  	case UVCIOC_CTRL_MAP32:
>  		ret = uvc_v4l2_get_xu_mapping(&karg.xmap, up);
>  		if (ret)
> -			return ret;
> +			break;
>  		ret = uvc_ioctl_ctrl_map(handle->chain, &karg.xmap);
>  		if (ret)
> -			return ret;
> +			break;
>  		ret = uvc_v4l2_put_xu_mapping(&karg.xmap, up);
>  		if (ret)
> -			return ret;
> -
> +			break;
>  		break;
> 
>  	case UVCIOC_CTRL_QUERY32:
>  		ret = uvc_v4l2_get_xu_query(&karg.xqry, up);
>  		if (ret)
> -			return ret;
> +			break;
>  		ret = uvc_xu_ctrl_query(handle->chain, &karg.xqry);
>  		if (ret)
> -			return ret;
> +			break;
>  		ret = uvc_v4l2_put_xu_query(&karg.xqry, up);
>  		if (ret)
> -			return ret;
> +			break;
>  		break;
> 
>  	default:
> -		return -ENOIOCTLCMD;
> +		ret = -ENOIOCTLCMD;
> +		break;
>  	}
> 
> +	uvc_pm_put(stream);
>  	return ret;
>  }
>  #endif
> @@ -1558,7 +1622,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_ioctl,
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl32	= uvc_v4l2_compat_ioctl32,
>  #endif
> 
> This means, however, that we'll call uvc_pm_get() and uvc_pm_put()
> around more ioctls. As we're using usb_autopm_put_interface(), I assume
> that the USB core delays device suspend, so it's not much of an issue,
> we won't actually suspend the device synchronously every time, and if
> userspace issues ioctls with short intervals (less than 2s with the
> default kernel configuration), it should be fine.
> 
> (Side note: I've looked at the USB autosuspend implementation, and it
> seems to use pm_runtime_put_sync(), with manual autosuspend handling in
> the RPM suspend handler. I got scared and ran away without making sure
> it actually has a low cost as stated above :-S)
> 
> However, I'm more bother by the fact that the status endpoint polling is
> started and stopped synchronously for each ioctl call, for two reasons.
> One of them is the overhead of doing so for each ioctl call. It would be
> made worse by wrapping the top-level ioctl handlers with get/put calls,
> but it's already an issue without that.
> 
> The other reason is that I think it introduces a regression. UVC devices
> can implement asynchronous controls, to report the new value of a
> control at a later time. This is used for controls that take time to be
> set, typically all controls related to physical motors (pan/tilt for
> instance). The uvcvideo driver sends control change events to userspace
> in that case. With this patch, and with commit "media: uvcvideo: Only
> create input devs if hw supports it" merged, if an aplication opens a
> video device but doesn't start streaming, the status URB will be killed
> as soon as a S_CTRL ioctl returns, which will prevent asynchronous event
> reporting from working.
> 
> It seems that one way to fix this would be to start/stop the status
> endpoint polling in the resume/suspend handlers. I haven't investigated
> though. I may give it a try, so please ping me on IRC before you resume
> work on this. Feel free to comment by e-mail on the above though, and
> tell me where I got it wrong :-)

One reason this may all go wrong is that USB autosuspend is implemented
at the USB device level, while UVC operates at the interface level.
Webcams typically also have an audio function, which may keep the device
active. We would then keep polling the status endpoint. I'm not
concerned about any power consumption increase in that case, as the
device would be active anyway, but the status start/stop sequencing will
be less predictable, possibly causing hard to debug issues (especially
if you throw buggy firmwares in the mix).

Contrary to what I've just said before, I think I'll let you experiment
with all this first :-) Let me know if you would like to discuss the
issue in more details before digging.

> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -408,8 +452,8 @@ static int uvc_v4l2_get_streamparm(struct uvc_streaming *stream,
> >  	return 0;
> >  }
> >  
> > -static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> > -		struct v4l2_streamparm *parm)
> > +static int __uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> > +				     struct v4l2_streamparm *parm)
> >  {
> >  	struct uvc_streaming_control probe;
> >  	struct v4l2_fract timeperframe;
> > @@ -419,9 +463,6 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> >  	unsigned int i;
> >  	int ret;
> >  
> > -	if (parm->type != stream->type)
> > -		return -EINVAL;
> > -
> >  	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> >  		timeperframe = parm->parm.capture.timeperframe;
> >  	else
> > @@ -495,6 +536,25 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> >  	return 0;
> >  }
> >  
> > +static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
> > +				   struct v4l2_streamparm *parm)
> > +{
> > +	int ret;
> > +
> > +	if (parm->type != stream->type)
> > +		return -EINVAL;
> > +
> > +	ret = uvc_pm_get(stream);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __uvc_v4l2_set_streamparm(stream, parm);
> > +
> > +	uvc_pm_put(stream);
> > +
> > +	return ret;
> > +}
> > +
> >  /* ------------------------------------------------------------------------
> >   * Privilege management
> >   */
> > @@ -559,36 +619,29 @@ 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__);
> >  
> > -	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;
> > -	}
> >  
> > -	mutex_lock(&stream->dev->lock);
> > -	if (stream->dev->users == 0) {
> > -		ret = uvc_status_start(stream->dev, GFP_KERNEL);
> > -		if (ret < 0) {
> > -			mutex_unlock(&stream->dev->lock);
> > -			usb_autopm_put_interface(stream->dev->intf);
> > +	/*
> > +	 * If the uvc evdev exists we cannot suspend when the device
> 
> s/uvc evdev/UVC input device/
> 
> > +	 * is idle. Otherwise we will miss button actions.
> > +	 */
> > +	if (stream->dev->input) {
> > +		int ret;
> > +
> > +		ret = uvc_pm_get(stream);
> > +		if (ret) {
> >  			kfree(handle);
> >  			return ret;
> >  		}
> >  	}
> >  
> > -	stream->dev->users++;
> > -	mutex_unlock(&stream->dev->lock);
> > -
> >  	v4l2_fh_init(&handle->vfh, &stream->vdev);
> >  	v4l2_fh_add(&handle->vfh);
> >  	handle->chain = stream->chain;
> > @@ -610,6 +663,16 @@ static int uvc_v4l2_release(struct file *file)
> >  	if (uvc_has_privileges(handle))
> >  		uvc_queue_release(&stream->queue);
> >  
> > +	/*
> > +	 * Release cannot happen at the same time as streamon/streamoff
> > +	 * no need to take the stream->mutex.
> 
> There seems to be something missing here, maybe
> 
> 	 * Release cannot happen at the same time as streamon/streamoff, so
> 	 * there is no need to take the stream->mutex.
> 
> > +	 */
> > +	if (handle->is_streaming)
> > +		uvc_pm_put(stream);
> > +
> > +	if (stream->dev->input)
> > +		uvc_pm_put(stream);
> > +
> >  	/* Release the file handle. */
> >  	uvc_dismiss_privileges(handle);
> >  	v4l2_fh_del(&handle->vfh);
> > @@ -617,12 +680,6 @@ static int uvc_v4l2_release(struct file *file)
> >  	kfree(handle);
> >  	file->private_data = NULL;
> >  
> > -	mutex_lock(&stream->dev->lock);
> > -	if (--stream->dev->users == 0)
> > -		uvc_status_stop(stream->dev);
> > -	mutex_unlock(&stream->dev->lock);
> > -
> > -	usb_autopm_put_interface(stream->dev->intf);
> >  	return 0;
> >  }
> >  
> > @@ -849,9 +906,17 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
> >  
> >  	if (handle->is_streaming)
> >  		goto unlock;
> > +
> > +	ret = uvc_pm_get(stream);
> > +	if (ret)
> > +		goto unlock;
> > +
> >  	ret = uvc_queue_streamon(&stream->queue, type);
> >  	handle->is_streaming = !ret;
> >  
> > +	if (!handle->is_streaming)
> > +		uvc_pm_put(stream);
> > +
> >  unlock:
> >  	mutex_unlock(&stream->mutex);
> >  
> > @@ -875,6 +940,9 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
> >  	ret = uvc_queue_streamoff(&stream->queue, type);
> >  	handle->is_streaming = !!ret;
> >  
> > +	if (!handle->is_streaming)
> > +		uvc_pm_put(stream);
> > +
> >  unlock:
> >  	mutex_unlock(&stream->mutex);
> >  
> > @@ -928,6 +996,7 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
> >  {
> >  	struct uvc_fh *handle = fh;
> >  	struct uvc_video_chain *chain = handle->chain;
> > +	struct uvc_streaming *stream = handle->stream;
> >  	u8 *buf;
> >  	int ret;
> >  
> > @@ -941,9 +1010,16 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
> >  	if (!buf)
> >  		return -ENOMEM;
> >  
> > +	ret = uvc_pm_get(stream);
> > +	if (ret) {
> > +		kfree(buf);
> > +		return ret;
> > +	}
> > +
> >  	ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id,
> >  			     chain->dev->intfnum,  UVC_SU_INPUT_SELECT_CONTROL,
> >  			     buf, 1);
> > +	uvc_pm_put(stream);
> >  	if (!ret)
> >  		*input = *buf - 1;
> >  
> > @@ -956,6 +1032,7 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
> >  {
> >  	struct uvc_fh *handle = fh;
> >  	struct uvc_video_chain *chain = handle->chain;
> > +	struct uvc_streaming *stream = handle->stream;
> >  	u8 *buf;
> >  	int ret;
> >  
> > @@ -977,10 +1054,17 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
> >  	if (!buf)
> >  		return -ENOMEM;
> >  
> > +	ret = uvc_pm_get(stream);
> > +	if (ret) {
> > +		kfree(buf);
> > +		return ret;
> > +	}
> > +
> >  	*buf = input + 1;
> >  	ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id,
> >  			     chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
> >  			     buf, 1);
> > +	uvc_pm_put(stream);
> >  	kfree(buf);
> >  
> >  	return ret;
> > @@ -991,8 +1075,15 @@ static int uvc_ioctl_queryctrl(struct file *file, void *fh,
> >  {
> >  	struct uvc_fh *handle = fh;
> >  	struct uvc_video_chain *chain = handle->chain;
> > +	struct uvc_streaming *stream = handle->stream;
> > +	int ret;
> >  
> > -	return uvc_query_v4l2_ctrl(chain, qc);
> > +	ret = uvc_pm_get(stream);
> > +	if (ret)
> > +		return ret;
> > +	ret = uvc_query_v4l2_ctrl(chain, qc);
> > +	uvc_pm_put(stream);
> > +	return ret;
> >  }
> >  
> >  static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
> > @@ -1000,10 +1091,15 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
> >  {
> >  	struct uvc_fh *handle = fh;
> >  	struct uvc_video_chain *chain = handle->chain;
> > +	struct uvc_streaming *stream = handle->stream;
> >  	struct v4l2_queryctrl qc = { qec->id };
> >  	int ret;
> >  
> > +	ret = uvc_pm_get(stream);
> > +	if (ret)
> > +		return ret;
> >  	ret = uvc_query_v4l2_ctrl(chain, &qc);
> > +	uvc_pm_put(stream);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1049,6 +1145,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
> >  {
> >  	struct uvc_fh *handle = fh;
> >  	struct uvc_video_chain *chain = handle->chain;
> > +	struct uvc_streaming *stream = handle->stream;
> >  	struct v4l2_ext_control *ctrl = ctrls->controls;
> >  	unsigned int i;
> >  	int ret;
> > @@ -1073,22 +1170,30 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
> >  		return 0;
> >  	}
> >  
> > +	ret = uvc_pm_get(stream);
> > +	if (ret)
> > +		return ret;
> >  	ret = uvc_ctrl_begin(chain);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		uvc_pm_put(stream);
> >  		return ret;
> > +	}
> >  
> >  	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> >  		ret = uvc_ctrl_get(chain, ctrl);
> >  		if (ret < 0) {
> >  			uvc_ctrl_rollback(handle);
> >  			ctrls->error_idx = i;
> > -			return ret;
> > +			goto done;
> >  		}
> >  	}
> >  
> >  	ctrls->error_idx = 0;
> >  
> > -	return uvc_ctrl_rollback(handle);
> > +	ret = uvc_ctrl_rollback(handle);
> > +done:
> > +	uvc_pm_put(stream);
> > +	return ret;
> >  }
> >  
> >  static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
> > @@ -1097,6 +1202,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
> >  {
> >  	struct v4l2_ext_control *ctrl = ctrls->controls;
> >  	struct uvc_video_chain *chain = handle->chain;
> > +	struct uvc_streaming *stream = handle->stream;
> >  	unsigned int i;
> >  	int ret;
> >  
> > @@ -1104,9 +1210,15 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	ret = uvc_pm_get(stream);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = uvc_ctrl_begin(chain);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		uvc_pm_put(stream);
> >  		return ret;
> > +	}
> >  
> >  	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> >  		ret = uvc_ctrl_set(handle, ctrl);
> > @@ -1114,16 +1226,20 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
> >  			uvc_ctrl_rollback(handle);
> >  			ctrls->error_idx = ioctl == VIDIOC_S_EXT_CTRLS ?
> >  						    ctrls->count : i;
> > -			return ret;
> > +			goto done;
> >  		}
> >  	}
> >  
> >  	ctrls->error_idx = 0;
> >  
> >  	if (ioctl == VIDIOC_S_EXT_CTRLS)
> > -		return uvc_ctrl_commit(handle, ctrls);
> > +		ret = uvc_ctrl_commit(handle, ctrls);
> >  	else
> > -		return uvc_ctrl_rollback(handle);
> > +		ret = uvc_ctrl_rollback(handle);
> > +
> > +done:
> > +	uvc_pm_put(stream);
> > +	return ret;
> >  }
> >  
> >  static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh,
> > @@ -1147,8 +1263,16 @@ static int uvc_ioctl_querymenu(struct file *file, void *fh,
> >  {
> >  	struct uvc_fh *handle = fh;
> >  	struct uvc_video_chain *chain = handle->chain;
> > +	struct uvc_streaming *stream = handle->stream;
> > +	int ret;
> >  
> > -	return uvc_query_v4l2_menu(chain, qm);
> > +	ret = uvc_pm_get(stream);
> > +	if (ret)
> > +		return ret;
> > +	ret = uvc_query_v4l2_menu(chain, qm);
> > +	uvc_pm_put(stream);
> > +
> > +	return ret;
> >  }
> >  
> >  static int uvc_ioctl_g_selection(struct file *file, void *fh,

-- 
Regards,

Laurent Pinchart