| Message ID | 20261027213240.140663-1-rylos78@gmail.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hello On Tue, Oct 27, 2026 at 10:32:38PM +0100, Marco Ziliani wrote: > Lens drivers power the lens up when their subdev is opened, and on ACPI > platforms ipu-bridge links the lens to its sensor with a runtime PM > device link. CameraLens opens the lens subdev in init() and never I wonder if the device link in the ipu_bridge is the right thing to do. Keeping the sensor powered as long as the vcm is powered (or open() in this case) doesn't seem desirable ? > closes it, so with a long-lived CameraManager such as the WirePlumber > libcamera monitor the lens and the sensor stay powered all the time. Of course, even if we remove the device_link (or make it STATELESS to only guarantee suspend/resume ordering), you'll have the VCM powered on all the time, but not the sensor at least. > > On a Surface Pro 5 (IPU3) the rear ov8865 and its dw9719 lens are > active for as long as WirePlumber runs, while the front ov5693, which > has no lens, is runtime suspended when idle. > > Patch 1 closes the lens subdev at the end of CameraLens::init(), adds > open() and close(), and makes setFocusPosition() reopen the subdev on > demand, so pipeline handlers that are not converted keep working. > Patch 2 converts the IPU3 pipeline handler to open the lens on start() > and close it on stop. > > I'm sending this as an RFC because the topic has been discussed > before. Harvey Yang proposed open() and close() in CameraLens in > October 2024 [1], and Laurent preferred to address lens power > management in the kernel, where Hans de Goede had proposed powering > the VCM along with its sensor [2]. As far as I can tell neither > approach has landed. This series only uses the existing kernel > behaviour and fixes the idle power draw on IPU3 systems today. If the > preferred direction is still the kernel one, I'm happy to help test > that instead on the Surface Pro 5. > > Tested on a Surface Pro 5 with kernel 7.2 and libcamera 0.7.2 with the > same changes: > - idle with WirePlumber running: ov8865 and dw9719 runtime suspended > (before: both active); > - cam -c 1 --capture: both active, autofocus moves the lens > (focus_absolute 60 -> 268), 30 fps; > - about 1 s after the capture stops: both runtime suspended again; > - front camera (ov5693, no lens) unchanged. > On master the series builds with the ipu3 and rpi/vc4 pipelines; the > Raspberry Pi pipeline handler is not converted and was not tested on > hardware. > > [1] https://patchwork.libcamera.org/patch/21649/ > [2] https://lore.kernel.org/linux-media/20240901211834.145186-1-hdegoede@redhat.com/ Ok, both approaches clearly didn't progress. On the kernel side, while I agree that .enable/disable_streams doesn't really apply to vcm as a concept, extending the v4l2 core to call .enable_streams on vcm would allow you to power it up only when needed. The userspace approach you and Harvey proposed was refused on the ground that we need better kernel APIs, but as 2 years have passed and we still are consuming power for no reasons by keeping the sensor powered on, so something has to be done I guess. Let's maybe start discussing if the device_link with DL_FLAG_PM_RUNTIME in ipu_bridge is desirable or not. > > Marco Ziliani (2): > libcamera: camera_lens: Close the lens subdev when not in use > libcamera: pipeline: ipu3: Power the lens only while the camera runs > > include/libcamera/internal/camera_lens.h | 2 ++ > src/libcamera/camera_lens.cpp | 42 ++++++++++++++++++++++++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++- > 3 files changed, 60 insertions(+), 1 deletion(-) > > -- > 2.56.0 >
On Wed, Oct 28, 2026 at 10:11:51AM +0100, Jacopo Mondi wrote: > Hello > > On Tue, Oct 27, 2026 at 10:32:38PM +0100, Marco Ziliani wrote: > > Lens drivers power the lens up when their subdev is opened, and on ACPI > > platforms ipu-bridge links the lens to its sensor with a runtime PM > > device link. CameraLens opens the lens subdev in init() and never > > I wonder if the device link in the ipu_bridge is the right thing to > do. Keeping the sensor powered as long as the vcm is powered (or > open() in this case) doesn't seem desirable ? Ah well 8e3e916e23f5 ("media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor") > > > closes it, so with a long-lived CameraManager such as the WirePlumber > > libcamera monitor the lens and the sensor stay powered all the time. > > Of course, even if we remove the device_link (or make it STATELESS to > only guarantee suspend/resume ordering), you'll have the VCM powered > on all the time, but not the sensor at least. > > > > > On a Surface Pro 5 (IPU3) the rear ov8865 and its dw9719 lens are > > active for as long as WirePlumber runs, while the front ov5693, which > > has no lens, is runtime suspended when idle. > > > > Patch 1 closes the lens subdev at the end of CameraLens::init(), adds > > open() and close(), and makes setFocusPosition() reopen the subdev on > > demand, so pipeline handlers that are not converted keep working. > > Patch 2 converts the IPU3 pipeline handler to open the lens on start() > > and close it on stop. > > > > I'm sending this as an RFC because the topic has been discussed > > before. Harvey Yang proposed open() and close() in CameraLens in > > October 2024 [1], and Laurent preferred to address lens power > > management in the kernel, where Hans de Goede had proposed powering > > the VCM along with its sensor [2]. As far as I can tell neither > > approach has landed. This series only uses the existing kernel > > behaviour and fixes the idle power draw on IPU3 systems today. If the > > preferred direction is still the kernel one, I'm happy to help test > > that instead on the Surface Pro 5. > > > > Tested on a Surface Pro 5 with kernel 7.2 and libcamera 0.7.2 with the > > same changes: > > - idle with WirePlumber running: ov8865 and dw9719 runtime suspended > > (before: both active); > > - cam -c 1 --capture: both active, autofocus moves the lens > > (focus_absolute 60 -> 268), 30 fps; > > - about 1 s after the capture stops: both runtime suspended again; > > - front camera (ov5693, no lens) unchanged. > > On master the series builds with the ipu3 and rpi/vc4 pipelines; the > > Raspberry Pi pipeline handler is not converted and was not tested on > > hardware. > > > > [1] https://patchwork.libcamera.org/patch/21649/ > > [2] https://lore.kernel.org/linux-media/20240901211834.145186-1-hdegoede@redhat.com/ > > Ok, both approaches clearly didn't progress. > > On the kernel side, while I agree that .enable/disable_streams doesn't > really apply to vcm as a concept, extending the v4l2 core to call > .enable_streams on vcm would allow you to power it up only when > needed. > > The userspace approach you and Harvey proposed was refused on the > ground that we need better kernel APIs, but as 2 years have passed and > we still are consuming power for no reasons by keeping the sensor > powered on, so something has to be done I guess. > > Let's maybe start discussing if the device_link with DL_FLAG_PM_RUNTIME > in ipu_bridge is desirable or not. > Apparently it is. As I don't see a new API materializing any time soon for VCM in Linux, I guess we should re-consider this open/close approach. Keeping a sensor powered for no reasons all the time is not acceptable imho. > > > > Marco Ziliani (2): > > libcamera: camera_lens: Close the lens subdev when not in use > > libcamera: pipeline: ipu3: Power the lens only while the camera runs > > > > include/libcamera/internal/camera_lens.h | 2 ++ > > src/libcamera/camera_lens.cpp | 42 ++++++++++++++++++++++++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 17 +++++++++- > > 3 files changed, 60 insertions(+), 1 deletion(-) > > > > -- > > 2.56.0 > >
Hi Jacopo, Thanks for looking at this. On Wed, Oct 28, 2026 at 10:11:51AM +0100, Jacopo Mondi wrote: > I wonder if the device link in the ipu_bridge is the right thing to > do. Keeping the sensor powered as long as the vcm is powered (or > open() in this case) doesn't seem desirable ? As far as I understand, the link is there because on these platforms the VCM has no power of its own. Commit 8e3e916e23f5 ("media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor") explains that the sensor, VCM and lens form a single module sharing regulators and possibly a powerdown pin, modelled in ACPI as one device, and that on atomisp the regulators and clocks are ACPI power resources tied to the sensor's power state. It also notes that the dw9719 driver previously had to enable the sensor's Vsio regulator itself, to get I2C pass-through on the PMIC of the sensor module. So with a stateless link the VCM would not be reachable, or not powered, while the sensor is suspended. On this hardware an open lens subdev costs the sensor rails, not only the VCM, which is what makes keeping it open for the lifetime of the CameraManager expensive. > On the kernel side, while I agree that .enable/disable_streams doesn't > really apply to vcm as a concept, extending the v4l2 core to call > .enable_streams on vcm would allow you to power it up only when > needed. That would fix it for every user without userspace changes, but it also needs the VCM drivers to stop powering up on open(), which is where the discussion on Hans' series [1] stopped. If there is agreement on that direction, I'm happy to test it on the Surface Pro 5 (ov8865 + dw9719 on IPU3). In the meantime, would the userspace side be acceptable as a first step? It works with today's kernels, and it stays correct if the kernel later stops powering the lens on open(): libcamera would then just hold the subdev open only while the camera runs. Regards, Marco [1] https://lore.kernel.org/linux-media/20240901211834.145186-1-hdegoede@redhat.com/
On Wed, Oct 28, 2026 at 10:44:23AM +0100, Marco Ziliani wrote: > Hi Jacopo, > > Thanks for looking at this. > > On Wed, Oct 28, 2026 at 10:11:51AM +0100, Jacopo Mondi wrote: > > I wonder if the device link in the ipu_bridge is the right thing to > > do. Keeping the sensor powered as long as the vcm is powered (or > > open() in this case) doesn't seem desirable ? > > As far as I understand, the link is there because on these platforms > the VCM has no power of its own. Commit 8e3e916e23f5 ("media: > ipu-bridge: Add a runtime-pm device-link between VCM and sensor") > explains that the sensor, VCM and lens form a single module sharing > regulators and possibly a powerdown pin, modelled in ACPI as one device, > and that on atomisp the regulators and clocks are ACPI power resources > tied to the sensor's power state. It also notes that the dw9719 driver > previously had to enable the sensor's Vsio regulator itself, to get I2C > pass-through on the PMIC of the sensor module. > > So with a stateless link the VCM would not be reachable, or not > powered, while the sensor is suspended. On this hardware an open lens > subdev costs the sensor rails, not only the VCM, which is what makes > keeping it open for the lifetime of the CameraManager expensive. > > > On the kernel side, while I agree that .enable/disable_streams doesn't > > really apply to vcm as a concept, extending the v4l2 core to call > > .enable_streams on vcm would allow you to power it up only when > > needed. > > That would fix it for every user without userspace changes, but it > also needs the VCM drivers to stop powering up on open(), which is > where the discussion on Hans' series [1] stopped. If there is agreement on that > direction, I'm happy to test it on the Surface Pro 5 (ov8865 + dw9719 > on IPU3). > > In the meantime, would the userspace side be acceptable as a first > step? It works with today's kernels, and it stays correct if the kernel > later stops powering the lens on open(): libcamera would then just hold > the subdev open only while the camera runs. I want this discussion to involve the kernel side. > [1] https://lore.kernel.org/linux-media/20240901211834.145186-1-hdegoede@redhat.com/
Hi, On 28-Oct-26 10:11, Jacopo Mondi wrote: > Hello > > On Tue, Oct 27, 2026 at 10:32:38PM +0100, Marco Ziliani wrote: >> Lens drivers power the lens up when their subdev is opened, and on ACPI >> platforms ipu-bridge links the lens to its sensor with a runtime PM >> device link. CameraLens opens the lens subdev in init() and never > > I wonder if the device link in the ipu_bridge is the right thing to > do. Keeping the sensor powered as long as the vcm is powered (or > open() in this case) doesn't seem desirable ? This is required because of shared power-rails which are not always modelled correctly and which may also power a I2C "gate" and sometimes the sensor will pull-down (connect to ground) the shared I2C lines when not powered. So experience has shown that the VCM will often only work when the sensor is powered up. > Let's maybe start discussing if the device_link with DL_FLAG_PM_RUNTIME > in ipu_bridge is desirable or not. See above. Regards, Hans
<resend from correct email address for the libcamera-list> Hi, On 28-Oct-26 10:11, Jacopo Mondi wrote: > Hello > > On Tue, Oct 27, 2026 at 10:32:38PM +0100, Marco Ziliani wrote: >> Lens drivers power the lens up when their subdev is opened, and on ACPI >> platforms ipu-bridge links the lens to its sensor with a runtime PM >> device link. CameraLens opens the lens subdev in init() and never > > I wonder if the device link in the ipu_bridge is the right thing to > do. Keeping the sensor powered as long as the vcm is powered (or > open() in this case) doesn't seem desirable ? This is required because of shared power-rails which are not always modelled correctly and which may also power a I2C "gate" and sometimes the sensor will pull-down (connect to ground) the shared I2C lines when not powered. So experience has shown that the VCM will often only work when the sensor is powered up. > Let's maybe start discussing if the device_link with DL_FLAG_PM_RUNTIME > in ipu_bridge is desirable or not. See above. Regards, Hans
Hi Laurent,
On Wed, Oct 28, 2026 at 12:23:23PM +0200, Laurent Pinchart wrote:
> I want this discussion to involve the kernel side.
Agreed, adding linux-media and Sakari.
For the kernel side, the problem in short: lens (VCM) drivers such as
dw9714, dw9719 and ak7375 power the lens up on subdev open() and down
on close(). On ACPI platforms ipu-bridge also adds a DL_FLAG_PM_RUNTIME
device link from the VCM to its sensor, which Hans confirmed earlier in
this thread is needed because of shared power rails and I2C gating.
libcamera opens the lens subdev when it enumerates the cameras and
keeps it open, and the WirePlumber camera monitor keeps the
CameraManager alive for the whole session. On IPU3 and IPU6 Surface
devices the rear sensor therefore stays powered for as long as a user
is logged in.
The options I can see:
1. userspace opens the lens subdev only while the camera runs (this
RFC, and Harvey's patch [1]), which works with current kernels;
2. the kernel powers the VCM together with its sensor's stream (Hans'
series [2], or calling .enable_streams() on the VCM as Jacopo
suggested), with VCM drivers no longer powering up on open().
They are not exclusive: 1 fixes current kernels and stays correct once
2 is in place.
I can test either on a Surface Pro 5 (IPU3, ov8865 + dw9719).
[1] https://patchwork.libcamera.org/patch/21649/
[2] https://lore.kernel.org/linux-media/20240901211834.145186-1-hdegoede@redhat.com/
Regards,
Marco