[libcamera-devel] [RFC PATCH v2 3/4] libcamera: raspberrypi: Control the lens from pipeline

[Laurent Pinchart]

On Thu, Mar 24, 2022 at 10:28:33AM +0000, Naushir Patuck wrote:
> On Thu, 24 Mar 2022 at 08:22, Jean-Michel Hautbois  wrote:
> > On 24/03/2022 00:54, Kieran Bingham wrote:
> > > Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-23 16:01:44)
> > >> The lens focus is controled by a VCM, which is linked to the sensor
> > >> using the ancillary links.
> > >> Pass the control to the config info structure and make it possible to
> > >> update by the IPA.
> > >>
> > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > >> ---
> > >>   include/libcamera/ipa/raspberrypi.mojom         |  1 +
> > >>   .../pipeline/raspberrypi/raspberrypi.cpp        | 17 +++++++++++++++++
> > >>   2 files changed, 18 insertions(+)
> > >>
> > >> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > >> index acd3cafe..0c3922b0 100644
> > >> --- a/include/libcamera/ipa/raspberrypi.mojom
> > >> +++ b/include/libcamera/ipa/raspberrypi.mojom
> > >> @@ -125,4 +125,5 @@ interface IPARPiEventInterface {
> > >>          embeddedComplete(uint32 bufferId);
> > >>          setIspControls(libcamera.ControlList controls);
> > >>          setDelayedControls(libcamera.ControlList controls);
> > >> +       setLensControls(libcamera.ControlList controls);
> > >>   };
> > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> index c2230199..970f9fe7 100644
> > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> @@ -33,6 +33,7 @@
> > >>
> > >>   #include "libcamera/internal/bayer_format.h"
> > >>   #include "libcamera/internal/camera.h"
> > >> +#include "libcamera/internal/camera_lens.h"
> > >>   #include "libcamera/internal/camera_sensor.h"
> > >>   #include "libcamera/internal/delayed_controls.h"
> > >>   #include "libcamera/internal/device_enumerator.h"
> > >> @@ -202,6 +203,7 @@ public:
> > >>          void setIspControls(const ControlList &controls);
> > >>          void setDelayedControls(const ControlList &controls);
> > >>          void setSensorControls(ControlList &controls);
> > >> +       void setLensControls(const ControlList &controls);
> > >>
> > >>          /* bufferComplete signal handlers. */
> > >>          void unicamBufferDequeue(FrameBuffer *buffer);
> > >> @@ -1483,6 +1485,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> > >>          ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> > >>          ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
> > >>          ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
> > >> +       ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);
> > >>
> > >>          /*
> > >>           * The configuration (tuning file) is made from the sensor name unless
> > >> @@ -1519,6 +1522,10 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > >>          entityControls.emplace(0, sensor_->controls());
> > >>          entityControls.emplace(1, isp_[Isp::Input].dev()->controls());
> > >>
> > >> +       CameraLens *lens = sensor_->focusLens();
> > >> +       if (lens)
> > >> +               entityControls.emplace(2, lens->controls());
> > >> +
> > >>          /* Always send the user transform to the IPA. */
> > >>          ipaConfig.transform = static_cast<unsigned int>(config->transform);
> > >>
> > >> @@ -1735,6 +1742,16 @@ void RPiCameraData::setDelayedControls(const ControlList &controls)
> > >>          handleState();
> > >>   }
> > >>
> > >> +void RPiCameraData::setLensControls(const ControlList &ctrls)
> > >> +{
> > >> +       CameraLens *lens = sensor_->focusLens();
> > >> +       if (!lens)
> > >> +               return;
> > >> +
> > >> +       /* \todo Should we keep track of the latest value applied ? */
> > >> +       lens->setFocusPosition(ctrls.get(V4L2_CID_FOCUS_ABSOLUTE).get<int32_t>());
> > >
> > > Does the algorithm currently take into account what position the lens is
> > > at for the statistics it is considering? I'm concerned that it isn't, or
> > > that it's considering the position / top of the hill as a value which is
> > > mis-matched against the frame...?
> > >
> > > I presume there are delays that mean it might even be hard to determine
> > > how long it takes to move the VCM to a given position - so that might
> > > make it more difficult to state which frame had which position?
> >
> > That's a good remark, and I don't really have an answer to this yet. I
> > suppose there is a delay, but I don't know how to measure it easily. And
> > I am not sure it is a very big one, so it could very well not be a big
> > issue (opened question !) ?
> 
> This is quite a tricky thing to do.  One option is have the af algorithm
> count time/frames and assume it knows about the characteristics of the
> actuator and knows when the lens stopped moving to sample the stats.

I think we'll need that. VCM actuators can be very tricky, especially
when they don't have a control loop. Large swing values will result in
overshoot and ringing. Some open-loop VCM actuators have hardware
features to generate a ramp instead of a step, which can already help
(the ramp parameters can be programmable).

> Another option would be to use the DelayedControls to return the "correct"
> lens position for each frame.  The problem with this is the lens movement
> time is dependent on displacement needed.  So, we need to adapt
> DelayedControls to allow something like "set the control now but inform me
> of the change N frames later." That would work, and keeps in line with how
> delayed controls report shutter/gain from the sensor.

I don't think DelayedControl should handle this, it will be very
VCM-specific. A VCM helper (or rather, most likely, VCM helper*s*) would
be better.

> > >> +}
> > >> +
> > >>   void RPiCameraData::setSensorControls(ControlList &controls)
> > >>   {
> > >>          /*

-- 
Regards,

Laurent Pinchart