[libcamera-devel,v4,09/10] rkisp1: Control camera lens position from IPA
diff mbox series

Message ID 20230314144834.85193-10-dse@thaumatec.com
State Superseded
Headers show
Series
  • ipa: rkisp1: Add autofocus algorithm
Related show

Commit Message

Daniel Semkowicz March 14, 2023, 2:48 p.m. UTC 
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@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(+)

Comments

Jacopo Mondi March 21, 2023, 2:53 p.m. UTC | #1 
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@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 ?

	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
>
Daniel Semkowicz March 22, 2023, 11:11 a.m. UTC | #2 
On Tue, Mar 21, 2023 at 3:53 PM Jacopo Mondi
<jacopo.mondi@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@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
> >

Patch
diff mbox series 

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