[libcamera-devel,v2,1/2] libcamera: raspberrypi: Control the lens from pipeline

15463 diff mbox series

The lens focus is controled by a VCM, which is linked to the sensor
using the ancillary links.
Pass the control to the config info structure and make it possible to
update by the IPA.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 include/libcamera/ipa/raspberrypi.mojom         |  1 +
 .../pipeline/raspberrypi/raspberrypi.cpp        | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-17 09:19:37)
> The lens focus is controled by a VCM, which is linked to the sensor
> using the ancillary links.
> Pass the control to the config info structure and make it possible to
> update by the IPA.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom         |  1 +
>  .../pipeline/raspberrypi/raspberrypi.cpp        | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index acd3cafe..0c3922b0 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -125,4 +125,5 @@ interface IPARPiEventInterface {
>         embeddedComplete(uint32 bufferId);
>         setIspControls(libcamera.ControlList controls);
>         setDelayedControls(libcamera.ControlList controls);
> +       setLensControls(libcamera.ControlList controls);
>  };
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index c2230199..41be10ff 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -33,6 +33,7 @@
>  
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/camera.h"
> +#include "libcamera/internal/camera_lens.h"
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/delayed_controls.h"
>  #include "libcamera/internal/device_enumerator.h"
> @@ -202,6 +203,7 @@ public:
>         void setIspControls(const ControlList &controls);
>         void setDelayedControls(const ControlList &controls);
>         void setSensorControls(ControlList &controls);
> +       void setLensControls(const ControlList &controls);
>  
>         /* bufferComplete signal handlers. */
>         void unicamBufferDequeue(FrameBuffer *buffer);
> @@ -1483,6 +1485,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
>         ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
>         ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>         ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
> +       ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls);
>  
>         /*
>          * The configuration (tuning file) is made from the sensor name unless
> @@ -1519,6 +1522,10 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>         entityControls.emplace(0, sensor_->controls());
>         entityControls.emplace(1, isp_[Isp::Input].dev()->controls());
>  
> +       CameraLens *lens = sensor_->focusLens();
> +       if (lens)
> +               entityControls.emplace(2, lens->controls());
> +
>         /* Always send the user transform to the IPA. */
>         ipaConfig.transform = static_cast<unsigned int>(config->transform);
>  
> @@ -1735,6 +1742,16 @@ void RPiCameraData::setDelayedControls(const ControlList &controls)
>         handleState();
>  }
>  
> +void RPiCameraData::setLensControls(const ControlList &controls)
> +{
> +       ControlList ctrls = controls;

Is this required? Is it a copy? Or is it just to save 3 chars?

> +
> +       CameraLens *lens = sensor_->focusLens();
> +       if (lens)
> +               lens->setFocusPosition(ctrls.get(V4L2_CID_FOCUS_ABSOLUTE).get<int32_t>());

I suspect it's fine, but what happens if there are no lens control
changes - is anything required to be handled in the State below?

As it's 'setLensControls' perhaps

	if (!lens)
		return;

would be more appropriate?

--
Kieran


> +       handleState();
> +}
> +
>  void RPiCameraData::setSensorControls(ControlList &controls)
>  {
>         /*
> -- 
> 2.32.0
>

Hi Jean-Michel,

On Thu, 17 Mar 2022 at 09:20, Jean-Michel Hautbois via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> The lens focus is controled by a VCM, which is linked to the sensor
> using the ancillary links.
> Pass the control to the config info structure and make it possible to
> update by the IPA.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom         |  1 +
>  .../pipeline/raspberrypi/raspberrypi.cpp        | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> index acd3cafe..0c3922b0 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -125,4 +125,5 @@ interface IPARPiEventInterface {
>         embeddedComplete(uint32 bufferId);
>         setIspControls(libcamera.ControlList controls);
>         setDelayedControls(libcamera.ControlList controls);
> +       setLensControls(libcamera.ControlList controls);
>  };
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index c2230199..41be10ff 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -33,6 +33,7 @@
>
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/camera.h"
> +#include "libcamera/internal/camera_lens.h"
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/delayed_controls.h"
>  #include "libcamera/internal/device_enumerator.h"
> @@ -202,6 +203,7 @@ public:
>         void setIspControls(const ControlList &controls);
>         void setDelayedControls(const ControlList &controls);
>         void setSensorControls(ControlList &controls);
> +       void setLensControls(const ControlList &controls);
>
>         /* bufferComplete signal handlers. */
>         void unicamBufferDequeue(FrameBuffer *buffer);
> @@ -1483,6 +1485,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> *sensorConfig)
>         ipa_->embeddedComplete.connect(this,
> &RPiCameraData::embeddedComplete);
>         ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>         ipa_->setDelayedControls.connect(this,
> &RPiCameraData::setDelayedControls);
> +       ipa_->setLensControls.connect(this,
> &RPiCameraData::setLensControls);
>
>         /*
>          * The configuration (tuning file) is made from the sensor name
> unless
> @@ -1519,6 +1522,10 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
>         entityControls.emplace(0, sensor_->controls());
>         entityControls.emplace(1, isp_[Isp::Input].dev()->controls());
>
> +       CameraLens *lens = sensor_->focusLens();
> +       if (lens)
> +               entityControls.emplace(2, lens->controls());
> +
>         /* Always send the user transform to the IPA. */
>         ipaConfig.transform = static_cast<unsigned int>(config->transform);
>
> @@ -1735,6 +1742,16 @@ void RPiCameraData::setDelayedControls(const
> ControlList &controls)
>         handleState();
>  }
>
> +void RPiCameraData::setLensControls(const ControlList &controls)
> +{
> +       ControlList ctrls = controls;
> +
> +       CameraLens *lens = sensor_->focusLens();
> +       if (lens)
> +
>  lens->setFocusPosition(ctrls.get(V4L2_CID_FOCUS_ABSOLUTE).get<int32_t>());
> +       handleState();
>

handleState() should not be needed heere.

Regards,
Naush


> +}
> +
>  void RPiCameraData::setSensorControls(ControlList &controls)
>  {
>         /*
> --
> 2.32.0
>
>