[libcamera-devel] [PATCH 03/16] ipa: ipu3: Update camera controls in configure()

[Paul Elder]

Hi Jacopo,

On Fri, Aug 27, 2021 at 02:07:44PM +0200, Jacopo Mondi wrote:
> When a new CameraConfiguration is applied to the Camera the IPA is
> configured as well, using the newly applied sensor configuration and its
> updated V4L2 controls.
> 
> Also update the Camera controls at IPA::configure() time by re-computing
> the controls::ExposureTime and controls::FrameDurationLimits limits and
> update the controls on the pipeline handler side after having configured
> the IPA.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>

Looks good to me.

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> ---
>  include/libcamera/ipa/ipu3.mojom     |  3 +-
>  src/ipa/ipu3/ipu3.cpp                | 82 +++++++++++++++++++---------
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  4 +-
>  3 files changed, 60 insertions(+), 29 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index d561c2244907..714f58ab8a42 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -45,7 +45,8 @@ interface IPAIPU3Interface {
>  	start() => (int32 ret);
>  	stop();
>  
> -	configure(IPAConfigInfo configInfo) => (int32 ret);
> +	configure(IPAConfigInfo configInfo)
> +		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
>  
>  	mapBuffers(array<libcamera.IPABuffer> buffers);
>  	unmapBuffers(array<uint32> ids);
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 0ed0a6f1a355..fc5f69ed5ddc 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -156,13 +156,17 @@ public:
>  	int start() override;
>  	void stop() override {}
>  
> -	int configure(const IPAConfigInfo &configInfo) override;
> +	int configure(const IPAConfigInfo &configInfo,
> +		      ControlInfoMap *ipaControls) override;
>  
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  	void processEvent(const IPU3Event &event) override;
>  
>  private:
> +	void updateControls(const IPACameraSensorInfo &sensorInfo,
> +			    const ControlInfoMap &sensorControls,
> +			    ControlInfoMap *ipaControls);
>  	void processControls(unsigned int frame, const ControlList &controls);
>  	void fillParams(unsigned int frame, ipu3_uapi_params *params);
>  	void parseStatistics(unsigned int frame,
> @@ -197,36 +201,30 @@ private:
>  	struct IPAContext context_;
>  };
>  
> -/**
> - * Initialize the IPA module and its controls.
> +
> +/*
> + * Compute camera controls using the sensor information and the sensor
> + * v4l2 controls.
>   *
> - * This function receives the camera sensor information from the pipeline
> - * handler, computes the limits of the controls it handles and returns
> - * them in the \a ipaControls output parameter.
> + * Some of the camera controls are computed by the pipeline handler, some others
> + * by the IPA module which is in charge of handling, in example, the exposure
> + * time and the frame duration.
> + *
> + * This functions computes:
> + * - controls::ExposureTime
> + * - controls::FrameDurationLimits
>   */
> -int IPAIPU3::init(const IPASettings &settings,
> -		  const IPACameraSensorInfo &sensorInfo,
> -		  const ControlInfoMap &sensorControls,
> -		  ControlInfoMap *ipaControls)
> +void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
> +			     const ControlInfoMap &sensorControls,
> +			     ControlInfoMap *ipaControls)
>  {
> -	camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
> -	if (camHelper_ == nullptr) {
> -		LOG(IPAIPU3, Error)
> -			<< "Failed to create camera sensor helper for "
> -			<< settings.sensorModel;
> -		return -ENODEV;
> -	}
> -
> -	/* Initialize Controls. */
>  	ControlInfoMap::Map controls{};
>  
>  	/*
> -	 * Compute exposure time limits.
> -	 *
> -	 * Initialize the control using the line length and pixel rate of the
> -	 * current configuration converted to microseconds. Use the
> -	 * V4L2_CID_EXPOSURE control to get exposure min, max and default and
> -	 * convert it from lines to microseconds.
> +	 * Compute exposure time limits by using line length and pixel rate
> +	 * converted to microseconds. Use the V4L2_CID_EXPOSURE control to get
> +	 * exposure min, max and default and convert it from lines to
> +	 * microseconds.
>  	 */
>  	double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);
>  	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> @@ -264,12 +262,36 @@ int IPAIPU3::init(const IPASettings &settings,
>  							       frameDurations[2]);
>  
>  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> +}
> +
> +/**
> + * Initialize the IPA module and its controls.
> + *
> + * This function receives the camera sensor information from the pipeline
> + * handler, computes the limits of the controls it handles and returns
> + * them in the \a ipaControls output parameter.
> + */
> +int IPAIPU3::init(const IPASettings &settings,
> +		  const IPACameraSensorInfo &sensorInfo,
> +		  const ControlInfoMap &sensorControls,
> +		  ControlInfoMap *ipaControls)
> +{
> +	camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
> +	if (camHelper_ == nullptr) {
> +		LOG(IPAIPU3, Error)
> +			<< "Failed to create camera sensor helper for "
> +			<< settings.sensorModel;
> +		return -ENODEV;
> +	}
>  
>  	/* Construct our Algorithms */
>  	algorithms_.push_back(std::make_unique<algorithms::Agc>());
>  	algorithms_.push_back(std::make_unique<algorithms::Awb>());
>  	algorithms_.push_back(std::make_unique<algorithms::ToneMapping>());
>  
> +	/* Initialize controls. */
> +	updateControls(sensorInfo, sensorControls, ipaControls);
> +
>  	return 0;
>  }
>  
> @@ -335,7 +357,8 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>  			    << (int)bdsGrid.height << " << " << (int)bdsGrid.block_height_log2 << ")";
>  }
>  
> -int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> +int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> +		       ControlInfoMap *ipaControls)
>  {
>  	if (configInfo.entityControls.empty()) {
>  		LOG(IPAIPU3, Error) << "No controls provided";
> @@ -344,6 +367,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  
>  	sensorInfo_ = configInfo.sensorInfo;
>  
> +	/*
> +	 * Compute the sensor V4L2 controls to be used by the algorithms and
> +	 * to be set on the sensor.
> +	 */
>  	ctrls_ = configInfo.entityControls.at(0);
>  
>  	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> @@ -385,6 +412,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  			return ret;
>  	}
>  
> +	/* Update the camera controls using the new sensor settings. */
> +	updateControls(sensorInfo_, ctrls_, ipaControls);
> +
>  	return 0;
>  }
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 885f5ddce139..f3ca52811df5 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -660,14 +660,14 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	configInfo.bdsOutputSize = config->imguConfig().bds;
>  	configInfo.iif = config->imguConfig().iif;
>  
> -	ret = data->ipa_->configure(configInfo);
> +	ret = data->ipa_->configure(configInfo, &data->ipaControls_);
>  	if (ret) {
>  		LOG(IPU3, Error) << "Failed to configure IPA: "
>  				 << strerror(-ret);
>  		return ret;
>  	}
>  
> -	return 0;
> +	return updateControls(data);
>  }
>  
>  int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> -- 
> 2.32.0
>