| Message ID | 20240901211834.145186-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: v4l: Call s_stream() on VCM when it is called on the associated sensor | expand |
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@redhat.com> > --- > 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; > }; > >
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; };
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. 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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- 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(-)