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

18515 diff mbox series

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 | 13 +++++++++++++
 3 files changed, 22 insertions(+)

Hi Daniel

On Fri, Mar 31, 2023 at 10:19:29AM +0200, Daniel Semkowicz via libcamera-devel 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>

Seems like the patch has not changed and you've lost my tag

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  include/libcamera/ipa/rkisp1.mojom       |  1 +
>  src/ipa/rkisp1/rkisp1.cpp                |  8 ++++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 +++++++++++++
>  3 files changed, 22 insertions(+)
>
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index d4ff1230..dc6a8ffc 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 9c8b4a82..37b1e0a8 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -497,6 +497,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 f966254a..f42a8368 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,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>  		return -ENOENT;
>
>  	ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);
> +	if (sensor_->focusLens())
> +		ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls);
>  	ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);
>  	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
>
> @@ -409,6 +412,16 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
>  	delayedCtrls_->push(sensorControls);
>  }
>
> +void RkISP1CameraData::setLensControls(const ControlList &lensControls)
> +{
> +	CameraLens *focusLens = sensor_->focusLens();
> +
> +	for (auto const &[id, value] : lensControls) {
> +		if (id == V4L2_CID_FOCUS_ABSOLUTE)
> +			focusLens->setFocusPosition(value.get<int32_t>());
> +	}
> +}
> +
>  void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
>  {
>  	RkISP1FrameInfo *info = frameInfo_.find(frame);
> --
> 2.39.2
>

Hi Laurent,

On Wed, Apr 26, 2023 at 6:09 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Daniel,
>
> Thank you for the patch.
>
> On Fri, Mar 31, 2023 at 10:19:29AM +0200, Daniel Semkowicz via libcamera-devel 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 | 13 +++++++++++++
> >  3 files changed, 22 insertions(+)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index d4ff1230..dc6a8ffc 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 9c8b4a82..37b1e0a8 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -497,6 +497,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);
>
> This means that the IPA module will call the pipeline handler twice to
> set controls, once for sensor controls and once for lens controls. As
> those calls are cross-thread, and possibly cross-process, that's not
> very efficient. It would be better to combine the two with a single
> setControls signal that takes sensor and lens control parameters.

This makes sense, I will merge these two functions.

>
> You could then possibly drop the applyLensCtrls variable and always send
> the lens position value to the pipeline handler. Avoiding going to the
> kernel driver when the value of the lens position hasn't changed could
> then be implemented in the CameraLens::setFocusPosition() function
> instead of each IPA module.

Ok, as now there is idea of extending the CameraLens functionality,
this micro-optimization has less sense as it will need to be removed in
the future. I will remove the applyLensCtrls field.

>
> > +     }
> >  }
> >
> >  } /* namespace ipa::rkisp1 */
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index f966254a..f42a8368 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,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> >               return -ENOENT;
> >
> >       ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);
> > +     if (sensor_->focusLens())
> > +             ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls);
> >       ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);
> >       ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
> >
> > @@ -409,6 +412,16 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> >       delayedCtrls_->push(sensorControls);
> >  }
> >
> > +void RkISP1CameraData::setLensControls(const ControlList &lensControls)
> > +{
> > +     CameraLens *focusLens = sensor_->focusLens();
> > +
> > +     for (auto const &[id, value] : lensControls) {
> > +             if (id == V4L2_CID_FOCUS_ABSOLUTE)
> > +                     focusLens->setFocusPosition(value.get<int32_t>());
> > +     }
> > +}
> > +
> >  void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
> >  {
> >       RkISP1FrameInfo *info = frameInfo_.find(frame);
>
> --
> Regards,
>
> Laurent Pinchart

Best regards
Daniel