[libcamera-devel] [PATCH 1/3] libcamera: rkisp1: Control the lens from pipeline

[Jacopo Mondi]

Hi Daniel,

On Tue, Jun 28, 2022 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> Control lens focus from rkisp1 pipeline, using CameraLens controller
> and expose lens controls to the IPA during configure().
>
> Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 7cf36524..363273b2 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -28,6 +28,7 @@
>  #include <libcamera/stream.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"
> @@ -107,6 +108,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);
>  };
> @@ -353,6 +355,20 @@ 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;

I would rather do so in the CameraLens class.

The class validates that V4L2_CID_FOCUS_ABSOLUTE is available in
CameraLens::init(). I would store a flag there and return immediately
from setFocusPosition() if not available.

> +
> +	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);
> @@ -654,6 +670,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	std::map<uint32_t, ControlInfoMap> entityControls;
>  	entityControls.emplace(0, data->sensor_->controls());
>
> +	CameraLens *lens = data->sensor_->focusLens();
> +	if (lens)
> +		entityControls.emplace(1, lens->controls());
> +

I would have made 1 patch for the PH/IPA initialization, and one patch
for the IPA/PH control set phase instead of making one patch for PH
and one for IPA. Hope this makes sense :P

The patch direction looks good!

Thanks
   j
>  	ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);
>  	if (ret) {
>  		LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
> --
> 2.34.1
>