Hi Hans,
Thank you for the patch.
On Sun, Sep 01, 2024 at 11:18:33PM +0200, Hans de Goede wrote:
> Currently VCM drivers power-up the VCM as soon as the VCM's /dev node
> is opened and through the runtime-pm device-link to the sensor this
> also powers up the sensor.
>
> Powering up the VCM and sensor when the /dev node is opened is undesirable,
> without a VCM sensors delay powering up until s_stream(1) is called. This
> allows querying / negotiating capabilities without powering things up.
>
> Sometimes a long running daemon like pipewire may keep the /dev node open
> all the time. The kernel should still be able to powerdown the VCM + sensor
> in this scenario.
>
> VCM drivers should be able to do the same as sensor drivers and only
> power-up the VCM when s_stream(1) is called on the VCM subdev, but this
> requires that s_stream() is actually called on the VCM when the sensor
> starts / stops streaming.
.s_stream() doesn't conceptually make sense for VCMs. Furthermore,
.s_stream() is being replaced with .enable_streams() and
.disable_streams(), which will make even less sense. We need a different
API.
> The s_stream() call on sensor subdevs is done by CSI-receiver/ISP drivers.
> To avoid needing to change all these call sites to also call s_stream()
> on the VCM (if there is one) handle the VCM in the v4l2-core similar to how
> the core takes care of turning on/off the privacy LED.
This needs to come with a design rationale, documented in
Documentation/. The design needs to explain the use cases. Lens motion
may take time, which I expect will influence how we will need to handle
power management.
I'm not very comfortable handling this in v4l2-subdev.c, it seems that
we'll hardcode use cases. Without a clear and detailed designed
rationale, this patch feels we'll paint ourselves in a corner. We have
enough badly designed (or not designed at all) APIs for cameras, it's
time to do better.
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 20 +++++++++++++++++++
> drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++++------
> include/media/v4l2-subdev.h | 2 ++
> 3 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index ee884a8221fb..9b854f1d1051 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -330,6 +330,11 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> return 0;
> }
>
> + if (sd->entity.function == MEDIA_ENT_F_LENS) {
> + dev_dbg(n->sd->dev, "Using %s VCM\n", dev_name(sd->dev));
> + n->sd->vcm = sd;
> + }
> +
> link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
>
> return IS_ERR(link) ? PTR_ERR(link) : 0;
> @@ -871,6 +876,21 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
> if (!sd->async_list.next)
> return;
>
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + if (sd->entity.function == MEDIA_ENT_F_LENS && sd->v4l2_dev && sd->v4l2_dev->mdev) {
> + struct media_entity *entity;
> +
> + media_device_for_each_entity(entity, sd->v4l2_dev->mdev) {
> + struct v4l2_subdev *it = media_entity_to_v4l2_subdev(entity);
> +
> + if (it->vcm == sd) {
> + dev_dbg(it->dev, "Clearing VCM\n");
> + it->vcm = NULL;
> + }
> + }
> + }
> +#endif
> +
> v4l2_subdev_put_privacy_led(sd);
>
> mutex_lock(&list_lock);
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 7c5812d55315..24a68d90f686 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -148,17 +148,33 @@ static int subdev_close(struct file *file)
> }
> #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>
> -static void v4l2_subdev_enable_privacy_led(struct v4l2_subdev *sd)
> +static void v4l2_subdev_enable_privacy_led_and_vcm(struct v4l2_subdev *sd)
> {
> #if IS_REACHABLE(CONFIG_LEDS_CLASS)
> if (!IS_ERR_OR_NULL(sd->privacy_led))
> led_set_brightness(sd->privacy_led,
> sd->privacy_led->max_brightness);
> #endif
> +
> + if (sd->vcm && !sd->vcm_enabled &&
> + v4l2_subdev_has_op(sd->vcm, video, s_stream)) {
> + int ret;
> +
> + ret = v4l2_subdev_call(sd->vcm, video, s_stream, 1);
> + if (ret)
> + dev_err(sd->vcm->dev, "Error powering on VCM: %d\n", ret);
> + else
> + sd->vcm_enabled = true;
> + }
> }
>
> -static void v4l2_subdev_disable_privacy_led(struct v4l2_subdev *sd)
> +static void v4l2_subdev_disable_privacy_led_and_vcm(struct v4l2_subdev *sd)
> {
> + if (sd->vcm && sd->vcm_enabled) {
> + v4l2_subdev_call(sd->vcm, video, s_stream, 0);
> + sd->vcm_enabled = false;
> + }
> +
> #if IS_REACHABLE(CONFIG_LEDS_CLASS)
> if (!IS_ERR_OR_NULL(sd->privacy_led))
> led_set_brightness(sd->privacy_led, 0);
> @@ -466,9 +482,9 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
> sd->s_stream_enabled = enable;
>
> if (enable)
> - v4l2_subdev_enable_privacy_led(sd);
> + v4l2_subdev_enable_privacy_led_and_vcm(sd);
> else
> - v4l2_subdev_disable_privacy_led(sd);
> + v4l2_subdev_disable_privacy_led_and_vcm(sd);
> }
>
> return ret;
> @@ -2289,7 +2305,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> * for all cases.
> */
> if (!use_s_stream && !already_streaming)
> - v4l2_subdev_enable_privacy_led(sd);
> + v4l2_subdev_enable_privacy_led_and_vcm(sd);
>
> done:
> if (!use_s_stream)
> @@ -2382,7 +2398,7 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> done:
> if (!use_s_stream) {
> if (!v4l2_subdev_is_streaming(sd))
> - v4l2_subdev_disable_privacy_led(sd);
> + v4l2_subdev_disable_privacy_led_and_vcm(sd);
>
> v4l2_subdev_unlock_state(state);
> }
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index bd235d325ff9..6568a0cc070b 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1094,6 +1094,7 @@ struct v4l2_subdev {
> */
>
> struct led_classdev *privacy_led;
> + struct v4l2_subdev *vcm;
>
> /*
> * TODO: active_state should most likely be changed from a pointer to an
> @@ -1104,6 +1105,7 @@ struct v4l2_subdev {
> struct v4l2_subdev_state *active_state;
> u64 enabled_pads;
> bool s_stream_enabled;
> + bool vcm_enabled;
> };
>
>
--
Regards,
Laurent Pinchart
