[libcamera-devel] [PATCH v2 3/3] DEMO: raspberrypi: Use custom parameters to init()

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, Mar 08, 2021 at 04:48:28PM +0900, Paul Elder wrote:
> This is just a demo to show custom parameters to init() with the
> raspberrypi IPA interface.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom       |  5 ++-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 44 ++++++++++---------
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 10 +++--
>  3 files changed, 32 insertions(+), 27 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index f733a2cd..99a62c02 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -51,7 +51,8 @@ struct StartControls {
>  };
>  
>  interface IPARPiInterface {
> -	init(IPASettings settings) => (int32 ret);
> +	init(IPASettings settings, string sensorName)
> +		=> (int32 ret, bool metadataSupport);
>  	start(StartControls controls) => (StartControls result);
>  	stop();
>  
> @@ -77,7 +78,7 @@ interface IPARPiInterface {
>  		  map<uint32, IPAStream> streamConfig,
>  		  map<uint32, ControlInfoMap> entityControls,
>  		  ConfigInput ipaConfig)
> -		=> (ConfigOutput results, int32 ret);
> +		=> (int32 ret, ConfigOutput results);

I wonder if it would make sense to split those two changes. The change
to configure() could be reviewed and merged already, and Naush could
take this DEMO patch as an example to move data->sensorMetadata_
handling to match() and IPA init() time.

For the configure() part of this patch,

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

You don't have to include an init() demo in v3 of the series (or just
v2.1 of this patch actually), this is enough.

>  
>  	/**
>  	 * \fn mapBuffers()
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 6348d071..ac18dcbd 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -79,16 +79,17 @@ public:
>  			munmap(lsTable_, ipa::RPi::MaxLsGridSize);
>  	}
>  
> -	int init(const IPASettings &settings) override;
> +	int init(const IPASettings &settings, const std::string &sensorName,
> +		 bool *metadataSupport) override;
>  	void start(const ipa::RPi::StartControls &data,
>  		   ipa::RPi::StartControls *result) override;
>  	void stop() override {}
>  
> -	void configure(const CameraSensorInfo &sensorInfo,
> -		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, ControlInfoMap> &entityControls,
> -		       const ipa::RPi::ConfigInput &data,
> -		       ipa::RPi::ConfigOutput *response, int32_t *ret) override;
> +	int configure(const CameraSensorInfo &sensorInfo,
> +		      const std::map<unsigned int, IPAStream> &streamConfig,
> +		      const std::map<unsigned int, ControlInfoMap> &entityControls,
> +		      const ipa::RPi::ConfigInput &data,
> +		      ipa::RPi::ConfigOutput *response) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  	void signalStatReady(const uint32_t bufferId) override;
> @@ -164,9 +165,14 @@ private:
>  	double maxFrameDuration_;
>  };
>  
> -int IPARPi::init(const IPASettings &settings)
> +int IPARPi::init(const IPASettings &settings, const std::string &sensorName,
> +		 bool *metadataSupport)
>  {
> +	LOG(IPARPI, Debug) << "sensor name is " << sensorName;
> +
>  	tuningFile_ = settings.configurationFile;
> +
> +	*metadataSupport = true;
>  	return 0;
>  }
>  
> @@ -290,16 +296,15 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
>  	mode_.max_frame_length = sensorInfo.maxFrameLength;
>  }
>  
> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> -		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, ControlInfoMap> &entityControls,
> -		       const ipa::RPi::ConfigInput &ipaConfig,
> -		       ipa::RPi::ConfigOutput *result, int32_t *ret)
> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> +		      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> +		      const std::map<unsigned int, ControlInfoMap> &entityControls,
> +		      const ipa::RPi::ConfigInput &ipaConfig,
> +		      ipa::RPi::ConfigOutput *result)
>  {
>  	if (entityControls.size() != 2) {
>  		LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> -		*ret = -1;
> -		return;
> +		return -1;
>  	}
>  
>  	result->params = 0;
> @@ -309,14 +314,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  
>  	if (!validateSensorControls()) {
>  		LOG(IPARPI, Error) << "Sensor control validation failed.";
> -		*ret = -1;
> -		return;
> +		return -1;
>  	}
>  
>  	if (!validateIspControls()) {
>  		LOG(IPARPI, Error) << "ISP control validation failed.";
> -		*ret = -1;
> -		return;
> +		return -1;
>  	}
>  
>  	/* Setup a metadata ControlList to output metadata. */
> @@ -334,8 +337,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		if (!helper_) {
>  			LOG(IPARPI, Error) << "Could not create camera helper for "
>  					   << cameraName;
> -			*ret = -1;
> -			return;
> +			return -1;
>  		}
>  
>  		/*
> @@ -403,7 +405,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  
>  	lastMode_ = mode_;
>  
> -	*ret = 0;
> +	return 0;
>  }
>  
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index db91f1b5..3ff0f1cd 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1194,7 +1194,10 @@ int RPiCameraData::loadIPA()
>  
>  	IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));
>  
> -	return ipa_->init(settings);
> +	bool metadataSupport;
> +	int ret = ipa_->init(settings, "sensor name", &metadataSupport);
> +	LOG(RPI, Debug) << "metadata support " << (metadataSupport ? "yes" : "no");
> +	return ret;
>  }
>  
>  int RPiCameraData::configureIPA(const CameraConfiguration *config)
> @@ -1250,9 +1253,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	/* Ready the IPA - it must know about the sensor resolution. */
>  	ipa::RPi::ConfigOutput result;
>  
> -	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> -			&result, &ret);
> -
> +	ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> +			      &result);
>  	if (ret < 0) {
>  		LOG(RPI, Error) << "IPA configuration failed!";
>  		return -EPIPE;

-- 
Regards,

Laurent Pinchart