[libcamera-devel,v2] pipeline: ipa: raspberrypi: Handle failures during IPA configuration
diff mbox series

Message ID 20201127145612.1105770-1-naush@raspberrypi.com
State Accepted
Commit ee477efde8e49e26cc02437fd50a45cd439ee65e
Headers show
Series
  • [libcamera-devel,v2] pipeline: ipa: raspberrypi: Handle failures during IPA configuration
Related show

Commit Message

Naushir Patuck Nov. 27, 2020, 2:56 p.m. UTC 
If the IPA fails during configuration, return an error flag to the
pipeline handler and fail the use case gracefully.

At present, the IPA configuration can fail for the following reasons:
- The sensor is not recognised, and fails to open a CamHelper object.
- The pipeline handler did not pass in controls for the ISP and sensor.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/ipa/raspberrypi.h                |  1 +
 src/ipa/raspberrypi/raspberrypi.cpp                | 12 +++++++++++-
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 +++++
 3 files changed, 17 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Dec. 1, 2020, 1:55 a.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:
> If the IPA fails during configuration, return an error flag to the
> pipeline handler and fail the use case gracefully.
> 
> At present, the IPA configuration can fail for the following reasons:
> - The sensor is not recognised, and fails to open a CamHelper object.
> - The pipeline handler did not pass in controls for the ISP and sensor.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Wouldn't it be simpler to modify the configure() function to return an
int ? How about the following ? If it works for you I'll submit a proper
patch.

Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date:   Tue Dec 1 03:54:18 2020 +0200

    libcamera: ipa_interface: Make configure() return an int

    It's useful for the IPAInterface::configure() function to be able to
    return a status. Make it return an int, to avoid forcing IPAs to return
    a status encoded in the IPAOperationData in a custom way.

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

diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
index 8f767e844221..a00b5e7b92eb 100644
--- a/include/libcamera/internal/ipa_context_wrapper.h
+++ b/include/libcamera/internal/ipa_context_wrapper.h
@@ -22,11 +22,11 @@ public:
 	int init(const IPASettings &settings) override;
 	int start() override;
 	void stop() override;
-	void configure(const CameraSensorInfo &sensorInfo,
-		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-		       const IPAOperationData &ipaConfig,
-		       IPAOperationData *result) override;
+	int configure(const CameraSensorInfo &sensorInfo,
+		      const std::map<unsigned int, IPAStream> &streamConfig,
+		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		      const IPAOperationData &ipaConfig,
+		      IPAOperationData *result) override;

 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
index 322b7079070e..1d8cf8dc1c56 100644
--- a/include/libcamera/ipa/ipa_interface.h
+++ b/include/libcamera/ipa/ipa_interface.h
@@ -95,12 +95,12 @@ struct ipa_context_ops {
 	void (*register_callbacks)(struct ipa_context *ctx,
 				   const struct ipa_callback_ops *callbacks,
 				   void *cb_ctx);
-	void (*configure)(struct ipa_context *ctx,
-			  const struct ipa_sensor_info *sensor_info,
-			  const struct ipa_stream *streams,
-			  unsigned int num_streams,
-			  const struct ipa_control_info_map *maps,
-			  unsigned int num_maps);
+	int (*configure)(struct ipa_context *ctx,
+			 const struct ipa_sensor_info *sensor_info,
+			 const struct ipa_stream *streams,
+			 unsigned int num_streams,
+			 const struct ipa_control_info_map *maps,
+			 unsigned int num_maps);
 	void (*map_buffers)(struct ipa_context *ctx,
 			    const struct ipa_buffer *buffers,
 			    size_t num_buffers);
@@ -156,11 +156,11 @@ public:
 	virtual int start() = 0;
 	virtual void stop() = 0;

-	virtual void configure(const CameraSensorInfo &sensorInfo,
-			       const std::map<unsigned int, IPAStream> &streamConfig,
-			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-			       const IPAOperationData &ipaConfig,
-			       IPAOperationData *result) = 0;
+	virtual int configure(const CameraSensorInfo &sensorInfo,
+			      const std::map<unsigned int, IPAStream> &streamConfig,
+			      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+			      const IPAOperationData &ipaConfig,
+			      IPAOperationData *result) = 0;

 	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
 	virtual void unmapBuffers(const std::vector<unsigned int> &ids) = 0;
diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
index cee532e3a747..74d7bc6e1c9b 100644
--- a/src/ipa/libipa/ipa_interface_wrapper.cpp
+++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
@@ -115,12 +115,12 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
 	ctx->cb_ctx_ = cb_ctx;
 }

-void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
-				    const struct ipa_sensor_info *sensor_info,
-				    const struct ipa_stream *streams,
-				    unsigned int num_streams,
-				    const struct ipa_control_info_map *maps,
-				    unsigned int num_maps)
+int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
+				   const struct ipa_sensor_info *sensor_info,
+				   const struct ipa_stream *streams,
+				   unsigned int num_streams,
+				   const struct ipa_control_info_map *maps,
+				   unsigned int num_maps)
 {
 	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);

@@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,

 	/* \todo Translate the ipaConfig and result. */
 	IPAOperationData ipaConfig;
-	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
-			     nullptr);
+	return ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
+				    ipaConfig, nullptr);
 }

 void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
index a1c701599b56..acd3160039b1 100644
--- a/src/ipa/libipa/ipa_interface_wrapper.h
+++ b/src/ipa/libipa/ipa_interface_wrapper.h
@@ -30,12 +30,12 @@ private:
 	static void register_callbacks(struct ipa_context *ctx,
 				       const struct ipa_callback_ops *callbacks,
 				       void *cb_ctx);
-	static void configure(struct ipa_context *ctx,
-			      const struct ipa_sensor_info *sensor_info,
-			      const struct ipa_stream *streams,
-			      unsigned int num_streams,
-			      const struct ipa_control_info_map *maps,
-			      unsigned int num_maps);
+	static int configure(struct ipa_context *ctx,
+			     const struct ipa_sensor_info *sensor_info,
+			     const struct ipa_stream *streams,
+			     unsigned int num_streams,
+			     const struct ipa_control_info_map *maps,
+			     unsigned int num_maps);
 	static void map_buffers(struct ipa_context *ctx,
 				const struct ipa_buffer *c_buffers,
 				size_t num_buffers);
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 9853a343c892..75f7af3430ef 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -81,11 +81,11 @@ public:
 	int start() override { return 0; }
 	void stop() override {}

-	void configure(const CameraSensorInfo &sensorInfo,
-		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-		       const IPAOperationData &data,
-		       IPAOperationData *response) override;
+	int configure(const CameraSensorInfo &sensorInfo,
+		      const std::map<unsigned int, IPAStream> &streamConfig,
+		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		      const IPAOperationData &data,
+		      IPAOperationData *response) override;
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
 	void processEvent(const IPAOperationData &event) override;
@@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
 	mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;
 }

-void IPARPi::configure(const CameraSensorInfo &sensorInfo,
-		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-		       const IPAOperationData &ipaConfig,
-		       IPAOperationData *result)
+int IPARPi::configure(const CameraSensorInfo &sensorInfo,
+		      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
+		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		      const IPAOperationData &ipaConfig,
+		      IPAOperationData *result)
 {
 	if (entityControls.empty())
-		return;
+		return -EINVAL;

 	result->operation = 0;

@@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 	}

 	lastMode_ = mode_;
+	return 0;
 }

 void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 07d7f1b2ddd8..78d78c5ac521 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -40,11 +40,11 @@ public:
 	int start() override { return 0; }
 	void stop() override {}

-	void configure(const CameraSensorInfo &info,
-		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-		       const IPAOperationData &ipaConfig,
-		       IPAOperationData *response) override;
+	int configure(const CameraSensorInfo &info,
+		      const std::map<unsigned int, IPAStream> &streamConfig,
+		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		      const IPAOperationData &ipaConfig,
+		      IPAOperationData *response) override;
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
 	void processEvent(const IPAOperationData &event) override;
@@ -79,27 +79,27 @@ private:
  * assemble one. Make sure the reported sensor information are relevant
  * before accessing them.
  */
-void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
-			  [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
-			  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-			  [[maybe_unused]] const IPAOperationData &ipaConfig,
-			  [[maybe_unused]] IPAOperationData *result)
+int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
+			 [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
+			 const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+			 [[maybe_unused]] const IPAOperationData &ipaConfig,
+			 [[maybe_unused]] IPAOperationData *result)
 {
 	if (entityControls.empty())
-		return;
+		return -EINVAL;

 	ctrls_ = entityControls.at(0);

 	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
 	if (itExp == ctrls_.end()) {
 		LOG(IPARkISP1, Error) << "Can't find exposure control";
-		return;
+		return -EINVAL;
 	}

 	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
 	if (itGain == ctrls_.end()) {
 		LOG(IPARkISP1, Error) << "Can't find gain control";
-		return;
+		return -EINVAL;
 	}

 	autoExposure_ = true;
@@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
 		<< " Gain: " << minGain_ << "-" << maxGain_;

 	setControls(0);
+	return 0;
 }

 void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
index cf8411359e40..79c8c2fb8927 100644
--- a/src/ipa/vimc/vimc.cpp
+++ b/src/ipa/vimc/vimc.cpp
@@ -37,11 +37,11 @@ public:
 	int start() override;
 	void stop() override;

-	void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
-		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
-		       [[maybe_unused]] const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-		       [[maybe_unused]] const IPAOperationData &ipaConfig,
-		       [[maybe_unused]] IPAOperationData *result) override {}
+	int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
+		      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
+		      [[maybe_unused]] const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		      [[maybe_unused]] const IPAOperationData &ipaConfig,
+		      [[maybe_unused]] IPAOperationData *result) override { return 0; }
 	void mapBuffers([[maybe_unused]] const std::vector<IPABuffer> &buffers) override {}
 	void unmapBuffers([[maybe_unused]] const std::vector<unsigned int> &ids) override {}
 	void processEvent([[maybe_unused]] const IPAOperationData &event) override {}
diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
index 231300ce0bec..f63ad830c003 100644
--- a/src/libcamera/ipa_context_wrapper.cpp
+++ b/src/libcamera/ipa_context_wrapper.cpp
@@ -108,18 +108,18 @@ void IPAContextWrapper::stop()
 	ctx_->ops->stop(ctx_);
 }

-void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
-				  const std::map<unsigned int, IPAStream> &streamConfig,
-				  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-				  const IPAOperationData &ipaConfig,
-				  IPAOperationData *result)
+int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
+				 const std::map<unsigned int, IPAStream> &streamConfig,
+				 const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+				 const IPAOperationData &ipaConfig,
+				 IPAOperationData *result)
 {
 	if (intf_)
 		return intf_->configure(sensorInfo, streamConfig,
 					entityControls, ipaConfig, result);

 	if (!ctx_)
-		return;
+		return 0;

 	serializer_.reset();

@@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
 	}

 	/* \todo Translate the ipaConfig and reponse */
-	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
-			     c_info_maps, entityControls.size());
+	return ctx_->ops->configure(ctx_, &sensor_info, c_streams,
+				    streamConfig.size(), c_info_maps,
+				    entityControls.size());
 }

 void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer> &buffers)
diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
index 23fc56d7d48e..516a8ecd4b53 100644
--- a/src/libcamera/ipa_interface.cpp
+++ b/src/libcamera/ipa_interface.cpp
@@ -347,6 +347,8 @@
  * \param[in] num_maps The number of entries in the \a maps array
  *
  * \sa libcamera::IPAInterface::configure()
+ *
+ * \return 0 on success or a negative error code on failure
  */

 /**
@@ -573,6 +575,8 @@ namespace libcamera {
  * pipeline handler to the IPA and back. The pipeline handler may set the \a
  * result parameter to null if the IPA protocol doesn't need to pass a result
  * back through the configure() function.
+ *
+ * \return 0 on success or a negative error code on failure
  */

 /**
diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
index b78a0e4535f5..ed250ce79c17 100644
--- a/src/libcamera/proxy/ipa_proxy_linux.cpp
+++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
@@ -32,11 +32,11 @@ public:
 	}
 	int start() override { return 0; }
 	void stop() override {}
-	void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
-		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
-		       [[maybe_unused]] const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-		       [[maybe_unused]] const IPAOperationData &ipaConfig,
-		       [[maybe_unused]] IPAOperationData *result) override {}
+	int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
+		      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
+		      [[maybe_unused]] const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		      [[maybe_unused]] const IPAOperationData &ipaConfig,
+		      [[maybe_unused]] IPAOperationData *result) override { return 0; }
 	void mapBuffers([[maybe_unused]] const std::vector<IPABuffer> &buffers) override {}
 	void unmapBuffers([[maybe_unused]] const std::vector<unsigned int> &ids) override {}
 	void processEvent([[maybe_unused]] const IPAOperationData &event) override {}
diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
index eead2883708d..fd91726c4840 100644
--- a/src/libcamera/proxy/ipa_proxy_thread.cpp
+++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
@@ -29,11 +29,11 @@ public:
 	int start() override;
 	void stop() override;

-	void configure(const CameraSensorInfo &sensorInfo,
-		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-		       const IPAOperationData &ipaConfig,
-		       IPAOperationData *result) override;
+	int configure(const CameraSensorInfo &sensorInfo,
+		      const std::map<unsigned int, IPAStream> &streamConfig,
+		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		      const IPAOperationData &ipaConfig,
+		      IPAOperationData *result) override;
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
 	void processEvent(const IPAOperationData &event) override;
@@ -132,14 +132,14 @@ void IPAProxyThread::stop()
 	thread_.wait();
 }

-void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
-			       const std::map<unsigned int, IPAStream> &streamConfig,
-			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-			       const IPAOperationData &ipaConfig,
-			       IPAOperationData *result)
+int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
+			      const std::map<unsigned int, IPAStream> &streamConfig,
+			      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+			      const IPAOperationData &ipaConfig,
+			      IPAOperationData *result)
 {
-	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
-			result);
+	return ipa_->configure(sensorInfo, streamConfig, entityControls,
+			       ipaConfig, result);
 }

 void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
index 59d991cbbf6a..ad0fb0386865 100644
--- a/test/ipa/ipa_wrappers_test.cpp
+++ b/test/ipa/ipa_wrappers_test.cpp
@@ -67,55 +67,63 @@ public:
 		report(Op_stop, TestPass);
 	}

-	void configure(const CameraSensorInfo &sensorInfo,
-		       const std::map<unsigned int, IPAStream> &streamConfig,
-		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
-		       [[maybe_unused]] const IPAOperationData &ipaConfig,
-		       [[maybe_unused]] IPAOperationData *result) override
+	int configure(const CameraSensorInfo &sensorInfo,
+		      const std::map<unsigned int, IPAStream> &streamConfig,
+		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
+		      [[maybe_unused]] const IPAOperationData &ipaConfig,
+		      [[maybe_unused]] IPAOperationData *result) override
 	{
 		/* Verify sensorInfo. */
 		if (sensorInfo.outputSize.width != 2560 ||
 		    sensorInfo.outputSize.height != 1940) {
 			cerr << "configure(): Invalid sensor info size "
 			     << sensorInfo.outputSize.toString();
+			report(Op_configure, TestFail);
+			return -EINVAL;
 		}

 		/* Verify streamConfig. */
 		if (streamConfig.size() != 2) {
 			cerr << "configure(): Invalid number of streams "
 			     << streamConfig.size() << endl;
-			return report(Op_configure, TestFail);
+			report(Op_configure, TestFail);
+			return -EINVAL;
 		}

 		auto iter = streamConfig.find(1);
 		if (iter == streamConfig.end()) {
 			cerr << "configure(): No configuration for stream 1" << endl;
-			return report(Op_configure, TestFail);
+			report(Op_configure, TestFail);
+			return -EINVAL;
 		}
 		const IPAStream *stream = &iter->second;
 		if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||
 		    stream->size != Size{ 1024, 768 }) {
 			cerr << "configure(): Invalid configuration for stream 1" << endl;
-			return report(Op_configure, TestFail);
+			report(Op_configure, TestFail);
+			return -EINVAL;
 		}

 		iter = streamConfig.find(2);
 		if (iter == streamConfig.end()) {
 			cerr << "configure(): No configuration for stream 2" << endl;
-			return report(Op_configure, TestFail);
+			report(Op_configure, TestFail);
+			return -EINVAL;
 		}
 		stream = &iter->second;
 		if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||
 		    stream->size != Size{ 800, 600 }) {
 			cerr << "configure(): Invalid configuration for stream 2" << endl;
-			return report(Op_configure, TestFail);
+			report(Op_configure, TestFail);
+			return -EINVAL;
 		}

 		/* Verify entityControls. */
 		auto ctrlIter = entityControls.find(42);
 		if (ctrlIter == entityControls.end()) {
 			cerr << "configure(): Controls not found" << endl;
-			return report(Op_configure, TestFail);
+			report(Op_configure, TestFail);
+			return -EINVAL;
 		}

 		const ControlInfoMap &infoMap = ctrlIter->second;
@@ -124,10 +132,12 @@ public:
 		    infoMap.count(V4L2_CID_CONTRAST) != 1 ||
 		    infoMap.count(V4L2_CID_SATURATION) != 1) {
 			cerr << "configure(): Invalid control IDs" << endl;
-			return report(Op_configure, TestFail);
+			report(Op_configure, TestFail);
+			return -EINVAL;
 		}

 		report(Op_configure, TestPass);
+		return 0;
 	}

 	void mapBuffers(const std::vector<IPABuffer> &buffers) override

> ---
>  include/libcamera/ipa/raspberrypi.h                |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp                | 12 +++++++++++-
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 +++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 2b55dbfc..86c97ffa 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -21,6 +21,7 @@ enum ConfigParameters {
>  	IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
>  	IPA_CONFIG_SENSOR = (1 << 2),
>  	IPA_CONFIG_DROP_FRAMES = (1 << 3),
> +	IPA_CONFIG_FAILED = (1 << 4),
>  };
>  
>  enum Operations {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9853a343..57dd9c61 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		       const IPAOperationData &ipaConfig,
>  		       IPAOperationData *result)
>  {
> -	if (entityControls.empty())
> +	if (entityControls.size() != 2) {
> +		LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> +		result->operation = RPi::IPA_CONFIG_FAILED;
>  		return;
> +	}
>  
>  	result->operation = 0;
>  
> @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	if (!helper_) {
>  		helper_ = std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
>  
> +		if (!helper_) {
> +			LOG(IPARPI, Error) << "Could not create camera helper for "
> +					   << cameraName;
> +			result->operation = RPi::IPA_CONFIG_FAILED;
> +			return;
> +		}
> +
>  		/*
>  		 * Pass out the sensor config to the pipeline handler in order
>  		 * to setup the staggered writer class.
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 6fcdf557..76252806 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
>  			&result);
>  
> +	if (result.operation & RPi::IPA_CONFIG_FAILED) {
> +		LOG(RPI, Error) << "IPA configuration failed!";
> +		return -EPIPE;
> +	}
> +
>  	unsigned int resultIdx = 0;
>  	if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
>  		/*
Naushir Patuck Dec. 1, 2020, 9:54 a.m. UTC | #2 
Hi Laurent,

On Tue, 1 Dec 2020 at 01:55, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:
> > If the IPA fails during configuration, return an error flag to the
> > pipeline handler and fail the use case gracefully.
> >
> > At present, the IPA configuration can fail for the following reasons:
> > - The sensor is not recognised, and fails to open a CamHelper object.
> > - The pipeline handler did not pass in controls for the ISP and sensor.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Wouldn't it be simpler to modify the configure() function to return an
> int ? How about the following ? If it works for you I'll submit a proper
> patch.
>

Yes, that works equally well for me.  Happy for you to submit the patch, or
would you prefer if I add your commit below to a new patch set that will
also include the Raspberry Pi IPA/PH changes?

Regards,
Naush


>
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date:   Tue Dec 1 03:54:18 2020 +0200
>
>     libcamera: ipa_interface: Make configure() return an int
>
>     It's useful for the IPAInterface::configure() function to be able to
>     return a status. Make it return an int, to avoid forcing IPAs to return
>     a status encoded in the IPAOperationData in a custom way.
>
>     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> diff --git a/include/libcamera/internal/ipa_context_wrapper.h
> b/include/libcamera/internal/ipa_context_wrapper.h
> index 8f767e844221..a00b5e7b92eb 100644
> --- a/include/libcamera/internal/ipa_context_wrapper.h
> +++ b/include/libcamera/internal/ipa_context_wrapper.h
> @@ -22,11 +22,11 @@ public:
>         int init(const IPASettings &settings) override;
>         int start() override;
>         void stop() override;
> -       void configure(const CameraSensorInfo &sensorInfo,
> -                      const std::map<unsigned int, IPAStream>
> &streamConfig,
> -                      const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> -                      const IPAOperationData &ipaConfig,
> -                      IPAOperationData *result) override;
> +       int configure(const CameraSensorInfo &sensorInfo,
> +                     const std::map<unsigned int, IPAStream>
> &streamConfig,
> +                     const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> +                     const IPAOperationData &ipaConfig,
> +                     IPAOperationData *result) override;
>
>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> diff --git a/include/libcamera/ipa/ipa_interface.h
> b/include/libcamera/ipa/ipa_interface.h
> index 322b7079070e..1d8cf8dc1c56 100644
> --- a/include/libcamera/ipa/ipa_interface.h
> +++ b/include/libcamera/ipa/ipa_interface.h
> @@ -95,12 +95,12 @@ struct ipa_context_ops {
>         void (*register_callbacks)(struct ipa_context *ctx,
>                                    const struct ipa_callback_ops
> *callbacks,
>                                    void *cb_ctx);
> -       void (*configure)(struct ipa_context *ctx,
> -                         const struct ipa_sensor_info *sensor_info,
> -                         const struct ipa_stream *streams,
> -                         unsigned int num_streams,
> -                         const struct ipa_control_info_map *maps,
> -                         unsigned int num_maps);
> +       int (*configure)(struct ipa_context *ctx,
> +                        const struct ipa_sensor_info *sensor_info,
> +                        const struct ipa_stream *streams,
> +                        unsigned int num_streams,
> +                        const struct ipa_control_info_map *maps,
> +                        unsigned int num_maps);
>         void (*map_buffers)(struct ipa_context *ctx,
>                             const struct ipa_buffer *buffers,
>                             size_t num_buffers);
> @@ -156,11 +156,11 @@ public:
>         virtual int start() = 0;
>         virtual void stop() = 0;
>
> -       virtual void configure(const CameraSensorInfo &sensorInfo,
> -                              const std::map<unsigned int, IPAStream>
> &streamConfig,
> -                              const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> -                              const IPAOperationData &ipaConfig,
> -                              IPAOperationData *result) = 0;
> +       virtual int configure(const CameraSensorInfo &sensorInfo,
> +                             const std::map<unsigned int, IPAStream>
> &streamConfig,
> +                             const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> +                             const IPAOperationData &ipaConfig,
> +                             IPAOperationData *result) = 0;
>
>         virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
>         virtual void unmapBuffers(const std::vector<unsigned int> &ids) =
> 0;
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
> b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index cee532e3a747..74d7bc6e1c9b 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -115,12 +115,12 @@ void IPAInterfaceWrapper::register_callbacks(struct
> ipa_context *_ctx,
>         ctx->cb_ctx_ = cb_ctx;
>  }
>
> -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> -                                   const struct ipa_sensor_info
> *sensor_info,
> -                                   const struct ipa_stream *streams,
> -                                   unsigned int num_streams,
> -                                   const struct ipa_control_info_map
> *maps,
> -                                   unsigned int num_maps)
> +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> +                                  const struct ipa_sensor_info
> *sensor_info,
> +                                  const struct ipa_stream *streams,
> +                                  unsigned int num_streams,
> +                                  const struct ipa_control_info_map *maps,
> +                                  unsigned int num_maps)
>  {
>         IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
> *>(_ctx);
>
> @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct ipa_context
> *_ctx,
>
>         /* \todo Translate the ipaConfig and result. */
>         IPAOperationData ipaConfig;
> -       ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
> ipaConfig,
> -                            nullptr);
> +       return ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
> +                                   ipaConfig, nullptr);
>  }
>
>  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h
> b/src/ipa/libipa/ipa_interface_wrapper.h
> index a1c701599b56..acd3160039b1 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.h
> +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> @@ -30,12 +30,12 @@ private:
>         static void register_callbacks(struct ipa_context *ctx,
>                                        const struct ipa_callback_ops
> *callbacks,
>                                        void *cb_ctx);
> -       static void configure(struct ipa_context *ctx,
> -                             const struct ipa_sensor_info *sensor_info,
> -                             const struct ipa_stream *streams,
> -                             unsigned int num_streams,
> -                             const struct ipa_control_info_map *maps,
> -                             unsigned int num_maps);
> +       static int configure(struct ipa_context *ctx,
> +                            const struct ipa_sensor_info *sensor_info,
> +                            const struct ipa_stream *streams,
> +                            unsigned int num_streams,
> +                            const struct ipa_control_info_map *maps,
> +                            unsigned int num_maps);
>         static void map_buffers(struct ipa_context *ctx,
>                                 const struct ipa_buffer *c_buffers,
>                                 size_t num_buffers);
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9853a343c892..75f7af3430ef 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -81,11 +81,11 @@ public:
>         int start() override { return 0; }
>         void stop() override {}
>
> -       void configure(const CameraSensorInfo &sensorInfo,
> -                      const std::map<unsigned int, IPAStream>
> &streamConfig,
> -                      const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> -                      const IPAOperationData &data,
> -                      IPAOperationData *response) override;
> +       int configure(const CameraSensorInfo &sensorInfo,
> +                     const std::map<unsigned int, IPAStream>
> &streamConfig,
> +                     const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> +                     const IPAOperationData &data,
> +                     IPAOperationData *response) override;
>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>         void processEvent(const IPAOperationData &event) override;
> @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo
> &sensorInfo)
>         mode_.line_length = 1e9 * sensorInfo.lineLength /
> sensorInfo.pixelRate;
>  }
>
> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> -                      [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> -                      const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> -                      const IPAOperationData &ipaConfig,
> -                      IPAOperationData *result)
> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> +                     [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> +                     const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> +                     const IPAOperationData &ipaConfig,
> +                     IPAOperationData *result)
>  {
>         if (entityControls.empty())
> -               return;
> +               return -EINVAL;
>
>         result->operation = 0;
>
> @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
>         }
>
>         lastMode_ = mode_;
> +       return 0;
>  }
>
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 07d7f1b2ddd8..78d78c5ac521 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -40,11 +40,11 @@ public:
>         int start() override { return 0; }
>         void stop() override {}
>
> -       void configure(const CameraSensorInfo &info,
> -                      const std::map<unsigned int, IPAStream>
> &streamConfig,
> -                      const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> -                      const IPAOperationData &ipaConfig,
> -                      IPAOperationData *response) override;
> +       int configure(const CameraSensorInfo &info,
> +                     const std::map<unsigned int, IPAStream>
> &streamConfig,
> +                     const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> +                     const IPAOperationData &ipaConfig,
> +                     IPAOperationData *response) override;
>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>         void processEvent(const IPAOperationData &event) override;
> @@ -79,27 +79,27 @@ private:
>   * assemble one. Make sure the reported sensor information are relevant
>   * before accessing them.
>   */
> -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> -                         [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> -                         const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> -                         [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> -                         [[maybe_unused]] IPAOperationData *result)
> +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> +                        [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> +                        const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> +                        [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> +                        [[maybe_unused]] IPAOperationData *result)
>  {
>         if (entityControls.empty())
> -               return;
> +               return -EINVAL;
>
>         ctrls_ = entityControls.at(0);
>
>         const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>         if (itExp == ctrls_.end()) {
>                 LOG(IPARkISP1, Error) << "Can't find exposure control";
> -               return;
> +               return -EINVAL;
>         }
>
>         const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>         if (itGain == ctrls_.end()) {
>                 LOG(IPARkISP1, Error) << "Can't find gain control";
> -               return;
> +               return -EINVAL;
>         }
>
>         autoExposure_ = true;
> @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const
> CameraSensorInfo &info,
>                 << " Gain: " << minGain_ << "-" << maxGain_;
>
>         setControls(0);
> +       return 0;
>  }
>
>  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index cf8411359e40..79c8c2fb8927 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -37,11 +37,11 @@ public:
>         int start() override;
>         void stop() override;
>
> -       void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> -                      [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> -                      [[maybe_unused]] const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> -                      [[maybe_unused]] const IPAOperationData &ipaConfig,
> -                      [[maybe_unused]] IPAOperationData *result) override
> {}
> +       int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> +                     [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> +                     [[maybe_unused]] const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> +                     [[maybe_unused]] const IPAOperationData &ipaConfig,
> +                     [[maybe_unused]] IPAOperationData *result) override
> { return 0; }
>         void mapBuffers([[maybe_unused]] const std::vector<IPABuffer>
> &buffers) override {}
>         void unmapBuffers([[maybe_unused]] const std::vector<unsigned int>
> &ids) override {}
>         void processEvent([[maybe_unused]] const IPAOperationData &event)
> override {}
> diff --git a/src/libcamera/ipa_context_wrapper.cpp
> b/src/libcamera/ipa_context_wrapper.cpp
> index 231300ce0bec..f63ad830c003 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()
>         ctx_->ops->stop(ctx_);
>  }
>
> -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> -                                 const std::map<unsigned int, IPAStream>
> &streamConfig,
> -                                 const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> -                                 const IPAOperationData &ipaConfig,
> -                                 IPAOperationData *result)
> +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> +                                const std::map<unsigned int, IPAStream>
> &streamConfig,
> +                                const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> +                                const IPAOperationData &ipaConfig,
> +                                IPAOperationData *result)
>  {
>         if (intf_)
>                 return intf_->configure(sensorInfo, streamConfig,
>                                         entityControls, ipaConfig, result);
>
>         if (!ctx_)
> -               return;
> +               return 0;
>
>         serializer_.reset();
>
> @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const
> CameraSensorInfo &sensorInfo,
>         }
>
>         /* \todo Translate the ipaConfig and reponse */
> -       ctx_->ops->configure(ctx_, &sensor_info, c_streams,
> streamConfig.size(),
> -                            c_info_maps, entityControls.size());
> +       return ctx_->ops->configure(ctx_, &sensor_info, c_streams,
> +                                   streamConfig.size(), c_info_maps,
> +                                   entityControls.size());
>  }
>
>  void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/ipa_interface.cpp
> b/src/libcamera/ipa_interface.cpp
> index 23fc56d7d48e..516a8ecd4b53 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -347,6 +347,8 @@
>   * \param[in] num_maps The number of entries in the \a maps array
>   *
>   * \sa libcamera::IPAInterface::configure()
> + *
> + * \return 0 on success or a negative error code on failure
>   */
>
>  /**
> @@ -573,6 +575,8 @@ namespace libcamera {
>   * pipeline handler to the IPA and back. The pipeline handler may set the
> \a
>   * result parameter to null if the IPA protocol doesn't need to pass a
> result
>   * back through the configure() function.
> + *
> + * \return 0 on success or a negative error code on failure
>   */
>
>  /**
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
> b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index b78a0e4535f5..ed250ce79c17 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -32,11 +32,11 @@ public:
>         }
>         int start() override { return 0; }
>         void stop() override {}
> -       void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> -                      [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> -                      [[maybe_unused]] const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> -                      [[maybe_unused]] const IPAOperationData &ipaConfig,
> -                      [[maybe_unused]] IPAOperationData *result) override
> {}
> +       int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> +                     [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> +                     [[maybe_unused]] const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> +                     [[maybe_unused]] const IPAOperationData &ipaConfig,
> +                     [[maybe_unused]] IPAOperationData *result) override
> { return 0; }
>         void mapBuffers([[maybe_unused]] const std::vector<IPABuffer>
> &buffers) override {}
>         void unmapBuffers([[maybe_unused]] const std::vector<unsigned int>
> &ids) override {}
>         void processEvent([[maybe_unused]] const IPAOperationData &event)
> override {}
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
> b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index eead2883708d..fd91726c4840 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -29,11 +29,11 @@ public:
>         int start() override;
>         void stop() override;
>
> -       void configure(const CameraSensorInfo &sensorInfo,
> -                      const std::map<unsigned int, IPAStream>
> &streamConfig,
> -                      const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> -                      const IPAOperationData &ipaConfig,
> -                      IPAOperationData *result) override;
> +       int configure(const CameraSensorInfo &sensorInfo,
> +                     const std::map<unsigned int, IPAStream>
> &streamConfig,
> +                     const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> +                     const IPAOperationData &ipaConfig,
> +                     IPAOperationData *result) override;
>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>         void processEvent(const IPAOperationData &event) override;
> @@ -132,14 +132,14 @@ void IPAProxyThread::stop()
>         thread_.wait();
>  }
>
> -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> -                              const std::map<unsigned int, IPAStream>
> &streamConfig,
> -                              const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> -                              const IPAOperationData &ipaConfig,
> -                              IPAOperationData *result)
> +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> +                             const std::map<unsigned int, IPAStream>
> &streamConfig,
> +                             const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> +                             const IPAOperationData &ipaConfig,
> +                             IPAOperationData *result)
>  {
> -       ipa_->configure(sensorInfo, streamConfig, entityControls,
> ipaConfig,
> -                       result);
> +       return ipa_->configure(sensorInfo, streamConfig, entityControls,
> +                              ipaConfig, result);
>  }
>
>  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/test/ipa/ipa_wrappers_test.cpp
> b/test/ipa/ipa_wrappers_test.cpp
> index 59d991cbbf6a..ad0fb0386865 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -67,55 +67,63 @@ public:
>                 report(Op_stop, TestPass);
>         }
>
> -       void configure(const CameraSensorInfo &sensorInfo,
> -                      const std::map<unsigned int, IPAStream>
> &streamConfig,
> -                      const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> -                      [[maybe_unused]] const IPAOperationData &ipaConfig,
> -                      [[maybe_unused]] IPAOperationData *result) override
> +       int configure(const CameraSensorInfo &sensorInfo,
> +                     const std::map<unsigned int, IPAStream>
> &streamConfig,
> +                     const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> +                     [[maybe_unused]] const IPAOperationData &ipaConfig,
> +                     [[maybe_unused]] IPAOperationData *result) override
>         {
>                 /* Verify sensorInfo. */
>                 if (sensorInfo.outputSize.width != 2560 ||
>                     sensorInfo.outputSize.height != 1940) {
>                         cerr << "configure(): Invalid sensor info size "
>                              << sensorInfo.outputSize.toString();
> +                       report(Op_configure, TestFail);
> +                       return -EINVAL;
>                 }
>
>                 /* Verify streamConfig. */
>                 if (streamConfig.size() != 2) {
>                         cerr << "configure(): Invalid number of streams "
>                              << streamConfig.size() << endl;
> -                       return report(Op_configure, TestFail);
> +                       report(Op_configure, TestFail);
> +                       return -EINVAL;
>                 }
>
>                 auto iter = streamConfig.find(1);
>                 if (iter == streamConfig.end()) {
>                         cerr << "configure(): No configuration for stream
> 1" << endl;
> -                       return report(Op_configure, TestFail);
> +                       report(Op_configure, TestFail);
> +                       return -EINVAL;
>                 }
>                 const IPAStream *stream = &iter->second;
>                 if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||
>                     stream->size != Size{ 1024, 768 }) {
>                         cerr << "configure(): Invalid configuration for
> stream 1" << endl;
> -                       return report(Op_configure, TestFail);
> +                       report(Op_configure, TestFail);
> +                       return -EINVAL;
>                 }
>
>                 iter = streamConfig.find(2);
>                 if (iter == streamConfig.end()) {
>                         cerr << "configure(): No configuration for stream
> 2" << endl;
> -                       return report(Op_configure, TestFail);
> +                       report(Op_configure, TestFail);
> +                       return -EINVAL;
>                 }
>                 stream = &iter->second;
>                 if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||
>                     stream->size != Size{ 800, 600 }) {
>                         cerr << "configure(): Invalid configuration for
> stream 2" << endl;
> -                       return report(Op_configure, TestFail);
> +                       report(Op_configure, TestFail);
> +                       return -EINVAL;
>                 }
>
>                 /* Verify entityControls. */
>                 auto ctrlIter = entityControls.find(42);
>                 if (ctrlIter == entityControls.end()) {
>                         cerr << "configure(): Controls not found" << endl;
> -                       return report(Op_configure, TestFail);
> +                       report(Op_configure, TestFail);
> +                       return -EINVAL;
>                 }
>
>                 const ControlInfoMap &infoMap = ctrlIter->second;
> @@ -124,10 +132,12 @@ public:
>                     infoMap.count(V4L2_CID_CONTRAST) != 1 ||
>                     infoMap.count(V4L2_CID_SATURATION) != 1) {
>                         cerr << "configure(): Invalid control IDs" << endl;
> -                       return report(Op_configure, TestFail);
> +                       report(Op_configure, TestFail);
> +                       return -EINVAL;
>                 }
>
>                 report(Op_configure, TestPass);
> +               return 0;
>         }
>
>         void mapBuffers(const std::vector<IPABuffer> &buffers) override
>
> > ---
> >  include/libcamera/ipa/raspberrypi.h                |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp                | 12 +++++++++++-
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 +++++
> >  3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h
> b/include/libcamera/ipa/raspberrypi.h
> > index 2b55dbfc..86c97ffa 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -21,6 +21,7 @@ enum ConfigParameters {
> >       IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> >       IPA_CONFIG_SENSOR = (1 << 2),
> >       IPA_CONFIG_DROP_FRAMES = (1 << 3),
> > +     IPA_CONFIG_FAILED = (1 << 4),
> >  };
> >
> >  enum Operations {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 9853a343..57dd9c61 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >                      const IPAOperationData &ipaConfig,
> >                      IPAOperationData *result)
> >  {
> > -     if (entityControls.empty())
> > +     if (entityControls.size() != 2) {
> > +             LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > +             result->operation = RPi::IPA_CONFIG_FAILED;
> >               return;
> > +     }
> >
> >       result->operation = 0;
> >
> > @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >       if (!helper_) {
> >               helper_ =
> std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
> >
> > +             if (!helper_) {
> > +                     LOG(IPARPI, Error) << "Could not create camera
> helper for "
> > +                                        << cameraName;
> > +                     result->operation = RPi::IPA_CONFIG_FAILED;
> > +                     return;
> > +             }
> > +
> >               /*
> >                * Pass out the sensor config to the pipeline handler in
> order
> >                * to setup the staggered writer class.
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 6fcdf557..76252806 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >       ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> >                       &result);
> >
> > +     if (result.operation & RPi::IPA_CONFIG_FAILED) {
> > +             LOG(RPI, Error) << "IPA configuration failed!";
> > +             return -EPIPE;
> > +     }
> > +
> >       unsigned int resultIdx = 0;
> >       if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
> >               /*
>
> --
> Regards,
>
> Laurent Pinchart
>
Naushir Patuck Dec. 1, 2020, 9:59 a.m. UTC | #3 
Hi Laurent,

On Tue, 1 Dec 2020 at 09:54, Naushir Patuck <naush@raspberrypi.com> wrote:

> Hi Laurent,
>
> On Tue, 1 Dec 2020 at 01:55, Laurent Pinchart <
> laurent.pinchart@ideasonboard.com> wrote:
>
>> Hi Naush,
>>
>> Thank you for the patch.
>>
>> On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:
>> > If the IPA fails during configuration, return an error flag to the
>> > pipeline handler and fail the use case gracefully.
>> >
>> > At present, the IPA configuration can fail for the following reasons:
>> > - The sensor is not recognised, and fails to open a CamHelper object.
>> > - The pipeline handler did not pass in controls for the ISP and sensor.
>> >
>> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>
>> Wouldn't it be simpler to modify the configure() function to return an
>> int ? How about the following ? If it works for you I'll submit a proper
>> patch.
>>
>
> Yes, that works equally well for me.  Happy for you to submit the patch,
> or would you prefer if I add your commit below to a new patch set that will
> also include the Raspberry Pi IPA/PH changes?
>

Sorry, should have been a bit clearer.  I have two more patches (not yet
submitted for review) that are based on this work.  I could include all of
this in a new series including your patch below.  Otherwise, I am happy to
submit them separately once this has gone through.


>
> Regards,
> Naush
>
>
>>
>> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Date:   Tue Dec 1 03:54:18 2020 +0200
>>
>>     libcamera: ipa_interface: Make configure() return an int
>>
>>     It's useful for the IPAInterface::configure() function to be able to
>>     return a status. Make it return an int, to avoid forcing IPAs to
>> return
>>     a status encoded in the IPAOperationData in a custom way.
>>
>>     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> diff --git a/include/libcamera/internal/ipa_context_wrapper.h
>> b/include/libcamera/internal/ipa_context_wrapper.h
>> index 8f767e844221..a00b5e7b92eb 100644
>> --- a/include/libcamera/internal/ipa_context_wrapper.h
>> +++ b/include/libcamera/internal/ipa_context_wrapper.h
>> @@ -22,11 +22,11 @@ public:
>>         int init(const IPASettings &settings) override;
>>         int start() override;
>>         void stop() override;
>> -       void configure(const CameraSensorInfo &sensorInfo,
>> -                      const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> -                      const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> -                      const IPAOperationData &ipaConfig,
>> -                      IPAOperationData *result) override;
>> +       int configure(const CameraSensorInfo &sensorInfo,
>> +                     const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> +                     const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> +                     const IPAOperationData &ipaConfig,
>> +                     IPAOperationData *result) override;
>>
>>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>> diff --git a/include/libcamera/ipa/ipa_interface.h
>> b/include/libcamera/ipa/ipa_interface.h
>> index 322b7079070e..1d8cf8dc1c56 100644
>> --- a/include/libcamera/ipa/ipa_interface.h
>> +++ b/include/libcamera/ipa/ipa_interface.h
>> @@ -95,12 +95,12 @@ struct ipa_context_ops {
>>         void (*register_callbacks)(struct ipa_context *ctx,
>>                                    const struct ipa_callback_ops
>> *callbacks,
>>                                    void *cb_ctx);
>> -       void (*configure)(struct ipa_context *ctx,
>> -                         const struct ipa_sensor_info *sensor_info,
>> -                         const struct ipa_stream *streams,
>> -                         unsigned int num_streams,
>> -                         const struct ipa_control_info_map *maps,
>> -                         unsigned int num_maps);
>> +       int (*configure)(struct ipa_context *ctx,
>> +                        const struct ipa_sensor_info *sensor_info,
>> +                        const struct ipa_stream *streams,
>> +                        unsigned int num_streams,
>> +                        const struct ipa_control_info_map *maps,
>> +                        unsigned int num_maps);
>>         void (*map_buffers)(struct ipa_context *ctx,
>>                             const struct ipa_buffer *buffers,
>>                             size_t num_buffers);
>> @@ -156,11 +156,11 @@ public:
>>         virtual int start() = 0;
>>         virtual void stop() = 0;
>>
>> -       virtual void configure(const CameraSensorInfo &sensorInfo,
>> -                              const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> -                              const std::map<unsigned int, const
>> ControlInfoMap &> &entityControls,
>> -                              const IPAOperationData &ipaConfig,
>> -                              IPAOperationData *result) = 0;
>> +       virtual int configure(const CameraSensorInfo &sensorInfo,
>> +                             const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> +                             const std::map<unsigned int, const
>> ControlInfoMap &> &entityControls,
>> +                             const IPAOperationData &ipaConfig,
>> +                             IPAOperationData *result) = 0;
>>
>>         virtual void mapBuffers(const std::vector<IPABuffer> &buffers) =
>> 0;
>>         virtual void unmapBuffers(const std::vector<unsigned int> &ids) =
>> 0;
>> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
>> b/src/ipa/libipa/ipa_interface_wrapper.cpp
>> index cee532e3a747..74d7bc6e1c9b 100644
>> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
>> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
>> @@ -115,12 +115,12 @@ void IPAInterfaceWrapper::register_callbacks(struct
>> ipa_context *_ctx,
>>         ctx->cb_ctx_ = cb_ctx;
>>  }
>>
>> -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
>> -                                   const struct ipa_sensor_info
>> *sensor_info,
>> -                                   const struct ipa_stream *streams,
>> -                                   unsigned int num_streams,
>> -                                   const struct ipa_control_info_map
>> *maps,
>> -                                   unsigned int num_maps)
>> +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
>> +                                  const struct ipa_sensor_info
>> *sensor_info,
>> +                                  const struct ipa_stream *streams,
>> +                                  unsigned int num_streams,
>> +                                  const struct ipa_control_info_map
>> *maps,
>> +                                  unsigned int num_maps)
>>  {
>>         IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
>> *>(_ctx);
>>
>> @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct
>> ipa_context *_ctx,
>>
>>         /* \todo Translate the ipaConfig and result. */
>>         IPAOperationData ipaConfig;
>> -       ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
>> ipaConfig,
>> -                            nullptr);
>> +       return ctx->ipa_->configure(sensorInfo, ipaStreams,
>> entityControls,
>> +                                   ipaConfig, nullptr);
>>  }
>>
>>  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
>> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h
>> b/src/ipa/libipa/ipa_interface_wrapper.h
>> index a1c701599b56..acd3160039b1 100644
>> --- a/src/ipa/libipa/ipa_interface_wrapper.h
>> +++ b/src/ipa/libipa/ipa_interface_wrapper.h
>> @@ -30,12 +30,12 @@ private:
>>         static void register_callbacks(struct ipa_context *ctx,
>>                                        const struct ipa_callback_ops
>> *callbacks,
>>                                        void *cb_ctx);
>> -       static void configure(struct ipa_context *ctx,
>> -                             const struct ipa_sensor_info *sensor_info,
>> -                             const struct ipa_stream *streams,
>> -                             unsigned int num_streams,
>> -                             const struct ipa_control_info_map *maps,
>> -                             unsigned int num_maps);
>> +       static int configure(struct ipa_context *ctx,
>> +                            const struct ipa_sensor_info *sensor_info,
>> +                            const struct ipa_stream *streams,
>> +                            unsigned int num_streams,
>> +                            const struct ipa_control_info_map *maps,
>> +                            unsigned int num_maps);
>>         static void map_buffers(struct ipa_context *ctx,
>>                                 const struct ipa_buffer *c_buffers,
>>                                 size_t num_buffers);
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>> b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 9853a343c892..75f7af3430ef 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -81,11 +81,11 @@ public:
>>         int start() override { return 0; }
>>         void stop() override {}
>>
>> -       void configure(const CameraSensorInfo &sensorInfo,
>> -                      const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> -                      const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> -                      const IPAOperationData &data,
>> -                      IPAOperationData *response) override;
>> +       int configure(const CameraSensorInfo &sensorInfo,
>> +                     const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> +                     const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> +                     const IPAOperationData &data,
>> +                     IPAOperationData *response) override;
>>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>>         void processEvent(const IPAOperationData &event) override;
>> @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo
>> &sensorInfo)
>>         mode_.line_length = 1e9 * sensorInfo.lineLength /
>> sensorInfo.pixelRate;
>>  }
>>
>> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>> -                      [[maybe_unused]] const std::map<unsigned int,
>> IPAStream> &streamConfig,
>> -                      const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> -                      const IPAOperationData &ipaConfig,
>> -                      IPAOperationData *result)
>> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
>> +                     [[maybe_unused]] const std::map<unsigned int,
>> IPAStream> &streamConfig,
>> +                     const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> +                     const IPAOperationData &ipaConfig,
>> +                     IPAOperationData *result)
>>  {
>>         if (entityControls.empty())
>> -               return;
>> +               return -EINVAL;
>>
>>         result->operation = 0;
>>
>> @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo
>> &sensorInfo,
>>         }
>>
>>         lastMode_ = mode_;
>> +       return 0;
>>  }
>>
>>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 07d7f1b2ddd8..78d78c5ac521 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -40,11 +40,11 @@ public:
>>         int start() override { return 0; }
>>         void stop() override {}
>>
>> -       void configure(const CameraSensorInfo &info,
>> -                      const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> -                      const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> -                      const IPAOperationData &ipaConfig,
>> -                      IPAOperationData *response) override;
>> +       int configure(const CameraSensorInfo &info,
>> +                     const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> +                     const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> +                     const IPAOperationData &ipaConfig,
>> +                     IPAOperationData *response) override;
>>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>>         void processEvent(const IPAOperationData &event) override;
>> @@ -79,27 +79,27 @@ private:
>>   * assemble one. Make sure the reported sensor information are relevant
>>   * before accessing them.
>>   */
>> -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
>> -                         [[maybe_unused]] const std::map<unsigned int,
>> IPAStream> &streamConfig,
>> -                         const std::map<unsigned int, const
>> ControlInfoMap &> &entityControls,
>> -                         [[maybe_unused]] const IPAOperationData
>> &ipaConfig,
>> -                         [[maybe_unused]] IPAOperationData *result)
>> +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
>> +                        [[maybe_unused]] const std::map<unsigned int,
>> IPAStream> &streamConfig,
>> +                        const std::map<unsigned int, const
>> ControlInfoMap &> &entityControls,
>> +                        [[maybe_unused]] const IPAOperationData
>> &ipaConfig,
>> +                        [[maybe_unused]] IPAOperationData *result)
>>  {
>>         if (entityControls.empty())
>> -               return;
>> +               return -EINVAL;
>>
>>         ctrls_ = entityControls.at(0);
>>
>>         const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>>         if (itExp == ctrls_.end()) {
>>                 LOG(IPARkISP1, Error) << "Can't find exposure control";
>> -               return;
>> +               return -EINVAL;
>>         }
>>
>>         const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>>         if (itGain == ctrls_.end()) {
>>                 LOG(IPARkISP1, Error) << "Can't find gain control";
>> -               return;
>> +               return -EINVAL;
>>         }
>>
>>         autoExposure_ = true;
>> @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const
>> CameraSensorInfo &info,
>>                 << " Gain: " << minGain_ << "-" << maxGain_;
>>
>>         setControls(0);
>> +       return 0;
>>  }
>>
>>  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
>> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
>> index cf8411359e40..79c8c2fb8927 100644
>> --- a/src/ipa/vimc/vimc.cpp
>> +++ b/src/ipa/vimc/vimc.cpp
>> @@ -37,11 +37,11 @@ public:
>>         int start() override;
>>         void stop() override;
>>
>> -       void configure([[maybe_unused]] const CameraSensorInfo
>> &sensorInfo,
>> -                      [[maybe_unused]] const std::map<unsigned int,
>> IPAStream> &streamConfig,
>> -                      [[maybe_unused]] const std::map<unsigned int,
>> const ControlInfoMap &> &entityControls,
>> -                      [[maybe_unused]] const IPAOperationData &ipaConfig,
>> -                      [[maybe_unused]] IPAOperationData *result)
>> override {}
>> +       int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
>> +                     [[maybe_unused]] const std::map<unsigned int,
>> IPAStream> &streamConfig,
>> +                     [[maybe_unused]] const std::map<unsigned int, const
>> ControlInfoMap &> &entityControls,
>> +                     [[maybe_unused]] const IPAOperationData &ipaConfig,
>> +                     [[maybe_unused]] IPAOperationData *result) override
>> { return 0; }
>>         void mapBuffers([[maybe_unused]] const std::vector<IPABuffer>
>> &buffers) override {}
>>         void unmapBuffers([[maybe_unused]] const std::vector<unsigned
>> int> &ids) override {}
>>         void processEvent([[maybe_unused]] const IPAOperationData &event)
>> override {}
>> diff --git a/src/libcamera/ipa_context_wrapper.cpp
>> b/src/libcamera/ipa_context_wrapper.cpp
>> index 231300ce0bec..f63ad830c003 100644
>> --- a/src/libcamera/ipa_context_wrapper.cpp
>> +++ b/src/libcamera/ipa_context_wrapper.cpp
>> @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()
>>         ctx_->ops->stop(ctx_);
>>  }
>>
>> -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>> -                                 const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> -                                 const std::map<unsigned int, const
>> ControlInfoMap &> &entityControls,
>> -                                 const IPAOperationData &ipaConfig,
>> -                                 IPAOperationData *result)
>> +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>> +                                const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> +                                const std::map<unsigned int, const
>> ControlInfoMap &> &entityControls,
>> +                                const IPAOperationData &ipaConfig,
>> +                                IPAOperationData *result)
>>  {
>>         if (intf_)
>>                 return intf_->configure(sensorInfo, streamConfig,
>>                                         entityControls, ipaConfig,
>> result);
>>
>>         if (!ctx_)
>> -               return;
>> +               return 0;
>>
>>         serializer_.reset();
>>
>> @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const
>> CameraSensorInfo &sensorInfo,
>>         }
>>
>>         /* \todo Translate the ipaConfig and reponse */
>> -       ctx_->ops->configure(ctx_, &sensor_info, c_streams,
>> streamConfig.size(),
>> -                            c_info_maps, entityControls.size());
>> +       return ctx_->ops->configure(ctx_, &sensor_info, c_streams,
>> +                                   streamConfig.size(), c_info_maps,
>> +                                   entityControls.size());
>>  }
>>
>>  void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer> &buffers)
>> diff --git a/src/libcamera/ipa_interface.cpp
>> b/src/libcamera/ipa_interface.cpp
>> index 23fc56d7d48e..516a8ecd4b53 100644
>> --- a/src/libcamera/ipa_interface.cpp
>> +++ b/src/libcamera/ipa_interface.cpp
>> @@ -347,6 +347,8 @@
>>   * \param[in] num_maps The number of entries in the \a maps array
>>   *
>>   * \sa libcamera::IPAInterface::configure()
>> + *
>> + * \return 0 on success or a negative error code on failure
>>   */
>>
>>  /**
>> @@ -573,6 +575,8 @@ namespace libcamera {
>>   * pipeline handler to the IPA and back. The pipeline handler may set
>> the \a
>>   * result parameter to null if the IPA protocol doesn't need to pass a
>> result
>>   * back through the configure() function.
>> + *
>> + * \return 0 on success or a negative error code on failure
>>   */
>>
>>  /**
>> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
>> b/src/libcamera/proxy/ipa_proxy_linux.cpp
>> index b78a0e4535f5..ed250ce79c17 100644
>> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
>> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
>> @@ -32,11 +32,11 @@ public:
>>         }
>>         int start() override { return 0; }
>>         void stop() override {}
>> -       void configure([[maybe_unused]] const CameraSensorInfo
>> &sensorInfo,
>> -                      [[maybe_unused]] const std::map<unsigned int,
>> IPAStream> &streamConfig,
>> -                      [[maybe_unused]] const std::map<unsigned int,
>> const ControlInfoMap &> &entityControls,
>> -                      [[maybe_unused]] const IPAOperationData &ipaConfig,
>> -                      [[maybe_unused]] IPAOperationData *result)
>> override {}
>> +       int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
>> +                     [[maybe_unused]] const std::map<unsigned int,
>> IPAStream> &streamConfig,
>> +                     [[maybe_unused]] const std::map<unsigned int, const
>> ControlInfoMap &> &entityControls,
>> +                     [[maybe_unused]] const IPAOperationData &ipaConfig,
>> +                     [[maybe_unused]] IPAOperationData *result) override
>> { return 0; }
>>         void mapBuffers([[maybe_unused]] const std::vector<IPABuffer>
>> &buffers) override {}
>>         void unmapBuffers([[maybe_unused]] const std::vector<unsigned
>> int> &ids) override {}
>>         void processEvent([[maybe_unused]] const IPAOperationData &event)
>> override {}
>> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
>> b/src/libcamera/proxy/ipa_proxy_thread.cpp
>> index eead2883708d..fd91726c4840 100644
>> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
>> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
>> @@ -29,11 +29,11 @@ public:
>>         int start() override;
>>         void stop() override;
>>
>> -       void configure(const CameraSensorInfo &sensorInfo,
>> -                      const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> -                      const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> -                      const IPAOperationData &ipaConfig,
>> -                      IPAOperationData *result) override;
>> +       int configure(const CameraSensorInfo &sensorInfo,
>> +                     const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> +                     const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> +                     const IPAOperationData &ipaConfig,
>> +                     IPAOperationData *result) override;
>>         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>>         void unmapBuffers(const std::vector<unsigned int> &ids) override;
>>         void processEvent(const IPAOperationData &event) override;
>> @@ -132,14 +132,14 @@ void IPAProxyThread::stop()
>>         thread_.wait();
>>  }
>>
>> -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
>> -                              const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> -                              const std::map<unsigned int, const
>> ControlInfoMap &> &entityControls,
>> -                              const IPAOperationData &ipaConfig,
>> -                              IPAOperationData *result)
>> +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
>> +                             const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> +                             const std::map<unsigned int, const
>> ControlInfoMap &> &entityControls,
>> +                             const IPAOperationData &ipaConfig,
>> +                             IPAOperationData *result)
>>  {
>> -       ipa_->configure(sensorInfo, streamConfig, entityControls,
>> ipaConfig,
>> -                       result);
>> +       return ipa_->configure(sensorInfo, streamConfig, entityControls,
>> +                              ipaConfig, result);
>>  }
>>
>>  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
>> diff --git a/test/ipa/ipa_wrappers_test.cpp
>> b/test/ipa/ipa_wrappers_test.cpp
>> index 59d991cbbf6a..ad0fb0386865 100644
>> --- a/test/ipa/ipa_wrappers_test.cpp
>> +++ b/test/ipa/ipa_wrappers_test.cpp
>> @@ -67,55 +67,63 @@ public:
>>                 report(Op_stop, TestPass);
>>         }
>>
>> -       void configure(const CameraSensorInfo &sensorInfo,
>> -                      const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> -                      const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> -                      [[maybe_unused]] const IPAOperationData &ipaConfig,
>> -                      [[maybe_unused]] IPAOperationData *result) override
>> +       int configure(const CameraSensorInfo &sensorInfo,
>> +                     const std::map<unsigned int, IPAStream>
>> &streamConfig,
>> +                     const std::map<unsigned int, const ControlInfoMap
>> &> &entityControls,
>> +                     [[maybe_unused]] const IPAOperationData &ipaConfig,
>> +                     [[maybe_unused]] IPAOperationData *result) override
>>         {
>>                 /* Verify sensorInfo. */
>>                 if (sensorInfo.outputSize.width != 2560 ||
>>                     sensorInfo.outputSize.height != 1940) {
>>                         cerr << "configure(): Invalid sensor info size "
>>                              << sensorInfo.outputSize.toString();
>> +                       report(Op_configure, TestFail);
>> +                       return -EINVAL;
>>                 }
>>
>>                 /* Verify streamConfig. */
>>                 if (streamConfig.size() != 2) {
>>                         cerr << "configure(): Invalid number of streams "
>>                              << streamConfig.size() << endl;
>> -                       return report(Op_configure, TestFail);
>> +                       report(Op_configure, TestFail);
>> +                       return -EINVAL;
>>                 }
>>
>>                 auto iter = streamConfig.find(1);
>>                 if (iter == streamConfig.end()) {
>>                         cerr << "configure(): No configuration for stream
>> 1" << endl;
>> -                       return report(Op_configure, TestFail);
>> +                       report(Op_configure, TestFail);
>> +                       return -EINVAL;
>>                 }
>>                 const IPAStream *stream = &iter->second;
>>                 if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||
>>                     stream->size != Size{ 1024, 768 }) {
>>                         cerr << "configure(): Invalid configuration for
>> stream 1" << endl;
>> -                       return report(Op_configure, TestFail);
>> +                       report(Op_configure, TestFail);
>> +                       return -EINVAL;
>>                 }
>>
>>                 iter = streamConfig.find(2);
>>                 if (iter == streamConfig.end()) {
>>                         cerr << "configure(): No configuration for stream
>> 2" << endl;
>> -                       return report(Op_configure, TestFail);
>> +                       report(Op_configure, TestFail);
>> +                       return -EINVAL;
>>                 }
>>                 stream = &iter->second;
>>                 if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||
>>                     stream->size != Size{ 800, 600 }) {
>>                         cerr << "configure(): Invalid configuration for
>> stream 2" << endl;
>> -                       return report(Op_configure, TestFail);
>> +                       report(Op_configure, TestFail);
>> +                       return -EINVAL;
>>                 }
>>
>>                 /* Verify entityControls. */
>>                 auto ctrlIter = entityControls.find(42);
>>                 if (ctrlIter == entityControls.end()) {
>>                         cerr << "configure(): Controls not found" << endl;
>> -                       return report(Op_configure, TestFail);
>> +                       report(Op_configure, TestFail);
>> +                       return -EINVAL;
>>                 }
>>
>>                 const ControlInfoMap &infoMap = ctrlIter->second;
>> @@ -124,10 +132,12 @@ public:
>>                     infoMap.count(V4L2_CID_CONTRAST) != 1 ||
>>                     infoMap.count(V4L2_CID_SATURATION) != 1) {
>>                         cerr << "configure(): Invalid control IDs" <<
>> endl;
>> -                       return report(Op_configure, TestFail);
>> +                       report(Op_configure, TestFail);
>> +                       return -EINVAL;
>>                 }
>>
>>                 report(Op_configure, TestPass);
>> +               return 0;
>>         }
>>
>>         void mapBuffers(const std::vector<IPABuffer> &buffers) override
>>
>> > ---
>> >  include/libcamera/ipa/raspberrypi.h                |  1 +
>> >  src/ipa/raspberrypi/raspberrypi.cpp                | 12 +++++++++++-
>> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 +++++
>> >  3 files changed, 17 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/libcamera/ipa/raspberrypi.h
>> b/include/libcamera/ipa/raspberrypi.h
>> > index 2b55dbfc..86c97ffa 100644
>> > --- a/include/libcamera/ipa/raspberrypi.h
>> > +++ b/include/libcamera/ipa/raspberrypi.h
>> > @@ -21,6 +21,7 @@ enum ConfigParameters {
>> >       IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
>> >       IPA_CONFIG_SENSOR = (1 << 2),
>> >       IPA_CONFIG_DROP_FRAMES = (1 << 3),
>> > +     IPA_CONFIG_FAILED = (1 << 4),
>> >  };
>> >
>> >  enum Operations {
>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>> b/src/ipa/raspberrypi/raspberrypi.cpp
>> > index 9853a343..57dd9c61 100644
>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> > @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo
>> &sensorInfo,
>> >                      const IPAOperationData &ipaConfig,
>> >                      IPAOperationData *result)
>> >  {
>> > -     if (entityControls.empty())
>> > +     if (entityControls.size() != 2) {
>> > +             LOG(IPARPI, Error) << "No ISP or sensor controls found.";
>> > +             result->operation = RPi::IPA_CONFIG_FAILED;
>> >               return;
>> > +     }
>> >
>> >       result->operation = 0;
>> >
>> > @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo
>> &sensorInfo,
>> >       if (!helper_) {
>> >               helper_ =
>> std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
>> >
>> > +             if (!helper_) {
>> > +                     LOG(IPARPI, Error) << "Could not create camera
>> helper for "
>> > +                                        << cameraName;
>> > +                     result->operation = RPi::IPA_CONFIG_FAILED;
>> > +                     return;
>> > +             }
>> > +
>> >               /*
>> >                * Pass out the sensor config to the pipeline handler in
>> order
>> >                * to setup the staggered writer class.
>> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > index 6fcdf557..76252806 100644
>> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const
>> CameraConfiguration *config)
>> >       ipa_->configure(sensorInfo_, streamConfig, entityControls,
>> ipaConfig,
>> >                       &result);
>> >
>> > +     if (result.operation & RPi::IPA_CONFIG_FAILED) {
>> > +             LOG(RPI, Error) << "IPA configuration failed!";
>> > +             return -EPIPE;
>> > +     }
>> > +
>> >       unsigned int resultIdx = 0;
>> >       if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
>> >               /*
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>
Laurent Pinchart Dec. 1, 2020, 10:04 a.m. UTC | #4 
Hi Naush,

On Tue, Dec 01, 2020 at 09:54:44AM +0000, Naushir Patuck wrote:
> On Tue, 1 Dec 2020 at 01:55, Laurent Pinchart wrote:
> > On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:
> > > If the IPA fails during configuration, return an error flag to the
> > > pipeline handler and fail the use case gracefully.
> > >
> > > At present, the IPA configuration can fail for the following reasons:
> > > - The sensor is not recognised, and fails to open a CamHelper object.
> > > - The pipeline handler did not pass in controls for the ISP and sensor.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Wouldn't it be simpler to modify the configure() function to return an
> > int ? How about the following ? If it works for you I'll submit a proper
> > patch.
> 
> Yes, that works equally well for me.  Happy for you to submit the patch, or
> would you prefer if I add your commit below to a new patch set that will
> also include the Raspberry Pi IPA/PH changes?

I've just submitted the patch :-) I've asked Paul to look at it though,
to see if it would be easier to rebase his in-progress IPA IPC work on
top of your patch or on top of mine.

> > Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Date:   Tue Dec 1 03:54:18 2020 +0200
> >
> >     libcamera: ipa_interface: Make configure() return an int
> >
> >     It's useful for the IPAInterface::configure() function to be able to
> >     return a status. Make it return an int, to avoid forcing IPAs to return
> >     a status encoded in the IPAOperationData in a custom way.
> >
> >     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
> > b/include/libcamera/internal/ipa_context_wrapper.h
> > index 8f767e844221..a00b5e7b92eb 100644
> > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > @@ -22,11 +22,11 @@ public:
> >         int init(const IPASettings &settings) override;
> >         int start() override;
> >         void stop() override;
> > -       void configure(const CameraSensorInfo &sensorInfo,
> > -                      const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > -                      const std::map<unsigned int, const ControlInfoMap
> > &> &entityControls,
> > -                      const IPAOperationData &ipaConfig,
> > -                      IPAOperationData *result) override;
> > +       int configure(const CameraSensorInfo &sensorInfo,
> > +                     const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > +                     const std::map<unsigned int, const ControlInfoMap &>
> > &entityControls,
> > +                     const IPAOperationData &ipaConfig,
> > +                     IPAOperationData *result) override;
> >
> >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > diff --git a/include/libcamera/ipa/ipa_interface.h
> > b/include/libcamera/ipa/ipa_interface.h
> > index 322b7079070e..1d8cf8dc1c56 100644
> > --- a/include/libcamera/ipa/ipa_interface.h
> > +++ b/include/libcamera/ipa/ipa_interface.h
> > @@ -95,12 +95,12 @@ struct ipa_context_ops {
> >         void (*register_callbacks)(struct ipa_context *ctx,
> >                                    const struct ipa_callback_ops
> > *callbacks,
> >                                    void *cb_ctx);
> > -       void (*configure)(struct ipa_context *ctx,
> > -                         const struct ipa_sensor_info *sensor_info,
> > -                         const struct ipa_stream *streams,
> > -                         unsigned int num_streams,
> > -                         const struct ipa_control_info_map *maps,
> > -                         unsigned int num_maps);
> > +       int (*configure)(struct ipa_context *ctx,
> > +                        const struct ipa_sensor_info *sensor_info,
> > +                        const struct ipa_stream *streams,
> > +                        unsigned int num_streams,
> > +                        const struct ipa_control_info_map *maps,
> > +                        unsigned int num_maps);
> >         void (*map_buffers)(struct ipa_context *ctx,
> >                             const struct ipa_buffer *buffers,
> >                             size_t num_buffers);
> > @@ -156,11 +156,11 @@ public:
> >         virtual int start() = 0;
> >         virtual void stop() = 0;
> >
> > -       virtual void configure(const CameraSensorInfo &sensorInfo,
> > -                              const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > -                              const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > -                              const IPAOperationData &ipaConfig,
> > -                              IPAOperationData *result) = 0;
> > +       virtual int configure(const CameraSensorInfo &sensorInfo,
> > +                             const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > +                             const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > +                             const IPAOperationData &ipaConfig,
> > +                             IPAOperationData *result) = 0;
> >
> >         virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> >         virtual void unmapBuffers(const std::vector<unsigned int> &ids) =
> > 0;
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index cee532e3a747..74d7bc6e1c9b 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -115,12 +115,12 @@ void IPAInterfaceWrapper::register_callbacks(struct
> > ipa_context *_ctx,
> >         ctx->cb_ctx_ = cb_ctx;
> >  }
> >
> > -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > -                                   const struct ipa_sensor_info
> > *sensor_info,
> > -                                   const struct ipa_stream *streams,
> > -                                   unsigned int num_streams,
> > -                                   const struct ipa_control_info_map
> > *maps,
> > -                                   unsigned int num_maps)
> > +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > +                                  const struct ipa_sensor_info
> > *sensor_info,
> > +                                  const struct ipa_stream *streams,
> > +                                  unsigned int num_streams,
> > +                                  const struct ipa_control_info_map *maps,
> > +                                  unsigned int num_maps)
> >  {
> >         IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
> > *>(_ctx);
> >
> > @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct ipa_context
> > *_ctx,
> >
> >         /* \todo Translate the ipaConfig and result. */
> >         IPAOperationData ipaConfig;
> > -       ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
> > ipaConfig,
> > -                            nullptr);
> > +       return ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
> > +                                   ipaConfig, nullptr);
> >  }
> >
> >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h
> > b/src/ipa/libipa/ipa_interface_wrapper.h
> > index a1c701599b56..acd3160039b1 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.h
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> > @@ -30,12 +30,12 @@ private:
> >         static void register_callbacks(struct ipa_context *ctx,
> >                                        const struct ipa_callback_ops
> > *callbacks,
> >                                        void *cb_ctx);
> > -       static void configure(struct ipa_context *ctx,
> > -                             const struct ipa_sensor_info *sensor_info,
> > -                             const struct ipa_stream *streams,
> > -                             unsigned int num_streams,
> > -                             const struct ipa_control_info_map *maps,
> > -                             unsigned int num_maps);
> > +       static int configure(struct ipa_context *ctx,
> > +                            const struct ipa_sensor_info *sensor_info,
> > +                            const struct ipa_stream *streams,
> > +                            unsigned int num_streams,
> > +                            const struct ipa_control_info_map *maps,
> > +                            unsigned int num_maps);
> >         static void map_buffers(struct ipa_context *ctx,
> >                                 const struct ipa_buffer *c_buffers,
> >                                 size_t num_buffers);
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 9853a343c892..75f7af3430ef 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -81,11 +81,11 @@ public:
> >         int start() override { return 0; }
> >         void stop() override {}
> >
> > -       void configure(const CameraSensorInfo &sensorInfo,
> > -                      const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > -                      const std::map<unsigned int, const ControlInfoMap
> > &> &entityControls,
> > -                      const IPAOperationData &data,
> > -                      IPAOperationData *response) override;
> > +       int configure(const CameraSensorInfo &sensorInfo,
> > +                     const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > +                     const std::map<unsigned int, const ControlInfoMap &>
> > &entityControls,
> > +                     const IPAOperationData &data,
> > +                     IPAOperationData *response) override;
> >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >         void processEvent(const IPAOperationData &event) override;
> > @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo
> > &sensorInfo)
> >         mode_.line_length = 1e9 * sensorInfo.lineLength /
> > sensorInfo.pixelRate;
> >  }
> >
> > -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > -                      [[maybe_unused]] const std::map<unsigned int,
> > IPAStream> &streamConfig,
> > -                      const std::map<unsigned int, const ControlInfoMap
> > &> &entityControls,
> > -                      const IPAOperationData &ipaConfig,
> > -                      IPAOperationData *result)
> > +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > +                     [[maybe_unused]] const std::map<unsigned int,
> > IPAStream> &streamConfig,
> > +                     const std::map<unsigned int, const ControlInfoMap &>
> > &entityControls,
> > +                     const IPAOperationData &ipaConfig,
> > +                     IPAOperationData *result)
> >  {
> >         if (entityControls.empty())
> > -               return;
> > +               return -EINVAL;
> >
> >         result->operation = 0;
> >
> > @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo
> > &sensorInfo,
> >         }
> >
> >         lastMode_ = mode_;
> > +       return 0;
> >  }
> >
> >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 07d7f1b2ddd8..78d78c5ac521 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -40,11 +40,11 @@ public:
> >         int start() override { return 0; }
> >         void stop() override {}
> >
> > -       void configure(const CameraSensorInfo &info,
> > -                      const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > -                      const std::map<unsigned int, const ControlInfoMap
> > &> &entityControls,
> > -                      const IPAOperationData &ipaConfig,
> > -                      IPAOperationData *response) override;
> > +       int configure(const CameraSensorInfo &info,
> > +                     const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > +                     const std::map<unsigned int, const ControlInfoMap &>
> > &entityControls,
> > +                     const IPAOperationData &ipaConfig,
> > +                     IPAOperationData *response) override;
> >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >         void processEvent(const IPAOperationData &event) override;
> > @@ -79,27 +79,27 @@ private:
> >   * assemble one. Make sure the reported sensor information are relevant
> >   * before accessing them.
> >   */
> > -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> > -                         [[maybe_unused]] const std::map<unsigned int,
> > IPAStream> &streamConfig,
> > -                         const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > -                         [[maybe_unused]] const IPAOperationData
> > &ipaConfig,
> > -                         [[maybe_unused]] IPAOperationData *result)
> > +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> > +                        [[maybe_unused]] const std::map<unsigned int,
> > IPAStream> &streamConfig,
> > +                        const std::map<unsigned int, const ControlInfoMap
> > &> &entityControls,
> > +                        [[maybe_unused]] const IPAOperationData
> > &ipaConfig,
> > +                        [[maybe_unused]] IPAOperationData *result)
> >  {
> >         if (entityControls.empty())
> > -               return;
> > +               return -EINVAL;
> >
> >         ctrls_ = entityControls.at(0);
> >
> >         const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> >         if (itExp == ctrls_.end()) {
> >                 LOG(IPARkISP1, Error) << "Can't find exposure control";
> > -               return;
> > +               return -EINVAL;
> >         }
> >
> >         const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> >         if (itGain == ctrls_.end()) {
> >                 LOG(IPARkISP1, Error) << "Can't find gain control";
> > -               return;
> > +               return -EINVAL;
> >         }
> >
> >         autoExposure_ = true;
> > @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const
> > CameraSensorInfo &info,
> >                 << " Gain: " << minGain_ << "-" << maxGain_;
> >
> >         setControls(0);
> > +       return 0;
> >  }
> >
> >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index cf8411359e40..79c8c2fb8927 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -37,11 +37,11 @@ public:
> >         int start() override;
> >         void stop() override;
> >
> > -       void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > -                      [[maybe_unused]] const std::map<unsigned int,
> > IPAStream> &streamConfig,
> > -                      [[maybe_unused]] const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > -                      [[maybe_unused]] const IPAOperationData &ipaConfig,
> > -                      [[maybe_unused]] IPAOperationData *result) override
> > {}
> > +       int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > +                     [[maybe_unused]] const std::map<unsigned int,
> > IPAStream> &streamConfig,
> > +                     [[maybe_unused]] const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > +                     [[maybe_unused]] const IPAOperationData &ipaConfig,
> > +                     [[maybe_unused]] IPAOperationData *result) override
> > { return 0; }
> >         void mapBuffers([[maybe_unused]] const std::vector<IPABuffer>
> > &buffers) override {}
> >         void unmapBuffers([[maybe_unused]] const std::vector<unsigned int>
> > &ids) override {}
> >         void processEvent([[maybe_unused]] const IPAOperationData &event)
> > override {}
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp
> > b/src/libcamera/ipa_context_wrapper.cpp
> > index 231300ce0bec..f63ad830c003 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()
> >         ctx_->ops->stop(ctx_);
> >  }
> >
> > -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > -                                 const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > -                                 const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > -                                 const IPAOperationData &ipaConfig,
> > -                                 IPAOperationData *result)
> > +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > +                                const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > +                                const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > +                                const IPAOperationData &ipaConfig,
> > +                                IPAOperationData *result)
> >  {
> >         if (intf_)
> >                 return intf_->configure(sensorInfo, streamConfig,
> >                                         entityControls, ipaConfig, result);
> >
> >         if (!ctx_)
> > -               return;
> > +               return 0;
> >
> >         serializer_.reset();
> >
> > @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const
> > CameraSensorInfo &sensorInfo,
> >         }
> >
> >         /* \todo Translate the ipaConfig and reponse */
> > -       ctx_->ops->configure(ctx_, &sensor_info, c_streams,
> > streamConfig.size(),
> > -                            c_info_maps, entityControls.size());
> > +       return ctx_->ops->configure(ctx_, &sensor_info, c_streams,
> > +                                   streamConfig.size(), c_info_maps,
> > +                                   entityControls.size());
> >  }
> >
> >  void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/src/libcamera/ipa_interface.cpp
> > b/src/libcamera/ipa_interface.cpp
> > index 23fc56d7d48e..516a8ecd4b53 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -347,6 +347,8 @@
> >   * \param[in] num_maps The number of entries in the \a maps array
> >   *
> >   * \sa libcamera::IPAInterface::configure()
> > + *
> > + * \return 0 on success or a negative error code on failure
> >   */
> >
> >  /**
> > @@ -573,6 +575,8 @@ namespace libcamera {
> >   * pipeline handler to the IPA and back. The pipeline handler may set the
> > \a
> >   * result parameter to null if the IPA protocol doesn't need to pass a
> > result
> >   * back through the configure() function.
> > + *
> > + * \return 0 on success or a negative error code on failure
> >   */
> >
> >  /**
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index b78a0e4535f5..ed250ce79c17 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -32,11 +32,11 @@ public:
> >         }
> >         int start() override { return 0; }
> >         void stop() override {}
> > -       void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > -                      [[maybe_unused]] const std::map<unsigned int,
> > IPAStream> &streamConfig,
> > -                      [[maybe_unused]] const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > -                      [[maybe_unused]] const IPAOperationData &ipaConfig,
> > -                      [[maybe_unused]] IPAOperationData *result) override
> > {}
> > +       int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > +                     [[maybe_unused]] const std::map<unsigned int,
> > IPAStream> &streamConfig,
> > +                     [[maybe_unused]] const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > +                     [[maybe_unused]] const IPAOperationData &ipaConfig,
> > +                     [[maybe_unused]] IPAOperationData *result) override
> > { return 0; }
> >         void mapBuffers([[maybe_unused]] const std::vector<IPABuffer>
> > &buffers) override {}
> >         void unmapBuffers([[maybe_unused]] const std::vector<unsigned int>
> > &ids) override {}
> >         void processEvent([[maybe_unused]] const IPAOperationData &event)
> > override {}
> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > index eead2883708d..fd91726c4840 100644
> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > @@ -29,11 +29,11 @@ public:
> >         int start() override;
> >         void stop() override;
> >
> > -       void configure(const CameraSensorInfo &sensorInfo,
> > -                      const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > -                      const std::map<unsigned int, const ControlInfoMap
> > &> &entityControls,
> > -                      const IPAOperationData &ipaConfig,
> > -                      IPAOperationData *result) override;
> > +       int configure(const CameraSensorInfo &sensorInfo,
> > +                     const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > +                     const std::map<unsigned int, const ControlInfoMap &>
> > &entityControls,
> > +                     const IPAOperationData &ipaConfig,
> > +                     IPAOperationData *result) override;
> >         void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >         void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >         void processEvent(const IPAOperationData &event) override;
> > @@ -132,14 +132,14 @@ void IPAProxyThread::stop()
> >         thread_.wait();
> >  }
> >
> > -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > -                              const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > -                              const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > -                              const IPAOperationData &ipaConfig,
> > -                              IPAOperationData *result)
> > +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > +                             const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > +                             const std::map<unsigned int, const
> > ControlInfoMap &> &entityControls,
> > +                             const IPAOperationData &ipaConfig,
> > +                             IPAOperationData *result)
> >  {
> > -       ipa_->configure(sensorInfo, streamConfig, entityControls,
> > ipaConfig,
> > -                       result);
> > +       return ipa_->configure(sensorInfo, streamConfig, entityControls,
> > +                              ipaConfig, result);
> >  }
> >
> >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/test/ipa/ipa_wrappers_test.cpp
> > b/test/ipa/ipa_wrappers_test.cpp
> > index 59d991cbbf6a..ad0fb0386865 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -67,55 +67,63 @@ public:
> >                 report(Op_stop, TestPass);
> >         }
> >
> > -       void configure(const CameraSensorInfo &sensorInfo,
> > -                      const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > -                      const std::map<unsigned int, const ControlInfoMap
> > &> &entityControls,
> > -                      [[maybe_unused]] const IPAOperationData &ipaConfig,
> > -                      [[maybe_unused]] IPAOperationData *result) override
> > +       int configure(const CameraSensorInfo &sensorInfo,
> > +                     const std::map<unsigned int, IPAStream>
> > &streamConfig,
> > +                     const std::map<unsigned int, const ControlInfoMap &>
> > &entityControls,
> > +                     [[maybe_unused]] const IPAOperationData &ipaConfig,
> > +                     [[maybe_unused]] IPAOperationData *result) override
> >         {
> >                 /* Verify sensorInfo. */
> >                 if (sensorInfo.outputSize.width != 2560 ||
> >                     sensorInfo.outputSize.height != 1940) {
> >                         cerr << "configure(): Invalid sensor info size "
> >                              << sensorInfo.outputSize.toString();
> > +                       report(Op_configure, TestFail);
> > +                       return -EINVAL;
> >                 }
> >
> >                 /* Verify streamConfig. */
> >                 if (streamConfig.size() != 2) {
> >                         cerr << "configure(): Invalid number of streams "
> >                              << streamConfig.size() << endl;
> > -                       return report(Op_configure, TestFail);
> > +                       report(Op_configure, TestFail);
> > +                       return -EINVAL;
> >                 }
> >
> >                 auto iter = streamConfig.find(1);
> >                 if (iter == streamConfig.end()) {
> >                         cerr << "configure(): No configuration for stream
> > 1" << endl;
> > -                       return report(Op_configure, TestFail);
> > +                       report(Op_configure, TestFail);
> > +                       return -EINVAL;
> >                 }
> >                 const IPAStream *stream = &iter->second;
> >                 if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||
> >                     stream->size != Size{ 1024, 768 }) {
> >                         cerr << "configure(): Invalid configuration for
> > stream 1" << endl;
> > -                       return report(Op_configure, TestFail);
> > +                       report(Op_configure, TestFail);
> > +                       return -EINVAL;
> >                 }
> >
> >                 iter = streamConfig.find(2);
> >                 if (iter == streamConfig.end()) {
> >                         cerr << "configure(): No configuration for stream
> > 2" << endl;
> > -                       return report(Op_configure, TestFail);
> > +                       report(Op_configure, TestFail);
> > +                       return -EINVAL;
> >                 }
> >                 stream = &iter->second;
> >                 if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||
> >                     stream->size != Size{ 800, 600 }) {
> >                         cerr << "configure(): Invalid configuration for
> > stream 2" << endl;
> > -                       return report(Op_configure, TestFail);
> > +                       report(Op_configure, TestFail);
> > +                       return -EINVAL;
> >                 }
> >
> >                 /* Verify entityControls. */
> >                 auto ctrlIter = entityControls.find(42);
> >                 if (ctrlIter == entityControls.end()) {
> >                         cerr << "configure(): Controls not found" << endl;
> > -                       return report(Op_configure, TestFail);
> > +                       report(Op_configure, TestFail);
> > +                       return -EINVAL;
> >                 }
> >
> >                 const ControlInfoMap &infoMap = ctrlIter->second;
> > @@ -124,10 +132,12 @@ public:
> >                     infoMap.count(V4L2_CID_CONTRAST) != 1 ||
> >                     infoMap.count(V4L2_CID_SATURATION) != 1) {
> >                         cerr << "configure(): Invalid control IDs" << endl;
> > -                       return report(Op_configure, TestFail);
> > +                       report(Op_configure, TestFail);
> > +                       return -EINVAL;
> >                 }
> >
> >                 report(Op_configure, TestPass);
> > +               return 0;
> >         }
> >
> >         void mapBuffers(const std::vector<IPABuffer> &buffers) override
> >
> > > ---
> > >  include/libcamera/ipa/raspberrypi.h                |  1 +
> > >  src/ipa/raspberrypi/raspberrypi.cpp                | 12 +++++++++++-
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 +++++
> > >  3 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.h
> > b/include/libcamera/ipa/raspberrypi.h
> > > index 2b55dbfc..86c97ffa 100644
> > > --- a/include/libcamera/ipa/raspberrypi.h
> > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > @@ -21,6 +21,7 @@ enum ConfigParameters {
> > >       IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> > >       IPA_CONFIG_SENSOR = (1 << 2),
> > >       IPA_CONFIG_DROP_FRAMES = (1 << 3),
> > > +     IPA_CONFIG_FAILED = (1 << 4),
> > >  };
> > >
> > >  enum Operations {
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 9853a343..57dd9c61 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo
> > &sensorInfo,
> > >                      const IPAOperationData &ipaConfig,
> > >                      IPAOperationData *result)
> > >  {
> > > -     if (entityControls.empty())
> > > +     if (entityControls.size() != 2) {
> > > +             LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > > +             result->operation = RPi::IPA_CONFIG_FAILED;
> > >               return;
> > > +     }
> > >
> > >       result->operation = 0;
> > >
> > > @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo
> > &sensorInfo,
> > >       if (!helper_) {
> > >               helper_ =
> > std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
> > >
> > > +             if (!helper_) {
> > > +                     LOG(IPARPI, Error) << "Could not create camera
> > helper for "
> > > +                                        << cameraName;
> > > +                     result->operation = RPi::IPA_CONFIG_FAILED;
> > > +                     return;
> > > +             }
> > > +
> > >               /*
> > >                * Pass out the sensor config to the pipeline handler in
> > order
> > >                * to setup the staggered writer class.
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 6fcdf557..76252806 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const
> > CameraConfiguration *config)
> > >       ipa_->configure(sensorInfo_, streamConfig, entityControls,
> > ipaConfig,
> > >                       &result);
> > >
> > > +     if (result.operation & RPi::IPA_CONFIG_FAILED) {
> > > +             LOG(RPI, Error) << "IPA configuration failed!";
> > > +             return -EPIPE;
> > > +     }
> > > +
> > >       unsigned int resultIdx = 0;
> > >       if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
> > >               /*
Paul Elder Dec. 4, 2020, 10:39 a.m. UTC | #5 
Hi Naush,

On Tue, Dec 01, 2020 at 03:55:43AM +0200, Laurent Pinchart wrote:
> Hi Naush,
> 
> Thank you for the patch.
> 
> On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:
> > If the IPA fails during configuration, return an error flag to the
> > pipeline handler and fail the use case gracefully.
> > 
> > At present, the IPA configuration can fail for the following reasons:
> > - The sensor is not recognised, and fails to open a CamHelper object.
> > - The pipeline handler did not pass in controls for the ISP and sensor.
> > 
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Wouldn't it be simpler to modify the configure() function to return an
> int ? How about the following ? If it works for you I'll submit a proper
> patch.

From the perspective of the IPC changes, Naush's solution is more easily
translatable. When the IPA interface will become customizable, there
isn't a way to specify which output parameters should be return values
vs return parameters. Thus keeping them all as return parameters is the
better solution here.

So imo this is fine to merge as-is.

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

> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date:   Tue Dec 1 03:54:18 2020 +0200
> 
>     libcamera: ipa_interface: Make configure() return an int
> 
>     It's useful for the IPAInterface::configure() function to be able to
>     return a status. Make it return an int, to avoid forcing IPAs to return
>     a status encoded in the IPAOperationData in a custom way.
> 
>     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> index 8f767e844221..a00b5e7b92eb 100644
> --- a/include/libcamera/internal/ipa_context_wrapper.h
> +++ b/include/libcamera/internal/ipa_context_wrapper.h
> @@ -22,11 +22,11 @@ public:
>  	int init(const IPASettings &settings) override;
>  	int start() override;
>  	void stop() override;
> -	void configure(const CameraSensorInfo &sensorInfo,
> -		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -		       const IPAOperationData &ipaConfig,
> -		       IPAOperationData *result) override;
> +	int configure(const CameraSensorInfo &sensorInfo,
> +		      const std::map<unsigned int, IPAStream> &streamConfig,
> +		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		      const IPAOperationData &ipaConfig,
> +		      IPAOperationData *result) override;
> 
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> index 322b7079070e..1d8cf8dc1c56 100644
> --- a/include/libcamera/ipa/ipa_interface.h
> +++ b/include/libcamera/ipa/ipa_interface.h
> @@ -95,12 +95,12 @@ struct ipa_context_ops {
>  	void (*register_callbacks)(struct ipa_context *ctx,
>  				   const struct ipa_callback_ops *callbacks,
>  				   void *cb_ctx);
> -	void (*configure)(struct ipa_context *ctx,
> -			  const struct ipa_sensor_info *sensor_info,
> -			  const struct ipa_stream *streams,
> -			  unsigned int num_streams,
> -			  const struct ipa_control_info_map *maps,
> -			  unsigned int num_maps);
> +	int (*configure)(struct ipa_context *ctx,
> +			 const struct ipa_sensor_info *sensor_info,
> +			 const struct ipa_stream *streams,
> +			 unsigned int num_streams,
> +			 const struct ipa_control_info_map *maps,
> +			 unsigned int num_maps);
>  	void (*map_buffers)(struct ipa_context *ctx,
>  			    const struct ipa_buffer *buffers,
>  			    size_t num_buffers);
> @@ -156,11 +156,11 @@ public:
>  	virtual int start() = 0;
>  	virtual void stop() = 0;
> 
> -	virtual void configure(const CameraSensorInfo &sensorInfo,
> -			       const std::map<unsigned int, IPAStream> &streamConfig,
> -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -			       const IPAOperationData &ipaConfig,
> -			       IPAOperationData *result) = 0;
> +	virtual int configure(const CameraSensorInfo &sensorInfo,
> +			      const std::map<unsigned int, IPAStream> &streamConfig,
> +			      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +			      const IPAOperationData &ipaConfig,
> +			      IPAOperationData *result) = 0;
> 
>  	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
>  	virtual void unmapBuffers(const std::vector<unsigned int> &ids) = 0;
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index cee532e3a747..74d7bc6e1c9b 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -115,12 +115,12 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
>  	ctx->cb_ctx_ = cb_ctx;
>  }
> 
> -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> -				    const struct ipa_sensor_info *sensor_info,
> -				    const struct ipa_stream *streams,
> -				    unsigned int num_streams,
> -				    const struct ipa_control_info_map *maps,
> -				    unsigned int num_maps)
> +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> +				   const struct ipa_sensor_info *sensor_info,
> +				   const struct ipa_stream *streams,
> +				   unsigned int num_streams,
> +				   const struct ipa_control_info_map *maps,
> +				   unsigned int num_maps)
>  {
>  	IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> 
> @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> 
>  	/* \todo Translate the ipaConfig and result. */
>  	IPAOperationData ipaConfig;
> -	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
> -			     nullptr);
> +	return ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
> +				    ipaConfig, nullptr);
>  }
> 
>  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> index a1c701599b56..acd3160039b1 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.h
> +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> @@ -30,12 +30,12 @@ private:
>  	static void register_callbacks(struct ipa_context *ctx,
>  				       const struct ipa_callback_ops *callbacks,
>  				       void *cb_ctx);
> -	static void configure(struct ipa_context *ctx,
> -			      const struct ipa_sensor_info *sensor_info,
> -			      const struct ipa_stream *streams,
> -			      unsigned int num_streams,
> -			      const struct ipa_control_info_map *maps,
> -			      unsigned int num_maps);
> +	static int configure(struct ipa_context *ctx,
> +			     const struct ipa_sensor_info *sensor_info,
> +			     const struct ipa_stream *streams,
> +			     unsigned int num_streams,
> +			     const struct ipa_control_info_map *maps,
> +			     unsigned int num_maps);
>  	static void map_buffers(struct ipa_context *ctx,
>  				const struct ipa_buffer *c_buffers,
>  				size_t num_buffers);
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 9853a343c892..75f7af3430ef 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -81,11 +81,11 @@ public:
>  	int start() override { return 0; }
>  	void stop() override {}
> 
> -	void configure(const CameraSensorInfo &sensorInfo,
> -		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -		       const IPAOperationData &data,
> -		       IPAOperationData *response) override;
> +	int configure(const CameraSensorInfo &sensorInfo,
> +		      const std::map<unsigned int, IPAStream> &streamConfig,
> +		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		      const IPAOperationData &data,
> +		      IPAOperationData *response) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  	void processEvent(const IPAOperationData &event) override;
> @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
>  	mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;
>  }
> 
> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> -		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -		       const IPAOperationData &ipaConfig,
> -		       IPAOperationData *result)
> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> +		      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> +		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		      const IPAOperationData &ipaConfig,
> +		      IPAOperationData *result)
>  {
>  	if (entityControls.empty())
> -		return;
> +		return -EINVAL;
> 
>  	result->operation = 0;
> 
> @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	}
> 
>  	lastMode_ = mode_;
> +	return 0;
>  }
> 
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 07d7f1b2ddd8..78d78c5ac521 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -40,11 +40,11 @@ public:
>  	int start() override { return 0; }
>  	void stop() override {}
> 
> -	void configure(const CameraSensorInfo &info,
> -		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -		       const IPAOperationData &ipaConfig,
> -		       IPAOperationData *response) override;
> +	int configure(const CameraSensorInfo &info,
> +		      const std::map<unsigned int, IPAStream> &streamConfig,
> +		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		      const IPAOperationData &ipaConfig,
> +		      IPAOperationData *response) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  	void processEvent(const IPAOperationData &event) override;
> @@ -79,27 +79,27 @@ private:
>   * assemble one. Make sure the reported sensor information are relevant
>   * before accessing them.
>   */
> -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> -			  [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> -			  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -			  [[maybe_unused]] const IPAOperationData &ipaConfig,
> -			  [[maybe_unused]] IPAOperationData *result)
> +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> +			 [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> +			 const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +			 [[maybe_unused]] const IPAOperationData &ipaConfig,
> +			 [[maybe_unused]] IPAOperationData *result)
>  {
>  	if (entityControls.empty())
> -		return;
> +		return -EINVAL;
> 
>  	ctrls_ = entityControls.at(0);
> 
>  	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>  	if (itExp == ctrls_.end()) {
>  		LOG(IPARkISP1, Error) << "Can't find exposure control";
> -		return;
> +		return -EINVAL;
>  	}
> 
>  	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>  	if (itGain == ctrls_.end()) {
>  		LOG(IPARkISP1, Error) << "Can't find gain control";
> -		return;
> +		return -EINVAL;
>  	}
> 
>  	autoExposure_ = true;
> @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
>  		<< " Gain: " << minGain_ << "-" << maxGain_;
> 
>  	setControls(0);
> +	return 0;
>  }
> 
>  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index cf8411359e40..79c8c2fb8927 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -37,11 +37,11 @@ public:
>  	int start() override;
>  	void stop() override;
> 
> -	void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> -		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> -		       [[maybe_unused]] const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -		       [[maybe_unused]] const IPAOperationData &ipaConfig,
> -		       [[maybe_unused]] IPAOperationData *result) override {}
> +	int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> +		      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> +		      [[maybe_unused]] const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		      [[maybe_unused]] const IPAOperationData &ipaConfig,
> +		      [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	void mapBuffers([[maybe_unused]] const std::vector<IPABuffer> &buffers) override {}
>  	void unmapBuffers([[maybe_unused]] const std::vector<unsigned int> &ids) override {}
>  	void processEvent([[maybe_unused]] const IPAOperationData &event) override {}
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index 231300ce0bec..f63ad830c003 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()
>  	ctx_->ops->stop(ctx_);
>  }
> 
> -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> -				  const std::map<unsigned int, IPAStream> &streamConfig,
> -				  const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -				  const IPAOperationData &ipaConfig,
> -				  IPAOperationData *result)
> +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> +				 const std::map<unsigned int, IPAStream> &streamConfig,
> +				 const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +				 const IPAOperationData &ipaConfig,
> +				 IPAOperationData *result)
>  {
>  	if (intf_)
>  		return intf_->configure(sensorInfo, streamConfig,
>  					entityControls, ipaConfig, result);
> 
>  	if (!ctx_)
> -		return;
> +		return 0;
> 
>  	serializer_.reset();
> 
> @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>  	}
> 
>  	/* \todo Translate the ipaConfig and reponse */
> -	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
> -			     c_info_maps, entityControls.size());
> +	return ctx_->ops->configure(ctx_, &sensor_info, c_streams,
> +				    streamConfig.size(), c_info_maps,
> +				    entityControls.size());
>  }
> 
>  void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 23fc56d7d48e..516a8ecd4b53 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -347,6 +347,8 @@
>   * \param[in] num_maps The number of entries in the \a maps array
>   *
>   * \sa libcamera::IPAInterface::configure()
> + *
> + * \return 0 on success or a negative error code on failure
>   */
> 
>  /**
> @@ -573,6 +575,8 @@ namespace libcamera {
>   * pipeline handler to the IPA and back. The pipeline handler may set the \a
>   * result parameter to null if the IPA protocol doesn't need to pass a result
>   * back through the configure() function.
> + *
> + * \return 0 on success or a negative error code on failure
>   */
> 
>  /**
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index b78a0e4535f5..ed250ce79c17 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -32,11 +32,11 @@ public:
>  	}
>  	int start() override { return 0; }
>  	void stop() override {}
> -	void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> -		       [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> -		       [[maybe_unused]] const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -		       [[maybe_unused]] const IPAOperationData &ipaConfig,
> -		       [[maybe_unused]] IPAOperationData *result) override {}
> +	int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> +		      [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> +		      [[maybe_unused]] const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		      [[maybe_unused]] const IPAOperationData &ipaConfig,
> +		      [[maybe_unused]] IPAOperationData *result) override { return 0; }
>  	void mapBuffers([[maybe_unused]] const std::vector<IPABuffer> &buffers) override {}
>  	void unmapBuffers([[maybe_unused]] const std::vector<unsigned int> &ids) override {}
>  	void processEvent([[maybe_unused]] const IPAOperationData &event) override {}
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index eead2883708d..fd91726c4840 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -29,11 +29,11 @@ public:
>  	int start() override;
>  	void stop() override;
> 
> -	void configure(const CameraSensorInfo &sensorInfo,
> -		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -		       const IPAOperationData &ipaConfig,
> -		       IPAOperationData *result) override;
> +	int configure(const CameraSensorInfo &sensorInfo,
> +		      const std::map<unsigned int, IPAStream> &streamConfig,
> +		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		      const IPAOperationData &ipaConfig,
> +		      IPAOperationData *result) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  	void processEvent(const IPAOperationData &event) override;
> @@ -132,14 +132,14 @@ void IPAProxyThread::stop()
>  	thread_.wait();
>  }
> 
> -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> -			       const std::map<unsigned int, IPAStream> &streamConfig,
> -			       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -			       const IPAOperationData &ipaConfig,
> -			       IPAOperationData *result)
> +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> +			      const std::map<unsigned int, IPAStream> &streamConfig,
> +			      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +			      const IPAOperationData &ipaConfig,
> +			      IPAOperationData *result)
>  {
> -	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> -			result);
> +	return ipa_->configure(sensorInfo, streamConfig, entityControls,
> +			       ipaConfig, result);
>  }
> 
>  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index 59d991cbbf6a..ad0fb0386865 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -67,55 +67,63 @@ public:
>  		report(Op_stop, TestPass);
>  	}
> 
> -	void configure(const CameraSensorInfo &sensorInfo,
> -		       const std::map<unsigned int, IPAStream> &streamConfig,
> -		       const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> -		       [[maybe_unused]] const IPAOperationData &ipaConfig,
> -		       [[maybe_unused]] IPAOperationData *result) override
> +	int configure(const CameraSensorInfo &sensorInfo,
> +		      const std::map<unsigned int, IPAStream> &streamConfig,
> +		      const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> +		      [[maybe_unused]] const IPAOperationData &ipaConfig,
> +		      [[maybe_unused]] IPAOperationData *result) override
>  	{
>  		/* Verify sensorInfo. */
>  		if (sensorInfo.outputSize.width != 2560 ||
>  		    sensorInfo.outputSize.height != 1940) {
>  			cerr << "configure(): Invalid sensor info size "
>  			     << sensorInfo.outputSize.toString();
> +			report(Op_configure, TestFail);
> +			return -EINVAL;
>  		}
> 
>  		/* Verify streamConfig. */
>  		if (streamConfig.size() != 2) {
>  			cerr << "configure(): Invalid number of streams "
>  			     << streamConfig.size() << endl;
> -			return report(Op_configure, TestFail);
> +			report(Op_configure, TestFail);
> +			return -EINVAL;
>  		}
> 
>  		auto iter = streamConfig.find(1);
>  		if (iter == streamConfig.end()) {
>  			cerr << "configure(): No configuration for stream 1" << endl;
> -			return report(Op_configure, TestFail);
> +			report(Op_configure, TestFail);
> +			return -EINVAL;
>  		}
>  		const IPAStream *stream = &iter->second;
>  		if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||
>  		    stream->size != Size{ 1024, 768 }) {
>  			cerr << "configure(): Invalid configuration for stream 1" << endl;
> -			return report(Op_configure, TestFail);
> +			report(Op_configure, TestFail);
> +			return -EINVAL;
>  		}
> 
>  		iter = streamConfig.find(2);
>  		if (iter == streamConfig.end()) {
>  			cerr << "configure(): No configuration for stream 2" << endl;
> -			return report(Op_configure, TestFail);
> +			report(Op_configure, TestFail);
> +			return -EINVAL;
>  		}
>  		stream = &iter->second;
>  		if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||
>  		    stream->size != Size{ 800, 600 }) {
>  			cerr << "configure(): Invalid configuration for stream 2" << endl;
> -			return report(Op_configure, TestFail);
> +			report(Op_configure, TestFail);
> +			return -EINVAL;
>  		}
> 
>  		/* Verify entityControls. */
>  		auto ctrlIter = entityControls.find(42);
>  		if (ctrlIter == entityControls.end()) {
>  			cerr << "configure(): Controls not found" << endl;
> -			return report(Op_configure, TestFail);
> +			report(Op_configure, TestFail);
> +			return -EINVAL;
>  		}
> 
>  		const ControlInfoMap &infoMap = ctrlIter->second;
> @@ -124,10 +132,12 @@ public:
>  		    infoMap.count(V4L2_CID_CONTRAST) != 1 ||
>  		    infoMap.count(V4L2_CID_SATURATION) != 1) {
>  			cerr << "configure(): Invalid control IDs" << endl;
> -			return report(Op_configure, TestFail);
> +			report(Op_configure, TestFail);
> +			return -EINVAL;
>  		}
> 
>  		report(Op_configure, TestPass);
> +		return 0;
>  	}
> 
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override
> 
> > ---
> >  include/libcamera/ipa/raspberrypi.h                |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp                | 12 +++++++++++-
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 +++++
> >  3 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index 2b55dbfc..86c97ffa 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -21,6 +21,7 @@ enum ConfigParameters {
> >  	IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> >  	IPA_CONFIG_SENSOR = (1 << 2),
> >  	IPA_CONFIG_DROP_FRAMES = (1 << 3),
> > +	IPA_CONFIG_FAILED = (1 << 4),
> >  };
> >  
> >  enum Operations {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 9853a343..57dd9c61 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >  		       const IPAOperationData &ipaConfig,
> >  		       IPAOperationData *result)
> >  {
> > -	if (entityControls.empty())
> > +	if (entityControls.size() != 2) {
> > +		LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > +		result->operation = RPi::IPA_CONFIG_FAILED;
> >  		return;
> > +	}
> >  
> >  	result->operation = 0;
> >  
> > @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >  	if (!helper_) {
> >  		helper_ = std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
> >  
> > +		if (!helper_) {
> > +			LOG(IPARPI, Error) << "Could not create camera helper for "
> > +					   << cameraName;
> > +			result->operation = RPi::IPA_CONFIG_FAILED;
> > +			return;
> > +		}
> > +
> >  		/*
> >  		 * Pass out the sensor config to the pipeline handler in order
> >  		 * to setup the staggered writer class.
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 6fcdf557..76252806 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> >  	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> >  			&result);
> >  
> > +	if (result.operation & RPi::IPA_CONFIG_FAILED) {
> > +		LOG(RPI, Error) << "IPA configuration failed!";
> > +		return -EPIPE;
> > +	}
> > +
> >  	unsigned int resultIdx = 0;
> >  	if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
> >  		/*
Naushir Patuck Dec. 7, 2020, 1:11 p.m. UTC | #6 
Hi Paul,

Thank you for your review.

On Fri, 4 Dec 2020 at 10:39, <paul.elder@ideasonboard.com> wrote:

> Hi Naush,
>
> On Tue, Dec 01, 2020 at 03:55:43AM +0200, Laurent Pinchart wrote:
> > Hi Naush,
> >
> > Thank you for the patch.
> >
> > On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:
> > > If the IPA fails during configuration, return an error flag to the
> > > pipeline handler and fail the use case gracefully.
> > >
> > > At present, the IPA configuration can fail for the following reasons:
> > > - The sensor is not recognised, and fails to open a CamHelper object.
> > > - The pipeline handler did not pass in controls for the ISP and sensor.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Wouldn't it be simpler to modify the configure() function to return an
> > int ? How about the following ? If it works for you I'll submit a proper
> > patch.
>
> From the perspective of the IPC changes, Naush's solution is more easily
> translatable. When the IPA interface will become customizable, there
> isn't a way to specify which output parameters should be return values
> vs return parameters. Thus keeping them all as return parameters is the
> better solution here.
>
> So imo this is fine to merge as-is.
>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>

In which case, I think this is ready to be merged :)

Regards,
Naush


>
> > Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Date:   Tue Dec 1 03:54:18 2020 +0200
> >
> >     libcamera: ipa_interface: Make configure() return an int
> >
> >     It's useful for the IPAInterface::configure() function to be able to
> >     return a status. Make it return an int, to avoid forcing IPAs to
> return
> >     a status encoded in the IPAOperationData in a custom way.
> >
> >     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
> b/include/libcamera/internal/ipa_context_wrapper.h
> > index 8f767e844221..a00b5e7b92eb 100644
> > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > @@ -22,11 +22,11 @@ public:
> >       int init(const IPASettings &settings) override;
> >       int start() override;
> >       void stop() override;
> > -     void configure(const CameraSensorInfo &sensorInfo,
> > -                    const std::map<unsigned int, IPAStream>
> &streamConfig,
> > -                    const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> > -                    const IPAOperationData &ipaConfig,
> > -                    IPAOperationData *result) override;
> > +     int configure(const CameraSensorInfo &sensorInfo,
> > +                   const std::map<unsigned int, IPAStream>
> &streamConfig,
> > +                   const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> > +                   const IPAOperationData &ipaConfig,
> > +                   IPAOperationData *result) override;
> >
> >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >       void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > diff --git a/include/libcamera/ipa/ipa_interface.h
> b/include/libcamera/ipa/ipa_interface.h
> > index 322b7079070e..1d8cf8dc1c56 100644
> > --- a/include/libcamera/ipa/ipa_interface.h
> > +++ b/include/libcamera/ipa/ipa_interface.h
> > @@ -95,12 +95,12 @@ struct ipa_context_ops {
> >       void (*register_callbacks)(struct ipa_context *ctx,
> >                                  const struct ipa_callback_ops
> *callbacks,
> >                                  void *cb_ctx);
> > -     void (*configure)(struct ipa_context *ctx,
> > -                       const struct ipa_sensor_info *sensor_info,
> > -                       const struct ipa_stream *streams,
> > -                       unsigned int num_streams,
> > -                       const struct ipa_control_info_map *maps,
> > -                       unsigned int num_maps);
> > +     int (*configure)(struct ipa_context *ctx,
> > +                      const struct ipa_sensor_info *sensor_info,
> > +                      const struct ipa_stream *streams,
> > +                      unsigned int num_streams,
> > +                      const struct ipa_control_info_map *maps,
> > +                      unsigned int num_maps);
> >       void (*map_buffers)(struct ipa_context *ctx,
> >                           const struct ipa_buffer *buffers,
> >                           size_t num_buffers);
> > @@ -156,11 +156,11 @@ public:
> >       virtual int start() = 0;
> >       virtual void stop() = 0;
> >
> > -     virtual void configure(const CameraSensorInfo &sensorInfo,
> > -                            const std::map<unsigned int, IPAStream>
> &streamConfig,
> > -                            const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > -                            const IPAOperationData &ipaConfig,
> > -                            IPAOperationData *result) = 0;
> > +     virtual int configure(const CameraSensorInfo &sensorInfo,
> > +                           const std::map<unsigned int, IPAStream>
> &streamConfig,
> > +                           const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > +                           const IPAOperationData &ipaConfig,
> > +                           IPAOperationData *result) = 0;
> >
> >       virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> >       virtual void unmapBuffers(const std::vector<unsigned int> &ids) =
> 0;
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
> b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index cee532e3a747..74d7bc6e1c9b 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -115,12 +115,12 @@ void
> IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
> >       ctx->cb_ctx_ = cb_ctx;
> >  }
> >
> > -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > -                                 const struct ipa_sensor_info
> *sensor_info,
> > -                                 const struct ipa_stream *streams,
> > -                                 unsigned int num_streams,
> > -                                 const struct ipa_control_info_map
> *maps,
> > -                                 unsigned int num_maps)
> > +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > +                                const struct ipa_sensor_info
> *sensor_info,
> > +                                const struct ipa_stream *streams,
> > +                                unsigned int num_streams,
> > +                                const struct ipa_control_info_map *maps,
> > +                                unsigned int num_maps)
> >  {
> >       IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
> *>(_ctx);
> >
> > @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct
> ipa_context *_ctx,
> >
> >       /* \todo Translate the ipaConfig and result. */
> >       IPAOperationData ipaConfig;
> > -     ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
> ipaConfig,
> > -                          nullptr);
> > +     return ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
> > +                                 ipaConfig, nullptr);
> >  }
> >
> >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h
> b/src/ipa/libipa/ipa_interface_wrapper.h
> > index a1c701599b56..acd3160039b1 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.h
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> > @@ -30,12 +30,12 @@ private:
> >       static void register_callbacks(struct ipa_context *ctx,
> >                                      const struct ipa_callback_ops
> *callbacks,
> >                                      void *cb_ctx);
> > -     static void configure(struct ipa_context *ctx,
> > -                           const struct ipa_sensor_info *sensor_info,
> > -                           const struct ipa_stream *streams,
> > -                           unsigned int num_streams,
> > -                           const struct ipa_control_info_map *maps,
> > -                           unsigned int num_maps);
> > +     static int configure(struct ipa_context *ctx,
> > +                          const struct ipa_sensor_info *sensor_info,
> > +                          const struct ipa_stream *streams,
> > +                          unsigned int num_streams,
> > +                          const struct ipa_control_info_map *maps,
> > +                          unsigned int num_maps);
> >       static void map_buffers(struct ipa_context *ctx,
> >                               const struct ipa_buffer *c_buffers,
> >                               size_t num_buffers);
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 9853a343c892..75f7af3430ef 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -81,11 +81,11 @@ public:
> >       int start() override { return 0; }
> >       void stop() override {}
> >
> > -     void configure(const CameraSensorInfo &sensorInfo,
> > -                    const std::map<unsigned int, IPAStream>
> &streamConfig,
> > -                    const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> > -                    const IPAOperationData &data,
> > -                    IPAOperationData *response) override;
> > +     int configure(const CameraSensorInfo &sensorInfo,
> > +                   const std::map<unsigned int, IPAStream>
> &streamConfig,
> > +                   const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> > +                   const IPAOperationData &data,
> > +                   IPAOperationData *response) override;
> >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >       void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >       void processEvent(const IPAOperationData &event) override;
> > @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo
> &sensorInfo)
> >       mode_.line_length = 1e9 * sensorInfo.lineLength /
> sensorInfo.pixelRate;
> >  }
> >
> > -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > -                    [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > -                    const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> > -                    const IPAOperationData &ipaConfig,
> > -                    IPAOperationData *result)
> > +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > +                   [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > +                   const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> > +                   const IPAOperationData &ipaConfig,
> > +                   IPAOperationData *result)
> >  {
> >       if (entityControls.empty())
> > -             return;
> > +             return -EINVAL;
> >
> >       result->operation = 0;
> >
> > @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >       }
> >
> >       lastMode_ = mode_;
> > +     return 0;
> >  }
> >
> >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 07d7f1b2ddd8..78d78c5ac521 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -40,11 +40,11 @@ public:
> >       int start() override { return 0; }
> >       void stop() override {}
> >
> > -     void configure(const CameraSensorInfo &info,
> > -                    const std::map<unsigned int, IPAStream>
> &streamConfig,
> > -                    const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> > -                    const IPAOperationData &ipaConfig,
> > -                    IPAOperationData *response) override;
> > +     int configure(const CameraSensorInfo &info,
> > +                   const std::map<unsigned int, IPAStream>
> &streamConfig,
> > +                   const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> > +                   const IPAOperationData &ipaConfig,
> > +                   IPAOperationData *response) override;
> >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >       void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >       void processEvent(const IPAOperationData &event) override;
> > @@ -79,27 +79,27 @@ private:
> >   * assemble one. Make sure the reported sensor information are relevant
> >   * before accessing them.
> >   */
> > -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> > -                       [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > -                       const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > -                       [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > -                       [[maybe_unused]] IPAOperationData *result)
> > +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
> > +                      [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > +                      const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> > +                      [[maybe_unused]] const IPAOperationData
> &ipaConfig,
> > +                      [[maybe_unused]] IPAOperationData *result)
> >  {
> >       if (entityControls.empty())
> > -             return;
> > +             return -EINVAL;
> >
> >       ctrls_ = entityControls.at(0);
> >
> >       const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> >       if (itExp == ctrls_.end()) {
> >               LOG(IPARkISP1, Error) << "Can't find exposure control";
> > -             return;
> > +             return -EINVAL;
> >       }
> >
> >       const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> >       if (itGain == ctrls_.end()) {
> >               LOG(IPARkISP1, Error) << "Can't find gain control";
> > -             return;
> > +             return -EINVAL;
> >       }
> >
> >       autoExposure_ = true;
> > @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const
> CameraSensorInfo &info,
> >               << " Gain: " << minGain_ << "-" << maxGain_;
> >
> >       setControls(0);
> > +     return 0;
> >  }
> >
> >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index cf8411359e40..79c8c2fb8927 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -37,11 +37,11 @@ public:
> >       int start() override;
> >       void stop() override;
> >
> > -     void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > -                    [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > -                    [[maybe_unused]] const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > -                    [[maybe_unused]] const IPAOperationData &ipaConfig,
> > -                    [[maybe_unused]] IPAOperationData *result) override
> {}
> > +     int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > +                   [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > +                   [[maybe_unused]] const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > +                   [[maybe_unused]] const IPAOperationData &ipaConfig,
> > +                   [[maybe_unused]] IPAOperationData *result) override
> { return 0; }
> >       void mapBuffers([[maybe_unused]] const std::vector<IPABuffer>
> &buffers) override {}
> >       void unmapBuffers([[maybe_unused]] const std::vector<unsigned int>
> &ids) override {}
> >       void processEvent([[maybe_unused]] const IPAOperationData &event)
> override {}
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp
> b/src/libcamera/ipa_context_wrapper.cpp
> > index 231300ce0bec..f63ad830c003 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()
> >       ctx_->ops->stop(ctx_);
> >  }
> >
> > -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > -                               const std::map<unsigned int, IPAStream>
> &streamConfig,
> > -                               const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > -                               const IPAOperationData &ipaConfig,
> > -                               IPAOperationData *result)
> > +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > +                              const std::map<unsigned int, IPAStream>
> &streamConfig,
> > +                              const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > +                              const IPAOperationData &ipaConfig,
> > +                              IPAOperationData *result)
> >  {
> >       if (intf_)
> >               return intf_->configure(sensorInfo, streamConfig,
> >                                       entityControls, ipaConfig, result);
> >
> >       if (!ctx_)
> > -             return;
> > +             return 0;
> >
> >       serializer_.reset();
> >
> > @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const
> CameraSensorInfo &sensorInfo,
> >       }
> >
> >       /* \todo Translate the ipaConfig and reponse */
> > -     ctx_->ops->configure(ctx_, &sensor_info, c_streams,
> streamConfig.size(),
> > -                          c_info_maps, entityControls.size());
> > +     return ctx_->ops->configure(ctx_, &sensor_info, c_streams,
> > +                                 streamConfig.size(), c_info_maps,
> > +                                 entityControls.size());
> >  }
> >
> >  void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer>
> &buffers)
> > diff --git a/src/libcamera/ipa_interface.cpp
> b/src/libcamera/ipa_interface.cpp
> > index 23fc56d7d48e..516a8ecd4b53 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -347,6 +347,8 @@
> >   * \param[in] num_maps The number of entries in the \a maps array
> >   *
> >   * \sa libcamera::IPAInterface::configure()
> > + *
> > + * \return 0 on success or a negative error code on failure
> >   */
> >
> >  /**
> > @@ -573,6 +575,8 @@ namespace libcamera {
> >   * pipeline handler to the IPA and back. The pipeline handler may set
> the \a
> >   * result parameter to null if the IPA protocol doesn't need to pass a
> result
> >   * back through the configure() function.
> > + *
> > + * \return 0 on success or a negative error code on failure
> >   */
> >
> >  /**
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
> b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index b78a0e4535f5..ed250ce79c17 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -32,11 +32,11 @@ public:
> >       }
> >       int start() override { return 0; }
> >       void stop() override {}
> > -     void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > -                    [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > -                    [[maybe_unused]] const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > -                    [[maybe_unused]] const IPAOperationData &ipaConfig,
> > -                    [[maybe_unused]] IPAOperationData *result) override
> {}
> > +     int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,
> > +                   [[maybe_unused]] const std::map<unsigned int,
> IPAStream> &streamConfig,
> > +                   [[maybe_unused]] const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > +                   [[maybe_unused]] const IPAOperationData &ipaConfig,
> > +                   [[maybe_unused]] IPAOperationData *result) override
> { return 0; }
> >       void mapBuffers([[maybe_unused]] const std::vector<IPABuffer>
> &buffers) override {}
> >       void unmapBuffers([[maybe_unused]] const std::vector<unsigned int>
> &ids) override {}
> >       void processEvent([[maybe_unused]] const IPAOperationData &event)
> override {}
> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
> b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > index eead2883708d..fd91726c4840 100644
> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > @@ -29,11 +29,11 @@ public:
> >       int start() override;
> >       void stop() override;
> >
> > -     void configure(const CameraSensorInfo &sensorInfo,
> > -                    const std::map<unsigned int, IPAStream>
> &streamConfig,
> > -                    const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> > -                    const IPAOperationData &ipaConfig,
> > -                    IPAOperationData *result) override;
> > +     int configure(const CameraSensorInfo &sensorInfo,
> > +                   const std::map<unsigned int, IPAStream>
> &streamConfig,
> > +                   const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> > +                   const IPAOperationData &ipaConfig,
> > +                   IPAOperationData *result) override;
> >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >       void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >       void processEvent(const IPAOperationData &event) override;
> > @@ -132,14 +132,14 @@ void IPAProxyThread::stop()
> >       thread_.wait();
> >  }
> >
> > -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > -                            const std::map<unsigned int, IPAStream>
> &streamConfig,
> > -                            const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > -                            const IPAOperationData &ipaConfig,
> > -                            IPAOperationData *result)
> > +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > +                           const std::map<unsigned int, IPAStream>
> &streamConfig,
> > +                           const std::map<unsigned int, const
> ControlInfoMap &> &entityControls,
> > +                           const IPAOperationData &ipaConfig,
> > +                           IPAOperationData *result)
> >  {
> > -     ipa_->configure(sensorInfo, streamConfig, entityControls,
> ipaConfig,
> > -                     result);
> > +     return ipa_->configure(sensorInfo, streamConfig, entityControls,
> > +                            ipaConfig, result);
> >  }
> >
> >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/test/ipa/ipa_wrappers_test.cpp
> b/test/ipa/ipa_wrappers_test.cpp
> > index 59d991cbbf6a..ad0fb0386865 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -67,55 +67,63 @@ public:
> >               report(Op_stop, TestPass);
> >       }
> >
> > -     void configure(const CameraSensorInfo &sensorInfo,
> > -                    const std::map<unsigned int, IPAStream>
> &streamConfig,
> > -                    const std::map<unsigned int, const ControlInfoMap
> &> &entityControls,
> > -                    [[maybe_unused]] const IPAOperationData &ipaConfig,
> > -                    [[maybe_unused]] IPAOperationData *result) override
> > +     int configure(const CameraSensorInfo &sensorInfo,
> > +                   const std::map<unsigned int, IPAStream>
> &streamConfig,
> > +                   const std::map<unsigned int, const ControlInfoMap &>
> &entityControls,
> > +                   [[maybe_unused]] const IPAOperationData &ipaConfig,
> > +                   [[maybe_unused]] IPAOperationData *result) override
> >       {
> >               /* Verify sensorInfo. */
> >               if (sensorInfo.outputSize.width != 2560 ||
> >                   sensorInfo.outputSize.height != 1940) {
> >                       cerr << "configure(): Invalid sensor info size "
> >                            << sensorInfo.outputSize.toString();
> > +                     report(Op_configure, TestFail);
> > +                     return -EINVAL;
> >               }
> >
> >               /* Verify streamConfig. */
> >               if (streamConfig.size() != 2) {
> >                       cerr << "configure(): Invalid number of streams "
> >                            << streamConfig.size() << endl;
> > -                     return report(Op_configure, TestFail);
> > +                     report(Op_configure, TestFail);
> > +                     return -EINVAL;
> >               }
> >
> >               auto iter = streamConfig.find(1);
> >               if (iter == streamConfig.end()) {
> >                       cerr << "configure(): No configuration for stream
> 1" << endl;
> > -                     return report(Op_configure, TestFail);
> > +                     report(Op_configure, TestFail);
> > +                     return -EINVAL;
> >               }
> >               const IPAStream *stream = &iter->second;
> >               if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||
> >                   stream->size != Size{ 1024, 768 }) {
> >                       cerr << "configure(): Invalid configuration for
> stream 1" << endl;
> > -                     return report(Op_configure, TestFail);
> > +                     report(Op_configure, TestFail);
> > +                     return -EINVAL;
> >               }
> >
> >               iter = streamConfig.find(2);
> >               if (iter == streamConfig.end()) {
> >                       cerr << "configure(): No configuration for stream
> 2" << endl;
> > -                     return report(Op_configure, TestFail);
> > +                     report(Op_configure, TestFail);
> > +                     return -EINVAL;
> >               }
> >               stream = &iter->second;
> >               if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||
> >                   stream->size != Size{ 800, 600 }) {
> >                       cerr << "configure(): Invalid configuration for
> stream 2" << endl;
> > -                     return report(Op_configure, TestFail);
> > +                     report(Op_configure, TestFail);
> > +                     return -EINVAL;
> >               }
> >
> >               /* Verify entityControls. */
> >               auto ctrlIter = entityControls.find(42);
> >               if (ctrlIter == entityControls.end()) {
> >                       cerr << "configure(): Controls not found" << endl;
> > -                     return report(Op_configure, TestFail);
> > +                     report(Op_configure, TestFail);
> > +                     return -EINVAL;
> >               }
> >
> >               const ControlInfoMap &infoMap = ctrlIter->second;
> > @@ -124,10 +132,12 @@ public:
> >                   infoMap.count(V4L2_CID_CONTRAST) != 1 ||
> >                   infoMap.count(V4L2_CID_SATURATION) != 1) {
> >                       cerr << "configure(): Invalid control IDs" << endl;
> > -                     return report(Op_configure, TestFail);
> > +                     report(Op_configure, TestFail);
> > +                     return -EINVAL;
> >               }
> >
> >               report(Op_configure, TestPass);
> > +             return 0;
> >       }
> >
> >       void mapBuffers(const std::vector<IPABuffer> &buffers) override
> >
> > > ---
> > >  include/libcamera/ipa/raspberrypi.h                |  1 +
> > >  src/ipa/raspberrypi/raspberrypi.cpp                | 12 +++++++++++-
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 +++++
> > >  3 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.h
> b/include/libcamera/ipa/raspberrypi.h
> > > index 2b55dbfc..86c97ffa 100644
> > > --- a/include/libcamera/ipa/raspberrypi.h
> > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > @@ -21,6 +21,7 @@ enum ConfigParameters {
> > >     IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> > >     IPA_CONFIG_SENSOR = (1 << 2),
> > >     IPA_CONFIG_DROP_FRAMES = (1 << 3),
> > > +   IPA_CONFIG_FAILED = (1 << 4),
> > >  };
> > >
> > >  enum Operations {
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 9853a343..57dd9c61 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > >                    const IPAOperationData &ipaConfig,
> > >                    IPAOperationData *result)
> > >  {
> > > -   if (entityControls.empty())
> > > +   if (entityControls.size() != 2) {
> > > +           LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > > +           result->operation = RPi::IPA_CONFIG_FAILED;
> > >             return;
> > > +   }
> > >
> > >     result->operation = 0;
> > >
> > > @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > >     if (!helper_) {
> > >             helper_ =
> std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
> > >
> > > +           if (!helper_) {
> > > +                   LOG(IPARPI, Error) << "Could not create camera
> helper for "
> > > +                                      << cameraName;
> > > +                   result->operation = RPi::IPA_CONFIG_FAILED;
> > > +                   return;
> > > +           }
> > > +
> > >             /*
> > >              * Pass out the sensor config to the pipeline handler in
> order
> > >              * to setup the staggered writer class.
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 6fcdf557..76252806 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> > >     ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> > >                     &result);
> > >
> > > +   if (result.operation & RPi::IPA_CONFIG_FAILED) {
> > > +           LOG(RPI, Error) << "IPA configuration failed!";
> > > +           return -EPIPE;
> > > +   }
> > > +
> > >     unsigned int resultIdx = 0;
> > >     if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
> > >             /*
>
Kieran Bingham Dec. 7, 2020, 3:14 p.m. UTC | #7 
Hi Naush, Paul,

On 07/12/2020 13:11, Naushir Patuck wrote:
> Hi Paul,
> 
> Thank you for your review.
> 
> On Fri, 4 Dec 2020 at 10:39, <paul.elder@ideasonboard.com
> <mailto:paul.elder@ideasonboard.com>> wrote:
> 
>     Hi Naush,
> 
>     On Tue, Dec 01, 2020 at 03:55:43AM +0200, Laurent Pinchart wrote:
>     > Hi Naush,
>     >
>     > Thank you for the patch.
>     >
>     > On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:
>     > > If the IPA fails during configuration, return an error flag to the
>     > > pipeline handler and fail the use case gracefully.
>     > >
>     > > At present, the IPA configuration can fail for the following
>     reasons:
>     > > - The sensor is not recognised, and fails to open a CamHelper
>     object.
>     > > - The pipeline handler did not pass in controls for the ISP and
>     sensor.
>     > >
>     > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com
>     <mailto:naush@raspberrypi.com>>
>     > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org
>     <mailto:jacopo@jmondi.org>>
>     >
>     > Wouldn't it be simpler to modify the configure() function to return an
>     > int ? How about the following ? If it works for you I'll submit a
>     proper
>     > patch.
> 
>     From the perspective of the IPC changes, Naush's solution is more easily
>     translatable. When the IPA interface will become customizable, there
>     isn't a way to specify which output parameters should be return values
>     vs return parameters. Thus keeping them all as return parameters is the
>     better solution here.
> 
>     So imo this is fine to merge as-is.
> 
>     Reviewed-by: Paul Elder <paul.elder@ideasonboard.com
>     <mailto:paul.elder@ideasonboard.com>>
> 
> 
> In which case, I think this is ready to be merged :)

Pushed.

Regards
--
Kieran


> Regards,
> Naush
>  
> 
> 
>     > Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com
>     <mailto:laurent.pinchart@ideasonboard.com>>
>     > Date:   Tue Dec 1 03:54:18 2020 +0200
>     >
>     >     libcamera: ipa_interface: Make configure() return an int
>     >
>     >     It's useful for the IPAInterface::configure() function to be
>     able to
>     >     return a status. Make it return an int, to avoid forcing IPAs
>     to return
>     >     a status encoded in the IPAOperationData in a custom way.
>     >
>     >     Signed-off-by: Laurent Pinchart
>     <laurent.pinchart@ideasonboard.com
>     <mailto:laurent.pinchart@ideasonboard.com>>
>     >
>     > diff --git a/include/libcamera/internal/ipa_context_wrapper.h
>     b/include/libcamera/internal/ipa_context_wrapper.h
>     > index 8f767e844221..a00b5e7b92eb 100644
>     > --- a/include/libcamera/internal/ipa_context_wrapper.h
>     > +++ b/include/libcamera/internal/ipa_context_wrapper.h
>     > @@ -22,11 +22,11 @@ public:
>     >       int init(const IPASettings &settings) override;
>     >       int start() override;
>     >       void stop() override;
>     > -     void configure(const CameraSensorInfo &sensorInfo,
>     > -                    const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    const IPAOperationData &ipaConfig,
>     > -                    IPAOperationData *result) override;
>     > +     int configure(const CameraSensorInfo &sensorInfo,
>     > +                   const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   const IPAOperationData &ipaConfig,
>     > +                   IPAOperationData *result) override;
>     >
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>     >       void unmapBuffers(const std::vector<unsigned int> &ids)
>     override;
>     > diff --git a/include/libcamera/ipa/ipa_interface.h
>     b/include/libcamera/ipa/ipa_interface.h
>     > index 322b7079070e..1d8cf8dc1c56 100644
>     > --- a/include/libcamera/ipa/ipa_interface.h
>     > +++ b/include/libcamera/ipa/ipa_interface.h
>     > @@ -95,12 +95,12 @@ struct ipa_context_ops {
>     >       void (*register_callbacks)(struct ipa_context *ctx,
>     >                                  const struct ipa_callback_ops
>     *callbacks,
>     >                                  void *cb_ctx);
>     > -     void (*configure)(struct ipa_context *ctx,
>     > -                       const struct ipa_sensor_info *sensor_info,
>     > -                       const struct ipa_stream *streams,
>     > -                       unsigned int num_streams,
>     > -                       const struct ipa_control_info_map *maps,
>     > -                       unsigned int num_maps);
>     > +     int (*configure)(struct ipa_context *ctx,
>     > +                      const struct ipa_sensor_info *sensor_info,
>     > +                      const struct ipa_stream *streams,
>     > +                      unsigned int num_streams,
>     > +                      const struct ipa_control_info_map *maps,
>     > +                      unsigned int num_maps);
>     >       void (*map_buffers)(struct ipa_context *ctx,
>     >                           const struct ipa_buffer *buffers,
>     >                           size_t num_buffers);
>     > @@ -156,11 +156,11 @@ public:
>     >       virtual int start() = 0;
>     >       virtual void stop() = 0;
>     >
>     > -     virtual void configure(const CameraSensorInfo &sensorInfo,
>     > -                            const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                            const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                            const IPAOperationData &ipaConfig,
>     > -                            IPAOperationData *result) = 0;
>     > +     virtual int configure(const CameraSensorInfo &sensorInfo,
>     > +                           const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                           const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                           const IPAOperationData &ipaConfig,
>     > +                           IPAOperationData *result) = 0;
>     >
>     >       virtual void mapBuffers(const std::vector<IPABuffer>
>     &buffers) = 0;
>     >       virtual void unmapBuffers(const std::vector<unsigned int>
>     &ids) = 0;
>     > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp
>     b/src/ipa/libipa/ipa_interface_wrapper.cpp
>     > index cee532e3a747..74d7bc6e1c9b 100644
>     > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
>     > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
>     > @@ -115,12 +115,12 @@ void
>     IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
>     >       ctx->cb_ctx_ = cb_ctx;
>     >  }
>     >
>     > -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
>     > -                                 const struct ipa_sensor_info
>     *sensor_info,
>     > -                                 const struct ipa_stream *streams,
>     > -                                 unsigned int num_streams,
>     > -                                 const struct
>     ipa_control_info_map *maps,
>     > -                                 unsigned int num_maps)
>     > +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
>     > +                                const struct ipa_sensor_info
>     *sensor_info,
>     > +                                const struct ipa_stream *streams,
>     > +                                unsigned int num_streams,
>     > +                                const struct ipa_control_info_map
>     *maps,
>     > +                                unsigned int num_maps)
>     >  {
>     >       IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper
>     *>(_ctx);
>     >
>     > @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct
>     ipa_context *_ctx,
>     >
>     >       /* \todo Translate the ipaConfig and result. */
>     >       IPAOperationData ipaConfig;
>     > -     ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,
>     ipaConfig,
>     > -                          nullptr);
>     > +     return ctx->ipa_->configure(sensorInfo, ipaStreams,
>     entityControls,
>     > +                                 ipaConfig, nullptr);
>     >  }
>     >
>     >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
>     > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h
>     b/src/ipa/libipa/ipa_interface_wrapper.h
>     > index a1c701599b56..acd3160039b1 100644
>     > --- a/src/ipa/libipa/ipa_interface_wrapper.h
>     > +++ b/src/ipa/libipa/ipa_interface_wrapper.h
>     > @@ -30,12 +30,12 @@ private:
>     >       static void register_callbacks(struct ipa_context *ctx,
>     >                                      const struct ipa_callback_ops
>     *callbacks,
>     >                                      void *cb_ctx);
>     > -     static void configure(struct ipa_context *ctx,
>     > -                           const struct ipa_sensor_info *sensor_info,
>     > -                           const struct ipa_stream *streams,
>     > -                           unsigned int num_streams,
>     > -                           const struct ipa_control_info_map *maps,
>     > -                           unsigned int num_maps);
>     > +     static int configure(struct ipa_context *ctx,
>     > +                          const struct ipa_sensor_info *sensor_info,
>     > +                          const struct ipa_stream *streams,
>     > +                          unsigned int num_streams,
>     > +                          const struct ipa_control_info_map *maps,
>     > +                          unsigned int num_maps);
>     >       static void map_buffers(struct ipa_context *ctx,
>     >                               const struct ipa_buffer *c_buffers,
>     >                               size_t num_buffers);
>     > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>     b/src/ipa/raspberrypi/raspberrypi.cpp
>     > index 9853a343c892..75f7af3430ef 100644
>     > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>     > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>     > @@ -81,11 +81,11 @@ public:
>     >       int start() override { return 0; }
>     >       void stop() override {}
>     >
>     > -     void configure(const CameraSensorInfo &sensorInfo,
>     > -                    const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    const IPAOperationData &data,
>     > -                    IPAOperationData *response) override;
>     > +     int configure(const CameraSensorInfo &sensorInfo,
>     > +                   const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   const IPAOperationData &data,
>     > +                   IPAOperationData *response) override;
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>     >       void unmapBuffers(const std::vector<unsigned int> &ids)
>     override;
>     >       void processEvent(const IPAOperationData &event) override;
>     > @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo
>     &sensorInfo)
>     >       mode_.line_length = 1e9 * sensorInfo.lineLength /
>     sensorInfo.pixelRate;
>     >  }
>     >
>     > -void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    const IPAOperationData &ipaConfig,
>     > -                    IPAOperationData *result)
>     > +int IPARPi::configure(const CameraSensorInfo &sensorInfo,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   const IPAOperationData &ipaConfig,
>     > +                   IPAOperationData *result)
>     >  {
>     >       if (entityControls.empty())
>     > -             return;
>     > +             return -EINVAL;
>     >
>     >       result->operation = 0;
>     >
>     > @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo
>     &sensorInfo,
>     >       }
>     >
>     >       lastMode_ = mode_;
>     > +     return 0;
>     >  }
>     >
>     >  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
>     > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>     > index 07d7f1b2ddd8..78d78c5ac521 100644
>     > --- a/src/ipa/rkisp1/rkisp1.cpp
>     > +++ b/src/ipa/rkisp1/rkisp1.cpp
>     > @@ -40,11 +40,11 @@ public:
>     >       int start() override { return 0; }
>     >       void stop() override {}
>     >
>     > -     void configure(const CameraSensorInfo &info,
>     > -                    const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    const IPAOperationData &ipaConfig,
>     > -                    IPAOperationData *response) override;
>     > +     int configure(const CameraSensorInfo &info,
>     > +                   const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   const IPAOperationData &ipaConfig,
>     > +                   IPAOperationData *response) override;
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>     >       void unmapBuffers(const std::vector<unsigned int> &ids)
>     override;
>     >       void processEvent(const IPAOperationData &event) override;
>     > @@ -79,27 +79,27 @@ private:
>     >   * assemble one. Make sure the reported sensor information are
>     relevant
>     >   * before accessing them.
>     >   */
>     > -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo
>     &info,
>     > -                       [[maybe_unused]] const std::map<unsigned
>     int, IPAStream> &streamConfig,
>     > -                       const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                       [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > -                       [[maybe_unused]] IPAOperationData *result)
>     > +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo
>     &info,
>     > +                      [[maybe_unused]] const std::map<unsigned
>     int, IPAStream> &streamConfig,
>     > +                      const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                      [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > +                      [[maybe_unused]] IPAOperationData *result)
>     >  {
>     >       if (entityControls.empty())
>     > -             return;
>     > +             return -EINVAL;
>     >
>     >       ctrls_ = entityControls.at(0);
>     >
>     >       const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
>     >       if (itExp == ctrls_.end()) {
>     >               LOG(IPARkISP1, Error) << "Can't find exposure control";
>     > -             return;
>     > +             return -EINVAL;
>     >       }
>     >
>     >       const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>     >       if (itGain == ctrls_.end()) {
>     >               LOG(IPARkISP1, Error) << "Can't find gain control";
>     > -             return;
>     > +             return -EINVAL;
>     >       }
>     >
>     >       autoExposure_ = true;
>     > @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]]
>     const CameraSensorInfo &info,
>     >               << " Gain: " << minGain_ << "-" << maxGain_;
>     >
>     >       setControls(0);
>     > +     return 0;
>     >  }
>     >
>     >  void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
>     > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
>     > index cf8411359e40..79c8c2fb8927 100644
>     > --- a/src/ipa/vimc/vimc.cpp
>     > +++ b/src/ipa/vimc/vimc.cpp
>     > @@ -37,11 +37,11 @@ public:
>     >       int start() override;
>     >       void stop() override;
>     >
>     > -     void configure([[maybe_unused]] const CameraSensorInfo
>     &sensorInfo,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     const ControlInfoMap &> &entityControls,
>     > -                    [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > -                    [[maybe_unused]] IPAOperationData *result)
>     override {}
>     > +     int configure([[maybe_unused]] const CameraSensorInfo
>     &sensorInfo,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     const ControlInfoMap &> &entityControls,
>     > +                   [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > +                   [[maybe_unused]] IPAOperationData *result)
>     override { return 0; }
>     >       void mapBuffers([[maybe_unused]] const
>     std::vector<IPABuffer> &buffers) override {}
>     >       void unmapBuffers([[maybe_unused]] const
>     std::vector<unsigned int> &ids) override {}
>     >       void processEvent([[maybe_unused]] const IPAOperationData
>     &event) override {}
>     > diff --git a/src/libcamera/ipa_context_wrapper.cpp
>     b/src/libcamera/ipa_context_wrapper.cpp
>     > index 231300ce0bec..f63ad830c003 100644
>     > --- a/src/libcamera/ipa_context_wrapper.cpp
>     > +++ b/src/libcamera/ipa_context_wrapper.cpp
>     > @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()
>     >       ctx_->ops->stop(ctx_);
>     >  }
>     >
>     > -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>     > -                               const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                               const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                               const IPAOperationData &ipaConfig,
>     > -                               IPAOperationData *result)
>     > +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
>     > +                              const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                              const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                              const IPAOperationData &ipaConfig,
>     > +                              IPAOperationData *result)
>     >  {
>     >       if (intf_)
>     >               return intf_->configure(sensorInfo, streamConfig,
>     >                                       entityControls, ipaConfig,
>     result);
>     >
>     >       if (!ctx_)
>     > -             return;
>     > +             return 0;
>     >
>     >       serializer_.reset();
>     >
>     > @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const
>     CameraSensorInfo &sensorInfo,
>     >       }
>     >
>     >       /* \todo Translate the ipaConfig and reponse */
>     > -     ctx_->ops->configure(ctx_, &sensor_info, c_streams,
>     streamConfig.size(),
>     > -                          c_info_maps, entityControls.size());
>     > +     return ctx_->ops->configure(ctx_, &sensor_info, c_streams,
>     > +                                 streamConfig.size(), c_info_maps,
>     > +                                 entityControls.size());
>     >  }
>     >
>     >  void IPAContextWrapper::mapBuffers(const std::vector<IPABuffer>
>     &buffers)
>     > diff --git a/src/libcamera/ipa_interface.cpp
>     b/src/libcamera/ipa_interface.cpp
>     > index 23fc56d7d48e..516a8ecd4b53 100644
>     > --- a/src/libcamera/ipa_interface.cpp
>     > +++ b/src/libcamera/ipa_interface.cpp
>     > @@ -347,6 +347,8 @@
>     >   * \param[in] num_maps The number of entries in the \a maps array
>     >   *
>     >   * \sa libcamera::IPAInterface::configure()
>     > + *
>     > + * \return 0 on success or a negative error code on failure
>     >   */
>     >
>     >  /**
>     > @@ -573,6 +575,8 @@ namespace libcamera {
>     >   * pipeline handler to the IPA and back. The pipeline handler may
>     set the \a
>     >   * result parameter to null if the IPA protocol doesn't need to
>     pass a result
>     >   * back through the configure() function.
>     > + *
>     > + * \return 0 on success or a negative error code on failure
>     >   */
>     >
>     >  /**
>     > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp
>     b/src/libcamera/proxy/ipa_proxy_linux.cpp
>     > index b78a0e4535f5..ed250ce79c17 100644
>     > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
>     > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
>     > @@ -32,11 +32,11 @@ public:
>     >       }
>     >       int start() override { return 0; }
>     >       void stop() override {}
>     > -     void configure([[maybe_unused]] const CameraSensorInfo
>     &sensorInfo,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                    [[maybe_unused]] const std::map<unsigned int,
>     const ControlInfoMap &> &entityControls,
>     > -                    [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > -                    [[maybe_unused]] IPAOperationData *result)
>     override {}
>     > +     int configure([[maybe_unused]] const CameraSensorInfo
>     &sensorInfo,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                   [[maybe_unused]] const std::map<unsigned int,
>     const ControlInfoMap &> &entityControls,
>     > +                   [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > +                   [[maybe_unused]] IPAOperationData *result)
>     override { return 0; }
>     >       void mapBuffers([[maybe_unused]] const
>     std::vector<IPABuffer> &buffers) override {}
>     >       void unmapBuffers([[maybe_unused]] const
>     std::vector<unsigned int> &ids) override {}
>     >       void processEvent([[maybe_unused]] const IPAOperationData
>     &event) override {}
>     > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp
>     b/src/libcamera/proxy/ipa_proxy_thread.cpp
>     > index eead2883708d..fd91726c4840 100644
>     > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
>     > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
>     > @@ -29,11 +29,11 @@ public:
>     >       int start() override;
>     >       void stop() override;
>     >
>     > -     void configure(const CameraSensorInfo &sensorInfo,
>     > -                    const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    const IPAOperationData &ipaConfig,
>     > -                    IPAOperationData *result) override;
>     > +     int configure(const CameraSensorInfo &sensorInfo,
>     > +                   const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   const IPAOperationData &ipaConfig,
>     > +                   IPAOperationData *result) override;
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>     >       void unmapBuffers(const std::vector<unsigned int> &ids)
>     override;
>     >       void processEvent(const IPAOperationData &event) override;
>     > @@ -132,14 +132,14 @@ void IPAProxyThread::stop()
>     >       thread_.wait();
>     >  }
>     >
>     > -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
>     > -                            const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > -                            const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                            const IPAOperationData &ipaConfig,
>     > -                            IPAOperationData *result)
>     > +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
>     > +                           const std::map<unsigned int,
>     IPAStream> &streamConfig,
>     > +                           const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                           const IPAOperationData &ipaConfig,
>     > +                           IPAOperationData *result)
>     >  {
>     > -     ipa_->configure(sensorInfo, streamConfig, entityControls,
>     ipaConfig,
>     > -                     result);
>     > +     return ipa_->configure(sensorInfo, streamConfig, entityControls,
>     > +                            ipaConfig, result);
>     >  }
>     >
>     >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer>
>     &buffers)
>     > diff --git a/test/ipa/ipa_wrappers_test.cpp
>     b/test/ipa/ipa_wrappers_test.cpp
>     > index 59d991cbbf6a..ad0fb0386865 100644
>     > --- a/test/ipa/ipa_wrappers_test.cpp
>     > +++ b/test/ipa/ipa_wrappers_test.cpp
>     > @@ -67,55 +67,63 @@ public:
>     >               report(Op_stop, TestPass);
>     >       }
>     >
>     > -     void configure(const CameraSensorInfo &sensorInfo,
>     > -                    const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > -                    const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > -                    [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > -                    [[maybe_unused]] IPAOperationData *result)
>     override
>     > +     int configure(const CameraSensorInfo &sensorInfo,
>     > +                   const std::map<unsigned int, IPAStream>
>     &streamConfig,
>     > +                   const std::map<unsigned int, const
>     ControlInfoMap &> &entityControls,
>     > +                   [[maybe_unused]] const IPAOperationData
>     &ipaConfig,
>     > +                   [[maybe_unused]] IPAOperationData *result)
>     override
>     >       {
>     >               /* Verify sensorInfo. */
>     >               if (sensorInfo.outputSize.width != 2560 ||
>     >                   sensorInfo.outputSize.height != 1940) {
>     >                       cerr << "configure(): Invalid sensor info size "
>     >                            << sensorInfo.outputSize.toString();
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               /* Verify streamConfig. */
>     >               if (streamConfig.size() != 2) {
>     >                       cerr << "configure(): Invalid number of
>     streams "
>     >                            << streamConfig.size() << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               auto iter = streamConfig.find(1);
>     >               if (iter == streamConfig.end()) {
>     >                       cerr << "configure(): No configuration for
>     stream 1" << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >               const IPAStream *stream = &iter->second;
>     >               if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||
>     >                   stream->size != Size{ 1024, 768 }) {
>     >                       cerr << "configure(): Invalid configuration
>     for stream 1" << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               iter = streamConfig.find(2);
>     >               if (iter == streamConfig.end()) {
>     >                       cerr << "configure(): No configuration for
>     stream 2" << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >               stream = &iter->second;
>     >               if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||
>     >                   stream->size != Size{ 800, 600 }) {
>     >                       cerr << "configure(): Invalid configuration
>     for stream 2" << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               /* Verify entityControls. */
>     >               auto ctrlIter = entityControls.find(42);
>     >               if (ctrlIter == entityControls.end()) {
>     >                       cerr << "configure(): Controls not found" <<
>     endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               const ControlInfoMap &infoMap = ctrlIter->second;
>     > @@ -124,10 +132,12 @@ public:
>     >                   infoMap.count(V4L2_CID_CONTRAST) != 1 ||
>     >                   infoMap.count(V4L2_CID_SATURATION) != 1) {
>     >                       cerr << "configure(): Invalid control IDs"
>     << endl;
>     > -                     return report(Op_configure, TestFail);
>     > +                     report(Op_configure, TestFail);
>     > +                     return -EINVAL;
>     >               }
>     >
>     >               report(Op_configure, TestPass);
>     > +             return 0;
>     >       }
>     >
>     >       void mapBuffers(const std::vector<IPABuffer> &buffers) override
>     >
>     > > ---
>     > >  include/libcamera/ipa/raspberrypi.h                |  1 +
>     > >  src/ipa/raspberrypi/raspberrypi.cpp                | 12
>     +++++++++++-
>     > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 +++++
>     > >  3 files changed, 17 insertions(+), 1 deletion(-)
>     > >
>     > > diff --git a/include/libcamera/ipa/raspberrypi.h
>     b/include/libcamera/ipa/raspberrypi.h
>     > > index 2b55dbfc..86c97ffa 100644
>     > > --- a/include/libcamera/ipa/raspberrypi.h
>     > > +++ b/include/libcamera/ipa/raspberrypi.h
>     > > @@ -21,6 +21,7 @@ enum ConfigParameters {
>     > >     IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
>     > >     IPA_CONFIG_SENSOR = (1 << 2),
>     > >     IPA_CONFIG_DROP_FRAMES = (1 << 3),
>     > > +   IPA_CONFIG_FAILED = (1 << 4),
>     > >  };
>     > > 
>     > >  enum Operations {
>     > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>     b/src/ipa/raspberrypi/raspberrypi.cpp
>     > > index 9853a343..57dd9c61 100644
>     > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>     > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>     > > @@ -197,8 +197,11 @@ void IPARPi::configure(const
>     CameraSensorInfo &sensorInfo,
>     > >                    const IPAOperationData &ipaConfig,
>     > >                    IPAOperationData *result)
>     > >  {
>     > > -   if (entityControls.empty())
>     > > +   if (entityControls.size() != 2) {
>     > > +           LOG(IPARPI, Error) << "No ISP or sensor controls
>     found.";
>     > > +           result->operation = RPi::IPA_CONFIG_FAILED;
>     > >             return;
>     > > +   }
>     > > 
>     > >     result->operation = 0;
>     > > 
>     > > @@ -217,6 +220,13 @@ void IPARPi::configure(const
>     CameraSensorInfo &sensorInfo,
>     > >     if (!helper_) {
>     > >             helper_ =
>     std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
>     > > 
>     > > +           if (!helper_) {
>     > > +                   LOG(IPARPI, Error) << "Could not create
>     camera helper for "
>     > > +                                      << cameraName;
>     > > +                   result->operation = RPi::IPA_CONFIG_FAILED;
>     > > +                   return;
>     > > +           }
>     > > +
>     > >             /*
>     > >              * Pass out the sensor config to the pipeline
>     handler in order
>     > >              * to setup the staggered writer class.
>     > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > > index 6fcdf557..76252806 100644
>     > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const
>     CameraConfiguration *config)
>     > >     ipa_->configure(sensorInfo_, streamConfig, entityControls,
>     ipaConfig,
>     > >                     &result);
>     > > 
>     > > +   if (result.operation & RPi::IPA_CONFIG_FAILED) {
>     > > +           LOG(RPI, Error) << "IPA configuration failed!";
>     > > +           return -EPIPE;
>     > > +   }
>     > > +
>     > >     unsigned int resultIdx = 0;
>     > >     if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
>     > >             /*
> 
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index 2b55dbfc..86c97ffa 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -21,6 +21,7 @@  enum ConfigParameters {
 	IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
 	IPA_CONFIG_SENSOR = (1 << 2),
 	IPA_CONFIG_DROP_FRAMES = (1 << 3),
+	IPA_CONFIG_FAILED = (1 << 4),
 };
 
 enum Operations {
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 9853a343..57dd9c61 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -197,8 +197,11 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		       const IPAOperationData &ipaConfig,
 		       IPAOperationData *result)
 {
-	if (entityControls.empty())
+	if (entityControls.size() != 2) {
+		LOG(IPARPI, Error) << "No ISP or sensor controls found.";
+		result->operation = RPi::IPA_CONFIG_FAILED;
 		return;
+	}
 
 	result->operation = 0;
 
@@ -217,6 +220,13 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 	if (!helper_) {
 		helper_ = std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
 
+		if (!helper_) {
+			LOG(IPARPI, Error) << "Could not create camera helper for "
+					   << cameraName;
+			result->operation = RPi::IPA_CONFIG_FAILED;
+			return;
+		}
+
 		/*
 		 * Pass out the sensor config to the pipeline handler in order
 		 * to setup the staggered writer class.
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 6fcdf557..76252806 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1194,6 +1194,11 @@  int RPiCameraData::configureIPA(const CameraConfiguration *config)
 	ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
 			&result);
 
+	if (result.operation & RPi::IPA_CONFIG_FAILED) {
+		LOG(RPI, Error) << "IPA configuration failed!";
+		return -EPIPE;
+	}
+
 	unsigned int resultIdx = 0;
 	if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
 		/*