* [PATCH v3 1/2] media: uvcvideo: Only create input devs if hw supports it @ 2022-01-24 19:00 Ricardo Ribalda 2022-01-24 19:00 ` [PATCH v3 2/2] media: uvcvideo: Do power management granularly Ricardo Ribalda 2022-02-03 3:18 ` [PATCH v3 1/2] media: uvcvideo: Only create input devs if hw supports it Laurent Pinchart 0 siblings, 2 replies; 6+ messages in thread From: Ricardo Ribalda @ 2022-01-24 19:00 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel Cc: Ricardo Ribalda Examine the stream headers to figure out if the device has a GPIO and can be used as an input. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_status.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c index 753c8226db70..3ef0b281ffc5 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 GPIO button event 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; -- 2.35.0.rc0.227.g00780c9af4-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] media: uvcvideo: Do power management granularly 2022-01-24 19:00 [PATCH v3 1/2] media: uvcvideo: Only create input devs if hw supports it Ricardo Ribalda @ 2022-01-24 19:00 ` Ricardo Ribalda 2022-01-27 9:20 ` Tomasz Figa 2022-02-03 3:52 ` Laurent Pinchart 2022-02-03 3:18 ` [PATCH v3 1/2] media: uvcvideo: Only create input devs if hw supports it Laurent Pinchart 1 sibling, 2 replies; 6+ messages in thread From: Ricardo Ribalda @ 2022-01-24 19:00 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel Cc: 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. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_v4l2.c | 199 +++++++++++++++++++++++++------ drivers/media/usb/uvc/uvcvideo.h | 1 + 2 files changed, 166 insertions(+), 34 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 711556d13d03..48217e47646f 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -25,6 +25,55 @@ #include "uvcvideo.h" +/* ------------------------------------------------------------------------ + * UVC power management + */ + +static int uvc_pm_get(struct uvc_streaming *stream) +{ + int ret = 0; + + if (!video_is_registered(&stream->vdev)) + return -ENODEV; + + /* + * We cannot hold dev->lock when calling autopm_get_interface. + */ + 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) +{ + if (!video_is_registered(&stream->vdev)) + return; + + 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 */ @@ -251,8 +300,14 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream, stream->ctrl.dwMaxVideoFrameSize; /* Probe the device. */ + ret = uvc_pm_get(stream); + if (ret) { + mutex_unlock(&stream->mutex); + goto done; + } ret = uvc_probe_video(stream, probe); mutex_unlock(&stream->mutex); + uvc_pm_put(stream); if (ret < 0) goto done; @@ -464,7 +519,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; @@ -555,36 +616,24 @@ 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); - kfree(handle); - return ret; - } + /* + * If the uvc evdev exists we cannot suspend when the device + * is idle. Otherwise we will miss button actions. + */ + if (stream->dev->input && uvc_pm_get(stream)) { + kfree(handle); + return -ENODEV; } - stream->dev->users++; - mutex_unlock(&stream->dev->lock); - v4l2_fh_init(&handle->vfh, &stream->vdev); v4l2_fh_add(&handle->vfh); handle->chain = stream->chain; @@ -606,6 +655,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); @@ -613,12 +668,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; } @@ -842,7 +891,21 @@ static int uvc_ioctl_streamon(struct file *file, void *fh, return -EBUSY; mutex_lock(&stream->mutex); + if (!handle->is_streaming) { + ret = uvc_pm_get(stream); + if (ret) + goto unlock; + } + ret = uvc_queue_streamon(&stream->queue, type); + + if (ret && !handle->is_streaming) + uvc_pm_put(stream); + + if (!ret) + handle->is_streaming = true; + +unlock: mutex_unlock(&stream->mutex); return ret; @@ -859,6 +922,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh, mutex_lock(&stream->mutex); uvc_queue_streamoff(&stream->queue, type); + if (handle->is_streaming) { + handle->is_streaming = false; + uvc_pm_put(stream); + } mutex_unlock(&stream->mutex); return 0; @@ -909,6 +976,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; @@ -922,9 +990,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; @@ -937,6 +1012,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; @@ -958,10 +1034,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; @@ -972,8 +1055,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, @@ -981,10 +1071,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; @@ -1030,6 +1125,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; @@ -1054,22 +1150,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; + uvc_pm_put(stream); return ret; } } ctrls->error_idx = 0; - return uvc_ctrl_rollback(handle); + ret = uvc_ctrl_rollback(handle); + uvc_pm_put(stream); + return ret; } static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle, @@ -1078,6 +1182,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; @@ -1085,9 +1190,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); @@ -1095,6 +1206,7 @@ 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; + uvc_pm_put(stream); return ret; } } @@ -1102,9 +1214,12 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle, 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); + + uvc_pm_put(stream); + return ret; } static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh, @@ -1119,8 +1234,16 @@ static int uvc_ioctl_try_ext_ctrls(struct file *file, void *fh, struct v4l2_ext_controls *ctrls) { struct uvc_fh *handle = fh; + struct uvc_streaming *stream = handle->stream; + int ret; + + ret = uvc_pm_get(stream); + if (ret) + return ret; + ret = uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_TRY_EXT_CTRLS); + uvc_pm_put(stream); - return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_TRY_EXT_CTRLS); + return ret; } static int uvc_ioctl_querymenu(struct file *file, void *fh, @@ -1128,8 +1251,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, diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 143230b3275b..5958b2a54dab 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -720,6 +720,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; -- 2.35.0.rc0.227.g00780c9af4-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] media: uvcvideo: Do power management granularly 2022-01-24 19:00 ` [PATCH v3 2/2] media: uvcvideo: Do power management granularly Ricardo Ribalda @ 2022-01-27 9:20 ` Tomasz Figa 2022-02-03 3:52 ` Laurent Pinchart 1 sibling, 0 replies; 6+ messages in thread From: Tomasz Figa @ 2022-01-27 9:20 UTC (permalink / raw) To: Ricardo Ribalda Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel On Tue, Jan 25, 2022 at 4:07 AM Ricardo Ribalda <ribalda@chromium.org> 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. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_v4l2.c | 199 +++++++++++++++++++++++++------ > drivers/media/usb/uvc/uvcvideo.h | 1 + > 2 files changed, 166 insertions(+), 34 deletions(-) Reviewed-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 711556d13d03..48217e47646f 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -25,6 +25,55 @@ > > #include "uvcvideo.h" > > +/* ------------------------------------------------------------------------ > + * UVC power management > + */ > + > +static int uvc_pm_get(struct uvc_streaming *stream) > +{ > + int ret = 0; > + > + if (!video_is_registered(&stream->vdev)) > + return -ENODEV; > + > + /* > + * We cannot hold dev->lock when calling autopm_get_interface. > + */ > + 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) > +{ > + if (!video_is_registered(&stream->vdev)) > + return; > + > + 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 > */ > @@ -251,8 +300,14 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream, > stream->ctrl.dwMaxVideoFrameSize; > > /* Probe the device. */ > + ret = uvc_pm_get(stream); > + if (ret) { > + mutex_unlock(&stream->mutex); > + goto done; > + } > ret = uvc_probe_video(stream, probe); > mutex_unlock(&stream->mutex); > + uvc_pm_put(stream); > if (ret < 0) > goto done; > > @@ -464,7 +519,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; > @@ -555,36 +616,24 @@ 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); > - kfree(handle); > - return ret; > - } > + /* > + * If the uvc evdev exists we cannot suspend when the device > + * is idle. Otherwise we will miss button actions. > + */ > + if (stream->dev->input && uvc_pm_get(stream)) { > + kfree(handle); > + return -ENODEV; > } > > - stream->dev->users++; > - mutex_unlock(&stream->dev->lock); > - > v4l2_fh_init(&handle->vfh, &stream->vdev); > v4l2_fh_add(&handle->vfh); > handle->chain = stream->chain; > @@ -606,6 +655,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); > @@ -613,12 +668,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; > } > > @@ -842,7 +891,21 @@ static int uvc_ioctl_streamon(struct file *file, void *fh, > return -EBUSY; > > mutex_lock(&stream->mutex); > + if (!handle->is_streaming) { > + ret = uvc_pm_get(stream); > + if (ret) > + goto unlock; > + } > + > ret = uvc_queue_streamon(&stream->queue, type); > + > + if (ret && !handle->is_streaming) > + uvc_pm_put(stream); > + > + if (!ret) > + handle->is_streaming = true; > + > +unlock: > mutex_unlock(&stream->mutex); > > return ret; > @@ -859,6 +922,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh, > > mutex_lock(&stream->mutex); > uvc_queue_streamoff(&stream->queue, type); > + if (handle->is_streaming) { > + handle->is_streaming = false; > + uvc_pm_put(stream); > + } > mutex_unlock(&stream->mutex); > > return 0; > @@ -909,6 +976,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; > > @@ -922,9 +990,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; > > @@ -937,6 +1012,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; > > @@ -958,10 +1034,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; > @@ -972,8 +1055,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, > @@ -981,10 +1071,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; > > @@ -1030,6 +1125,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; > @@ -1054,22 +1150,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; > + uvc_pm_put(stream); > return ret; > } > } > > ctrls->error_idx = 0; > > - return uvc_ctrl_rollback(handle); > + ret = uvc_ctrl_rollback(handle); > + uvc_pm_put(stream); > + return ret; > } > > static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle, > @@ -1078,6 +1182,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; > > @@ -1085,9 +1190,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); > @@ -1095,6 +1206,7 @@ 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; > + uvc_pm_put(stream); > return ret; > } > } > @@ -1102,9 +1214,12 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle, > 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); > + > + uvc_pm_put(stream); > + return ret; > } > > static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh, > @@ -1119,8 +1234,16 @@ static int uvc_ioctl_try_ext_ctrls(struct file *file, void *fh, > struct v4l2_ext_controls *ctrls) > { > struct uvc_fh *handle = fh; > + struct uvc_streaming *stream = handle->stream; > + int ret; > + > + ret = uvc_pm_get(stream); > + if (ret) > + return ret; > + ret = uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_TRY_EXT_CTRLS); > + uvc_pm_put(stream); > > - return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_TRY_EXT_CTRLS); > + return ret; > } > > static int uvc_ioctl_querymenu(struct file *file, void *fh, > @@ -1128,8 +1251,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, > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 143230b3275b..5958b2a54dab 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -720,6 +720,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; > -- > 2.35.0.rc0.227.g00780c9af4-goog > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] media: uvcvideo: Do power management granularly 2022-01-24 19:00 ` [PATCH v3 2/2] media: uvcvideo: Do power management granularly Ricardo Ribalda 2022-01-27 9:20 ` Tomasz Figa @ 2022-02-03 3:52 ` Laurent Pinchart 2022-02-04 17:14 ` Ricardo Ribalda 1 sibling, 1 reply; 6+ messages in thread From: Laurent Pinchart @ 2022-02-03 3:52 UTC (permalink / raw) To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, linux-media, linux-kernel Hi Ricardo, Thank you for the patch. On Mon, Jan 24, 2022 at 08:00:13PM +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. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_v4l2.c | 199 +++++++++++++++++++++++++------ > drivers/media/usb/uvc/uvcvideo.h | 1 + > 2 files changed, 166 insertions(+), 34 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 711556d13d03..48217e47646f 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -25,6 +25,55 @@ > > #include "uvcvideo.h" > > +/* ------------------------------------------------------------------------ > + * UVC power management > + */ > + > +static int uvc_pm_get(struct uvc_streaming *stream) > +{ > + int ret = 0; > + > + if (!video_is_registered(&stream->vdev)) > + return -ENODEV; Can this happen ? > + > + /* > + * We cannot hold dev->lock when calling autopm_get_interface. > + */ Why is that ? > + 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); Does this use autosuspend with a delay ? > + > + return ret; > +} > + > +static void uvc_pm_put(struct uvc_streaming *stream) > +{ > + if (!video_is_registered(&stream->vdev)) > + return; If the device gets disconnected during streaming, we'll unregister the video device. When uvc_pm_put() is called in uvc_v4l2_release(), it will then return immediately. Won't this cause an unbalanced PM issue ? > + > + 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 > */ > @@ -251,8 +300,14 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream, > stream->ctrl.dwMaxVideoFrameSize; > > /* Probe the device. */ > + ret = uvc_pm_get(stream); > + if (ret) { > + mutex_unlock(&stream->mutex); > + goto done; > + } > ret = uvc_probe_video(stream, probe); > mutex_unlock(&stream->mutex); > + uvc_pm_put(stream); uvc_pm_get() is called with the stream->mutex held, while uvc_pm_put() isn't. Is there any specific reason ? > if (ret < 0) > goto done; > > @@ -464,7 +519,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; > @@ -555,36 +616,24 @@ 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); > - kfree(handle); > - return ret; > - } > + /* > + * If the uvc evdev exists we cannot suspend when the device > + * is idle. Otherwise we will miss button actions. > + */ > + if (stream->dev->input && uvc_pm_get(stream)) { > + kfree(handle); > + return -ENODEV; > } > > - stream->dev->users++; > - mutex_unlock(&stream->dev->lock); > - > v4l2_fh_init(&handle->vfh, &stream->vdev); > v4l2_fh_add(&handle->vfh); > handle->chain = stream->chain; > @@ -606,6 +655,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); > @@ -613,12 +668,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; > } > > @@ -842,7 +891,21 @@ static int uvc_ioctl_streamon(struct file *file, void *fh, > return -EBUSY; > > mutex_lock(&stream->mutex); > + if (!handle->is_streaming) { > + ret = uvc_pm_get(stream); > + if (ret) > + goto unlock; > + } Is there any reason to continue if we're already streaming ? The other option would be to call uvc_pm_get() unconditionally here. > + > ret = uvc_queue_streamon(&stream->queue, type); > + > + if (ret && !handle->is_streaming) > + uvc_pm_put(stream); > + > + if (!ret) > + handle->is_streaming = true; > + > +unlock: > mutex_unlock(&stream->mutex); > > return ret; > @@ -859,6 +922,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh, > > mutex_lock(&stream->mutex); > uvc_queue_streamoff(&stream->queue, type); Should we also handle errors from uvc_queue_streamoff() ? Maybe this could be done in a separate patch that would also introduce handle->is_streaming but without the PM. That would be easier to review. > + if (handle->is_streaming) { > + handle->is_streaming = false; > + uvc_pm_put(stream); > + } > mutex_unlock(&stream->mutex); > > return 0; > @@ -909,6 +976,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; > > @@ -922,9 +990,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; > > @@ -937,6 +1012,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; > > @@ -958,10 +1034,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; > @@ -972,8 +1055,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, > @@ -981,10 +1071,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; > > @@ -1030,6 +1125,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; > @@ -1054,22 +1150,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; I'd prefer a "goto done" style. > + } > > 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; > + uvc_pm_put(stream); > return ret; > } > } > > 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, > @@ -1078,6 +1182,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; > > @@ -1085,9 +1190,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; Same in this function. > + } > > for (i = 0; i < ctrls->count; ++ctrl, ++i) { > ret = uvc_ctrl_set(handle, ctrl); > @@ -1095,6 +1206,7 @@ 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; > + uvc_pm_put(stream); > return ret; > } > } > @@ -1102,9 +1214,12 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle, > 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); > + > + uvc_pm_put(stream); > + return ret; > } > > static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh, > @@ -1119,8 +1234,16 @@ static int uvc_ioctl_try_ext_ctrls(struct file *file, void *fh, > struct v4l2_ext_controls *ctrls) > { > struct uvc_fh *handle = fh; > + struct uvc_streaming *stream = handle->stream; > + int ret; > + > + ret = uvc_pm_get(stream); > + if (ret) > + return ret; > + ret = uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_TRY_EXT_CTRLS); > + uvc_pm_put(stream); uvc_ioctl_s_try_ext_ctrls() handles PM, do you need it here too ? The other option is to drop it from uvc_ioctl_s_try_ext_ctrls(), keep it here and add it to uvc_ioctl_try_ext_ctrls(). That would result in a smaller diff, and standardize on handling PM as close as possible to the top of the call stack, so it could be better. > > - return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_TRY_EXT_CTRLS); > + return ret; > } > > static int uvc_ioctl_querymenu(struct file *file, void *fh, > @@ -1128,8 +1251,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, > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 143230b3275b..5958b2a54dab 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -720,6 +720,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; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] media: uvcvideo: Do power management granularly 2022-02-03 3:52 ` Laurent Pinchart @ 2022-02-04 17:14 ` Ricardo Ribalda 0 siblings, 0 replies; 6+ messages in thread From: Ricardo Ribalda @ 2022-02-04 17:14 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Mauro Carvalho Chehab, linux-media, linux-kernel Hi Laurent Thanks for the review! :) I have uploaded v5 with all the changes, (please ignore v4 ;)) Best regards! On Thu, 3 Feb 2022 at 04:53, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Mon, Jan 24, 2022 at 08:00:13PM +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. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_v4l2.c | 199 +++++++++++++++++++++++++------ > > drivers/media/usb/uvc/uvcvideo.h | 1 + > > 2 files changed, 166 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 711556d13d03..48217e47646f 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -25,6 +25,55 @@ > > > > #include "uvcvideo.h" > > > > +/* ------------------------------------------------------------------------ > > + * UVC power management > > + */ > > + > > +static int uvc_pm_get(struct uvc_streaming *stream) > > +{ > > + int ret = 0; > > + > > + if (!video_is_registered(&stream->vdev)) > > + return -ENODEV; > > Can this happen ? It is some leftovers from code backported from our kernel sorry. https://lkml.org/lkml/2020/8/30/183 > > > + > > + /* > > + * We cannot hold dev->lock when calling autopm_get_interface. > > + */ > > Why is that ? Because _uvc_resume takes the same lock. > > > + 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); > > Does this use autosuspend with a delay ? I believe the default autosuspend is 2 sec. https://www.kernel.org/doc/Documentation/usb/power-management.txt > > > + > > + return ret; > > +} > > + > > +static void uvc_pm_put(struct uvc_streaming *stream) > > +{ > > + if (!video_is_registered(&stream->vdev)) > > + return; > > If the device gets disconnected during streaming, we'll unregister the > video device. When uvc_pm_put() is called in uvc_v4l2_release(), it will > then return immediately. Won't this cause an unbalanced PM issue ? I just removed it thanks :) > > > + > > + 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 > > */ > > @@ -251,8 +300,14 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream, > > stream->ctrl.dwMaxVideoFrameSize; > > > > /* Probe the device. */ > > + ret = uvc_pm_get(stream); > > + if (ret) { > > + mutex_unlock(&stream->mutex); > > + goto done; > > + } > > ret = uvc_probe_video(stream, probe); > > mutex_unlock(&stream->mutex); > > + uvc_pm_put(stream); > > uvc_pm_get() is called with the stream->mutex held, while uvc_pm_put() > isn't. Is there any specific reason ? Good catch, moving get outside of the lock > > > if (ret < 0) > > goto done; > > > > @@ -464,7 +519,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; > > @@ -555,36 +616,24 @@ 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); > > - kfree(handle); > > - return ret; > > - } > > + /* > > + * If the uvc evdev exists we cannot suspend when the device > > + * is idle. Otherwise we will miss button actions. > > + */ > > + if (stream->dev->input && uvc_pm_get(stream)) { > > + kfree(handle); > > + return -ENODEV; > > } > > > > - stream->dev->users++; > > - mutex_unlock(&stream->dev->lock); > > - > > v4l2_fh_init(&handle->vfh, &stream->vdev); > > v4l2_fh_add(&handle->vfh); > > handle->chain = stream->chain; > > @@ -606,6 +655,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); > > @@ -613,12 +668,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; > > } > > > > @@ -842,7 +891,21 @@ static int uvc_ioctl_streamon(struct file *file, void *fh, > > return -EBUSY; > > > > mutex_lock(&stream->mutex); > > + if (!handle->is_streaming) { > > + ret = uvc_pm_get(stream); > > + if (ret) > > + goto unlock; > > + } > > Is there any reason to continue if we're already streaming ? The other > option would be to call uvc_pm_get() unconditionally here. I would rather handle it with the next suggestion ;) > > > + > > ret = uvc_queue_streamon(&stream->queue, type); > > + > > + if (ret && !handle->is_streaming) > > + uvc_pm_put(stream); > > + > > + if (!ret) > > + handle->is_streaming = true; > > + > > +unlock: > > mutex_unlock(&stream->mutex); > > > > return ret; > > @@ -859,6 +922,10 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh, > > > > mutex_lock(&stream->mutex); > > uvc_queue_streamoff(&stream->queue, type); > > Should we also handle errors from uvc_queue_streamoff() ? Maybe this > could be done in a separate patch that would also introduce > handle->is_streaming but without the PM. That would be easier to review. Done! > > > + if (handle->is_streaming) { > > + handle->is_streaming = false; > > + uvc_pm_put(stream); > > + } > > mutex_unlock(&stream->mutex); > > > > return 0; > > @@ -909,6 +976,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; > > > > @@ -922,9 +990,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; > > > > @@ -937,6 +1012,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; > > > > @@ -958,10 +1034,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; > > @@ -972,8 +1055,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, > > @@ -981,10 +1071,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; > > > > @@ -1030,6 +1125,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; > > @@ -1054,22 +1150,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; > > I'd prefer a "goto done" style. > > > + } > > > > 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; > > + uvc_pm_put(stream); > > return ret; > > } > > } > > > > 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, > > @@ -1078,6 +1182,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; > > > > @@ -1085,9 +1190,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; > > Same in this function. > > > + } > > > > for (i = 0; i < ctrls->count; ++ctrl, ++i) { > > ret = uvc_ctrl_set(handle, ctrl); > > @@ -1095,6 +1206,7 @@ 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; > > + uvc_pm_put(stream); > > return ret; > > } > > } > > @@ -1102,9 +1214,12 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle, > > 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); > > + > > + uvc_pm_put(stream); > > + return ret; > > } > > > > static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh, > > @@ -1119,8 +1234,16 @@ static int uvc_ioctl_try_ext_ctrls(struct file *file, void *fh, > > struct v4l2_ext_controls *ctrls) > > { > > struct uvc_fh *handle = fh; > > + struct uvc_streaming *stream = handle->stream; > > + int ret; > > + > > + ret = uvc_pm_get(stream); > > + if (ret) > > + return ret; > > + ret = uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_TRY_EXT_CTRLS); > > + uvc_pm_put(stream); > > uvc_ioctl_s_try_ext_ctrls() handles PM, do you need it here too ? The > other option is to drop it from uvc_ioctl_s_try_ext_ctrls(), keep it > here and add it to uvc_ioctl_try_ext_ctrls(). That would result in a > smaller diff, and standardize on handling PM as close as possible to the > top of the call stack, so it could be better. > > > > > - return uvc_ioctl_s_try_ext_ctrls(handle, ctrls, VIDIOC_TRY_EXT_CTRLS); > > + return ret; > > } > > > > static int uvc_ioctl_querymenu(struct file *file, void *fh, > > @@ -1128,8 +1251,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, > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index 143230b3275b..5958b2a54dab 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -720,6 +720,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; > > -- > Regards, > > Laurent Pinchart -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] media: uvcvideo: Only create input devs if hw supports it 2022-01-24 19:00 [PATCH v3 1/2] media: uvcvideo: Only create input devs if hw supports it Ricardo Ribalda 2022-01-24 19:00 ` [PATCH v3 2/2] media: uvcvideo: Do power management granularly Ricardo Ribalda @ 2022-02-03 3:18 ` Laurent Pinchart 1 sibling, 0 replies; 6+ messages in thread From: Laurent Pinchart @ 2022-02-03 3:18 UTC (permalink / raw) To: Ricardo Ribalda; +Cc: Mauro Carvalho Chehab, linux-media, linux-kernel Hi Ricardo, Thank you for the patch. On Mon, Jan 24, 2022 at 08:00:12PM +0100, Ricardo Ribalda wrote: > Examine the stream headers to figure out if the device has a GPIO and > can be used as an input. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_status.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c > index 753c8226db70..3ef0b281ffc5 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 GPIO button event if both bTriggerSupport and The UVC specification doesn't talk about GPIOs but about buttons. With s/GPIO button event/button events/ here and s/GPIO/button/ in the commit message, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * 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; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-04 17:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-01-24 19:00 [PATCH v3 1/2] media: uvcvideo: Only create input devs if hw supports it Ricardo Ribalda 2022-01-24 19:00 ` [PATCH v3 2/2] media: uvcvideo: Do power management granularly Ricardo Ribalda 2022-01-27 9:20 ` Tomasz Figa 2022-02-03 3:52 ` Laurent Pinchart 2022-02-04 17:14 ` Ricardo Ribalda 2022-02-03 3:18 ` [PATCH v3 1/2] media: uvcvideo: Only create input devs if hw supports it Laurent Pinchart
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.