[libcamera-devel,v3,10/17] ipa: rkisp1: Align configure() with IPU3
diff mbox series

Message ID 20220818094410.1671-11-jacopo@jmondi.org
State New
Headers show
Series
  • libcamera: Align IPU3 and RKISP1 interfaces
Related show

Commit Message

Jacopo Mondi Aug. 18, 2022, 9:44 a.m. UTC
The RkISP1 IPA::configure() function signature requires a map of
entities controls and a map of stream configuration to be provided by
the pipeline handler to the IPA.

This design comes from the early days when the first IPA module was
implemented. With the introduction of mojom-defined IPA interfaces it's
now easier to define custom structures to group parameters together, as
the IPU3 IPA does.

Align the IPU3 and RkISP1 IPA interfaces to use the same function
signature and introduce rkisp1::IPAConfigInfo for the purpose of
grouping configuration parameters together.

Update the implementation of the RkISP1 IPA to validate the supplied
list of controls and update the session configuration in separate
functions.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/ipa/rkisp1.mojom       | 10 ++-
 src/ipa/rkisp1/rkisp1.cpp                | 98 ++++++++++++++----------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 +++---
 3 files changed, 74 insertions(+), 59 deletions(-)

Patch
diff mbox series

diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
index eaf3955e4096..7efe17746804 100644
--- a/include/libcamera/ipa/rkisp1.mojom
+++ b/include/libcamera/ipa/rkisp1.mojom
@@ -8,6 +8,12 @@  module ipa.rkisp1;
 
 import "include/libcamera/ipa/core.mojom";
 
+struct IPAConfigInfo {
+	libcamera.IPACameraSensorInfo sensorInfo;
+	libcamera.ControlInfoMap sensorControls;
+	libcamera.ControlInfoMap lensControls;
+};
+
 interface IPARkISP1Interface {
 	init(libcamera.IPASettings settings,
 	     uint32 hwRevision)
@@ -15,9 +21,7 @@  interface IPARkISP1Interface {
 	start() => (int32 ret);
 	stop();
 
-	configure(libcamera.IPACameraSensorInfo sensorInfo,
-		  map<uint32, libcamera.IPAStream> streamConfig,
-		  map<uint32, libcamera.ControlInfoMap> entityControls)
+	configure(IPAConfigInfo configInfo)
 		=> (int32 ret);
 
 	mapBuffers(array<libcamera.IPABuffer> buffers);
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 3a98aaf75d98..f2075c893d29 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -49,9 +49,7 @@  public:
 	int start() override;
 	void stop() override;
 
-	int configure(const IPACameraSensorInfo &info,
-		      const std::map<uint32_t, IPAStream> &streamConfig,
-		      const std::map<uint32_t, ControlInfoMap> &entityControls) override;
+	int configure(const IPAConfigInfo &configInfo) override;
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
 
@@ -64,6 +62,9 @@  protected:
 	std::string logPrefix() const override;
 
 private:
+	void updateSessionConfiguration(const IPAConfigInfo &configInfo);
+	bool validateSensorControls(const ControlInfoMap &sensorControls);
+
 	void setControls(unsigned int frame);
 	void prepareMetadata(unsigned int frame, unsigned int aeState);
 
@@ -193,44 +194,18 @@  void IPARkISP1::stop()
 	context_.frameContexts.clear();
 }
 
-/**
- * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
- * if the connected sensor does not provide enough information to properly
- * assemble one. Make sure the reported sensor information are relevant
- * before accessing them.
- */
-int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
-			 [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,
-			 const std::map<uint32_t, ControlInfoMap> &entityControls)
+void IPARkISP1::updateSessionConfiguration(const IPAConfigInfo &configInfo)
 {
-	if (entityControls.empty())
-		return -EINVAL;
-
-	sensorCtrls_ = entityControls.at(0);
-
-	auto lensControls = entityControls.find(1);
-	if (lensControls != entityControls.end())
-		lensCtrls_ = lensControls->second;
+	const IPACameraSensorInfo &sensorInfo = configInfo.sensorInfo;
+	const ControlInfoMap &sensorControls = configInfo.sensorControls;
 
-	const auto itExp = sensorCtrls_.find(V4L2_CID_EXPOSURE);
-	if (itExp == sensorCtrls_.end()) {
-		LOG(IPARkISP1, Error) << "Can't find exposure control";
-		return -EINVAL;
-	}
-
-	const auto itGain = sensorCtrls_.find(V4L2_CID_ANALOGUE_GAIN);
-	if (itGain == sensorCtrls_.end()) {
-		LOG(IPARkISP1, Error) << "Can't find gain control";
-		return -EINVAL;
-	}
+	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
+	const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;
+	int32_t minExposure = v4l2Exposure.min().get<int32_t>();
+	int32_t maxExposure = v4l2Exposure.max().get<int32_t>();
 
-	autoExposure_ = true;
-
-	int32_t minExposure = itExp->second.min().get<int32_t>();
-	int32_t maxExposure = itExp->second.max().get<int32_t>();
-
-	int32_t minGain = itGain->second.min().get<int32_t>();
-	int32_t maxGain = itGain->second.max().get<int32_t>();
+	int32_t minGain = v4l2Gain.min().get<int32_t>();
+	int32_t maxGain = v4l2Gain.max().get<int32_t>();
 
 	LOG(IPARkISP1, Info)
 		<< "Exposure: " << minExposure << "-" << maxExposure
@@ -243,8 +218,9 @@  int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
 	/* Set the hardware revision for the algorithms. */
 	context_.configuration.hw.revision = hwRevision_;
 
-	context_.configuration.sensor.size = info.outputSize;
-	context_.configuration.sensor.lineDuration = info.lineLength * 1.0s / info.pixelRate;
+	context_.configuration.sensor.size = sensorInfo.outputSize;
+	context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s
+						   / sensorInfo.pixelRate;
 
 	/*
 	 * When the AGC computes the new exposure values for a frame, it needs
@@ -259,9 +235,49 @@  int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
 	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
 
 	context_.activeState.frameCount = 0;
+}
+
+bool IPARkISP1::validateSensorControls(const ControlInfoMap &sensorControls)
+{
+	static const uint32_t ctrls[] = {
+		V4L2_CID_ANALOGUE_GAIN,
+		V4L2_CID_EXPOSURE,
+	};
+
+	for (auto c : ctrls) {
+		if (sensorControls.find(c) == sensorControls.end()) {
+			LOG(IPARkISP1, Error) << "Unable to find sensor control "
+					      << utils::hex(c);
+			return false;
+		}
+	}
+
+	return true;
+
+}
+
+/**
+ * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo
+ * if the connected sensor does not provide enough information to properly
+ * assemble one. Make sure the reported sensor information are relevant
+ * before accessing them.
+ */
+int IPARkISP1::configure(const IPAConfigInfo &configInfo)
+{
+	if (!validateSensorControls(configInfo.sensorControls)) {
+		LOG(IPARkISP1, Error) << "Sensor control validation failed.";
+		return -EINVAL;
+	}
+
+	sensorCtrls_ = configInfo.sensorControls;
+	lensCtrls_ = configInfo.lensControls;
+
+	autoExposure_ = true;
+
+	updateSessionConfiguration(configInfo);
 
 	for (auto const &algo : algorithms()) {
-		int ret = algo->configure(context_, info);
+		int ret = algo->configure(context_, configInfo.sensorInfo);
 		if (ret)
 			return ret;
 	}
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 5f10c26bcb4d..3b250b0ae346 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -651,20 +651,13 @@  int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
 		<< "ISP output pad configured with " << format
 		<< " crop " << rect;
 
-	std::map<unsigned int, IPAStream> streamConfig;
-
 	for (const StreamConfiguration &cfg : *config) {
-		if (cfg.stream() == &data->mainPathStream_) {
+		if (cfg.stream() == &data->mainPathStream_)
 			ret = mainPath_.configure(cfg, format);
-			streamConfig[0] = IPAStream(cfg.pixelFormat,
-						    cfg.size);
-		} else if (hasSelfPath_) {
+		else if (hasSelfPath_)
 			ret = selfPath_.configure(cfg, format);
-			streamConfig[1] = IPAStream(cfg.pixelFormat,
-						    cfg.size);
-		} else {
+		else
 			return -ENODEV;
-		}
 
 		if (ret)
 			return ret;
@@ -682,7 +675,9 @@  int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
 	if (ret)
 		return ret;
 
-	/* Inform IPA of stream configuration and sensor controls. */
+	/* Configure the IPA module. */
+	ipa::rkisp1::IPAConfigInfo configInfo;
+
 	IPACameraSensorInfo sensorInfo = {};
 	ret = data->sensor_->sensorInfo(&sensorInfo);
 	if (ret) {
@@ -692,14 +687,14 @@  int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
 		ret = 0;
 	}
 
-	std::map<uint32_t, ControlInfoMap> entityControls;
-	entityControls.emplace(0, data->sensor_->controls());
+	configInfo.sensorInfo = sensorInfo;
+	configInfo.sensorControls = data->sensor_->controls();
 
 	CameraLens *lens = data->sensor_->focusLens();
 	if (lens)
-		entityControls.emplace(1, lens->controls());
+		configInfo.lensControls = lens->controls();
 
-	ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);
+	ret = data->ipa_->configure(configInfo);
 	if (ret) {
 		LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
 		return ret;