[PATCH v5 09/18] libcamera: software_isp: Call Algorithm::configure

[Laurent Pinchart]

Hi Milan,

Thank you for the patch.

On Fri, Aug 30, 2024 at 09:25:45AM +0200, Milan Zamazal wrote:
> This patch adds Algorithm::configure call for the defined algorithms.
> This is preparation only since there are currently no Algorithm based
> algorithms defined.
> 
> A part of this change is passing IPAConfigInfo instead of ControlInfoMap
> to configure() calls as this is what Algorithm::configure expects.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  .../libcamera/internal/software_isp/software_isp.h   |  2 +-
>  include/libcamera/ipa/soft.mojom                     |  2 +-
>  src/ipa/simple/module.h                              |  2 ++
>  src/ipa/simple/soft_simple.cpp                       | 12 +++++++++---
>  src/libcamera/pipeline/simple/simple.cpp             | 11 +++++++----
>  src/libcamera/software_isp/software_isp.cpp          |  7 ++++---
>  6 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index 3a84418e..c5d5a46f 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -62,7 +62,7 @@ public:
>  
>  	int configure(const StreamConfiguration &inputCfg,
>  		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> -		      const ControlInfoMap &sensorControls);
> +		      const ipa::soft::IPAConfigInfo &configInfo);
>  
>  	int exportBuffers(const Stream *stream, unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> index cc5c37b2..6b049d20 100644
> --- a/include/libcamera/ipa/soft.mojom
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -20,7 +20,7 @@ interface IPASoftInterface {
>  		=> (int32 ret);
>  	start() => (int32 ret);
>  	stop();
> -	configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
> +	configure(IPAConfigInfo configInfo)
>  		=> (int32 ret);
>  
>  	[async] processStats(uint32 frame,
> diff --git a/src/ipa/simple/module.h b/src/ipa/simple/module.h
> index 5aaf8eb0..d1321f59 100644
> --- a/src/ipa/simple/module.h
> +++ b/src/ipa/simple/module.h
> @@ -9,6 +9,8 @@
>  
>  #include <libcamera/controls.h>
>  
> +#include <libcamera/ipa/soft_ipa_interface.h>

Does this belong to 03/18 ? Actually, in that patch, you add

#include "soft_ipa_interface.h"

which I think should be dropped as it refers to
libcamera/ipa/soft_ipa_interface.h. With that,

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

> +
>  #include "libcamera/internal/software_isp/debayer_params.h"
>  #include "libcamera/internal/software_isp/swisp_stats.h"
>  
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 79ed4891..0ea62266 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -73,7 +73,7 @@ public:
>  		 const SharedFD &fdStats,
>  		 const SharedFD &fdParams,
>  		 const ControlInfoMap &sensorInfoMap) override;
> -	int configure(const ControlInfoMap &sensorInfoMap) override;
> +	int configure(const IPAConfigInfo &configInfo) override;
>  
>  	int start() override;
>  	void stop() override;
> @@ -207,9 +207,9 @@ int IPASoftSimple::init(const IPASettings &settings,
>  	return 0;
>  }
>  
> -int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
> +int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>  {
> -	sensorInfoMap_ = sensorInfoMap;
> +	sensorInfoMap_ = configInfo.sensorControls;
>  
>  	const ControlInfo &exposureInfo = sensorInfoMap_.find(V4L2_CID_EXPOSURE)->second;
>  	const ControlInfo &gainInfo = sensorInfoMap_.find(V4L2_CID_ANALOGUE_GAIN)->second;
> @@ -248,6 +248,12 @@ int IPASoftSimple::configure(const ControlInfoMap &sensorInfoMap)
>  		againMinStep_ = 1.0;
>  	}
>  
> +	for (auto const &algo : algorithms()) {
> +		int ret = algo->configure(context_, configInfo);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	LOG(IPASoft, Info) << "Exposure " << exposureMin_ << "-" << exposureMax_
>  			   << ", gain " << againMin_ << "-" << againMax_
>  			   << " (" << againMinStep_ << ")";
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index ebec592a..834b33d9 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -1294,10 +1294,13 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	inputCfg.stride = captureFormat.planes[0].bpl;
>  	inputCfg.bufferCount = kNumInternalBuffers;
>  
> -	return data->converter_
> -		       ? data->converter_->configure(inputCfg, outputCfgs)
> -		       : data->swIsp_->configure(inputCfg, outputCfgs,
> -						 data->sensor_->controls());
> +	if (data->converter_) {
> +		return data->converter_->configure(inputCfg, outputCfgs);
> +	} else {
> +		ipa::soft::IPAConfigInfo configInfo;
> +		configInfo.sensorControls = data->sensor_->controls();
> +		return data->swIsp_->configure(inputCfg, outputCfgs, configInfo);
> +	}
>  }
>  
>  int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index c5db45ae..8f1c0f65 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -223,16 +223,17 @@ SoftwareIsp::strideAndFrameSize(const PixelFormat &outputFormat, const Size &siz
>   * \brief Configure the SoftwareIsp object according to the passed in parameters
>   * \param[in] inputCfg The input configuration
>   * \param[in] outputCfgs The output configurations
> - * \param[in] sensorControls ControlInfoMap of the controls supported by the sensor
> + * \param[in] configInfo The IPA configuration data, received from the pipeline
> + * handler
>   * \return 0 on success, a negative errno on failure
>   */
>  int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>  			   const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> -			   const ControlInfoMap &sensorControls)
> +			   const ipa::soft::IPAConfigInfo &configInfo)
>  {
>  	ASSERT(ipa_ && debayer_);
>  
> -	int ret = ipa_->configure(sensorControls);
> +	int ret = ipa_->configure(configInfo);
>  	if (ret < 0)
>  		return ret;
>  

-- 
Regards,

Laurent Pinchart