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

[Daniel Semkowicz]

On Tue, Mar 21, 2023 at 3:53 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Daniel
>
> On Tue, Mar 14, 2023 at 03:48:33PM +0100, Daniel Semkowicz 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 | 17 +++++++++++++++++
> >  3 files changed, 26 insertions(+)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index bf6e9141..c3ed87aa 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 4b30844f..4aef2e30 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -469,6 +469,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);
> > +     }
> >  }
> >
> >  } /* namespace ipa::rkisp1 */
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 83fb6287..1dc3f957 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,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);
> >
> > @@ -403,6 +405,21 @@ 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;
>
> As you correctly pointed out in the reply to my review of 2/10,
> Am I wrong thinking that if any of the two above condition is false
> we don't get a valid CameraLens * from the CameraSensor, hence we
> never initialize ipaConfig.lensControls, effecively disabling lens
> support in the IPA ?

Yes, you are right. I think I should even connect the setLensControls()
slot only if the lens were successfully initialized. Then, We can
safely assume, this function will not be called from the IPA even by
accident. In this case I can remove the:

    if (!focusLens)
        return;

code block.

The following block:

    if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))
        return;

will need to stay, as there is no guarantee that V4L2_CID_FOCUS_ABSOLUTE
was included in the lensControls by the IPA calling
the setLensControls().

>
>         CameraLens *lens = data->sensor_->focusLens();
>         if (lens)
>                 ipaConfig.lensControls = lens->controls();
>
> > +
> > +     const ControlValue &focusValue =
> > +             lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);
> > +
> > +     focusLens->setFocusPosition(focusValue.get<int32_t>());
> > +}
> > +
> >  void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
> >  {
> >       RkISP1FrameInfo *info = frameInfo_.find(frame);
> > --
> > 2.39.2
> >