[libcamera-devel] [PATCH 7/7] ipa: raspberrypi: Rationalise parameters to ipa::configure()

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Wed, Mar 17, 2021 at 10:02:11AM +0000, Naushir Patuck wrote:
> Rename ConfigInput to IPAConfig to be more consistent with the naming,
> and remove ConfigInput::op, as it is never used.
> 
> Replace ConfigOutput with a ControlList type, as that is the only return
> type from ipa::configure().
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom          | 16 +++++-----------
>  src/ipa/raspberrypi/raspberrypi.cpp              | 13 ++++++-------
>  .../pipeline/raspberrypi/raspberrypi.cpp         | 13 +++++--------
>  3 files changed, 16 insertions(+), 26 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index 7549397a27c4..f38c22611cc4 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -29,17 +29,11 @@ struct ISPConfig {
>  	ControlList controls;
>  };
>  
> -struct ConfigInput {
> -	uint32 op;
> +struct IPAConfig {
>  	uint32 transform;
>  	FileDescriptor lsTableHandle;
>  };
>  
> -struct ConfigOutput {
> -	uint32 params;

Removal of params could be squashed with 2/7.

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

> -	ControlList controls;
> -};
> -
>  struct StartConfig {
>  	ControlList controls;
>  	int32 dropFrameCount;
> @@ -57,7 +51,7 @@ interface IPARPiInterface {
>  	 * \param[in] streamConfig Configuration of all active streams
>  	 * \param[in] entityControls Controls provided by the pipeline entities
>  	 * \param[in] ipaConfig Pipeline-handler-specific configuration data
> -	 * \param[out] results Pipeline-handler-specific configuration result
> +	 * \param[out] controls Controls to apply by the pipeline entity
>  	 *
>  	 * This method shall be called when the camera is configured to inform
>  	 * the IPA of the camera's streams and the sensor settings.
> @@ -65,14 +59,14 @@ interface IPARPiInterface {
>  	 * The \a sensorInfo conveys information about the camera sensor settings that
>  	 * the pipeline handler has selected for the configuration.
>  	 *
> -	 * The \a ipaConfig and \a results parameters carry data passed by the
> +	 * The \a ipaConfig and \a controls parameters carry data passed by the
>  	 * pipeline handler to the IPA and back.
>  	 */
>  	configure(CameraSensorInfo sensorInfo,
>  		  map<uint32, IPAStream> streamConfig,
>  		  map<uint32, ControlInfoMap> entityControls,
> -		  ConfigInput ipaConfig)
> -		=> (int32 ret, ConfigOutput results);
> +		  IPAConfig ipaConfig)
> +		=> (int32 ret, ControlList controls);
>  
>  	/**
>  	 * \fn mapBuffers()
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 417a7922c3bd..1c928b72fbd0 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -85,8 +85,8 @@ public:
>  	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;
> +		      const ipa::RPi::IPAConfig &data,
> +		      ControlList *controls) 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;
> @@ -313,16 +313,14 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
>  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)
> +		      const ipa::RPi::IPAConfig &ipaConfig,
> +		      ControlList *controls)
>  {
>  	if (entityControls.size() != 2) {
>  		LOG(IPARPI, Error) << "No ISP or sensor controls found.";
>  		return -1;
>  	}
>  
> -	result->params = 0;
> -
>  	sensorCtrls_ = entityControls.at(0);
>  	ispCtrls_ = entityControls.at(1);
>  
> @@ -379,7 +377,8 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		agcStatus.analogue_gain = DefaultAnalogueGain;
>  		applyAGC(&agcStatus, ctrls);
>  
> -		result->controls = std::move(ctrls);
> +		ASSERT(controls);
> +		*controls = std::move(ctrls);
>  	}
>  
>  	return 0;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 16568d56f31c..1f8307a3c1c5 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1240,7 +1240,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  
>  	std::map<unsigned int, IPAStream> streamConfig;
>  	std::map<unsigned int, ControlInfoMap> entityControls;
> -	ipa::RPi::ConfigInput ipaConfig;
> +	ipa::RPi::IPAConfig ipaConfig;
>  
>  	/* Get the device format to pass to the IPA. */
>  	V4L2DeviceFormat sensorFormat;
> @@ -1283,19 +1283,16 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	}
>  
>  	/* Ready the IPA - it must know about the sensor resolution. */
> -	ipa::RPi::ConfigOutput result;
> -
> +	ControlList controls;
>  	ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> -			      &result);
> +			      &controls);
>  	if (ret < 0) {
>  		LOG(RPI, Error) << "IPA configuration failed!";
>  		return -EPIPE;
>  	}
>  
> -	if (!result.controls.empty()) {
> -		ControlList &ctrls = result.controls;
> -		unicam_[Unicam::Image].dev()->setControls(&ctrls);
> -	}
> +	if (!controls.empty())
> +		unicam_[Unicam::Image].dev()->setControls(&controls);
>  
>  	/*
>  	 * Configure the H/V flip controls based on the combination of

-- 
Regards,

Laurent Pinchart