* [PATCH v1 0/3] [RESEND] media: uvcvideo: Implement granular power management
@ 2022-09-20 14:09 Ricardo Ribalda
2022-09-20 14:09 ` [PATCH v1 1/3] media: uvcvideo: Only create input devs if hw supports it Ricardo Ribalda
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Ricardo Ribalda @ 2022-09-20 14:09 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-kernel, Tomasz Figa, Laurent Pinchart, Ricardo Ribalda,
linux-media
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>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (3):
media: uvcvideo: Only create input devs if hw supports it
media: uvcvideo: Refactor streamon/streamoff
media: uvcvideo: Do power management granularly
drivers/media/usb/uvc/uvc_status.c | 23 +++++
drivers/media/usb/uvc/uvc_v4l2.c | 193 ++++++++++++++++++++++++++++++-------
drivers/media/usb/uvc/uvcvideo.h | 1 +
3 files changed, 182 insertions(+), 35 deletions(-)
---
base-commit: 521a547ced6477c54b4b0cc206000406c221b4d6
change-id: 20220920-resend-powersave-5981719ed267
Best regards,
--
Ricardo Ribalda <ribalda@chromium.org>
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v1 1/3] media: uvcvideo: Only create input devs if hw supports it 2022-09-20 14:09 [PATCH v1 0/3] [RESEND] media: uvcvideo: Implement granular power management Ricardo Ribalda @ 2022-09-20 14:09 ` Ricardo Ribalda 2022-09-20 14:09 ` [PATCH v1 2/3] media: uvcvideo: Refactor streamon/streamoff Ricardo Ribalda 2022-09-20 14:09 ` [PATCH v1 3/3] media: uvcvideo: Do power management granularly Ricardo Ribalda 2 siblings, 0 replies; 7+ messages in thread From: Ricardo Ribalda @ 2022-09-20 14:09 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linux-kernel, Tomasz Figa, Laurent Pinchart, Ricardo Ribalda, linux-media Examine the stream headers to figure out if the device has a button and can be used as an input. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c index 7518ffce22ed..cb90aff344bc 100644 --- a/drivers/media/usb/uvc/uvc_status.c +++ b/drivers/media/usb/uvc/uvc_status.c @@ -18,11 +18,34 @@ * Input device */ #ifdef CONFIG_USB_VIDEO_CLASS_INPUT_EVDEV + +static bool uvc_input_has_button(struct uvc_device *dev) +{ + struct uvc_streaming *stream; + + /* + * The device has button events if both bTriggerSupport and + * bTriggerUsage are one. Otherwise the camera button does not + * exist or is handled automatically by the camera without host + * driver or client application intervention. + */ + list_for_each_entry(stream, &dev->streams, list) { + if (stream->header.bTriggerSupport == 1 && + stream->header.bTriggerUsage == 1) + return true; + } + + return false; +} + static int uvc_input_init(struct uvc_device *dev) { struct input_dev *input; int ret; + if (!uvc_input_has_button(dev)) + return 0; + input = input_allocate_device(); if (input == NULL) return -ENOMEM; -- b4 0.11.0-dev-d93f8 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 2/3] media: uvcvideo: Refactor streamon/streamoff 2022-09-20 14:09 [PATCH v1 0/3] [RESEND] media: uvcvideo: Implement granular power management Ricardo Ribalda 2022-09-20 14:09 ` [PATCH v1 1/3] media: uvcvideo: Only create input devs if hw supports it Ricardo Ribalda @ 2022-09-20 14:09 ` Ricardo Ribalda 2022-09-20 14:09 ` [PATCH v1 3/3] media: uvcvideo: Do power management granularly Ricardo Ribalda 2 siblings, 0 replies; 7+ messages in thread From: Ricardo Ribalda @ 2022-09-20 14:09 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linux-kernel, Tomasz Figa, Laurent Pinchart, Ricardo Ribalda, linux-media 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> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 4cc3fa6b8c98..8d5002543e2c 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 24c911aeebce..caf74c87aaf1 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -725,6 +725,7 @@ enum uvc_handle_state { struct uvc_fh { struct v4l2_fh vfh; + bool is_streaming; struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state; -- b4 0.11.0-dev-d93f8 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 3/3] media: uvcvideo: Do power management granularly 2022-09-20 14:09 [PATCH v1 0/3] [RESEND] media: uvcvideo: Implement granular power management Ricardo Ribalda 2022-09-20 14:09 ` [PATCH v1 1/3] media: uvcvideo: Only create input devs if hw supports it Ricardo Ribalda 2022-09-20 14:09 ` [PATCH v1 2/3] media: uvcvideo: Refactor streamon/streamoff Ricardo Ribalda @ 2022-09-20 14:09 ` Ricardo Ribalda 2022-10-25 12:45 ` Laurent Pinchart 2 siblings, 1 reply; 7+ messages in thread From: Ricardo Ribalda @ 2022-09-20 14:09 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linux-kernel, Tomasz Figa, Laurent Pinchart, Ricardo Ribalda, linux-media 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> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 8d5002543e2c..b9642afabd9b 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 = 0; + + 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; @@ -468,7 +512,13 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream, } /* Probe the device with the new settings. */ + ret = uvc_pm_get(stream); + if (ret) { + mutex_unlock(&stream->mutex); + return ret; + } ret = uvc_probe_video(stream, &probe); + uvc_pm_put(stream); if (ret < 0) { mutex_unlock(&stream->mutex); return ret; @@ -559,36 +609,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 +653,12 @@ 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); + + if (stream->dev->input) + uvc_pm_put(stream); + /* Release the file handle. */ uvc_dismiss_privileges(handle); v4l2_fh_del(&handle->vfh); @@ -617,12 +666,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 +892,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 +926,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 +982,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 +996,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 +1018,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 +1040,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 +1061,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 +1077,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 +1131,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 +1156,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 +1188,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 +1196,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 +1212,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 +1249,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; + + ret = uvc_pm_get(stream); + if (ret) + return ret; + ret = uvc_query_v4l2_menu(chain, qm); + uvc_pm_put(stream); - return uvc_query_v4l2_menu(chain, qm); + return ret; } static int uvc_ioctl_g_selection(struct file *file, void *fh, -- b4 0.11.0-dev-d93f8 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 3/3] media: uvcvideo: Do power management granularly 2022-09-20 14:09 ` [PATCH v1 3/3] media: uvcvideo: Do power management granularly Ricardo Ribalda @ 2022-10-25 12:45 ` Laurent Pinchart 2022-10-25 12:55 ` Ricardo Ribalda 0 siblings, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2022-10-25 12:45 UTC (permalink / raw) To: Ricardo Ribalda Cc: Mauro Carvalho Chehab, linux-kernel, Tomasz Figa, linux-media Hi Ricardo, Thank you for the patch. On Tue, Sep 20, 2022 at 04:09:52PM +0200, 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> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 8d5002543e2c..b9642afabd9b 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 = 0; No need to initialize ret to 0. > + > + 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); For devices that don't have a status endpoint, we will end up calling uvc_status_start() and uvc_status_stop() around most ioctl calls (all call sites of uvc_pm_get() and uvc_pm_put() below). uvc_status_start() and uvc_status_stop() are no-op in that case, but conceptually I don't think that's very nice. Could we instead keep the status start/stop calls to open()/release() ? If you don't mind, I'll give this a try and post a new version of the patch, that should be faster than going through another review round. > + 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; > > @@ -468,7 +512,13 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream, > } > > /* Probe the device with the new settings. */ > + ret = uvc_pm_get(stream); > + if (ret) { > + mutex_unlock(&stream->mutex); > + return ret; > + } > ret = uvc_probe_video(stream, &probe); > + uvc_pm_put(stream); > if (ret < 0) { > mutex_unlock(&stream->mutex); > return ret; > @@ -559,36 +609,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 +653,12 @@ 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); > + > + if (stream->dev->input) > + uvc_pm_put(stream); > + > /* Release the file handle. */ > uvc_dismiss_privileges(handle); > v4l2_fh_del(&handle->vfh); > @@ -617,12 +666,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 +892,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 +926,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 +982,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 +996,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 +1018,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 +1040,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 +1061,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 +1077,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 +1131,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 +1156,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 +1188,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 +1196,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 +1212,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 +1249,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; > + > + ret = uvc_pm_get(stream); > + if (ret) > + return ret; > + ret = uvc_query_v4l2_menu(chain, qm); > + uvc_pm_put(stream); > > - return uvc_query_v4l2_menu(chain, qm); > + return ret; > } > > static int uvc_ioctl_g_selection(struct file *file, void *fh, > -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 3/3] media: uvcvideo: Do power management granularly 2022-10-25 12:45 ` Laurent Pinchart @ 2022-10-25 12:55 ` Ricardo Ribalda 2022-10-25 13:07 ` Ricardo Ribalda 0 siblings, 1 reply; 7+ messages in thread From: Ricardo Ribalda @ 2022-10-25 12:55 UTC (permalink / raw) To: Laurent Pinchart Cc: Mauro Carvalho Chehab, linux-kernel, Tomasz Figa, linux-media Hi Laurent Thanks for the review! On Tue, 25 Oct 2022 at 14:46, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Tue, Sep 20, 2022 at 04:09:52PM +0200, 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> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 8d5002543e2c..b9642afabd9b 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 = 0; > > No need to initialize ret to 0. > > > + > > + 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); > > For devices that don't have a status endpoint, we will end up calling > uvc_status_start() and uvc_status_stop() around most ioctl calls (all > call sites of uvc_pm_get() and uvc_pm_put() below). uvc_status_start() > and uvc_status_stop() are no-op in that case, but conceptually I don't > think that's very nice. Could we instead keep the status start/stop > calls to open()/release() ? I do not think that we can call uvc_status_start without calling usb_autopm_get_interface() We could add a flag on the device, and do something like: if (!stream->dev->users && stream->dev->has_ststatus_enpoint) ret = uvc_status_start(stream->dev, GFP_KERNEL); But I am not sure what is cleaner... > > If you don't mind, I'll give this a try and post a new version of the > patch, that should be faster than going through another review round. > > > + 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; > > > > @@ -468,7 +512,13 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream, > > } > > > > /* Probe the device with the new settings. */ > > + ret = uvc_pm_get(stream); > > + if (ret) { > > + mutex_unlock(&stream->mutex); > > + return ret; > > + } > > ret = uvc_probe_video(stream, &probe); > > + uvc_pm_put(stream); > > if (ret < 0) { > > mutex_unlock(&stream->mutex); > > return ret; > > @@ -559,36 +609,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 +653,12 @@ 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); > > + > > + if (stream->dev->input) > > + uvc_pm_put(stream); > > + > > /* Release the file handle. */ > > uvc_dismiss_privileges(handle); > > v4l2_fh_del(&handle->vfh); > > @@ -617,12 +666,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 +892,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 +926,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 +982,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 +996,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 +1018,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 +1040,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 +1061,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 +1077,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 +1131,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 +1156,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 +1188,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 +1196,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 +1212,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 +1249,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; > > + > > + ret = uvc_pm_get(stream); > > + if (ret) > > + return ret; > > + ret = uvc_query_v4l2_menu(chain, qm); > > + uvc_pm_put(stream); > > > > - return uvc_query_v4l2_menu(chain, qm); > > + return ret; > > } > > > > static int uvc_ioctl_g_selection(struct file *file, void *fh, > > > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 3/3] media: uvcvideo: Do power management granularly 2022-10-25 12:55 ` Ricardo Ribalda @ 2022-10-25 13:07 ` Ricardo Ribalda 0 siblings, 0 replies; 7+ messages in thread From: Ricardo Ribalda @ 2022-10-25 13:07 UTC (permalink / raw) To: Laurent Pinchart Cc: Mauro Carvalho Chehab, linux-kernel, Tomasz Figa, linux-media Hi again On Tue, 25 Oct 2022 at 14:55, Ricardo Ribalda <ribalda@chromium.org> wrote: > > Hi Laurent > > Thanks for the review! > > On Tue, 25 Oct 2022 at 14:46, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Ricardo, > > > > Thank you for the patch. > > > > On Tue, Sep 20, 2022 at 04:09:52PM +0200, 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> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > > index 8d5002543e2c..b9642afabd9b 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 = 0; > > > > No need to initialize ret to 0. > > > > > + > > > + 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); > > > > For devices that don't have a status endpoint, we will end up calling > > uvc_status_start() and uvc_status_stop() around most ioctl calls (all > > call sites of uvc_pm_get() and uvc_pm_put() below). uvc_status_start() > > and uvc_status_stop() are no-op in that case, but conceptually I don't > > think that's very nice. Could we instead keep the status start/stop > > calls to open()/release() ? > > > I do not think that we can call uvc_status_start without calling > usb_autopm_get_interface() > > We could add a flag on the device, and do something like: > > if (!stream->dev->users && stream->dev->has_ststatus_enpoint) > ret = uvc_status_start(stream->dev, GFP_KERNEL); > > But I am not sure what is cleaner... > with the mentioned flag, we could make the locking a bit simpler on resume/start Let me prepare something Thanks! > > > > > If you don't mind, I'll give this a try and post a new version of the > > patch, that should be faster than going through another review round. > > > > > + 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; > > > > > > @@ -468,7 +512,13 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream, > > > } > > > > > > /* Probe the device with the new settings. */ > > > + ret = uvc_pm_get(stream); > > > + if (ret) { > > > + mutex_unlock(&stream->mutex); > > > + return ret; > > > + } > > > ret = uvc_probe_video(stream, &probe); > > > + uvc_pm_put(stream); > > > if (ret < 0) { > > > mutex_unlock(&stream->mutex); > > > return ret; > > > @@ -559,36 +609,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 +653,12 @@ 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); > > > + > > > + if (stream->dev->input) > > > + uvc_pm_put(stream); > > > + > > > /* Release the file handle. */ > > > uvc_dismiss_privileges(handle); > > > v4l2_fh_del(&handle->vfh); > > > @@ -617,12 +666,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 +892,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 +926,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 +982,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 +996,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 +1018,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 +1040,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 +1061,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 +1077,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 +1131,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 +1156,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 +1188,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 +1196,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 +1212,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 +1249,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; > > > + > > > + ret = uvc_pm_get(stream); > > > + if (ret) > > > + return ret; > > > + ret = uvc_query_v4l2_menu(chain, qm); > > > + uvc_pm_put(stream); > > > > > > - return uvc_query_v4l2_menu(chain, qm); > > > + return ret; > > > } > > > > > > static int uvc_ioctl_g_selection(struct file *file, void *fh, > > > > > > > -- > > Regards, > > > > Laurent Pinchart > > > > -- > Ricardo Ribalda -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-10-25 13:07 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-20 14:09 [PATCH v1 0/3] [RESEND] media: uvcvideo: Implement granular power management Ricardo Ribalda 2022-09-20 14:09 ` [PATCH v1 1/3] media: uvcvideo: Only create input devs if hw supports it Ricardo Ribalda 2022-09-20 14:09 ` [PATCH v1 2/3] media: uvcvideo: Refactor streamon/streamoff Ricardo Ribalda 2022-09-20 14:09 ` [PATCH v1 3/3] media: uvcvideo: Do power management granularly Ricardo Ribalda 2022-10-25 12:45 ` Laurent Pinchart 2022-10-25 12:55 ` Ricardo Ribalda 2022-10-25 13:07 ` Ricardo Ribalda
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.