[libcamera-devel] [PATCH v6 09/10] rkisp1: Control camera lens position from IPA

[Daniel Semkowicz]

Hi Laurent,

On Wed, Apr 26, 2023 at 6:09 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Daniel,
>
> Thank you for the patch.
>
> On Fri, Mar 31, 2023 at 10:19:29AM +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/rkisp1.cpp                |  8 ++++++++
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 +++++++++++++
> >  3 files changed, 22 insertions(+)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index d4ff1230..dc6a8ffc 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -39,5 +39,6 @@ interface IPARkISP1Interface {
> >  interface IPARkISP1EventInterface {
> >       paramsBufferReady(uint32 frame);
> >       setSensorControls(uint32 frame, libcamera.ControlList sensorControls);
> > +     setLensControls(libcamera.ControlList lensControls);
> >       metadataReady(uint32 frame, libcamera.ControlList metadata);
> >  };
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 9c8b4a82..37b1e0a8 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -497,6 +497,14 @@ void IPARkISP1::setControls(unsigned int frame)
> >       ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> >
> >       setSensorControls.emit(frame, ctrls);
> > +
> > +     if (lensControls_ && context_.activeState.af.applyLensCtrls) {
> > +             context_.activeState.af.applyLensCtrls = false;
> > +             ControlList lensCtrls(*lensControls_);
> > +             lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
> > +                           context_.activeState.af.lensPosition);
> > +             setLensControls.emit(lensCtrls);
>
> This means that the IPA module will call the pipeline handler twice to
> set controls, once for sensor controls and once for lens controls. As
> those calls are cross-thread, and possibly cross-process, that's not
> very efficient. It would be better to combine the two with a single
> setControls signal that takes sensor and lens control parameters.

This makes sense, I will merge these two functions.

>
> You could then possibly drop the applyLensCtrls variable and always send
> the lens position value to the pipeline handler. Avoiding going to the
> kernel driver when the value of the lens position hasn't changed could
> then be implemented in the CameraLens::setFocusPosition() function
> instead of each IPA module.

Ok, as now there is idea of extending the CameraLens functionality,
this micro-optimization has less sense as it will need to be removed in
the future. I will remove the applyLensCtrls field.

>
> > +     }
> >  }
> >
> >  } /* namespace ipa::rkisp1 */
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index f966254a..f42a8368 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -114,6 +114,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);
> >  };
> > @@ -340,6 +341,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> >               return -ENOENT;
> >
> >       ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);
> > +     if (sensor_->focusLens())
> > +             ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls);
> >       ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);
> >       ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
> >
> > @@ -409,6 +412,16 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> >       delayedCtrls_->push(sensorControls);
> >  }
> >
> > +void RkISP1CameraData::setLensControls(const ControlList &lensControls)
> > +{
> > +     CameraLens *focusLens = sensor_->focusLens();
> > +
> > +     for (auto const &[id, value] : lensControls) {
> > +             if (id == V4L2_CID_FOCUS_ABSOLUTE)
> > +                     focusLens->setFocusPosition(value.get<int32_t>());
> > +     }
> > +}
> > +
> >  void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
> >  {
> >       RkISP1FrameInfo *info = frameInfo_.find(frame);
>
> --
> Regards,
>
> Laurent Pinchart

Best regards
Daniel