* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).