[libcamera-devel] [PATCH] pipeline: rkisp1: Move ControlInfoMap to IPA module

[Umang Jain]

Hi Laurent,

Thank you for the patch

On 8/2/22 05:43, Laurent Pinchart via libcamera-devel wrote:
> Currently the pipeline handler advertises controls handled by the IPA
> from a ControlInfoMap it manually constructs. This is wrong, as the IPA
> module is the component that knows what controls it supports. Fix this
> by moving the ControlInfoMap construction to the IPA module, and pass it
> to the pipeline handler as a return value from the IPA init() function.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

> ---
>   include/libcamera/ipa/rkisp1.mojom       |  2 +-
>   src/ipa/rkisp1/rkisp1.cpp                | 30 ++++++++++++++++++++---
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++----------------------
>   3 files changed, 30 insertions(+), 33 deletions(-)
>
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index e35373852b7c..eaf3955e4096 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -11,7 +11,7 @@ import "include/libcamera/ipa/core.mojom";
>   interface IPARkISP1Interface {
>   	init(libcamera.IPASettings settings,
>   	     uint32 hwRevision)
> -		=> (int32 ret);
> +		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
>   	start() => (int32 ret);
>   	stop();
>   
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index e5010f6ab629..52f7143980fe 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -46,7 +46,8 @@ namespace ipa::rkisp1 {
>   class IPARkISP1 : public IPARkISP1Interface, public Module
>   {
>   public:
> -	int init(const IPASettings &settings, unsigned int hwRevision) override;
> +	int init(const IPASettings &settings, unsigned int hwRevision,
> +		 ControlInfoMap *ipaControls) override;
>   	int start() override;
>   	void stop() override {}
>   
> @@ -89,12 +90,27 @@ private:
>   	struct IPAContext context_;
>   };
>   
> +namespace {
> +
> +/* List of controls handled by the RkISP1 IPA */
> +const ControlInfoMap::Map rkisp1Controls{
> +	{ &controls::AeEnable, ControlInfo(false, true) },
> +	{ &controls::Brightness, ControlInfo(-1.0f, 0.993f) },
> +	{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },
> +	{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },
> +	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> +	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> +};
> +
> +} /* namespace */
> +
>   std::string IPARkISP1::logPrefix() const
>   {
>   	return "rkisp1";
>   }
>   
> -int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
> +int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> +		    ControlInfoMap *ipaControls)
>   {
>   	/* \todo Add support for other revisions */
>   	switch (hwRevision) {
> @@ -156,7 +172,15 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
>   		return -EINVAL;
>   	}
>   
> -	return createAlgorithms(context_, (*data)["algorithms"]);
> +	int ret = createAlgorithms(context_, (*data)["algorithms"]);
> +	if (ret)
> +		return ret;
> +
> +	/* Return the controls handled by the IPA. */
> +	ControlInfoMap::Map ctrlMap = rkisp1Controls;
> +	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> +
> +	return 0;
>   }
>   
>   int IPARkISP1::start()
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index de687f4dc6b0..932873329e89 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -344,7 +344,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>   		ipaTuningFile = std::string(configFromEnv);
>   	}
>   
> -	int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision);
> +	int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> +			     &controlInfo_);
>   	if (ret < 0) {
>   		LOG(RkISP1, Error) << "IPA initialization failure";
>   		return ret;
> @@ -967,34 +968,6 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   		std::make_unique<RkISP1CameraData>(this, &mainPath_,
>   						   hasSelfPath_ ? &selfPath_ : nullptr);
>   
> -	ControlInfoMap::Map ctrls;
> -	ctrls.emplace(std::piecewise_construct,
> -		      std::forward_as_tuple(&controls::Sharpness),
> -		      std::forward_as_tuple(0.0f, 10.0f, 1.0f));
> -
> -	ctrls.emplace(std::piecewise_construct,
> -		      std::forward_as_tuple(&controls::Brightness),
> -		      std::forward_as_tuple(-1.0f, 0.993f));
> -
> -	ctrls.emplace(std::piecewise_construct,
> -		      std::forward_as_tuple(&controls::Contrast),
> -		      std::forward_as_tuple(0.0f, 1.993f));
> -
> -	ctrls.emplace(std::piecewise_construct,
> -		      std::forward_as_tuple(&controls::Saturation),
> -		      std::forward_as_tuple(0.0f, 1.993f));
> -
> -	ctrls.emplace(std::piecewise_construct,
> -		      std::forward_as_tuple(&controls::draft::NoiseReductionMode),
> -		      std::forward_as_tuple(controls::draft::NoiseReductionModeValues));
> -
> -	ctrls.emplace(std::piecewise_construct,
> -		      std::forward_as_tuple(&controls::AeEnable),
> -		      std::forward_as_tuple(false, true));
> -
> -	data->controlInfo_ = ControlInfoMap(std::move(ctrls),
> -					    controls::controls);
> -
>   	data->sensor_ = std::make_unique<CameraSensor>(sensor);
>   	ret = data->sensor_->init();
>   	if (ret)