[libcamera-devel,v2,4/5] libcamera: ipu3: Initialize controls in the IPA
diff mbox series

Message ID 20210728161116.64489-5-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: Initialize controls in the IPA
Related show

Commit Message

Jacopo Mondi July 28, 2021, 4:11 p.m. UTC
All the IPU3 Camera controls are currently initialized by the pipeline
handler which initializes them using the camera sensor configuration and
platform specific requirements.

However, some controls are better initialized by the IPA, which might,
in example, cap the exposure times and frame duration to the constraints
of its algorithms implementation.

Also, moving forward, the IPA should register controls to report its
capabilities, in example the ability to enable/disable 3A algorithms on
request.

Move the existing controls initialization to the IPA, by providing
the sensor configuration and its controls to the IPU3IPA::init()
function, which initializes controls and returns them to the pipeline
through an output parameter.

The existing controls initialization has been copied verbatim from the
pipeline handler to the IPA, if not a for few line breaks adjustments
and the resulting Camera controls values are not changed .

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/ipa/ipu3.mojom     |   9 ++-
 src/ipa/ipu3/ipu3.cpp                |  71 +++++++++++++++++--
 src/libcamera/pipeline/ipu3/ipu3.cpp | 100 ++++++++++++---------------
 3 files changed, 121 insertions(+), 59 deletions(-)

Comments

Paul Elder Aug. 3, 2021, 10:02 a.m. UTC | #1
Hi Jacopo,

On Wed, Jul 28, 2021 at 06:11:15PM +0200, Jacopo Mondi wrote:
> All the IPU3 Camera controls are currently initialized by the pipeline
> handler which initializes them using the camera sensor configuration and
> platform specific requirements.
> 
> However, some controls are better initialized by the IPA, which might,
> in example, cap the exposure times and frame duration to the constraints
> of its algorithms implementation.
> 
> Also, moving forward, the IPA should register controls to report its
> capabilities, in example the ability to enable/disable 3A algorithms on
> request.
> 
> Move the existing controls initialization to the IPA, by providing
> the sensor configuration and its controls to the IPU3IPA::init()
> function, which initializes controls and returns them to the pipeline
> through an output parameter.
> 
> The existing controls initialization has been copied verbatim from the
> pipeline handler to the IPA, if not a for few line breaks adjustments
> and the resulting Camera controls values are not changed .
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

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

> ---
>  include/libcamera/ipa/ipu3.mojom     |   9 ++-
>  src/ipa/ipu3/ipu3.cpp                |  71 +++++++++++++++++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 100 ++++++++++++---------------
>  3 files changed, 121 insertions(+), 59 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 911a3a072464..5fb53d0fcc4f 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -30,6 +30,12 @@ struct IPU3Action {
>  	libcamera.ControlList controls;
>  };
>  
> +struct IPAInitInfo {
> +	libcamera.IPASettings settings;
> +	libcamera.IPACameraSensorInfo sensorInfo;
> +	libcamera.ControlInfoMap sensorControls;
> +};
> +
>  struct IPAConfigInfo {
>  	libcamera.IPACameraSensorInfo sensorInfo;
>  	map<uint32, libcamera.ControlInfoMap> entityControls;
> @@ -38,7 +44,8 @@ struct IPAConfigInfo {
>  };
>  
>  interface IPAIPU3Interface {
> -	init(libcamera.IPASettings settings) => (int32 ret);
> +	init(IPAInitInfo initInfo)
> +		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
>  	start() => (int32 ret);
>  	stop();
>  
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 71698d36e50f..7a0c89ecb5fa 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -5,8 +5,10 @@
>   * ipu3.cpp - IPU3 Image Processing Algorithms
>   */
>  
> +#include <array>
>  #include <stdint.h>
>  #include <sys/mman.h>
> +#include <utility>
>  
>  #include <linux/intel-ipu3.h>
>  #include <linux/v4l2-controls.h>
> @@ -38,7 +40,8 @@ namespace ipa::ipu3 {
>  class IPAIPU3 : public IPAIPU3Interface
>  {
>  public:
> -	int init(const IPASettings &settings) override;
> +	int init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;
> +
>  	int start() override;
>  	void stop() override {}
>  
> @@ -86,14 +89,74 @@ private:
>  	struct ipu3_uapi_grid_config bdsGrid_;
>  };
>  
> -int IPAIPU3::init(const IPASettings &settings)
> +/**
> + * 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 IPAInitInfo &initInfo, ControlInfoMap *ipaControls)
>  {
> -	camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
> +	const std::string &sensorModel = initInfo.settings.sensorModel;
> +	/* Initialize the camera sensor helper. */
> +	camHelper_ = CameraSensorHelperFactory::create(sensorModel);
>  	if (camHelper_ == nullptr) {
> -		LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for " << settings.sensorModel;
> +		LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for "
> +				    << sensorModel;
>  		return -ENODEV;
>  	}
>  
> +	/* Initialize Controls. */
> +	const ControlInfoMap &sensorControls = initInfo.sensorControls;
> +	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.
> +	 */
> +	const IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;
> +	double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);
> +	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> +	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
> +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
> +	int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration;
> +	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> +							defExposure);
> +
> +	/*
> +	 * Compute the frame duration limits.
> +	 *
> +	 * The frame length is computed assuming a fixed line length combined
> +	 * with the vertical frame sizes.
> +	 */
> +	const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
> +	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> +	uint32_t lineLength = sensorInfo.outputSize.width + hblank;
> +
> +	const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
> +	std::array<uint32_t, 3> frameHeights{
> +		v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,
> +		v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
> +		v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
> +	};
> +
> +	std::array<int64_t, 3> frameDurations;
> +	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
> +		uint64_t frameSize = lineLength * frameHeights[i];
> +		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> +	}
> +
> +	controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
> +							       frameDurations[1],
> +							       frameDurations[2]);
> +
> +	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> +
>  	return 0;
>  }
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 048993365b44..91fc1f7dc9b7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -88,6 +88,8 @@ public:
>  
>  	std::queue<Request *> pendingRequests_;
>  
> +	ControlInfoMap ipaControls_;
> +
>  private:
>  	void queueFrameAction(unsigned int id,
>  			      const ipa::ipu3::IPU3Action &action);
> @@ -940,7 +942,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  		return ret;
>  
>  	ControlInfoMap::Map controls = IPU3Controls;
> -	const ControlInfoMap &sensorControls = sensor->controls();
>  	const std::vector<int32_t> &testPatternModes = sensor->testPatternModes();
>  	if (!testPatternModes.empty()) {
>  		std::vector<ControlValue> values;
> @@ -952,58 +953,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  		controls[&controls::draft::TestPatternMode] = ControlInfo(values);
>  	}
>  
> -	/*
> -	 * 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.
> -	 */
> -	double lineDuration = sensorInfo.lineLength
> -			    / (sensorInfo.pixelRate / 1e6);
> -	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> -	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
> -	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
> -	int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration;
> -
> -	/*
> -	 * \todo Report the actual exposure time, use the default for the
> -	 * moment.
> -	 */
> -	data->exposureTime_ = defExposure;
> -
> -	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> -							defExposure);
> -
> -	/*
> -	 * Compute the frame duration limits.
> -	 *
> -	 * The frame length is computed assuming a fixed line length combined
> -	 * with the vertical frame sizes.
> -	 */
> -	const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
> -	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> -	uint32_t lineLength = sensorInfo.outputSize.width + hblank;
> -
> -	const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
> -	std::array<uint32_t, 3> frameHeights{
> -		v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,
> -		v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
> -		v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
> -	};
> -
> -	std::array<int64_t, 3> frameDurations;
> -	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
> -		uint64_t frameSize = lineLength * frameHeights[i];
> -		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> -	}
> -
> -	controls[&controls::FrameDurationLimits] =
> -		ControlInfo(frameDurations[0],
> -			    frameDurations[1],
> -			    frameDurations[2]);
> -
>  	/*
>  	 * Compute the scaler crop limits.
>  	 *
> @@ -1057,9 +1006,14 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  
>  	controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
>  
> +	/* Add the IPA registered controls to list of camera controls. */
> +	for (const auto &ipaControl : data->ipaControls_)
> +		controls[ipaControl.first] = ipaControl.second;
> +
>  	data->controlInfo_ = ControlInfoMap(std::move(controls),
>  					    controls::controls);
>  
> +
>  	return 0;
>  }
>  
> @@ -1209,13 +1163,51 @@ int IPU3CameraData::loadIPA()
>  
>  	ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);
>  
> +	/*
> +	 * Pass the sensor info to the IPA to initialize controls.
> +	 *
> +         * \todo Find a way to initialize IPA controls without basing their
> +         * limits on a particular sensor mode. We currently pass sensor
> +         * information corresponding to the largest sensor resolution, and the
> +         * IPA uses this to compute limits for supported controls. There's a
> +         * discrepancy between the need to compute IPA control limits at init
> +         * time, and the fact that those limits may depend on the sensor mode.
> +         * Research is required to find out to handle this issue.
> +         */
>  	CameraSensor *sensor = cio2_.sensor();
> -	int ret = ipa_->init(IPASettings{ "", sensor->model() });
> +	V4L2SubdeviceFormat sensorFormat = {};
> +	sensorFormat.size = sensor->resolution();
> +	int ret = sensor->setFormat(&sensorFormat);
> +	if (ret)
> +		return ret;
> +
> +	IPACameraSensorInfo sensorInfo{};
> +	ret = sensor->sensorInfo(&sensorInfo);
> +	if (ret)
> +		return ret;
> +
> +	ipa::ipu3::IPAInitInfo initInfo{
> +		{ "", sensor->model() },
> +		sensorInfo,
> +		sensor->controls(),
> +	};
> +	ret = ipa_->init(initInfo, &ipaControls_);
>  	if (ret) {
>  		LOG(IPU3, Error) << "Failed to initialise the IPU3 IPA";
>  		return ret;
>  	}
>  
> +	/*
> +	 * \todo Report the actual exposure time, use the default for the
> +	 * moment.
> +	 */
> +	const auto exposureInfo = ipaControls_.find(&controls::ExposureTime);
> +	if (exposureInfo == ipaControls_.end()) {
> +		LOG(IPU3, Error) << "Exposure control not initializaed by the IPA";
> +		return -EINVAL;
> +	}
> +	exposureTime_ = exposureInfo->second.def().get<int32_t>();
> +
>  	return 0;
>  }
>  
> -- 
> 2.32.0
>
Laurent Pinchart Aug. 3, 2021, 4:48 p.m. UTC | #2
Hi Jacopo,

Thank you for the patch.

On Wed, Jul 28, 2021 at 06:11:15PM +0200, Jacopo Mondi wrote:
> All the IPU3 Camera controls are currently initialized by the pipeline
> handler which initializes them using the camera sensor configuration and
> platform specific requirements.
> 
> However, some controls are better initialized by the IPA, which might,
> in example, cap the exposure times and frame duration to the constraints
> of its algorithms implementation.
> 
> Also, moving forward, the IPA should register controls to report its
> capabilities, in example the ability to enable/disable 3A algorithms on
> request.
> 
> Move the existing controls initialization to the IPA, by providing
> the sensor configuration and its controls to the IPU3IPA::init()
> function, which initializes controls and returns them to the pipeline
> through an output parameter.
> 
> The existing controls initialization has been copied verbatim from the
> pipeline handler to the IPA, if not a for few line breaks adjustments
> and the resulting Camera controls values are not changed .

s/ .$/./

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/ipa/ipu3.mojom     |   9 ++-
>  src/ipa/ipu3/ipu3.cpp                |  71 +++++++++++++++++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 100 ++++++++++++---------------
>  3 files changed, 121 insertions(+), 59 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> index 911a3a072464..5fb53d0fcc4f 100644
> --- a/include/libcamera/ipa/ipu3.mojom
> +++ b/include/libcamera/ipa/ipu3.mojom
> @@ -30,6 +30,12 @@ struct IPU3Action {
>  	libcamera.ControlList controls;
>  };
>  
> +struct IPAInitInfo {
> +	libcamera.IPASettings settings;
> +	libcamera.IPACameraSensorInfo sensorInfo;
> +	libcamera.ControlInfoMap sensorControls;
> +};
> +
>  struct IPAConfigInfo {
>  	libcamera.IPACameraSensorInfo sensorInfo;
>  	map<uint32, libcamera.ControlInfoMap> entityControls;
> @@ -38,7 +44,8 @@ struct IPAConfigInfo {
>  };
>  
>  interface IPAIPU3Interface {
> -	init(libcamera.IPASettings settings) => (int32 ret);
> +	init(IPAInitInfo initInfo)

By the way, there's no requirement for functions to take a single
parameter, so this could also be written

	init(libcamera.IPASettings settings,
	     libcamera.IPACameraSensorInfo sensorInfo,
	     libcamera.ControlInfoMap sensorControls)

if you think the resulting code would be better.

> +		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
>  	start() => (int32 ret);
>  	stop();
>  
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 71698d36e50f..7a0c89ecb5fa 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -5,8 +5,10 @@
>   * ipu3.cpp - IPU3 Image Processing Algorithms
>   */
>  
> +#include <array>
>  #include <stdint.h>
>  #include <sys/mman.h>
> +#include <utility>
>  
>  #include <linux/intel-ipu3.h>
>  #include <linux/v4l2-controls.h>
> @@ -38,7 +40,8 @@ namespace ipa::ipu3 {
>  class IPAIPU3 : public IPAIPU3Interface
>  {
>  public:
> -	int init(const IPASettings &settings) override;
> +	int init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;
> +
>  	int start() override;
>  	void stop() override {}
>  
> @@ -86,14 +89,74 @@ private:
>  	struct ipu3_uapi_grid_config bdsGrid_;
>  };
>  
> -int IPAIPU3::init(const IPASettings &settings)
> +/**
> + * 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 IPAInitInfo &initInfo, ControlInfoMap *ipaControls)
>  {
> -	camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
> +	const std::string &sensorModel = initInfo.settings.sensorModel;

Nitpicking, I'd move this after the comment, it feels weird to have the
comment tightly hugged by two lines.

> +	/* Initialize the camera sensor helper. */
> +	camHelper_ = CameraSensorHelperFactory::create(sensorModel);
>  	if (camHelper_ == nullptr) {
> -		LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for " << settings.sensorModel;
> +		LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for "
> +				    << sensorModel;
>  		return -ENODEV;
>  	}
>  
> +	/* Initialize Controls. */
> +	const ControlInfoMap &sensorControls = initInfo.sensorControls;
> +	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.
> +	 */
> +	const IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;
> +	double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);
> +	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> +	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
> +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
> +	int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration;
> +	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> +							defExposure);
> +
> +	/*
> +	 * Compute the frame duration limits.
> +	 *
> +	 * The frame length is computed assuming a fixed line length combined
> +	 * with the vertical frame sizes.
> +	 */
> +	const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
> +	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> +	uint32_t lineLength = sensorInfo.outputSize.width + hblank;
> +
> +	const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
> +	std::array<uint32_t, 3> frameHeights{
> +		v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,
> +		v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
> +		v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
> +	};
> +
> +	std::array<int64_t, 3> frameDurations;
> +	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
> +		uint64_t frameSize = lineLength * frameHeights[i];
> +		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> +	}
> +
> +	controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
> +							       frameDurations[1],
> +							       frameDurations[2]);
> +
> +	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> +
>  	return 0;
>  }
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 048993365b44..91fc1f7dc9b7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -88,6 +88,8 @@ public:
>  
>  	std::queue<Request *> pendingRequests_;
>  
> +	ControlInfoMap ipaControls_;
> +
>  private:
>  	void queueFrameAction(unsigned int id,
>  			      const ipa::ipu3::IPU3Action &action);
> @@ -940,7 +942,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  		return ret;
>  
>  	ControlInfoMap::Map controls = IPU3Controls;
> -	const ControlInfoMap &sensorControls = sensor->controls();
>  	const std::vector<int32_t> &testPatternModes = sensor->testPatternModes();
>  	if (!testPatternModes.empty()) {
>  		std::vector<ControlValue> values;
> @@ -952,58 +953,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  		controls[&controls::draft::TestPatternMode] = ControlInfo(values);
>  	}
>  
> -	/*
> -	 * 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.
> -	 */
> -	double lineDuration = sensorInfo.lineLength
> -			    / (sensorInfo.pixelRate / 1e6);
> -	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> -	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
> -	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
> -	int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration;
> -
> -	/*
> -	 * \todo Report the actual exposure time, use the default for the
> -	 * moment.
> -	 */
> -	data->exposureTime_ = defExposure;
> -
> -	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> -							defExposure);
> -
> -	/*
> -	 * Compute the frame duration limits.
> -	 *
> -	 * The frame length is computed assuming a fixed line length combined
> -	 * with the vertical frame sizes.
> -	 */
> -	const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
> -	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> -	uint32_t lineLength = sensorInfo.outputSize.width + hblank;
> -
> -	const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
> -	std::array<uint32_t, 3> frameHeights{
> -		v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,
> -		v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
> -		v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
> -	};
> -
> -	std::array<int64_t, 3> frameDurations;
> -	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
> -		uint64_t frameSize = lineLength * frameHeights[i];
> -		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> -	}
> -
> -	controls[&controls::FrameDurationLimits] =
> -		ControlInfo(frameDurations[0],
> -			    frameDurations[1],
> -			    frameDurations[2]);
> -
>  	/*
>  	 * Compute the scaler crop limits.
>  	 *
> @@ -1057,9 +1006,14 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  
>  	controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
>  
> +	/* Add the IPA registered controls to list of camera controls. */
> +	for (const auto &ipaControl : data->ipaControls_)
> +		controls[ipaControl.first] = ipaControl.second;
> +
>  	data->controlInfo_ = ControlInfoMap(std::move(controls),
>  					    controls::controls);
>  
> +

Extra blank line.

>  	return 0;
>  }
>  
> @@ -1209,13 +1163,51 @@ int IPU3CameraData::loadIPA()
>  
>  	ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);
>  
> +	/*
> +	 * Pass the sensor info to the IPA to initialize controls.
> +	 *
> +         * \todo Find a way to initialize IPA controls without basing their
> +         * limits on a particular sensor mode. We currently pass sensor
> +         * information corresponding to the largest sensor resolution, and the
> +         * IPA uses this to compute limits for supported controls. There's a
> +         * discrepancy between the need to compute IPA control limits at init
> +         * time, and the fact that those limits may depend on the sensor mode.
> +         * Research is required to find out to handle this issue.

s/        /\t/

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

> +         */
>  	CameraSensor *sensor = cio2_.sensor();
> -	int ret = ipa_->init(IPASettings{ "", sensor->model() });
> +	V4L2SubdeviceFormat sensorFormat = {};
> +	sensorFormat.size = sensor->resolution();
> +	int ret = sensor->setFormat(&sensorFormat);
> +	if (ret)
> +		return ret;
> +
> +	IPACameraSensorInfo sensorInfo{};
> +	ret = sensor->sensorInfo(&sensorInfo);
> +	if (ret)
> +		return ret;
> +
> +	ipa::ipu3::IPAInitInfo initInfo{
> +		{ "", sensor->model() },
> +		sensorInfo,
> +		sensor->controls(),
> +	};
> +	ret = ipa_->init(initInfo, &ipaControls_);
>  	if (ret) {
>  		LOG(IPU3, Error) << "Failed to initialise the IPU3 IPA";
>  		return ret;
>  	}
>  
> +	/*
> +	 * \todo Report the actual exposure time, use the default for the
> +	 * moment.
> +	 */
> +	const auto exposureInfo = ipaControls_.find(&controls::ExposureTime);
> +	if (exposureInfo == ipaControls_.end()) {
> +		LOG(IPU3, Error) << "Exposure control not initializaed by the IPA";
> +		return -EINVAL;
> +	}
> +	exposureTime_ = exposureInfo->second.def().get<int32_t>();
> +
>  	return 0;
>  }
>

Patch
diff mbox series

diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
index 911a3a072464..5fb53d0fcc4f 100644
--- a/include/libcamera/ipa/ipu3.mojom
+++ b/include/libcamera/ipa/ipu3.mojom
@@ -30,6 +30,12 @@  struct IPU3Action {
 	libcamera.ControlList controls;
 };
 
+struct IPAInitInfo {
+	libcamera.IPASettings settings;
+	libcamera.IPACameraSensorInfo sensorInfo;
+	libcamera.ControlInfoMap sensorControls;
+};
+
 struct IPAConfigInfo {
 	libcamera.IPACameraSensorInfo sensorInfo;
 	map<uint32, libcamera.ControlInfoMap> entityControls;
@@ -38,7 +44,8 @@  struct IPAConfigInfo {
 };
 
 interface IPAIPU3Interface {
-	init(libcamera.IPASettings settings) => (int32 ret);
+	init(IPAInitInfo initInfo)
+		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
 	start() => (int32 ret);
 	stop();
 
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index 71698d36e50f..7a0c89ecb5fa 100644
--- a/src/ipa/ipu3/ipu3.cpp
+++ b/src/ipa/ipu3/ipu3.cpp
@@ -5,8 +5,10 @@ 
  * ipu3.cpp - IPU3 Image Processing Algorithms
  */
 
+#include <array>
 #include <stdint.h>
 #include <sys/mman.h>
+#include <utility>
 
 #include <linux/intel-ipu3.h>
 #include <linux/v4l2-controls.h>
@@ -38,7 +40,8 @@  namespace ipa::ipu3 {
 class IPAIPU3 : public IPAIPU3Interface
 {
 public:
-	int init(const IPASettings &settings) override;
+	int init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;
+
 	int start() override;
 	void stop() override {}
 
@@ -86,14 +89,74 @@  private:
 	struct ipu3_uapi_grid_config bdsGrid_;
 };
 
-int IPAIPU3::init(const IPASettings &settings)
+/**
+ * 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 IPAInitInfo &initInfo, ControlInfoMap *ipaControls)
 {
-	camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
+	const std::string &sensorModel = initInfo.settings.sensorModel;
+	/* Initialize the camera sensor helper. */
+	camHelper_ = CameraSensorHelperFactory::create(sensorModel);
 	if (camHelper_ == nullptr) {
-		LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for " << settings.sensorModel;
+		LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for "
+				    << sensorModel;
 		return -ENODEV;
 	}
 
+	/* Initialize Controls. */
+	const ControlInfoMap &sensorControls = initInfo.sensorControls;
+	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.
+	 */
+	const IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;
+	double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);
+	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
+	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
+	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
+	int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration;
+	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
+							defExposure);
+
+	/*
+	 * Compute the frame duration limits.
+	 *
+	 * The frame length is computed assuming a fixed line length combined
+	 * with the vertical frame sizes.
+	 */
+	const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
+	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
+	uint32_t lineLength = sensorInfo.outputSize.width + hblank;
+
+	const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
+	std::array<uint32_t, 3> frameHeights{
+		v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,
+		v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
+		v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
+	};
+
+	std::array<int64_t, 3> frameDurations;
+	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
+		uint64_t frameSize = lineLength * frameHeights[i];
+		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
+	}
+
+	controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
+							       frameDurations[1],
+							       frameDurations[2]);
+
+	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
+
 	return 0;
 }
 
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 048993365b44..91fc1f7dc9b7 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -88,6 +88,8 @@  public:
 
 	std::queue<Request *> pendingRequests_;
 
+	ControlInfoMap ipaControls_;
+
 private:
 	void queueFrameAction(unsigned int id,
 			      const ipa::ipu3::IPU3Action &action);
@@ -940,7 +942,6 @@  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
 		return ret;
 
 	ControlInfoMap::Map controls = IPU3Controls;
-	const ControlInfoMap &sensorControls = sensor->controls();
 	const std::vector<int32_t> &testPatternModes = sensor->testPatternModes();
 	if (!testPatternModes.empty()) {
 		std::vector<ControlValue> values;
@@ -952,58 +953,6 @@  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
 		controls[&controls::draft::TestPatternMode] = ControlInfo(values);
 	}
 
-	/*
-	 * 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.
-	 */
-	double lineDuration = sensorInfo.lineLength
-			    / (sensorInfo.pixelRate / 1e6);
-	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
-	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
-	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
-	int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration;
-
-	/*
-	 * \todo Report the actual exposure time, use the default for the
-	 * moment.
-	 */
-	data->exposureTime_ = defExposure;
-
-	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
-							defExposure);
-
-	/*
-	 * Compute the frame duration limits.
-	 *
-	 * The frame length is computed assuming a fixed line length combined
-	 * with the vertical frame sizes.
-	 */
-	const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
-	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
-	uint32_t lineLength = sensorInfo.outputSize.width + hblank;
-
-	const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
-	std::array<uint32_t, 3> frameHeights{
-		v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,
-		v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
-		v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
-	};
-
-	std::array<int64_t, 3> frameDurations;
-	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
-		uint64_t frameSize = lineLength * frameHeights[i];
-		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
-	}
-
-	controls[&controls::FrameDurationLimits] =
-		ControlInfo(frameDurations[0],
-			    frameDurations[1],
-			    frameDurations[2]);
-
 	/*
 	 * Compute the scaler crop limits.
 	 *
@@ -1057,9 +1006,14 @@  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
 
 	controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
 
+	/* Add the IPA registered controls to list of camera controls. */
+	for (const auto &ipaControl : data->ipaControls_)
+		controls[ipaControl.first] = ipaControl.second;
+
 	data->controlInfo_ = ControlInfoMap(std::move(controls),
 					    controls::controls);
 
+
 	return 0;
 }
 
@@ -1209,13 +1163,51 @@  int IPU3CameraData::loadIPA()
 
 	ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);
 
+	/*
+	 * Pass the sensor info to the IPA to initialize controls.
+	 *
+         * \todo Find a way to initialize IPA controls without basing their
+         * limits on a particular sensor mode. We currently pass sensor
+         * information corresponding to the largest sensor resolution, and the
+         * IPA uses this to compute limits for supported controls. There's a
+         * discrepancy between the need to compute IPA control limits at init
+         * time, and the fact that those limits may depend on the sensor mode.
+         * Research is required to find out to handle this issue.
+         */
 	CameraSensor *sensor = cio2_.sensor();
-	int ret = ipa_->init(IPASettings{ "", sensor->model() });
+	V4L2SubdeviceFormat sensorFormat = {};
+	sensorFormat.size = sensor->resolution();
+	int ret = sensor->setFormat(&sensorFormat);
+	if (ret)
+		return ret;
+
+	IPACameraSensorInfo sensorInfo{};
+	ret = sensor->sensorInfo(&sensorInfo);
+	if (ret)
+		return ret;
+
+	ipa::ipu3::IPAInitInfo initInfo{
+		{ "", sensor->model() },
+		sensorInfo,
+		sensor->controls(),
+	};
+	ret = ipa_->init(initInfo, &ipaControls_);
 	if (ret) {
 		LOG(IPU3, Error) << "Failed to initialise the IPU3 IPA";
 		return ret;
 	}
 
+	/*
+	 * \todo Report the actual exposure time, use the default for the
+	 * moment.
+	 */
+	const auto exposureInfo = ipaControls_.find(&controls::ExposureTime);
+	if (exposureInfo == ipaControls_.end()) {
+		LOG(IPU3, Error) << "Exposure control not initializaed by the IPA";
+		return -EINVAL;
+	}
+	exposureTime_ = exposureInfo->second.def().get<int32_t>();
+
 	return 0;
 }