Re: [PATCH 5/9] media: uvcvideo: Turn on the camera if V4L2_EVENT_SUB_FL_SEND_INITIAL

[Hans de Goede]

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@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>

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:
>