[libcamera-devel] [PATCH v3 21/23] ipa: ipu3: Add and use LensFocusAbsolute control

[Jacopo Mondi]

Hi Kieran

On Thu, Jun 30, 2022 at 11:14:13PM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:39:00)
> > Add to the internal controls LensFocusAbsolute as a draft control
> > as it is currently identical to V4L2_CID_FOCUS_ABSOLUTE.
> >
> > Use the newly introduced control in the IPU3 pipeline handler and IPA
> > module.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/ipa/ipu3.mojom        |  3 +--
> >  src/ipa/ipu3/ipu3.cpp                   |  7 ++-----
> >  src/libcamera/internal_control_ids.yaml | 13 +++++++++++++
> >  src/libcamera/pipeline/ipu3/ipu3.cpp    | 13 +++++--------
> >  4 files changed, 21 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> > index 64fe65fdd5fc..bfeadc58c52a 100644
> > --- a/include/libcamera/ipa/ipu3.mojom
> > +++ b/include/libcamera/ipa/ipu3.mojom
> > @@ -35,8 +35,7 @@ interface IPAIPU3Interface {
> >  };
> >
> >  interface IPAIPU3EventInterface {
> > -       setSensorControls(uint32 frame, libcamera.ControlList sensorControls,
> > -                         libcamera.ControlList lensControls);
> > +       setSensorControls(uint32 frame, libcamera.ControlList sensorControls);
> >         paramsBufferReady(uint32 frame);
> >         metadataReady(uint32 frame, libcamera.ControlList metadata);
> >  };
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 6d622b4c290b..2f158df367a8 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -541,12 +541,9 @@ void IPAIPU3::setControls(unsigned int frame, const IPAActiveState &state)
> >         ControlList ctrls(controls::internal::controls);
> >         ctrls.set(controls::internal::ExposureTime, state.agc.exposure);
> >         ctrls.set(controls::internal::AnalogueGain, static_cast<float>(state.agc.gain));
> > +       ctrls.set(controls::internal::draft::LensFocusAbsolute, state.af.focus);
> >
> > -       ControlList lensCtrls(controls::controls);
> > -       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
> > -                     static_cast<int32_t>(state.af.focus));
> > -
> > -       setSensorControls.emit(frame, ctrls, lensCtrls);
> > +       setSensorControls.emit(frame, ctrls);
> >  }
> >
> >  } /* namespace ipa::ipu3 */
> > diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml
> > index 6d3775afcf67..804c227213a7 100644
> > --- a/src/libcamera/internal_control_ids.yaml
> > +++ b/src/libcamera/internal_control_ids.yaml
> > @@ -24,4 +24,17 @@ controls:
> >        description: |
> >          The sensor analogue gain value.
> >
> > +  # ----------------------------------------------------------------------------
> > +  # Draft controls section
> > +
> > +  - LensFocusAbsolute:
> > +      type: int32_t
> > +      draft: true
> > +      description: |
> > +        This control sets the focal point of the camera to the specified
> > +        position. The unit is undefined. Positive values set the focus closer to
> > +        the camera, negative values towards infinity.
>
> I think this is something that we should be returning in
> request->metadata().
>
> So that means it can't be an internal only control ?
>
> We should define the units too. I thought David had a definition based
> around a calibrated/known infinity point?
>

We're missing a lot of pieces yet. We have defined a unit for
LensPosition which requires a clear way to measure/calibrate the lens,
and for internal usage we have no way yet to translate the generic
control to the sensor-specific value.

So this is identical to the V4L2 version, as that's what the IPA uses
at the moment.

> > +
> > +        Currently identical to V4L2_CID_FOCUS_ABSOLUTE.
> > +
> >  ...
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 55b96137f065..1dbcdd0b56e6 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -86,8 +86,7 @@ public:
> >  private:
> >         void metadataReady(unsigned int id, const ControlList &metadata);
> >         void paramsBufferReady(unsigned int id);
> > -       void setSensorControls(unsigned int id, const ControlList &sensorControls,
> > -                              const ControlList &lensControls);
> > +       void setSensorControls(unsigned int id, const ControlList &sensorControls);
> >  };
> >
> >  class IPU3CameraConfiguration : public CameraConfiguration
> > @@ -1247,8 +1246,7 @@ int IPU3CameraData::loadIPA()
> >  }
> >
> >  void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,
> > -                                      const ControlList &sensorControls,
> > -                                      const ControlList &lensControls)
> > +                                      const ControlList &sensorControls)
> >  {
> >         CameraSensor *sensor = cio2_.sensor();
> >
> > @@ -1258,12 +1256,11 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,
> >         if (!focusLens)
> >                 return;
> >
> > -       if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))
> > +       if (!sensorControls.contains(controls::internal::draft::LensFocusAbsolute))
> >                 return;
> >
> > -       const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);
> > -
> > -       focusLens->setFocusPosition(focusValue.get<int32_t>());
> > +       int32_t focusAbsolute = sensorControls.get(controls::internal::draft::LensFocusAbsolute);
> > +       focusLens->setFocusPosition(focusAbsolute);
>
> And I think conversion between our defined units and the units specific to a
> lens driver should then be handled in the CameraLens class, so passing
> in our 'defined' libcamera units type here should be fine.
>
>
> >  }
> >
> >  void IPU3CameraData::paramsBufferReady(unsigned int id)
> > --
> > 2.36.1
> >