[libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens position control from IPA

[Kieran Bingham]

Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)
> Hi Daniel
> 
> On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > Allow control of lens position from the IPA, by setting corresponding
> > af fields in the IPAFrameContext structure. Controls are then passed to
> > the pipeline handler, which sets the lens position in CameraLens.
> >
> > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > ---
> >  include/libcamera/ipa/rkisp1.mojom       |  1 +
> >  src/ipa/rkisp1/ipa_context.h             |  5 +++++
> >  src/ipa/rkisp1/rkisp1.cpp                | 12 ++++++++++++
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++
> >  4 files changed, 34 insertions(+)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index eaf3955e..caa1121a 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -32,5 +32,6 @@ interface IPARkISP1Interface {
> >  interface IPARkISP1EventInterface {
> >       paramsBufferReady(uint32 frame);
> >       setSensorControls(uint32 frame, libcamera.ControlList sensorControls);
> > +     setLensControls(libcamera.ControlList sensorControls);
> >       metadataReady(uint32 frame, libcamera.ControlList metadata);
> >  };
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 2bdb6a81..d6d46b57 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -42,6 +42,11 @@ struct IPASessionConfiguration {
> >  };
> >
> >  struct IPAFrameContext {
> > +     struct {
> > +             uint32_t lensPosition;
> > +             bool applyLensCtrls;
> > +     } af;
> > +
> 
> So, I feel like this patch is better delayed after we have completed
> the rework of the IPU3 and RkISP1 IPA modules and move the algorithms
> to retain their state internally instead of using the IPAFrameContext.
> 
> There's a series almost landed that renames IPAFrameContext to
> IPAActiveState and there will be a few more to align the two IPA
> modules.
> Would you mind taking this in the series that introduces the AF
> algorithm while I try to collect 1 and 2 asap ?

It does seem like a lot of this will change indeed. So we really should
try to clean up the interfaces to get it a bit more stable.


> 
> >       struct {
> >               uint32_t exposure;
> >               double gain;
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 9e4c48a2..39e1ab2f 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> >       context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
> >       context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
> >
> > +     /* Lens position is unknown at the startup, so initilize the variable
> > +      * that holds the current position to something out of the range. */
> > +     context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max();
> > +
> >       context_.frameContext.frameCount = 0;
> >
> >       for (auto const &algo : algorithms()) {
> > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame)
> >       ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> >
> >       setSensorControls.emit(frame, ctrls);
> > +
> > +     if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {
> > +             context_.frameContext.af.applyLensCtrls = false;
> > +             ControlList lensCtrls(*lensCtrls_);
> > +             lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
> > +                           static_cast<int32_t>(context_.frameContext.af.lensPosition));
> > +             setLensControls.emit(lensCtrls);
> > +     }
> >  }
> >
> >  void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 5f10c26b..de0d37da 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -108,6 +108,7 @@ private:
> >       void paramFilled(unsigned int frame);
> >       void setSensorControls(unsigned int frame,
> >                              const ControlList &sensorControls);
> > +     void setLensControls(const ControlList &lensControls);
> >
> >       void metadataReady(unsigned int frame, const ControlList &metadata);
> >  };
> > @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> >               return -ENOENT;
> >
> >       ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);
> > +     ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls);
> >       ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);
> >       ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
> >
> > @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> >       delayedCtrls_->push(sensorControls);
> >  }
> >
> > +void RkISP1CameraData::setLensControls(const ControlList &lensControls)
> > +{
> > +     CameraLens *focusLens = sensor_->focusLens();
> > +     if (!focusLens)
> > +             return;
> > +
> > +     if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))
> > +             return;

I think the construction of sensor_->focusLens should make sure that the
correct control is set, so that if we have a valid CameraLens
*focusLens, we know it's valid to call focusLens->setFocusPosition().

So the above lensControls.contains() should be redundant.


> > +
> > +     const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);
> > +
> > +     focusLens->setFocusPosition(focusValue.get<int32_t>());

But I think Jacopo also has plans to change this so that a libcamera
control is passed in anyway, meaning the conversion from libcamera focus
position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the
CameraLens class anyway.


> > +}
> > +
> >  void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
> >  {
> >       RkISP1FrameInfo *info = frameInfo_.find(frame);
> > --
> > 2.34.1
> >