[libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to PH and expose it to the IPA

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Fri, Mar 31, 2023 at 10:19:21AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> Extend the IPA init() function by additional lensControls input
> parameter. Check in pipeline handler if camera lens exists, and expose
> its controls to the IPA using the new parameter.
> 
> Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> ---
>  include/libcamera/ipa/rkisp1.mojom       | 3 ++-
>  src/ipa/rkisp1/rkisp1.cpp                | 7 +++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 1009e970..d4ff1230 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -17,7 +17,8 @@ interface IPARkISP1Interface {
>  	init(libcamera.IPASettings settings,
>  	     uint32 hwRevision,
>  	     libcamera.IPACameraSensorInfo sensorInfo,
> -	     libcamera.ControlInfoMap sensorControls)
> +	     libcamera.ControlInfoMap sensorControls,
> +	     libcamera.ControlInfoMap lensControls)
>  		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
>  	start() => (int32 ret);
>  	stop();
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 6544c925..d338d374 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -7,6 +7,7 @@
>  
>  #include <algorithm>
>  #include <math.h>
> +#include <optional>
>  #include <queue>
>  #include <stdint.h>
>  #include <string.h>
> @@ -52,6 +53,7 @@ public:
>  	int init(const IPASettings &settings, unsigned int hwRevision,
>  		 const IPACameraSensorInfo &sensorInfo,
>  		 const ControlInfoMap &sensorControls,
> +		 const ControlInfoMap &lensControls,
>  		 ControlInfoMap *ipaControls) override;
>  	int start() override;
>  	void stop() override;
> @@ -80,6 +82,7 @@ private:
>  	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>  
>  	ControlInfoMap sensorControls_;
> +	std::optional<ControlInfoMap> lensControls_;
>  
>  	/* revision-specific data */
>  	rkisp1_cif_isp_version hwRevision_;
> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const
>  int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>  		    const IPACameraSensorInfo &sensorInfo,
>  		    const ControlInfoMap &sensorControls,
> +		    const ControlInfoMap &lensControls,
>  		    ControlInfoMap *ipaControls)
>  {
>  	/* \todo Add support for other revisions */
> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>  	context_.configuration.sensor.lineDuration = sensorInfo.minLineLength
>  						   * 1.0s / sensorInfo.pixelRate;
>  
> +	if (!lensControls.empty())
> +		lensControls_ = lensControls;

Is there a reason to use std::optional<> for lensControls_, instead of a
plain ControlInfoMap that you would unconditionally initialize here to

	lensControls_ = lensControls;

? When checking if the lens controls exist in further patches in the
series, you could test

	if (!lensControls_.empty())

instead of

	if (lensControls_)

I'm fine with std::optional<> if there's a reason for it, so, either
way,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
>  	/* Load the tuning data file. */
>  	File file(settings.configurationFile);
>  	if (!file.open(File::OpenModeFlag::ReadOnly)) {
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 8a30fe06..f966254a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -32,6 +32,7 @@
>  #include <libcamera/ipa/rkisp1_ipa_proxy.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"
> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>  		return ret;
>  	}
>  
> +	ControlInfoMap lensControls;
> +	CameraLens *lens = sensor_->focusLens();
> +	if (lens)
> +		lensControls = lens->controls();
> +
>  	ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> -			 sensorInfo, sensor_->controls(), &controlInfo_);
> +			 sensorInfo, sensor_->controls(), lensControls,
> +			 &controlInfo_);
>  	if (ret < 0) {
>  		LOG(RkISP1, Error) << "IPA initialization failure";
>  		return ret;

-- 
Regards,

Laurent Pinchart