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

[Kieran Bingham]

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?

> +
> +        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
>