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

[paul.elder at ideasonboard.com]

On Tue, Aug 02, 2022 at 03:13:30AM +0300, Laurent Pinchart 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: Paul Elder <paul.elder 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)
> -- 
> Regards,
> 
> Laurent Pinchart
>