Hi Ricardo,
Thank you for your patch.
On 28-May-25 19:58, Ricardo Ribalda wrote:
> If we subscribe to an event with V4L2_EVENT_SUB_FL_SEND_INITIAL, the
> driver needs to report back some values that require the camera to be
> powered on. But VIDIOC_SUBSCRIBE_EVENT is not part of the ioctls that
> turn on the camera.
>
> We could unconditionally turn on the camera during
> VIDIOC_SUBSCRIBE_EVENT, but it is more efficient to turn it on only
> during V4L2_EVENT_SUB_FL_SEND_INITIAL, which we believe is not a common
> usecase.
>
> Fixes: d1b618e79548 ("media: uvcvideo: Do not turn on the camera for some ioctls")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 44b6513c526421943bb9841fb53dc5f8e9f93f02..a7b8f3ea01edd8157e0d8cc36351d511225f89d7 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2039,6 +2039,12 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
> u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
> s32 val = 0;
>
> + ret = uvc_pm_get(handle->chain->dev);
> + if (ret) {
> + list_del(&sev->node);
uvc_ctrl_add_event() holds chain->ctrl_mutex and the only consumer of
the ev_subs list "uvc_ctrl_send_event() also only gets called with
chain->ctrl_mutex held.
So instead of undoing the list_add_tail() here, it would be better
to just move the list_add_tail() call to below the
"if (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL) { ... }" block
(just above the done label).
This avoids the need for the list_del() here.
With that changed:
Reviewed-by: Hans de Goede <hansg@kernel.org>
Regards,
Hans
> + goto done;
> + }
> +
> if (uvc_ctrl_mapping_is_compound(mapping) ||
> __uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
> changes |= V4L2_EVENT_CTRL_CH_VALUE;
> @@ -2051,6 +2057,8 @@ static int uvc_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
> */
> sev->elems = elems;
> v4l2_event_queue_fh(sev->fh, &ev);
> +
> + uvc_pm_put(handle->chain->dev);
> }
>
> done:
>