[{"id":13992,"web_url":"https://patchwork.libcamera.org/comment/13992/","msgid":"<20201201015543.GF4315@pendragon.ideasonboard.com>","date":"2020-12-01T01:55:43","subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:\n> If the IPA fails during configuration, return an error flag to the\n> pipeline handler and fail the use case gracefully.\n> \n> At present, the IPA configuration can fail for the following reasons:\n> - The sensor is not recognised, and fails to open a CamHelper object.\n> - The pipeline handler did not pass in controls for the ISP and sensor.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: Jacopo Mondi \n\nWouldn't it be simpler to modify the configure() function to return an\nint ? How about the following ? If it works for you I'll submit a proper\npatch.\n\nAuthor: Laurent Pinchart \nDate: Tue Dec 1 03:54:18 2020 +0200\n\n libcamera: ipa_interface: Make configure() return an int\n\n It's useful for the IPAInterface::configure() function to be able to\n return a status. Make it return an int, to avoid forcing IPAs to return\n a status encoded in the IPAOperationData in a custom way.\n\n Signed-off-by: Laurent Pinchart \n\ndiff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h\nindex 8f767e844221..a00b5e7b92eb 100644\n--- a/include/libcamera/internal/ipa_context_wrapper.h\n+++ b/include/libcamera/internal/ipa_context_wrapper.h\n@@ -22,11 +22,11 @@ public:\n \tint init(const IPASettings &settings) override;\n \tint start() override;\n \tvoid stop() override;\n-\tvoid configure(const CameraSensorInfo &sensorInfo,\n-\t\t const std::map &streamConfig,\n-\t\t const std::map &entityControls,\n-\t\t const IPAOperationData &ipaConfig,\n-\t\t IPAOperationData *result) override;\n+\tint configure(const CameraSensorInfo &sensorInfo,\n+\t\t const std::map &streamConfig,\n+\t\t const std::map &entityControls,\n+\t\t const IPAOperationData &ipaConfig,\n+\t\t IPAOperationData *result) override;\n\n \tvoid mapBuffers(const std::vector &buffers) override;\n \tvoid unmapBuffers(const std::vector &ids) override;\ndiff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h\nindex 322b7079070e..1d8cf8dc1c56 100644\n--- a/include/libcamera/ipa/ipa_interface.h\n+++ b/include/libcamera/ipa/ipa_interface.h\n@@ -95,12 +95,12 @@ struct ipa_context_ops {\n \tvoid (*register_callbacks)(struct ipa_context *ctx,\n \t\t\t\t const struct ipa_callback_ops *callbacks,\n \t\t\t\t void *cb_ctx);\n-\tvoid (*configure)(struct ipa_context *ctx,\n-\t\t\t const struct ipa_sensor_info *sensor_info,\n-\t\t\t const struct ipa_stream *streams,\n-\t\t\t unsigned int num_streams,\n-\t\t\t const struct ipa_control_info_map *maps,\n-\t\t\t unsigned int num_maps);\n+\tint (*configure)(struct ipa_context *ctx,\n+\t\t\t const struct ipa_sensor_info *sensor_info,\n+\t\t\t const struct ipa_stream *streams,\n+\t\t\t unsigned int num_streams,\n+\t\t\t const struct ipa_control_info_map *maps,\n+\t\t\t unsigned int num_maps);\n \tvoid (*map_buffers)(struct ipa_context *ctx,\n \t\t\t const struct ipa_buffer *buffers,\n \t\t\t size_t num_buffers);\n@@ -156,11 +156,11 @@ public:\n \tvirtual int start() = 0;\n \tvirtual void stop() = 0;\n\n-\tvirtual void configure(const CameraSensorInfo &sensorInfo,\n-\t\t\t const std::map &streamConfig,\n-\t\t\t const std::map &entityControls,\n-\t\t\t const IPAOperationData &ipaConfig,\n-\t\t\t IPAOperationData *result) = 0;\n+\tvirtual int configure(const CameraSensorInfo &sensorInfo,\n+\t\t\t const std::map &streamConfig,\n+\t\t\t const std::map &entityControls,\n+\t\t\t const IPAOperationData &ipaConfig,\n+\t\t\t IPAOperationData *result) = 0;\n\n \tvirtual void mapBuffers(const std::vector &buffers) = 0;\n \tvirtual void unmapBuffers(const std::vector &ids) = 0;\ndiff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\nindex cee532e3a747..74d7bc6e1c9b 100644\n--- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n+++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n@@ -115,12 +115,12 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,\n \tctx->cb_ctx_ = cb_ctx;\n }\n\n-void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n-\t\t\t\t const struct ipa_sensor_info *sensor_info,\n-\t\t\t\t const struct ipa_stream *streams,\n-\t\t\t\t unsigned int num_streams,\n-\t\t\t\t const struct ipa_control_info_map *maps,\n-\t\t\t\t unsigned int num_maps)\n+int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n+\t\t\t\t const struct ipa_sensor_info *sensor_info,\n+\t\t\t\t const struct ipa_stream *streams,\n+\t\t\t\t unsigned int num_streams,\n+\t\t\t\t const struct ipa_control_info_map *maps,\n+\t\t\t\t unsigned int num_maps)\n {\n \tIPAInterfaceWrapper *ctx = static_cast(_ctx);\n\n@@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n\n \t/* \\todo Translate the ipaConfig and result. */\n \tIPAOperationData ipaConfig;\n-\tctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,\n-\t\t\t nullptr);\n+\treturn ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,\n+\t\t\t\t ipaConfig, nullptr);\n }\n\n void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\ndiff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h\nindex a1c701599b56..acd3160039b1 100644\n--- a/src/ipa/libipa/ipa_interface_wrapper.h\n+++ b/src/ipa/libipa/ipa_interface_wrapper.h\n@@ -30,12 +30,12 @@ private:\n \tstatic void register_callbacks(struct ipa_context *ctx,\n \t\t\t\t const struct ipa_callback_ops *callbacks,\n \t\t\t\t void *cb_ctx);\n-\tstatic void configure(struct ipa_context *ctx,\n-\t\t\t const struct ipa_sensor_info *sensor_info,\n-\t\t\t const struct ipa_stream *streams,\n-\t\t\t unsigned int num_streams,\n-\t\t\t const struct ipa_control_info_map *maps,\n-\t\t\t unsigned int num_maps);\n+\tstatic int configure(struct ipa_context *ctx,\n+\t\t\t const struct ipa_sensor_info *sensor_info,\n+\t\t\t const struct ipa_stream *streams,\n+\t\t\t unsigned int num_streams,\n+\t\t\t const struct ipa_control_info_map *maps,\n+\t\t\t unsigned int num_maps);\n \tstatic void map_buffers(struct ipa_context *ctx,\n \t\t\t\tconst struct ipa_buffer *c_buffers,\n \t\t\t\tsize_t num_buffers);\ndiff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\nindex 9853a343c892..75f7af3430ef 100644\n--- a/src/ipa/raspberrypi/raspberrypi.cpp\n+++ b/src/ipa/raspberrypi/raspberrypi.cpp\n@@ -81,11 +81,11 @@ public:\n \tint start() override { return 0; }\n \tvoid stop() override {}\n\n-\tvoid configure(const CameraSensorInfo &sensorInfo,\n-\t\t const std::map &streamConfig,\n-\t\t const std::map &entityControls,\n-\t\t const IPAOperationData &data,\n-\t\t IPAOperationData *response) override;\n+\tint configure(const CameraSensorInfo &sensorInfo,\n+\t\t const std::map &streamConfig,\n+\t\t const std::map &entityControls,\n+\t\t const IPAOperationData &data,\n+\t\t IPAOperationData *response) override;\n \tvoid mapBuffers(const std::vector &buffers) override;\n \tvoid unmapBuffers(const std::vector &ids) override;\n \tvoid processEvent(const IPAOperationData &event) override;\n@@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n \tmode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;\n }\n\n-void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n-\t\t [[maybe_unused]] const std::map &streamConfig,\n-\t\t const std::map &entityControls,\n-\t\t const IPAOperationData &ipaConfig,\n-\t\t IPAOperationData *result)\n+int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n+\t\t [[maybe_unused]] const std::map &streamConfig,\n+\t\t const std::map &entityControls,\n+\t\t const IPAOperationData &ipaConfig,\n+\t\t IPAOperationData *result)\n {\n \tif (entityControls.empty())\n-\t\treturn;\n+\t\treturn -EINVAL;\n\n \tresult->operation = 0;\n\n@@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n \t}\n\n \tlastMode_ = mode_;\n+\treturn 0;\n }\n\n void IPARPi::mapBuffers(const std::vector &buffers)\ndiff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\nindex 07d7f1b2ddd8..78d78c5ac521 100644\n--- a/src/ipa/rkisp1/rkisp1.cpp\n+++ b/src/ipa/rkisp1/rkisp1.cpp\n@@ -40,11 +40,11 @@ public:\n \tint start() override { return 0; }\n \tvoid stop() override {}\n\n-\tvoid configure(const CameraSensorInfo &info,\n-\t\t const std::map &streamConfig,\n-\t\t const std::map &entityControls,\n-\t\t const IPAOperationData &ipaConfig,\n-\t\t IPAOperationData *response) override;\n+\tint configure(const CameraSensorInfo &info,\n+\t\t const std::map &streamConfig,\n+\t\t const std::map &entityControls,\n+\t\t const IPAOperationData &ipaConfig,\n+\t\t IPAOperationData *response) override;\n \tvoid mapBuffers(const std::vector &buffers) override;\n \tvoid unmapBuffers(const std::vector &ids) override;\n \tvoid processEvent(const IPAOperationData &event) override;\n@@ -79,27 +79,27 @@ private:\n * assemble one. Make sure the reported sensor information are relevant\n * before accessing them.\n */\n-void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n-\t\t\t [[maybe_unused]] const std::map &streamConfig,\n-\t\t\t const std::map &entityControls,\n-\t\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n-\t\t\t [[maybe_unused]] IPAOperationData *result)\n+int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n+\t\t\t [[maybe_unused]] const std::map &streamConfig,\n+\t\t\t const std::map &entityControls,\n+\t\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n+\t\t\t [[maybe_unused]] IPAOperationData *result)\n {\n \tif (entityControls.empty())\n-\t\treturn;\n+\t\treturn -EINVAL;\n\n \tctrls_ = entityControls.at(0);\n\n \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n \tif (itExp == ctrls_.end()) {\n \t\tLOG(IPARkISP1, Error) << \"Can't find exposure control\";\n-\t\treturn;\n+\t\treturn -EINVAL;\n \t}\n\n \tconst auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n \tif (itGain == ctrls_.end()) {\n \t\tLOG(IPARkISP1, Error) << \"Can't find gain control\";\n-\t\treturn;\n+\t\treturn -EINVAL;\n \t}\n\n \tautoExposure_ = true;\n@@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n \t\t<< \" Gain: \" << minGain_ << \"-\" << maxGain_;\n\n \tsetControls(0);\n+\treturn 0;\n }\n\n void IPARkISP1::mapBuffers(const std::vector &buffers)\ndiff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\nindex cf8411359e40..79c8c2fb8927 100644\n--- a/src/ipa/vimc/vimc.cpp\n+++ b/src/ipa/vimc/vimc.cpp\n@@ -37,11 +37,11 @@ public:\n \tint start() override;\n \tvoid stop() override;\n\n-\tvoid configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n-\t\t [[maybe_unused]] const std::map &streamConfig,\n-\t\t [[maybe_unused]] const std::map &entityControls,\n-\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n-\t\t [[maybe_unused]] IPAOperationData *result) override {}\n+\tint configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n+\t\t [[maybe_unused]] const std::map &streamConfig,\n+\t\t [[maybe_unused]] const std::map &entityControls,\n+\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n+\t\t [[maybe_unused]] IPAOperationData *result) override { return 0; }\n \tvoid mapBuffers([[maybe_unused]] const std::vector &buffers) override {}\n \tvoid unmapBuffers([[maybe_unused]] const std::vector &ids) override {}\n \tvoid processEvent([[maybe_unused]] const IPAOperationData &event) override {}\ndiff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\nindex 231300ce0bec..f63ad830c003 100644\n--- a/src/libcamera/ipa_context_wrapper.cpp\n+++ b/src/libcamera/ipa_context_wrapper.cpp\n@@ -108,18 +108,18 @@ void IPAContextWrapper::stop()\n \tctx_->ops->stop(ctx_);\n }\n\n-void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n-\t\t\t\t const std::map &streamConfig,\n-\t\t\t\t const std::map &entityControls,\n-\t\t\t\t const IPAOperationData &ipaConfig,\n-\t\t\t\t IPAOperationData *result)\n+int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n+\t\t\t\t const std::map &streamConfig,\n+\t\t\t\t const std::map &entityControls,\n+\t\t\t\t const IPAOperationData &ipaConfig,\n+\t\t\t\t IPAOperationData *result)\n {\n \tif (intf_)\n \t\treturn intf_->configure(sensorInfo, streamConfig,\n \t\t\t\t\tentityControls, ipaConfig, result);\n\n \tif (!ctx_)\n-\t\treturn;\n+\t\treturn 0;\n\n \tserializer_.reset();\n\n@@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n \t}\n\n \t/* \\todo Translate the ipaConfig and reponse */\n-\tctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),\n-\t\t\t c_info_maps, entityControls.size());\n+\treturn ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n+\t\t\t\t streamConfig.size(), c_info_maps,\n+\t\t\t\t entityControls.size());\n }\n\n void IPAContextWrapper::mapBuffers(const std::vector &buffers)\ndiff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\nindex 23fc56d7d48e..516a8ecd4b53 100644\n--- a/src/libcamera/ipa_interface.cpp\n+++ b/src/libcamera/ipa_interface.cpp\n@@ -347,6 +347,8 @@\n * \\param[in] num_maps The number of entries in the \\a maps array\n *\n * \\sa libcamera::IPAInterface::configure()\n+ *\n+ * \\return 0 on success or a negative error code on failure\n */\n\n /**\n@@ -573,6 +575,8 @@ namespace libcamera {\n * pipeline handler to the IPA and back. The pipeline handler may set the \\a\n * result parameter to null if the IPA protocol doesn't need to pass a result\n * back through the configure() function.\n+ *\n+ * \\return 0 on success or a negative error code on failure\n */\n\n /**\ndiff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\nindex b78a0e4535f5..ed250ce79c17 100644\n--- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n+++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n@@ -32,11 +32,11 @@ public:\n \t}\n \tint start() override { return 0; }\n \tvoid stop() override {}\n-\tvoid configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n-\t\t [[maybe_unused]] const std::map &streamConfig,\n-\t\t [[maybe_unused]] const std::map &entityControls,\n-\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n-\t\t [[maybe_unused]] IPAOperationData *result) override {}\n+\tint configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n+\t\t [[maybe_unused]] const std::map &streamConfig,\n+\t\t [[maybe_unused]] const std::map &entityControls,\n+\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n+\t\t [[maybe_unused]] IPAOperationData *result) override { return 0; }\n \tvoid mapBuffers([[maybe_unused]] const std::vector &buffers) override {}\n \tvoid unmapBuffers([[maybe_unused]] const std::vector &ids) override {}\n \tvoid processEvent([[maybe_unused]] const IPAOperationData &event) override {}\ndiff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\nindex eead2883708d..fd91726c4840 100644\n--- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n+++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n@@ -29,11 +29,11 @@ public:\n \tint start() override;\n \tvoid stop() override;\n\n-\tvoid configure(const CameraSensorInfo &sensorInfo,\n-\t\t const std::map &streamConfig,\n-\t\t const std::map &entityControls,\n-\t\t const IPAOperationData &ipaConfig,\n-\t\t IPAOperationData *result) override;\n+\tint configure(const CameraSensorInfo &sensorInfo,\n+\t\t const std::map &streamConfig,\n+\t\t const std::map &entityControls,\n+\t\t const IPAOperationData &ipaConfig,\n+\t\t IPAOperationData *result) override;\n \tvoid mapBuffers(const std::vector &buffers) override;\n \tvoid unmapBuffers(const std::vector &ids) override;\n \tvoid processEvent(const IPAOperationData &event) override;\n@@ -132,14 +132,14 @@ void IPAProxyThread::stop()\n \tthread_.wait();\n }\n\n-void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n-\t\t\t const std::map &streamConfig,\n-\t\t\t const std::map &entityControls,\n-\t\t\t const IPAOperationData &ipaConfig,\n-\t\t\t IPAOperationData *result)\n+int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n+\t\t\t const std::map &streamConfig,\n+\t\t\t const std::map &entityControls,\n+\t\t\t const IPAOperationData &ipaConfig,\n+\t\t\t IPAOperationData *result)\n {\n-\tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n-\t\t\tresult);\n+\treturn ipa_->configure(sensorInfo, streamConfig, entityControls,\n+\t\t\t ipaConfig, result);\n }\n\n void IPAProxyThread::mapBuffers(const std::vector &buffers)\ndiff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\nindex 59d991cbbf6a..ad0fb0386865 100644\n--- a/test/ipa/ipa_wrappers_test.cpp\n+++ b/test/ipa/ipa_wrappers_test.cpp\n@@ -67,55 +67,63 @@ public:\n \t\treport(Op_stop, TestPass);\n \t}\n\n-\tvoid configure(const CameraSensorInfo &sensorInfo,\n-\t\t const std::map &streamConfig,\n-\t\t const std::map &entityControls,\n-\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n-\t\t [[maybe_unused]] IPAOperationData *result) override\n+\tint configure(const CameraSensorInfo &sensorInfo,\n+\t\t const std::map &streamConfig,\n+\t\t const std::map &entityControls,\n+\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n+\t\t [[maybe_unused]] IPAOperationData *result) override\n \t{\n \t\t/* Verify sensorInfo. */\n \t\tif (sensorInfo.outputSize.width != 2560 ||\n \t\t sensorInfo.outputSize.height != 1940) {\n \t\t\tcerr << \"configure(): Invalid sensor info size \"\n \t\t\t << sensorInfo.outputSize.toString();\n+\t\t\treport(Op_configure, TestFail);\n+\t\t\treturn -EINVAL;\n \t\t}\n\n \t\t/* Verify streamConfig. */\n \t\tif (streamConfig.size() != 2) {\n \t\t\tcerr << \"configure(): Invalid number of streams \"\n \t\t\t << streamConfig.size() << endl;\n-\t\t\treturn report(Op_configure, TestFail);\n+\t\t\treport(Op_configure, TestFail);\n+\t\t\treturn -EINVAL;\n \t\t}\n\n \t\tauto iter = streamConfig.find(1);\n \t\tif (iter == streamConfig.end()) {\n \t\t\tcerr << \"configure(): No configuration for stream 1\" << endl;\n-\t\t\treturn report(Op_configure, TestFail);\n+\t\t\treport(Op_configure, TestFail);\n+\t\t\treturn -EINVAL;\n \t\t}\n \t\tconst IPAStream *stream = &iter->second;\n \t\tif (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||\n \t\t stream->size != Size{ 1024, 768 }) {\n \t\t\tcerr << \"configure(): Invalid configuration for stream 1\" << endl;\n-\t\t\treturn report(Op_configure, TestFail);\n+\t\t\treport(Op_configure, TestFail);\n+\t\t\treturn -EINVAL;\n \t\t}\n\n \t\titer = streamConfig.find(2);\n \t\tif (iter == streamConfig.end()) {\n \t\t\tcerr << \"configure(): No configuration for stream 2\" << endl;\n-\t\t\treturn report(Op_configure, TestFail);\n+\t\t\treport(Op_configure, TestFail);\n+\t\t\treturn -EINVAL;\n \t\t}\n \t\tstream = &iter->second;\n \t\tif (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||\n \t\t stream->size != Size{ 800, 600 }) {\n \t\t\tcerr << \"configure(): Invalid configuration for stream 2\" << endl;\n-\t\t\treturn report(Op_configure, TestFail);\n+\t\t\treport(Op_configure, TestFail);\n+\t\t\treturn -EINVAL;\n \t\t}\n\n \t\t/* Verify entityControls. */\n \t\tauto ctrlIter = entityControls.find(42);\n \t\tif (ctrlIter == entityControls.end()) {\n \t\t\tcerr << \"configure(): Controls not found\" << endl;\n-\t\t\treturn report(Op_configure, TestFail);\n+\t\t\treport(Op_configure, TestFail);\n+\t\t\treturn -EINVAL;\n \t\t}\n\n \t\tconst ControlInfoMap &infoMap = ctrlIter->second;\n@@ -124,10 +132,12 @@ public:\n \t\t infoMap.count(V4L2_CID_CONTRAST) != 1 ||\n \t\t infoMap.count(V4L2_CID_SATURATION) != 1) {\n \t\t\tcerr << \"configure(): Invalid control IDs\" << endl;\n-\t\t\treturn report(Op_configure, TestFail);\n+\t\t\treport(Op_configure, TestFail);\n+\t\t\treturn -EINVAL;\n \t\t}\n\n \t\treport(Op_configure, TestPass);\n+\t\treturn 0;\n \t}\n\n \tvoid mapBuffers(const std::vector &buffers) override\n\n> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++++++-\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++++\n> 3 files changed, 17 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 2b55dbfc..86c97ffa 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -21,6 +21,7 @@ enum ConfigParameters {\n> \tIPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> \tIPA_CONFIG_SENSOR = (1 << 2),\n> \tIPA_CONFIG_DROP_FRAMES = (1 << 3),\n> +\tIPA_CONFIG_FAILED = (1 << 4),\n> };\n> \n> enum Operations {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 9853a343..57dd9c61 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\t const IPAOperationData &ipaConfig,\n> \t\t IPAOperationData *result)\n> {\n> -\tif (entityControls.empty())\n> +\tif (entityControls.size() != 2) {\n> +\t\tLOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> +\t\tresult->operation = RPi::IPA_CONFIG_FAILED;\n> \t\treturn;\n> +\t}\n> \n> \tresult->operation = 0;\n> \n> @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \tif (!helper_) {\n> \t\thelper_ = std::unique_ptr(RPiController::CamHelper::Create(cameraName));\n> \n> +\t\tif (!helper_) {\n> +\t\t\tLOG(IPARPI, Error) << \"Could not create camera helper for \"\n> +\t\t\t\t\t << cameraName;\n> +\t\t\tresult->operation = RPi::IPA_CONFIG_FAILED;\n> +\t\t\treturn;\n> +\t\t}\n> +\n> \t\t/*\n> \t\t * Pass out the sensor config to the pipeline handler in order\n> \t\t * to setup the staggered writer class.\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 6fcdf557..76252806 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \tipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> \t\t\t&result);\n> \n> +\tif (result.operation & RPi::IPA_CONFIG_FAILED) {\n> +\t\tLOG(RPI, Error) << \"IPA configuration failed!\";\n> +\t\treturn -EPIPE;\n> +\t}\n> +\n> \tunsigned int resultIdx = 0;\n> \tif (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {\n> \t\t/*","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D3856BE177\n\tfor ;\n\tTue, 1 Dec 2020 01:55:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 445BD634A5;\n\tTue, 1 Dec 2020 02:55:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C132560329\n\tfor ;\n\tTue, 1 Dec 2020 02:55:52 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 21EB3538;\n\tTue, 1 Dec 2020 02:55:52 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"v4vf6f1a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606787752;\n\tbh=xi+s6sXvgJINM3O3j1hau0enRkmD7rC9gGb+5Aqtww4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v4vf6f1aUXXOw7xRf30A+qkpQNxeg/loeR/8WxNURcPZcU0kzPL/IvlCqokPbt3wH\n\tXiTmsO0AkRHKaXV+/iNIIQPSOkFCE2J8fhvEDr+ZBX7wpU6gGKH78b05Ydg9MXphjB\n\tZ+CC42jivhJjxH/kTVYraArgW8YZIRPzLKwMyLW4=","Date":"Tue, 1 Dec 2020 03:55:43 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20201201015543.GF4315@pendragon.ideasonboard.com>","References":"<20201127145612.1105770-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201127145612.1105770-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13995,"web_url":"https://patchwork.libcamera.org/comment/13995/","msgid":"","date":"2020-12-01T09:54:44","subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 1 Dec 2020 at 01:55, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:\n> > If the IPA fails during configuration, return an error flag to the\n> > pipeline handler and fail the use case gracefully.\n> >\n> > At present, the IPA configuration can fail for the following reasons:\n> > - The sensor is not recognised, and fails to open a CamHelper object.\n> > - The pipeline handler did not pass in controls for the ISP and sensor.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: Jacopo Mondi \n>\n> Wouldn't it be simpler to modify the configure() function to return an\n> int ? How about the following ? If it works for you I'll submit a proper\n> patch.\n>\n\nYes, that works equally well for me. Happy for you to submit the patch, or\nwould you prefer if I add your commit below to a new patch set that will\nalso include the Raspberry Pi IPA/PH changes?\n\nRegards,\nNaush\n\n\n>\n> Author: Laurent Pinchart \n> Date: Tue Dec 1 03:54:18 2020 +0200\n>\n> libcamera: ipa_interface: Make configure() return an int\n>\n> It's useful for the IPAInterface::configure() function to be able to\n> return a status. Make it return an int, to avoid forcing IPAs to return\n> a status encoded in the IPAOperationData in a custom way.\n>\n> Signed-off-by: Laurent Pinchart \n>\n> diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n> b/include/libcamera/internal/ipa_context_wrapper.h\n> index 8f767e844221..a00b5e7b92eb 100644\n> --- a/include/libcamera/internal/ipa_context_wrapper.h\n> +++ b/include/libcamera/internal/ipa_context_wrapper.h\n> @@ -22,11 +22,11 @@ public:\n> int init(const IPASettings &settings) override;\n> int start() override;\n> void stop() override;\n> - void configure(const CameraSensorInfo &sensorInfo,\n> - const std::map\n> &streamConfig,\n> - const std::map &> &entityControls,\n> - const IPAOperationData &ipaConfig,\n> - IPAOperationData *result) override;\n> + int configure(const CameraSensorInfo &sensorInfo,\n> + const std::map\n> &streamConfig,\n> + const std::map\n> &entityControls,\n> + const IPAOperationData &ipaConfig,\n> + IPAOperationData *result) override;\n>\n> void mapBuffers(const std::vector &buffers) override;\n> void unmapBuffers(const std::vector &ids) override;\n> diff --git a/include/libcamera/ipa/ipa_interface.h\n> b/include/libcamera/ipa/ipa_interface.h\n> index 322b7079070e..1d8cf8dc1c56 100644\n> --- a/include/libcamera/ipa/ipa_interface.h\n> +++ b/include/libcamera/ipa/ipa_interface.h\n> @@ -95,12 +95,12 @@ struct ipa_context_ops {\n> void (*register_callbacks)(struct ipa_context *ctx,\n> const struct ipa_callback_ops\n> *callbacks,\n> void *cb_ctx);\n> - void (*configure)(struct ipa_context *ctx,\n> - const struct ipa_sensor_info *sensor_info,\n> - const struct ipa_stream *streams,\n> - unsigned int num_streams,\n> - const struct ipa_control_info_map *maps,\n> - unsigned int num_maps);\n> + int (*configure)(struct ipa_context *ctx,\n> + const struct ipa_sensor_info *sensor_info,\n> + const struct ipa_stream *streams,\n> + unsigned int num_streams,\n> + const struct ipa_control_info_map *maps,\n> + unsigned int num_maps);\n> void (*map_buffers)(struct ipa_context *ctx,\n> const struct ipa_buffer *buffers,\n> size_t num_buffers);\n> @@ -156,11 +156,11 @@ public:\n> virtual int start() = 0;\n> virtual void stop() = 0;\n>\n> - virtual void configure(const CameraSensorInfo &sensorInfo,\n> - const std::map\n> &streamConfig,\n> - const std::map ControlInfoMap &> &entityControls,\n> - const IPAOperationData &ipaConfig,\n> - IPAOperationData *result) = 0;\n> + virtual int configure(const CameraSensorInfo &sensorInfo,\n> + const std::map\n> &streamConfig,\n> + const std::map ControlInfoMap &> &entityControls,\n> + const IPAOperationData &ipaConfig,\n> + IPAOperationData *result) = 0;\n>\n> virtual void mapBuffers(const std::vector &buffers) = 0;\n> virtual void unmapBuffers(const std::vector &ids) =\n> 0;\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> index cee532e3a747..74d7bc6e1c9b 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> @@ -115,12 +115,12 @@ void IPAInterfaceWrapper::register_callbacks(struct\n> ipa_context *_ctx,\n> ctx->cb_ctx_ = cb_ctx;\n> }\n>\n> -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> - const struct ipa_sensor_info\n> *sensor_info,\n> - const struct ipa_stream *streams,\n> - unsigned int num_streams,\n> - const struct ipa_control_info_map\n> *maps,\n> - unsigned int num_maps)\n> +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> + const struct ipa_sensor_info\n> *sensor_info,\n> + const struct ipa_stream *streams,\n> + unsigned int num_streams,\n> + const struct ipa_control_info_map *maps,\n> + unsigned int num_maps)\n> {\n> IPAInterfaceWrapper *ctx = static_cast *>(_ctx);\n>\n> @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct ipa_context\n> *_ctx,\n>\n> /* \\todo Translate the ipaConfig and result. */\n> IPAOperationData ipaConfig;\n> - ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,\n> ipaConfig,\n> - nullptr);\n> + return ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,\n> + ipaConfig, nullptr);\n> }\n>\n> void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h\n> b/src/ipa/libipa/ipa_interface_wrapper.h\n> index a1c701599b56..acd3160039b1 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> @@ -30,12 +30,12 @@ private:\n> static void register_callbacks(struct ipa_context *ctx,\n> const struct ipa_callback_ops\n> *callbacks,\n> void *cb_ctx);\n> - static void configure(struct ipa_context *ctx,\n> - const struct ipa_sensor_info *sensor_info,\n> - const struct ipa_stream *streams,\n> - unsigned int num_streams,\n> - const struct ipa_control_info_map *maps,\n> - unsigned int num_maps);\n> + static int configure(struct ipa_context *ctx,\n> + const struct ipa_sensor_info *sensor_info,\n> + const struct ipa_stream *streams,\n> + unsigned int num_streams,\n> + const struct ipa_control_info_map *maps,\n> + unsigned int num_maps);\n> static void map_buffers(struct ipa_context *ctx,\n> const struct ipa_buffer *c_buffers,\n> size_t num_buffers);\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 9853a343c892..75f7af3430ef 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -81,11 +81,11 @@ public:\n> int start() override { return 0; }\n> void stop() override {}\n>\n> - void configure(const CameraSensorInfo &sensorInfo,\n> - const std::map\n> &streamConfig,\n> - const std::map &> &entityControls,\n> - const IPAOperationData &data,\n> - IPAOperationData *response) override;\n> + int configure(const CameraSensorInfo &sensorInfo,\n> + const std::map\n> &streamConfig,\n> + const std::map\n> &entityControls,\n> + const IPAOperationData &data,\n> + IPAOperationData *response) override;\n> void mapBuffers(const std::vector &buffers) override;\n> void unmapBuffers(const std::vector &ids) override;\n> void processEvent(const IPAOperationData &event) override;\n> @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> mode_.line_length = 1e9 * sensorInfo.lineLength /\n> sensorInfo.pixelRate;\n> }\n>\n> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> - [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> - const std::map &> &entityControls,\n> - const IPAOperationData &ipaConfig,\n> - IPAOperationData *result)\n> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> + [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> + const std::map\n> &entityControls,\n> + const IPAOperationData &ipaConfig,\n> + IPAOperationData *result)\n> {\n> if (entityControls.empty())\n> - return;\n> + return -EINVAL;\n>\n> result->operation = 0;\n>\n> @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> }\n>\n> lastMode_ = mode_;\n> + return 0;\n> }\n>\n> void IPARPi::mapBuffers(const std::vector &buffers)\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 07d7f1b2ddd8..78d78c5ac521 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -40,11 +40,11 @@ public:\n> int start() override { return 0; }\n> void stop() override {}\n>\n> - void configure(const CameraSensorInfo &info,\n> - const std::map\n> &streamConfig,\n> - const std::map &> &entityControls,\n> - const IPAOperationData &ipaConfig,\n> - IPAOperationData *response) override;\n> + int configure(const CameraSensorInfo &info,\n> + const std::map\n> &streamConfig,\n> + const std::map\n> &entityControls,\n> + const IPAOperationData &ipaConfig,\n> + IPAOperationData *response) override;\n> void mapBuffers(const std::vector &buffers) override;\n> void unmapBuffers(const std::vector &ids) override;\n> void processEvent(const IPAOperationData &event) override;\n> @@ -79,27 +79,27 @@ private:\n> * assemble one. Make sure the reported sensor information are relevant\n> * before accessing them.\n> */\n> -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> - [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> - const std::map ControlInfoMap &> &entityControls,\n> - [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> - [[maybe_unused]] IPAOperationData *result)\n> +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> + [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> + const std::map &> &entityControls,\n> + [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> + [[maybe_unused]] IPAOperationData *result)\n> {\n> if (entityControls.empty())\n> - return;\n> + return -EINVAL;\n>\n> ctrls_ = entityControls.at(0);\n>\n> const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> if (itExp == ctrls_.end()) {\n> LOG(IPARkISP1, Error) << \"Can't find exposure control\";\n> - return;\n> + return -EINVAL;\n> }\n>\n> const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> if (itGain == ctrls_.end()) {\n> LOG(IPARkISP1, Error) << \"Can't find gain control\";\n> - return;\n> + return -EINVAL;\n> }\n>\n> autoExposure_ = true;\n> @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const\n> CameraSensorInfo &info,\n> << \" Gain: \" << minGain_ << \"-\" << maxGain_;\n>\n> setControls(0);\n> + return 0;\n> }\n>\n> void IPARkISP1::mapBuffers(const std::vector &buffers)\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index cf8411359e40..79c8c2fb8927 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -37,11 +37,11 @@ public:\n> int start() override;\n> void stop() override;\n>\n> - void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> - [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> - [[maybe_unused]] const std::map ControlInfoMap &> &entityControls,\n> - [[maybe_unused]] const IPAOperationData &ipaConfig,\n> - [[maybe_unused]] IPAOperationData *result) override\n> {}\n> + int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> + [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> + [[maybe_unused]] const std::map ControlInfoMap &> &entityControls,\n> + [[maybe_unused]] const IPAOperationData &ipaConfig,\n> + [[maybe_unused]] IPAOperationData *result) override\n> { return 0; }\n> void mapBuffers([[maybe_unused]] const std::vector\n> &buffers) override {}\n> void unmapBuffers([[maybe_unused]] const std::vector\n> &ids) override {}\n> void processEvent([[maybe_unused]] const IPAOperationData &event)\n> override {}\n> diff --git a/src/libcamera/ipa_context_wrapper.cpp\n> b/src/libcamera/ipa_context_wrapper.cpp\n> index 231300ce0bec..f63ad830c003 100644\n> --- a/src/libcamera/ipa_context_wrapper.cpp\n> +++ b/src/libcamera/ipa_context_wrapper.cpp\n> @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()\n> ctx_->ops->stop(ctx_);\n> }\n>\n> -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> - const std::map\n> &streamConfig,\n> - const std::map ControlInfoMap &> &entityControls,\n> - const IPAOperationData &ipaConfig,\n> - IPAOperationData *result)\n> +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> + const std::map\n> &streamConfig,\n> + const std::map ControlInfoMap &> &entityControls,\n> + const IPAOperationData &ipaConfig,\n> + IPAOperationData *result)\n> {\n> if (intf_)\n> return intf_->configure(sensorInfo, streamConfig,\n> entityControls, ipaConfig, result);\n>\n> if (!ctx_)\n> - return;\n> + return 0;\n>\n> serializer_.reset();\n>\n> @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const\n> CameraSensorInfo &sensorInfo,\n> }\n>\n> /* \\todo Translate the ipaConfig and reponse */\n> - ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n> streamConfig.size(),\n> - c_info_maps, entityControls.size());\n> + return ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n> + streamConfig.size(), c_info_maps,\n> + entityControls.size());\n> }\n>\n> void IPAContextWrapper::mapBuffers(const std::vector &buffers)\n> diff --git a/src/libcamera/ipa_interface.cpp\n> b/src/libcamera/ipa_interface.cpp\n> index 23fc56d7d48e..516a8ecd4b53 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n> @@ -347,6 +347,8 @@\n> * \\param[in] num_maps The number of entries in the \\a maps array\n> *\n> * \\sa libcamera::IPAInterface::configure()\n> + *\n> + * \\return 0 on success or a negative error code on failure\n> */\n>\n> /**\n> @@ -573,6 +575,8 @@ namespace libcamera {\n> * pipeline handler to the IPA and back. The pipeline handler may set the\n> \\a\n> * result parameter to null if the IPA protocol doesn't need to pass a\n> result\n> * back through the configure() function.\n> + *\n> + * \\return 0 on success or a negative error code on failure\n> */\n>\n> /**\n> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> index b78a0e4535f5..ed250ce79c17 100644\n> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> @@ -32,11 +32,11 @@ public:\n> }\n> int start() override { return 0; }\n> void stop() override {}\n> - void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> - [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> - [[maybe_unused]] const std::map ControlInfoMap &> &entityControls,\n> - [[maybe_unused]] const IPAOperationData &ipaConfig,\n> - [[maybe_unused]] IPAOperationData *result) override\n> {}\n> + int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> + [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> + [[maybe_unused]] const std::map ControlInfoMap &> &entityControls,\n> + [[maybe_unused]] const IPAOperationData &ipaConfig,\n> + [[maybe_unused]] IPAOperationData *result) override\n> { return 0; }\n> void mapBuffers([[maybe_unused]] const std::vector\n> &buffers) override {}\n> void unmapBuffers([[maybe_unused]] const std::vector\n> &ids) override {}\n> void processEvent([[maybe_unused]] const IPAOperationData &event)\n> override {}\n> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> index eead2883708d..fd91726c4840 100644\n> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> @@ -29,11 +29,11 @@ public:\n> int start() override;\n> void stop() override;\n>\n> - void configure(const CameraSensorInfo &sensorInfo,\n> - const std::map\n> &streamConfig,\n> - const std::map &> &entityControls,\n> - const IPAOperationData &ipaConfig,\n> - IPAOperationData *result) override;\n> + int configure(const CameraSensorInfo &sensorInfo,\n> + const std::map\n> &streamConfig,\n> + const std::map\n> &entityControls,\n> + const IPAOperationData &ipaConfig,\n> + IPAOperationData *result) override;\n> void mapBuffers(const std::vector &buffers) override;\n> void unmapBuffers(const std::vector &ids) override;\n> void processEvent(const IPAOperationData &event) override;\n> @@ -132,14 +132,14 @@ void IPAProxyThread::stop()\n> thread_.wait();\n> }\n>\n> -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> - const std::map\n> &streamConfig,\n> - const std::map ControlInfoMap &> &entityControls,\n> - const IPAOperationData &ipaConfig,\n> - IPAOperationData *result)\n> +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> + const std::map\n> &streamConfig,\n> + const std::map ControlInfoMap &> &entityControls,\n> + const IPAOperationData &ipaConfig,\n> + IPAOperationData *result)\n> {\n> - ipa_->configure(sensorInfo, streamConfig, entityControls,\n> ipaConfig,\n> - result);\n> + return ipa_->configure(sensorInfo, streamConfig, entityControls,\n> + ipaConfig, result);\n> }\n>\n> void IPAProxyThread::mapBuffers(const std::vector &buffers)\n> diff --git a/test/ipa/ipa_wrappers_test.cpp\n> b/test/ipa/ipa_wrappers_test.cpp\n> index 59d991cbbf6a..ad0fb0386865 100644\n> --- a/test/ipa/ipa_wrappers_test.cpp\n> +++ b/test/ipa/ipa_wrappers_test.cpp\n> @@ -67,55 +67,63 @@ public:\n> report(Op_stop, TestPass);\n> }\n>\n> - void configure(const CameraSensorInfo &sensorInfo,\n> - const std::map\n> &streamConfig,\n> - const std::map &> &entityControls,\n> - [[maybe_unused]] const IPAOperationData &ipaConfig,\n> - [[maybe_unused]] IPAOperationData *result) override\n> + int configure(const CameraSensorInfo &sensorInfo,\n> + const std::map\n> &streamConfig,\n> + const std::map\n> &entityControls,\n> + [[maybe_unused]] const IPAOperationData &ipaConfig,\n> + [[maybe_unused]] IPAOperationData *result) override\n> {\n> /* Verify sensorInfo. */\n> if (sensorInfo.outputSize.width != 2560 ||\n> sensorInfo.outputSize.height != 1940) {\n> cerr << \"configure(): Invalid sensor info size \"\n> << sensorInfo.outputSize.toString();\n> + report(Op_configure, TestFail);\n> + return -EINVAL;\n> }\n>\n> /* Verify streamConfig. */\n> if (streamConfig.size() != 2) {\n> cerr << \"configure(): Invalid number of streams \"\n> << streamConfig.size() << endl;\n> - return report(Op_configure, TestFail);\n> + report(Op_configure, TestFail);\n> + return -EINVAL;\n> }\n>\n> auto iter = streamConfig.find(1);\n> if (iter == streamConfig.end()) {\n> cerr << \"configure(): No configuration for stream\n> 1\" << endl;\n> - return report(Op_configure, TestFail);\n> + report(Op_configure, TestFail);\n> + return -EINVAL;\n> }\n> const IPAStream *stream = &iter->second;\n> if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||\n> stream->size != Size{ 1024, 768 }) {\n> cerr << \"configure(): Invalid configuration for\n> stream 1\" << endl;\n> - return report(Op_configure, TestFail);\n> + report(Op_configure, TestFail);\n> + return -EINVAL;\n> }\n>\n> iter = streamConfig.find(2);\n> if (iter == streamConfig.end()) {\n> cerr << \"configure(): No configuration for stream\n> 2\" << endl;\n> - return report(Op_configure, TestFail);\n> + report(Op_configure, TestFail);\n> + return -EINVAL;\n> }\n> stream = &iter->second;\n> if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||\n> stream->size != Size{ 800, 600 }) {\n> cerr << \"configure(): Invalid configuration for\n> stream 2\" << endl;\n> - return report(Op_configure, TestFail);\n> + report(Op_configure, TestFail);\n> + return -EINVAL;\n> }\n>\n> /* Verify entityControls. */\n> auto ctrlIter = entityControls.find(42);\n> if (ctrlIter == entityControls.end()) {\n> cerr << \"configure(): Controls not found\" << endl;\n> - return report(Op_configure, TestFail);\n> + report(Op_configure, TestFail);\n> + return -EINVAL;\n> }\n>\n> const ControlInfoMap &infoMap = ctrlIter->second;\n> @@ -124,10 +132,12 @@ public:\n> infoMap.count(V4L2_CID_CONTRAST) != 1 ||\n> infoMap.count(V4L2_CID_SATURATION) != 1) {\n> cerr << \"configure(): Invalid control IDs\" << endl;\n> - return report(Op_configure, TestFail);\n> + report(Op_configure, TestFail);\n> + return -EINVAL;\n> }\n>\n> report(Op_configure, TestPass);\n> + return 0;\n> }\n>\n> void mapBuffers(const std::vector &buffers) override\n>\n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 1 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++++++-\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++++\n> > 3 files changed, 17 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > index 2b55dbfc..86c97ffa 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -21,6 +21,7 @@ enum ConfigParameters {\n> > IPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> > IPA_CONFIG_SENSOR = (1 << 2),\n> > IPA_CONFIG_DROP_FRAMES = (1 << 3),\n> > + IPA_CONFIG_FAILED = (1 << 4),\n> > };\n> >\n> > enum Operations {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 9853a343..57dd9c61 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > const IPAOperationData &ipaConfig,\n> > IPAOperationData *result)\n> > {\n> > - if (entityControls.empty())\n> > + if (entityControls.size() != 2) {\n> > + LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > + result->operation = RPi::IPA_CONFIG_FAILED;\n> > return;\n> > + }\n> >\n> > result->operation = 0;\n> >\n> > @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > if (!helper_) {\n> > helper_ =\n> std::unique_ptr(RPiController::CamHelper::Create(cameraName));\n> >\n> > + if (!helper_) {\n> > + LOG(IPARPI, Error) << \"Could not create camera\n> helper for \"\n> > + << cameraName;\n> > + result->operation = RPi::IPA_CONFIG_FAILED;\n> > + return;\n> > + }\n> > +\n> > /*\n> > * Pass out the sensor config to the pipeline handler in\n> order\n> > * to setup the staggered writer class.\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 6fcdf557..76252806 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> ipaConfig,\n> > &result);\n> >\n> > + if (result.operation & RPi::IPA_CONFIG_FAILED) {\n> > + LOG(RPI, Error) << \"IPA configuration failed!\";\n> > + return -EPIPE;\n> > + }\n> > +\n> > unsigned int resultIdx = 0;\n> > if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {\n> > /*\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6BA12BE176\n\tfor ;\n\tTue, 1 Dec 2020 09:55:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA9CD634A6;\n\tTue, 1 Dec 2020 10:55:03 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 941D46032B\n\tfor ;\n\tTue, 1 Dec 2020 10:55:01 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id r18so1906699ljc.2\n\tfor ;\n\tTue, 01 Dec 2020 01:55:01 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"gjFqOO/b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Q4+sxLw/SgnZPt+EA9VKVVEnMjmKTEhPe1QEo9ljwR0=;\n\tb=gjFqOO/b8MwtQ4pyGhfj3TwR51XaxthLAhlT6Oqkbz0r9CZtYnKRj7q6dOtSPMnCt7\n\tOanE9oeQD6jCHAdwecstXJef6QFlxiX+xJvPfVFYlkc/uBerFvUCK7qj/Iek93H3KWdC\n\tEE7jeBDjofb9izsV/t8m1R1AVH8QX/jXpxiy5f/WplId5O0dO49DH0APgEx6K1Jck20y\n\tbwoPZlundK0iVQLTjU+uoskz/8nLiAmUpfY+YgsJwmbH4esGbNodOdUIeCmAI+x4plzn\n\ttnfJAHKA7acq7FKAtcTc4G7tPHctSfwiVs7J1W6E2gvs319mQzwdv4aDGb0PEh9hlu1Y\n\tgC4g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Q4+sxLw/SgnZPt+EA9VKVVEnMjmKTEhPe1QEo9ljwR0=;\n\tb=qcGFXvuZY4TmU+N7x0aDcVN9peX5nwIXBPFNrUykEz0cDNAYZdEu9Dn60sZ2mrtEa5\n\tTRAovNEiKFAMvpjd3f9JDGyZzInhemWX0U0fDbqc5naqEw3v24a2CKAo5YgRXXIIiGpG\n\tWSORTXwvMigrWroKd1Al7lQNVzp+LBNR4e9NMHh1cG+DXLZOSoS+JKIVLI+xFUqvoDS3\n\to69LRpkFl0G2eKyL2bvtSXNs/z+VY2RSbd0rByS6b6+JKNWZ9OyHZCaTclGLu+3uYlwi\n\td6O2DzRjP5WM/dar6vFW90KXiEJkml8oQC+l4lqz7QJe2COMcsjfFnlkdK996lN004WJ\n\tpvug==","X-Gm-Message-State":"AOAM533OSgmO/18hD2G1nS/XaFkPoeTWb7lHiIT/g+qSUgN2Kq1EPE3I\n\t1EzbweOvoz6cwir40rr9Oa3rBpYYxfr/LoRl7sAK3w==","X-Google-Smtp-Source":"ABdhPJwKBFJx3mDV9vtt4OUCU7KlH5wj8272AoHt0frEQycRXen3N6rslxsAfyw3zu5ELsjJPRv0JhS3Cvn1Iq9HhqQ=","X-Received":"by 2002:a05:651c:1b7:: with SMTP id\n\tc23mr1000400ljn.112.1606816500567; \n\tTue, 01 Dec 2020 01:55:00 -0800 (PST)","MIME-Version":"1.0","References":"<20201127145612.1105770-1-naush@raspberrypi.com>\n\t<20201201015543.GF4315@pendragon.ideasonboard.com>","In-Reply-To":"<20201201015543.GF4315@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Tue, 1 Dec 2020 09:54:44 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============3346068452316446585==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13996,"web_url":"https://patchwork.libcamera.org/comment/13996/","msgid":"","date":"2020-12-01T09:59:57","subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 1 Dec 2020 at 09:54, Naushir Patuck wrote:\n\n> Hi Laurent,\n>\n> On Tue, 1 Dec 2020 at 01:55, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n>> Hi Naush,\n>>\n>> Thank you for the patch.\n>>\n>> On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:\n>> > If the IPA fails during configuration, return an error flag to the\n>> > pipeline handler and fail the use case gracefully.\n>> >\n>> > At present, the IPA configuration can fail for the following reasons:\n>> > - The sensor is not recognised, and fails to open a CamHelper object.\n>> > - The pipeline handler did not pass in controls for the ISP and sensor.\n>> >\n>> > Signed-off-by: Naushir Patuck \n>> > Reviewed-by: Jacopo Mondi \n>>\n>> Wouldn't it be simpler to modify the configure() function to return an\n>> int ? How about the following ? If it works for you I'll submit a proper\n>> patch.\n>>\n>\n> Yes, that works equally well for me. Happy for you to submit the patch,\n> or would you prefer if I add your commit below to a new patch set that will\n> also include the Raspberry Pi IPA/PH changes?\n>\n\nSorry, should have been a bit clearer. I have two more patches (not yet\nsubmitted for review) that are based on this work. I could include all of\nthis in a new series including your patch below. Otherwise, I am happy to\nsubmit them separately once this has gone through.\n\n\n>\n> Regards,\n> Naush\n>\n>\n>>\n>> Author: Laurent Pinchart \n>> Date: Tue Dec 1 03:54:18 2020 +0200\n>>\n>> libcamera: ipa_interface: Make configure() return an int\n>>\n>> It's useful for the IPAInterface::configure() function to be able to\n>> return a status. Make it return an int, to avoid forcing IPAs to\n>> return\n>> a status encoded in the IPAOperationData in a custom way.\n>>\n>> Signed-off-by: Laurent Pinchart \n>>\n>> diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n>> b/include/libcamera/internal/ipa_context_wrapper.h\n>> index 8f767e844221..a00b5e7b92eb 100644\n>> --- a/include/libcamera/internal/ipa_context_wrapper.h\n>> +++ b/include/libcamera/internal/ipa_context_wrapper.h\n>> @@ -22,11 +22,11 @@ public:\n>> int init(const IPASettings &settings) override;\n>> int start() override;\n>> void stop() override;\n>> - void configure(const CameraSensorInfo &sensorInfo,\n>> - const std::map\n>> &streamConfig,\n>> - const std::map> &> &entityControls,\n>> - const IPAOperationData &ipaConfig,\n>> - IPAOperationData *result) override;\n>> + int configure(const CameraSensorInfo &sensorInfo,\n>> + const std::map\n>> &streamConfig,\n>> + const std::map> &> &entityControls,\n>> + const IPAOperationData &ipaConfig,\n>> + IPAOperationData *result) override;\n>>\n>> void mapBuffers(const std::vector &buffers) override;\n>> void unmapBuffers(const std::vector &ids) override;\n>> diff --git a/include/libcamera/ipa/ipa_interface.h\n>> b/include/libcamera/ipa/ipa_interface.h\n>> index 322b7079070e..1d8cf8dc1c56 100644\n>> --- a/include/libcamera/ipa/ipa_interface.h\n>> +++ b/include/libcamera/ipa/ipa_interface.h\n>> @@ -95,12 +95,12 @@ struct ipa_context_ops {\n>> void (*register_callbacks)(struct ipa_context *ctx,\n>> const struct ipa_callback_ops\n>> *callbacks,\n>> void *cb_ctx);\n>> - void (*configure)(struct ipa_context *ctx,\n>> - const struct ipa_sensor_info *sensor_info,\n>> - const struct ipa_stream *streams,\n>> - unsigned int num_streams,\n>> - const struct ipa_control_info_map *maps,\n>> - unsigned int num_maps);\n>> + int (*configure)(struct ipa_context *ctx,\n>> + const struct ipa_sensor_info *sensor_info,\n>> + const struct ipa_stream *streams,\n>> + unsigned int num_streams,\n>> + const struct ipa_control_info_map *maps,\n>> + unsigned int num_maps);\n>> void (*map_buffers)(struct ipa_context *ctx,\n>> const struct ipa_buffer *buffers,\n>> size_t num_buffers);\n>> @@ -156,11 +156,11 @@ public:\n>> virtual int start() = 0;\n>> virtual void stop() = 0;\n>>\n>> - virtual void configure(const CameraSensorInfo &sensorInfo,\n>> - const std::map\n>> &streamConfig,\n>> - const std::map> ControlInfoMap &> &entityControls,\n>> - const IPAOperationData &ipaConfig,\n>> - IPAOperationData *result) = 0;\n>> + virtual int configure(const CameraSensorInfo &sensorInfo,\n>> + const std::map\n>> &streamConfig,\n>> + const std::map> ControlInfoMap &> &entityControls,\n>> + const IPAOperationData &ipaConfig,\n>> + IPAOperationData *result) = 0;\n>>\n>> virtual void mapBuffers(const std::vector &buffers) =\n>> 0;\n>> virtual void unmapBuffers(const std::vector &ids) =\n>> 0;\n>> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp\n>> b/src/ipa/libipa/ipa_interface_wrapper.cpp\n>> index cee532e3a747..74d7bc6e1c9b 100644\n>> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n>> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n>> @@ -115,12 +115,12 @@ void IPAInterfaceWrapper::register_callbacks(struct\n>> ipa_context *_ctx,\n>> ctx->cb_ctx_ = cb_ctx;\n>> }\n>>\n>> -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n>> - const struct ipa_sensor_info\n>> *sensor_info,\n>> - const struct ipa_stream *streams,\n>> - unsigned int num_streams,\n>> - const struct ipa_control_info_map\n>> *maps,\n>> - unsigned int num_maps)\n>> +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n>> + const struct ipa_sensor_info\n>> *sensor_info,\n>> + const struct ipa_stream *streams,\n>> + unsigned int num_streams,\n>> + const struct ipa_control_info_map\n>> *maps,\n>> + unsigned int num_maps)\n>> {\n>> IPAInterfaceWrapper *ctx = static_cast> *>(_ctx);\n>>\n>> @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct\n>> ipa_context *_ctx,\n>>\n>> /* \\todo Translate the ipaConfig and result. */\n>> IPAOperationData ipaConfig;\n>> - ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,\n>> ipaConfig,\n>> - nullptr);\n>> + return ctx->ipa_->configure(sensorInfo, ipaStreams,\n>> entityControls,\n>> + ipaConfig, nullptr);\n>> }\n>>\n>> void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n>> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h\n>> b/src/ipa/libipa/ipa_interface_wrapper.h\n>> index a1c701599b56..acd3160039b1 100644\n>> --- a/src/ipa/libipa/ipa_interface_wrapper.h\n>> +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n>> @@ -30,12 +30,12 @@ private:\n>> static void register_callbacks(struct ipa_context *ctx,\n>> const struct ipa_callback_ops\n>> *callbacks,\n>> void *cb_ctx);\n>> - static void configure(struct ipa_context *ctx,\n>> - const struct ipa_sensor_info *sensor_info,\n>> - const struct ipa_stream *streams,\n>> - unsigned int num_streams,\n>> - const struct ipa_control_info_map *maps,\n>> - unsigned int num_maps);\n>> + static int configure(struct ipa_context *ctx,\n>> + const struct ipa_sensor_info *sensor_info,\n>> + const struct ipa_stream *streams,\n>> + unsigned int num_streams,\n>> + const struct ipa_control_info_map *maps,\n>> + unsigned int num_maps);\n>> static void map_buffers(struct ipa_context *ctx,\n>> const struct ipa_buffer *c_buffers,\n>> size_t num_buffers);\n>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n>> b/src/ipa/raspberrypi/raspberrypi.cpp\n>> index 9853a343c892..75f7af3430ef 100644\n>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> @@ -81,11 +81,11 @@ public:\n>> int start() override { return 0; }\n>> void stop() override {}\n>>\n>> - void configure(const CameraSensorInfo &sensorInfo,\n>> - const std::map\n>> &streamConfig,\n>> - const std::map> &> &entityControls,\n>> - const IPAOperationData &data,\n>> - IPAOperationData *response) override;\n>> + int configure(const CameraSensorInfo &sensorInfo,\n>> + const std::map\n>> &streamConfig,\n>> + const std::map> &> &entityControls,\n>> + const IPAOperationData &data,\n>> + IPAOperationData *response) override;\n>> void mapBuffers(const std::vector &buffers) override;\n>> void unmapBuffers(const std::vector &ids) override;\n>> void processEvent(const IPAOperationData &event) override;\n>> @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo\n>> &sensorInfo)\n>> mode_.line_length = 1e9 * sensorInfo.lineLength /\n>> sensorInfo.pixelRate;\n>> }\n>>\n>> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>> - [[maybe_unused]] const std::map> IPAStream> &streamConfig,\n>> - const std::map> &> &entityControls,\n>> - const IPAOperationData &ipaConfig,\n>> - IPAOperationData *result)\n>> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>> + [[maybe_unused]] const std::map> IPAStream> &streamConfig,\n>> + const std::map> &> &entityControls,\n>> + const IPAOperationData &ipaConfig,\n>> + IPAOperationData *result)\n>> {\n>> if (entityControls.empty())\n>> - return;\n>> + return -EINVAL;\n>>\n>> result->operation = 0;\n>>\n>> @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo\n>> &sensorInfo,\n>> }\n>>\n>> lastMode_ = mode_;\n>> + return 0;\n>> }\n>>\n>> void IPARPi::mapBuffers(const std::vector &buffers)\n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index 07d7f1b2ddd8..78d78c5ac521 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -40,11 +40,11 @@ public:\n>> int start() override { return 0; }\n>> void stop() override {}\n>>\n>> - void configure(const CameraSensorInfo &info,\n>> - const std::map\n>> &streamConfig,\n>> - const std::map> &> &entityControls,\n>> - const IPAOperationData &ipaConfig,\n>> - IPAOperationData *response) override;\n>> + int configure(const CameraSensorInfo &info,\n>> + const std::map\n>> &streamConfig,\n>> + const std::map> &> &entityControls,\n>> + const IPAOperationData &ipaConfig,\n>> + IPAOperationData *response) override;\n>> void mapBuffers(const std::vector &buffers) override;\n>> void unmapBuffers(const std::vector &ids) override;\n>> void processEvent(const IPAOperationData &event) override;\n>> @@ -79,27 +79,27 @@ private:\n>> * assemble one. Make sure the reported sensor information are relevant\n>> * before accessing them.\n>> */\n>> -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n>> - [[maybe_unused]] const std::map> IPAStream> &streamConfig,\n>> - const std::map> ControlInfoMap &> &entityControls,\n>> - [[maybe_unused]] const IPAOperationData\n>> &ipaConfig,\n>> - [[maybe_unused]] IPAOperationData *result)\n>> +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n>> + [[maybe_unused]] const std::map> IPAStream> &streamConfig,\n>> + const std::map> ControlInfoMap &> &entityControls,\n>> + [[maybe_unused]] const IPAOperationData\n>> &ipaConfig,\n>> + [[maybe_unused]] IPAOperationData *result)\n>> {\n>> if (entityControls.empty())\n>> - return;\n>> + return -EINVAL;\n>>\n>> ctrls_ = entityControls.at(0);\n>>\n>> const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>> if (itExp == ctrls_.end()) {\n>> LOG(IPARkISP1, Error) << \"Can't find exposure control\";\n>> - return;\n>> + return -EINVAL;\n>> }\n>>\n>> const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n>> if (itGain == ctrls_.end()) {\n>> LOG(IPARkISP1, Error) << \"Can't find gain control\";\n>> - return;\n>> + return -EINVAL;\n>> }\n>>\n>> autoExposure_ = true;\n>> @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const\n>> CameraSensorInfo &info,\n>> << \" Gain: \" << minGain_ << \"-\" << maxGain_;\n>>\n>> setControls(0);\n>> + return 0;\n>> }\n>>\n>> void IPARkISP1::mapBuffers(const std::vector &buffers)\n>> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n>> index cf8411359e40..79c8c2fb8927 100644\n>> --- a/src/ipa/vimc/vimc.cpp\n>> +++ b/src/ipa/vimc/vimc.cpp\n>> @@ -37,11 +37,11 @@ public:\n>> int start() override;\n>> void stop() override;\n>>\n>> - void configure([[maybe_unused]] const CameraSensorInfo\n>> &sensorInfo,\n>> - [[maybe_unused]] const std::map> IPAStream> &streamConfig,\n>> - [[maybe_unused]] const std::map> const ControlInfoMap &> &entityControls,\n>> - [[maybe_unused]] const IPAOperationData &ipaConfig,\n>> - [[maybe_unused]] IPAOperationData *result)\n>> override {}\n>> + int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n>> + [[maybe_unused]] const std::map> IPAStream> &streamConfig,\n>> + [[maybe_unused]] const std::map> ControlInfoMap &> &entityControls,\n>> + [[maybe_unused]] const IPAOperationData &ipaConfig,\n>> + [[maybe_unused]] IPAOperationData *result) override\n>> { return 0; }\n>> void mapBuffers([[maybe_unused]] const std::vector\n>> &buffers) override {}\n>> void unmapBuffers([[maybe_unused]] const std::vector> int> &ids) override {}\n>> void processEvent([[maybe_unused]] const IPAOperationData &event)\n>> override {}\n>> diff --git a/src/libcamera/ipa_context_wrapper.cpp\n>> b/src/libcamera/ipa_context_wrapper.cpp\n>> index 231300ce0bec..f63ad830c003 100644\n>> --- a/src/libcamera/ipa_context_wrapper.cpp\n>> +++ b/src/libcamera/ipa_context_wrapper.cpp\n>> @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()\n>> ctx_->ops->stop(ctx_);\n>> }\n>>\n>> -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n>> - const std::map\n>> &streamConfig,\n>> - const std::map> ControlInfoMap &> &entityControls,\n>> - const IPAOperationData &ipaConfig,\n>> - IPAOperationData *result)\n>> +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n>> + const std::map\n>> &streamConfig,\n>> + const std::map> ControlInfoMap &> &entityControls,\n>> + const IPAOperationData &ipaConfig,\n>> + IPAOperationData *result)\n>> {\n>> if (intf_)\n>> return intf_->configure(sensorInfo, streamConfig,\n>> entityControls, ipaConfig,\n>> result);\n>>\n>> if (!ctx_)\n>> - return;\n>> + return 0;\n>>\n>> serializer_.reset();\n>>\n>> @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const\n>> CameraSensorInfo &sensorInfo,\n>> }\n>>\n>> /* \\todo Translate the ipaConfig and reponse */\n>> - ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n>> streamConfig.size(),\n>> - c_info_maps, entityControls.size());\n>> + return ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n>> + streamConfig.size(), c_info_maps,\n>> + entityControls.size());\n>> }\n>>\n>> void IPAContextWrapper::mapBuffers(const std::vector &buffers)\n>> diff --git a/src/libcamera/ipa_interface.cpp\n>> b/src/libcamera/ipa_interface.cpp\n>> index 23fc56d7d48e..516a8ecd4b53 100644\n>> --- a/src/libcamera/ipa_interface.cpp\n>> +++ b/src/libcamera/ipa_interface.cpp\n>> @@ -347,6 +347,8 @@\n>> * \\param[in] num_maps The number of entries in the \\a maps array\n>> *\n>> * \\sa libcamera::IPAInterface::configure()\n>> + *\n>> + * \\return 0 on success or a negative error code on failure\n>> */\n>>\n>> /**\n>> @@ -573,6 +575,8 @@ namespace libcamera {\n>> * pipeline handler to the IPA and back. The pipeline handler may set\n>> the \\a\n>> * result parameter to null if the IPA protocol doesn't need to pass a\n>> result\n>> * back through the configure() function.\n>> + *\n>> + * \\return 0 on success or a negative error code on failure\n>> */\n>>\n>> /**\n>> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp\n>> b/src/libcamera/proxy/ipa_proxy_linux.cpp\n>> index b78a0e4535f5..ed250ce79c17 100644\n>> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n>> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n>> @@ -32,11 +32,11 @@ public:\n>> }\n>> int start() override { return 0; }\n>> void stop() override {}\n>> - void configure([[maybe_unused]] const CameraSensorInfo\n>> &sensorInfo,\n>> - [[maybe_unused]] const std::map> IPAStream> &streamConfig,\n>> - [[maybe_unused]] const std::map> const ControlInfoMap &> &entityControls,\n>> - [[maybe_unused]] const IPAOperationData &ipaConfig,\n>> - [[maybe_unused]] IPAOperationData *result)\n>> override {}\n>> + int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n>> + [[maybe_unused]] const std::map> IPAStream> &streamConfig,\n>> + [[maybe_unused]] const std::map> ControlInfoMap &> &entityControls,\n>> + [[maybe_unused]] const IPAOperationData &ipaConfig,\n>> + [[maybe_unused]] IPAOperationData *result) override\n>> { return 0; }\n>> void mapBuffers([[maybe_unused]] const std::vector\n>> &buffers) override {}\n>> void unmapBuffers([[maybe_unused]] const std::vector> int> &ids) override {}\n>> void processEvent([[maybe_unused]] const IPAOperationData &event)\n>> override {}\n>> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n>> b/src/libcamera/proxy/ipa_proxy_thread.cpp\n>> index eead2883708d..fd91726c4840 100644\n>> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n>> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n>> @@ -29,11 +29,11 @@ public:\n>> int start() override;\n>> void stop() override;\n>>\n>> - void configure(const CameraSensorInfo &sensorInfo,\n>> - const std::map\n>> &streamConfig,\n>> - const std::map> &> &entityControls,\n>> - const IPAOperationData &ipaConfig,\n>> - IPAOperationData *result) override;\n>> + int configure(const CameraSensorInfo &sensorInfo,\n>> + const std::map\n>> &streamConfig,\n>> + const std::map> &> &entityControls,\n>> + const IPAOperationData &ipaConfig,\n>> + IPAOperationData *result) override;\n>> void mapBuffers(const std::vector &buffers) override;\n>> void unmapBuffers(const std::vector &ids) override;\n>> void processEvent(const IPAOperationData &event) override;\n>> @@ -132,14 +132,14 @@ void IPAProxyThread::stop()\n>> thread_.wait();\n>> }\n>>\n>> -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n>> - const std::map\n>> &streamConfig,\n>> - const std::map> ControlInfoMap &> &entityControls,\n>> - const IPAOperationData &ipaConfig,\n>> - IPAOperationData *result)\n>> +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n>> + const std::map\n>> &streamConfig,\n>> + const std::map> ControlInfoMap &> &entityControls,\n>> + const IPAOperationData &ipaConfig,\n>> + IPAOperationData *result)\n>> {\n>> - ipa_->configure(sensorInfo, streamConfig, entityControls,\n>> ipaConfig,\n>> - result);\n>> + return ipa_->configure(sensorInfo, streamConfig, entityControls,\n>> + ipaConfig, result);\n>> }\n>>\n>> void IPAProxyThread::mapBuffers(const std::vector &buffers)\n>> diff --git a/test/ipa/ipa_wrappers_test.cpp\n>> b/test/ipa/ipa_wrappers_test.cpp\n>> index 59d991cbbf6a..ad0fb0386865 100644\n>> --- a/test/ipa/ipa_wrappers_test.cpp\n>> +++ b/test/ipa/ipa_wrappers_test.cpp\n>> @@ -67,55 +67,63 @@ public:\n>> report(Op_stop, TestPass);\n>> }\n>>\n>> - void configure(const CameraSensorInfo &sensorInfo,\n>> - const std::map\n>> &streamConfig,\n>> - const std::map> &> &entityControls,\n>> - [[maybe_unused]] const IPAOperationData &ipaConfig,\n>> - [[maybe_unused]] IPAOperationData *result) override\n>> + int configure(const CameraSensorInfo &sensorInfo,\n>> + const std::map\n>> &streamConfig,\n>> + const std::map> &> &entityControls,\n>> + [[maybe_unused]] const IPAOperationData &ipaConfig,\n>> + [[maybe_unused]] IPAOperationData *result) override\n>> {\n>> /* Verify sensorInfo. */\n>> if (sensorInfo.outputSize.width != 2560 ||\n>> sensorInfo.outputSize.height != 1940) {\n>> cerr << \"configure(): Invalid sensor info size \"\n>> << sensorInfo.outputSize.toString();\n>> + report(Op_configure, TestFail);\n>> + return -EINVAL;\n>> }\n>>\n>> /* Verify streamConfig. */\n>> if (streamConfig.size() != 2) {\n>> cerr << \"configure(): Invalid number of streams \"\n>> << streamConfig.size() << endl;\n>> - return report(Op_configure, TestFail);\n>> + report(Op_configure, TestFail);\n>> + return -EINVAL;\n>> }\n>>\n>> auto iter = streamConfig.find(1);\n>> if (iter == streamConfig.end()) {\n>> cerr << \"configure(): No configuration for stream\n>> 1\" << endl;\n>> - return report(Op_configure, TestFail);\n>> + report(Op_configure, TestFail);\n>> + return -EINVAL;\n>> }\n>> const IPAStream *stream = &iter->second;\n>> if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||\n>> stream->size != Size{ 1024, 768 }) {\n>> cerr << \"configure(): Invalid configuration for\n>> stream 1\" << endl;\n>> - return report(Op_configure, TestFail);\n>> + report(Op_configure, TestFail);\n>> + return -EINVAL;\n>> }\n>>\n>> iter = streamConfig.find(2);\n>> if (iter == streamConfig.end()) {\n>> cerr << \"configure(): No configuration for stream\n>> 2\" << endl;\n>> - return report(Op_configure, TestFail);\n>> + report(Op_configure, TestFail);\n>> + return -EINVAL;\n>> }\n>> stream = &iter->second;\n>> if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||\n>> stream->size != Size{ 800, 600 }) {\n>> cerr << \"configure(): Invalid configuration for\n>> stream 2\" << endl;\n>> - return report(Op_configure, TestFail);\n>> + report(Op_configure, TestFail);\n>> + return -EINVAL;\n>> }\n>>\n>> /* Verify entityControls. */\n>> auto ctrlIter = entityControls.find(42);\n>> if (ctrlIter == entityControls.end()) {\n>> cerr << \"configure(): Controls not found\" << endl;\n>> - return report(Op_configure, TestFail);\n>> + report(Op_configure, TestFail);\n>> + return -EINVAL;\n>> }\n>>\n>> const ControlInfoMap &infoMap = ctrlIter->second;\n>> @@ -124,10 +132,12 @@ public:\n>> infoMap.count(V4L2_CID_CONTRAST) != 1 ||\n>> infoMap.count(V4L2_CID_SATURATION) != 1) {\n>> cerr << \"configure(): Invalid control IDs\" <<\n>> endl;\n>> - return report(Op_configure, TestFail);\n>> + report(Op_configure, TestFail);\n>> + return -EINVAL;\n>> }\n>>\n>> report(Op_configure, TestPass);\n>> + return 0;\n>> }\n>>\n>> void mapBuffers(const std::vector &buffers) override\n>>\n>> > ---\n>> > include/libcamera/ipa/raspberrypi.h | 1 +\n>> > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++++++-\n>> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++++\n>> > 3 files changed, 17 insertions(+), 1 deletion(-)\n>> >\n>> > diff --git a/include/libcamera/ipa/raspberrypi.h\n>> b/include/libcamera/ipa/raspberrypi.h\n>> > index 2b55dbfc..86c97ffa 100644\n>> > --- a/include/libcamera/ipa/raspberrypi.h\n>> > +++ b/include/libcamera/ipa/raspberrypi.h\n>> > @@ -21,6 +21,7 @@ enum ConfigParameters {\n>> > IPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n>> > IPA_CONFIG_SENSOR = (1 << 2),\n>> > IPA_CONFIG_DROP_FRAMES = (1 << 3),\n>> > + IPA_CONFIG_FAILED = (1 << 4),\n>> > };\n>> >\n>> > enum Operations {\n>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n>> b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > index 9853a343..57dd9c61 100644\n>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo\n>> &sensorInfo,\n>> > const IPAOperationData &ipaConfig,\n>> > IPAOperationData *result)\n>> > {\n>> > - if (entityControls.empty())\n>> > + if (entityControls.size() != 2) {\n>> > + LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n>> > + result->operation = RPi::IPA_CONFIG_FAILED;\n>> > return;\n>> > + }\n>> >\n>> > result->operation = 0;\n>> >\n>> > @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo\n>> &sensorInfo,\n>> > if (!helper_) {\n>> > helper_ =\n>> std::unique_ptr(RPiController::CamHelper::Create(cameraName));\n>> >\n>> > + if (!helper_) {\n>> > + LOG(IPARPI, Error) << \"Could not create camera\n>> helper for \"\n>> > + << cameraName;\n>> > + result->operation = RPi::IPA_CONFIG_FAILED;\n>> > + return;\n>> > + }\n>> > +\n>> > /*\n>> > * Pass out the sensor config to the pipeline handler in\n>> order\n>> > * to setup the staggered writer class.\n>> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > index 6fcdf557..76252806 100644\n>> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const\n>> CameraConfiguration *config)\n>> > ipa_->configure(sensorInfo_, streamConfig, entityControls,\n>> ipaConfig,\n>> > &result);\n>> >\n>> > + if (result.operation & RPi::IPA_CONFIG_FAILED) {\n>> > + LOG(RPI, Error) << \"IPA configuration failed!\";\n>> > + return -EPIPE;\n>> > + }\n>> > +\n>> > unsigned int resultIdx = 0;\n>> > if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {\n>> > /*\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\n>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 64ACBBE177\n\tfor ;\n\tTue, 1 Dec 2020 10:00:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 314D7634A5;\n\tTue, 1 Dec 2020 11:00:16 +0100 (CET)","from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D9506032B\n\tfor ;\n\tTue, 1 Dec 2020 11:00:14 +0100 (CET)","by mail-lj1-x231.google.com with SMTP id o24so1904482ljj.6\n\tfor ;\n\tTue, 01 Dec 2020 02:00:14 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Rm3hpoHj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=a6aWNCIG5P2J3sXl/JliMDskmzkX4n/Zwp0oTzhy+ps=;\n\tb=Rm3hpoHjHzV8HdGTiomIpOZWrfHgCfM3auiqYqStxbu1Br2zuVs+9kTbl8Osh/Ocj+\n\tTG/Up1+uIN7PZs1sidnvusgA+3ck9nB/U0NC3mqk7V+B/pp6CJtiRxJZB4ilOgRsEgD4\n\tUd6kyrWiFljIw4ksX7t7xtTT7eNQEhRPYkAr62S/4yoOSFKSmbZm7Est4SalfCl0hpdw\n\twP4STTXW+qftmWesWmKqdc26W5iZNIwaCf2MlSO7GzWXZwecaC/vWXJ5Lgv/Rsf8cQWJ\n\twJ8NzFw7x6OioJ7ef9YXyvHhYsujx7eQtWwFuN91fppwuMpXNIz10iIZ7nsMB6nO9hOA\n\taUyw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=a6aWNCIG5P2J3sXl/JliMDskmzkX4n/Zwp0oTzhy+ps=;\n\tb=inYtzSJfqqZOqpUL1h6J5gUhOu7uU48whgeCfnS+v8mjZoV45J9xHuy9fW5XLiQtEL\n\thVfa0rCA0dDwSJKrs7o1L+F7rikqUUcVzEOwuDvl7J1AsOksIjmlzXF8yhdwFBwHCUh/\n\te/vrv4QZD6OAqGaW/tBcFQu2M+hOxvM+3O6+vMSfC8JP/DJigrgyylJfUvSmkUnU8cYS\n\tEYJsHQtHZzkf7YCss14ZtdQkJf6FqJnvmLQK8mKg5TNg26u+c1BIU1hCuInwnpifRRl6\n\tHMzZ3ttROAGO9pnHqTBEo+cD4z1qu6X/hy/gE9+MG2Tn9XQna3LJJLuHivFpf4hGH4OJ\n\t3gBA==","X-Gm-Message-State":"AOAM5329VUW6qi0orsqg5KKuz1oU+Q0kMZxKvTKYQul7oSmgwjGHi3kw\n\tzjbCYxvbXySeZD1COBlmPO3CWjhNi8YdsSgXlH8pjQ==","X-Google-Smtp-Source":"ABdhPJzUrx/7DoaE56raFlpPaDMEkUNVQD3BD3VwqO6r7yPb2fL52OysJqqq9gcTbhDMc/QFfiU5wqTuAV99DbTYnBc=","X-Received":"by 2002:a2e:80c6:: with SMTP id r6mr903382ljg.83.1606816813609; \n\tTue, 01 Dec 2020 02:00:13 -0800 (PST)","MIME-Version":"1.0","References":"<20201127145612.1105770-1-naush@raspberrypi.com>\n\t<20201201015543.GF4315@pendragon.ideasonboard.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 1 Dec 2020 09:59:57 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============3152758264370470799==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13997,"web_url":"https://patchwork.libcamera.org/comment/13997/","msgid":"<20201201100411.GA4569@pendragon.ideasonboard.com>","date":"2020-12-01T10:04:11","subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Dec 01, 2020 at 09:54:44AM +0000, Naushir Patuck wrote:\n> On Tue, 1 Dec 2020 at 01:55, Laurent Pinchart wrote:\n> > On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:\n> > > If the IPA fails during configuration, return an error flag to the\n> > > pipeline handler and fail the use case gracefully.\n> > >\n> > > At present, the IPA configuration can fail for the following reasons:\n> > > - The sensor is not recognised, and fails to open a CamHelper object.\n> > > - The pipeline handler did not pass in controls for the ISP and sensor.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: Jacopo Mondi \n> >\n> > Wouldn't it be simpler to modify the configure() function to return an\n> > int ? How about the following ? If it works for you I'll submit a proper\n> > patch.\n> \n> Yes, that works equally well for me. Happy for you to submit the patch, or\n> would you prefer if I add your commit below to a new patch set that will\n> also include the Raspberry Pi IPA/PH changes?\n\nI've just submitted the patch :-) I've asked Paul to look at it though,\nto see if it would be easier to rebase his in-progress IPA IPC work on\ntop of your patch or on top of mine.\n\n> > Author: Laurent Pinchart \n> > Date: Tue Dec 1 03:54:18 2020 +0200\n> >\n> > libcamera: ipa_interface: Make configure() return an int\n> >\n> > It's useful for the IPAInterface::configure() function to be able to\n> > return a status. Make it return an int, to avoid forcing IPAs to return\n> > a status encoded in the IPAOperationData in a custom way.\n> >\n> > Signed-off-by: Laurent Pinchart \n> >\n> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n> > b/include/libcamera/internal/ipa_context_wrapper.h\n> > index 8f767e844221..a00b5e7b92eb 100644\n> > --- a/include/libcamera/internal/ipa_context_wrapper.h\n> > +++ b/include/libcamera/internal/ipa_context_wrapper.h\n> > @@ -22,11 +22,11 @@ public:\n> > int init(const IPASettings &settings) override;\n> > int start() override;\n> > void stop() override;\n> > - void configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> > &streamConfig,\n> > - const std::map > &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result) override;\n> > + int configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> > &streamConfig,\n> > + const std::map\n> > &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result) override;\n> >\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > diff --git a/include/libcamera/ipa/ipa_interface.h\n> > b/include/libcamera/ipa/ipa_interface.h\n> > index 322b7079070e..1d8cf8dc1c56 100644\n> > --- a/include/libcamera/ipa/ipa_interface.h\n> > +++ b/include/libcamera/ipa/ipa_interface.h\n> > @@ -95,12 +95,12 @@ struct ipa_context_ops {\n> > void (*register_callbacks)(struct ipa_context *ctx,\n> > const struct ipa_callback_ops\n> > *callbacks,\n> > void *cb_ctx);\n> > - void (*configure)(struct ipa_context *ctx,\n> > - const struct ipa_sensor_info *sensor_info,\n> > - const struct ipa_stream *streams,\n> > - unsigned int num_streams,\n> > - const struct ipa_control_info_map *maps,\n> > - unsigned int num_maps);\n> > + int (*configure)(struct ipa_context *ctx,\n> > + const struct ipa_sensor_info *sensor_info,\n> > + const struct ipa_stream *streams,\n> > + unsigned int num_streams,\n> > + const struct ipa_control_info_map *maps,\n> > + unsigned int num_maps);\n> > void (*map_buffers)(struct ipa_context *ctx,\n> > const struct ipa_buffer *buffers,\n> > size_t num_buffers);\n> > @@ -156,11 +156,11 @@ public:\n> > virtual int start() = 0;\n> > virtual void stop() = 0;\n> >\n> > - virtual void configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> > &streamConfig,\n> > - const std::map > ControlInfoMap &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result) = 0;\n> > + virtual int configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> > &streamConfig,\n> > + const std::map > ControlInfoMap &> &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result) = 0;\n> >\n> > virtual void mapBuffers(const std::vector &buffers) = 0;\n> > virtual void unmapBuffers(const std::vector &ids) =\n> > 0;\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > index cee532e3a747..74d7bc6e1c9b 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > @@ -115,12 +115,12 @@ void IPAInterfaceWrapper::register_callbacks(struct\n> > ipa_context *_ctx,\n> > ctx->cb_ctx_ = cb_ctx;\n> > }\n> >\n> > -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> > - const struct ipa_sensor_info\n> > *sensor_info,\n> > - const struct ipa_stream *streams,\n> > - unsigned int num_streams,\n> > - const struct ipa_control_info_map\n> > *maps,\n> > - unsigned int num_maps)\n> > +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> > + const struct ipa_sensor_info\n> > *sensor_info,\n> > + const struct ipa_stream *streams,\n> > + unsigned int num_streams,\n> > + const struct ipa_control_info_map *maps,\n> > + unsigned int num_maps)\n> > {\n> > IPAInterfaceWrapper *ctx = static_cast > *>(_ctx);\n> >\n> > @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct ipa_context\n> > *_ctx,\n> >\n> > /* \\todo Translate the ipaConfig and result. */\n> > IPAOperationData ipaConfig;\n> > - ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,\n> > ipaConfig,\n> > - nullptr);\n> > + return ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,\n> > + ipaConfig, nullptr);\n> > }\n> >\n> > void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h\n> > b/src/ipa/libipa/ipa_interface_wrapper.h\n> > index a1c701599b56..acd3160039b1 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> > @@ -30,12 +30,12 @@ private:\n> > static void register_callbacks(struct ipa_context *ctx,\n> > const struct ipa_callback_ops\n> > *callbacks,\n> > void *cb_ctx);\n> > - static void configure(struct ipa_context *ctx,\n> > - const struct ipa_sensor_info *sensor_info,\n> > - const struct ipa_stream *streams,\n> > - unsigned int num_streams,\n> > - const struct ipa_control_info_map *maps,\n> > - unsigned int num_maps);\n> > + static int configure(struct ipa_context *ctx,\n> > + const struct ipa_sensor_info *sensor_info,\n> > + const struct ipa_stream *streams,\n> > + unsigned int num_streams,\n> > + const struct ipa_control_info_map *maps,\n> > + unsigned int num_maps);\n> > static void map_buffers(struct ipa_context *ctx,\n> > const struct ipa_buffer *c_buffers,\n> > size_t num_buffers);\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 9853a343c892..75f7af3430ef 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -81,11 +81,11 @@ public:\n> > int start() override { return 0; }\n> > void stop() override {}\n> >\n> > - void configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> > &streamConfig,\n> > - const std::map > &> &entityControls,\n> > - const IPAOperationData &data,\n> > - IPAOperationData *response) override;\n> > + int configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> > &streamConfig,\n> > + const std::map\n> > &entityControls,\n> > + const IPAOperationData &data,\n> > + IPAOperationData *response) override;\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > void processEvent(const IPAOperationData &event) override;\n> > @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo\n> > &sensorInfo)\n> > mode_.line_length = 1e9 * sensorInfo.lineLength /\n> > sensorInfo.pixelRate;\n> > }\n> >\n> > -void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > - [[maybe_unused]] const std::map > IPAStream> &streamConfig,\n> > - const std::map > &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result)\n> > +int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > + [[maybe_unused]] const std::map > IPAStream> &streamConfig,\n> > + const std::map\n> > &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result)\n> > {\n> > if (entityControls.empty())\n> > - return;\n> > + return -EINVAL;\n> >\n> > result->operation = 0;\n> >\n> > @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo\n> > &sensorInfo,\n> > }\n> >\n> > lastMode_ = mode_;\n> > + return 0;\n> > }\n> >\n> > void IPARPi::mapBuffers(const std::vector &buffers)\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 07d7f1b2ddd8..78d78c5ac521 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -40,11 +40,11 @@ public:\n> > int start() override { return 0; }\n> > void stop() override {}\n> >\n> > - void configure(const CameraSensorInfo &info,\n> > - const std::map\n> > &streamConfig,\n> > - const std::map > &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *response) override;\n> > + int configure(const CameraSensorInfo &info,\n> > + const std::map\n> > &streamConfig,\n> > + const std::map\n> > &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *response) override;\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > void processEvent(const IPAOperationData &event) override;\n> > @@ -79,27 +79,27 @@ private:\n> > * assemble one. Make sure the reported sensor information are relevant\n> > * before accessing them.\n> > */\n> > -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> > - [[maybe_unused]] const std::map > IPAStream> &streamConfig,\n> > - const std::map > ControlInfoMap &> &entityControls,\n> > - [[maybe_unused]] const IPAOperationData\n> > &ipaConfig,\n> > - [[maybe_unused]] IPAOperationData *result)\n> > +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> > + [[maybe_unused]] const std::map > IPAStream> &streamConfig,\n> > + const std::map > &> &entityControls,\n> > + [[maybe_unused]] const IPAOperationData\n> > &ipaConfig,\n> > + [[maybe_unused]] IPAOperationData *result)\n> > {\n> > if (entityControls.empty())\n> > - return;\n> > + return -EINVAL;\n> >\n> > ctrls_ = entityControls.at(0);\n> >\n> > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> > if (itExp == ctrls_.end()) {\n> > LOG(IPARkISP1, Error) << \"Can't find exposure control\";\n> > - return;\n> > + return -EINVAL;\n> > }\n> >\n> > const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> > if (itGain == ctrls_.end()) {\n> > LOG(IPARkISP1, Error) << \"Can't find gain control\";\n> > - return;\n> > + return -EINVAL;\n> > }\n> >\n> > autoExposure_ = true;\n> > @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const\n> > CameraSensorInfo &info,\n> > << \" Gain: \" << minGain_ << \"-\" << maxGain_;\n> >\n> > setControls(0);\n> > + return 0;\n> > }\n> >\n> > void IPARkISP1::mapBuffers(const std::vector &buffers)\n> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> > index cf8411359e40..79c8c2fb8927 100644\n> > --- a/src/ipa/vimc/vimc.cpp\n> > +++ b/src/ipa/vimc/vimc.cpp\n> > @@ -37,11 +37,11 @@ public:\n> > int start() override;\n> > void stop() override;\n> >\n> > - void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> > - [[maybe_unused]] const std::map > IPAStream> &streamConfig,\n> > - [[maybe_unused]] const std::map > ControlInfoMap &> &entityControls,\n> > - [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > - [[maybe_unused]] IPAOperationData *result) override\n> > {}\n> > + int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> > + [[maybe_unused]] const std::map > IPAStream> &streamConfig,\n> > + [[maybe_unused]] const std::map > ControlInfoMap &> &entityControls,\n> > + [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > + [[maybe_unused]] IPAOperationData *result) override\n> > { return 0; }\n> > void mapBuffers([[maybe_unused]] const std::vector\n> > &buffers) override {}\n> > void unmapBuffers([[maybe_unused]] const std::vector\n> > &ids) override {}\n> > void processEvent([[maybe_unused]] const IPAOperationData &event)\n> > override {}\n> > diff --git a/src/libcamera/ipa_context_wrapper.cpp\n> > b/src/libcamera/ipa_context_wrapper.cpp\n> > index 231300ce0bec..f63ad830c003 100644\n> > --- a/src/libcamera/ipa_context_wrapper.cpp\n> > +++ b/src/libcamera/ipa_context_wrapper.cpp\n> > @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()\n> > ctx_->ops->stop(ctx_);\n> > }\n> >\n> > -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> > &streamConfig,\n> > - const std::map > ControlInfoMap &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result)\n> > +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> > &streamConfig,\n> > + const std::map > ControlInfoMap &> &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result)\n> > {\n> > if (intf_)\n> > return intf_->configure(sensorInfo, streamConfig,\n> > entityControls, ipaConfig, result);\n> >\n> > if (!ctx_)\n> > - return;\n> > + return 0;\n> >\n> > serializer_.reset();\n> >\n> > @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const\n> > CameraSensorInfo &sensorInfo,\n> > }\n> >\n> > /* \\todo Translate the ipaConfig and reponse */\n> > - ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n> > streamConfig.size(),\n> > - c_info_maps, entityControls.size());\n> > + return ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n> > + streamConfig.size(), c_info_maps,\n> > + entityControls.size());\n> > }\n> >\n> > void IPAContextWrapper::mapBuffers(const std::vector &buffers)\n> > diff --git a/src/libcamera/ipa_interface.cpp\n> > b/src/libcamera/ipa_interface.cpp\n> > index 23fc56d7d48e..516a8ecd4b53 100644\n> > --- a/src/libcamera/ipa_interface.cpp\n> > +++ b/src/libcamera/ipa_interface.cpp\n> > @@ -347,6 +347,8 @@\n> > * \\param[in] num_maps The number of entries in the \\a maps array\n> > *\n> > * \\sa libcamera::IPAInterface::configure()\n> > + *\n> > + * \\return 0 on success or a negative error code on failure\n> > */\n> >\n> > /**\n> > @@ -573,6 +575,8 @@ namespace libcamera {\n> > * pipeline handler to the IPA and back. The pipeline handler may set the\n> > \\a\n> > * result parameter to null if the IPA protocol doesn't need to pass a\n> > result\n> > * back through the configure() function.\n> > + *\n> > + * \\return 0 on success or a negative error code on failure\n> > */\n> >\n> > /**\n> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > index b78a0e4535f5..ed250ce79c17 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > @@ -32,11 +32,11 @@ public:\n> > }\n> > int start() override { return 0; }\n> > void stop() override {}\n> > - void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> > - [[maybe_unused]] const std::map > IPAStream> &streamConfig,\n> > - [[maybe_unused]] const std::map > ControlInfoMap &> &entityControls,\n> > - [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > - [[maybe_unused]] IPAOperationData *result) override\n> > {}\n> > + int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> > + [[maybe_unused]] const std::map > IPAStream> &streamConfig,\n> > + [[maybe_unused]] const std::map > ControlInfoMap &> &entityControls,\n> > + [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > + [[maybe_unused]] IPAOperationData *result) override\n> > { return 0; }\n> > void mapBuffers([[maybe_unused]] const std::vector\n> > &buffers) override {}\n> > void unmapBuffers([[maybe_unused]] const std::vector\n> > &ids) override {}\n> > void processEvent([[maybe_unused]] const IPAOperationData &event)\n> > override {}\n> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > index eead2883708d..fd91726c4840 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > @@ -29,11 +29,11 @@ public:\n> > int start() override;\n> > void stop() override;\n> >\n> > - void configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> > &streamConfig,\n> > - const std::map > &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result) override;\n> > + int configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> > &streamConfig,\n> > + const std::map\n> > &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result) override;\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > void processEvent(const IPAOperationData &event) override;\n> > @@ -132,14 +132,14 @@ void IPAProxyThread::stop()\n> > thread_.wait();\n> > }\n> >\n> > -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> > &streamConfig,\n> > - const std::map > ControlInfoMap &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result)\n> > +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> > &streamConfig,\n> > + const std::map > ControlInfoMap &> &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result)\n> > {\n> > - ipa_->configure(sensorInfo, streamConfig, entityControls,\n> > ipaConfig,\n> > - result);\n> > + return ipa_->configure(sensorInfo, streamConfig, entityControls,\n> > + ipaConfig, result);\n> > }\n> >\n> > void IPAProxyThread::mapBuffers(const std::vector &buffers)\n> > diff --git a/test/ipa/ipa_wrappers_test.cpp\n> > b/test/ipa/ipa_wrappers_test.cpp\n> > index 59d991cbbf6a..ad0fb0386865 100644\n> > --- a/test/ipa/ipa_wrappers_test.cpp\n> > +++ b/test/ipa/ipa_wrappers_test.cpp\n> > @@ -67,55 +67,63 @@ public:\n> > report(Op_stop, TestPass);\n> > }\n> >\n> > - void configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> > &streamConfig,\n> > - const std::map > &> &entityControls,\n> > - [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > - [[maybe_unused]] IPAOperationData *result) override\n> > + int configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> > &streamConfig,\n> > + const std::map\n> > &entityControls,\n> > + [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > + [[maybe_unused]] IPAOperationData *result) override\n> > {\n> > /* Verify sensorInfo. */\n> > if (sensorInfo.outputSize.width != 2560 ||\n> > sensorInfo.outputSize.height != 1940) {\n> > cerr << \"configure(): Invalid sensor info size \"\n> > << sensorInfo.outputSize.toString();\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > /* Verify streamConfig. */\n> > if (streamConfig.size() != 2) {\n> > cerr << \"configure(): Invalid number of streams \"\n> > << streamConfig.size() << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > auto iter = streamConfig.find(1);\n> > if (iter == streamConfig.end()) {\n> > cerr << \"configure(): No configuration for stream\n> > 1\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> > const IPAStream *stream = &iter->second;\n> > if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||\n> > stream->size != Size{ 1024, 768 }) {\n> > cerr << \"configure(): Invalid configuration for\n> > stream 1\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > iter = streamConfig.find(2);\n> > if (iter == streamConfig.end()) {\n> > cerr << \"configure(): No configuration for stream\n> > 2\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> > stream = &iter->second;\n> > if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||\n> > stream->size != Size{ 800, 600 }) {\n> > cerr << \"configure(): Invalid configuration for\n> > stream 2\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > /* Verify entityControls. */\n> > auto ctrlIter = entityControls.find(42);\n> > if (ctrlIter == entityControls.end()) {\n> > cerr << \"configure(): Controls not found\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > const ControlInfoMap &infoMap = ctrlIter->second;\n> > @@ -124,10 +132,12 @@ public:\n> > infoMap.count(V4L2_CID_CONTRAST) != 1 ||\n> > infoMap.count(V4L2_CID_SATURATION) != 1) {\n> > cerr << \"configure(): Invalid control IDs\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > report(Op_configure, TestPass);\n> > + return 0;\n> > }\n> >\n> > void mapBuffers(const std::vector &buffers) override\n> >\n> > > ---\n> > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++++++-\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++++\n> > > 3 files changed, 17 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h\n> > b/include/libcamera/ipa/raspberrypi.h\n> > > index 2b55dbfc..86c97ffa 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -21,6 +21,7 @@ enum ConfigParameters {\n> > > IPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> > > IPA_CONFIG_SENSOR = (1 << 2),\n> > > IPA_CONFIG_DROP_FRAMES = (1 << 3),\n> > > + IPA_CONFIG_FAILED = (1 << 4),\n> > > };\n> > >\n> > > enum Operations {\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 9853a343..57dd9c61 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo\n> > &sensorInfo,\n> > > const IPAOperationData &ipaConfig,\n> > > IPAOperationData *result)\n> > > {\n> > > - if (entityControls.empty())\n> > > + if (entityControls.size() != 2) {\n> > > + LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > > + result->operation = RPi::IPA_CONFIG_FAILED;\n> > > return;\n> > > + }\n> > >\n> > > result->operation = 0;\n> > >\n> > > @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo\n> > &sensorInfo,\n> > > if (!helper_) {\n> > > helper_ =\n> > std::unique_ptr(RPiController::CamHelper::Create(cameraName));\n> > >\n> > > + if (!helper_) {\n> > > + LOG(IPARPI, Error) << \"Could not create camera\n> > helper for \"\n> > > + << cameraName;\n> > > + result->operation = RPi::IPA_CONFIG_FAILED;\n> > > + return;\n> > > + }\n> > > +\n> > > /*\n> > > * Pass out the sensor config to the pipeline handler in\n> > order\n> > > * to setup the staggered writer class.\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 6fcdf557..76252806 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const\n> > CameraConfiguration *config)\n> > > ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> > ipaConfig,\n> > > &result);\n> > >\n> > > + if (result.operation & RPi::IPA_CONFIG_FAILED) {\n> > > + LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > + return -EPIPE;\n> > > + }\n> > > +\n> > > unsigned int resultIdx = 0;\n> > > if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {\n> > > /*","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7C18DBE177\n\tfor ;\n\tTue, 1 Dec 2020 10:04:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0555E634AA;\n\tTue, 1 Dec 2020 11:04:21 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D647B6032B\n\tfor ;\n\tTue, 1 Dec 2020 11:04:19 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4DA55538;\n\tTue, 1 Dec 2020 11:04:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"p0GVLdiJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606817059;\n\tbh=zrjStI6COX2PWfE1b7jHbWEjxHRIDZM3X2+sp/KFZ38=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p0GVLdiJhpbuPGRoy7DQVDIq9a+H+jMAiE22rFj2eWD16aW/rUnfAv/rvmMgX6iNw\n\tMRxfSFS9k2S14cMX1Jal7US+NYzWx8fgbStAG/4Gt2Jc0IpUJ+Thhz4yiFhNurE5sp\n\thVmEXaLH9ok9pda8ZC5/xUoItB3TXwrAoIHfGh7g=","Date":"Tue, 1 Dec 2020 12:04:11 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20201201100411.GA4569@pendragon.ideasonboard.com>","References":"<20201127145612.1105770-1-naush@raspberrypi.com>\n\t<20201201015543.GF4315@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14044,"web_url":"https://patchwork.libcamera.org/comment/14044/","msgid":"<20201204103927.GD2050@pyrite.rasen.tech>","date":"2020-12-04T10:39:27","subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Dec 01, 2020 at 03:55:43AM +0200, Laurent Pinchart wrote:\n> Hi Naush,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:\n> > If the IPA fails during configuration, return an error flag to the\n> > pipeline handler and fail the use case gracefully.\n> > \n> > At present, the IPA configuration can fail for the following reasons:\n> > - The sensor is not recognised, and fails to open a CamHelper object.\n> > - The pipeline handler did not pass in controls for the ISP and sensor.\n> > \n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: Jacopo Mondi \n> \n> Wouldn't it be simpler to modify the configure() function to return an\n> int ? How about the following ? If it works for you I'll submit a proper\n> patch.\n\nFrom the perspective of the IPC changes, Naush's solution is more easily\ntranslatable. When the IPA interface will become customizable, there\nisn't a way to specify which output parameters should be return values\nvs return parameters. Thus keeping them all as return parameters is the\nbetter solution here.\n\nSo imo this is fine to merge as-is.\n\nReviewed-by: Paul Elder \n\n> Author: Laurent Pinchart \n> Date: Tue Dec 1 03:54:18 2020 +0200\n> \n> libcamera: ipa_interface: Make configure() return an int\n> \n> It's useful for the IPAInterface::configure() function to be able to\n> return a status. Make it return an int, to avoid forcing IPAs to return\n> a status encoded in the IPAOperationData in a custom way.\n> \n> Signed-off-by: Laurent Pinchart \n> \n> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h\n> index 8f767e844221..a00b5e7b92eb 100644\n> --- a/include/libcamera/internal/ipa_context_wrapper.h\n> +++ b/include/libcamera/internal/ipa_context_wrapper.h\n> @@ -22,11 +22,11 @@ public:\n> \tint init(const IPASettings &settings) override;\n> \tint start() override;\n> \tvoid stop() override;\n> -\tvoid configure(const CameraSensorInfo &sensorInfo,\n> -\t\t const std::map &streamConfig,\n> -\t\t const std::map &entityControls,\n> -\t\t const IPAOperationData &ipaConfig,\n> -\t\t IPAOperationData *result) override;\n> +\tint configure(const CameraSensorInfo &sensorInfo,\n> +\t\t const std::map &streamConfig,\n> +\t\t const std::map &entityControls,\n> +\t\t const IPAOperationData &ipaConfig,\n> +\t\t IPAOperationData *result) override;\n> \n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h\n> index 322b7079070e..1d8cf8dc1c56 100644\n> --- a/include/libcamera/ipa/ipa_interface.h\n> +++ b/include/libcamera/ipa/ipa_interface.h\n> @@ -95,12 +95,12 @@ struct ipa_context_ops {\n> \tvoid (*register_callbacks)(struct ipa_context *ctx,\n> \t\t\t\t const struct ipa_callback_ops *callbacks,\n> \t\t\t\t void *cb_ctx);\n> -\tvoid (*configure)(struct ipa_context *ctx,\n> -\t\t\t const struct ipa_sensor_info *sensor_info,\n> -\t\t\t const struct ipa_stream *streams,\n> -\t\t\t unsigned int num_streams,\n> -\t\t\t const struct ipa_control_info_map *maps,\n> -\t\t\t unsigned int num_maps);\n> +\tint (*configure)(struct ipa_context *ctx,\n> +\t\t\t const struct ipa_sensor_info *sensor_info,\n> +\t\t\t const struct ipa_stream *streams,\n> +\t\t\t unsigned int num_streams,\n> +\t\t\t const struct ipa_control_info_map *maps,\n> +\t\t\t unsigned int num_maps);\n> \tvoid (*map_buffers)(struct ipa_context *ctx,\n> \t\t\t const struct ipa_buffer *buffers,\n> \t\t\t size_t num_buffers);\n> @@ -156,11 +156,11 @@ public:\n> \tvirtual int start() = 0;\n> \tvirtual void stop() = 0;\n> \n> -\tvirtual void configure(const CameraSensorInfo &sensorInfo,\n> -\t\t\t const std::map &streamConfig,\n> -\t\t\t const std::map &entityControls,\n> -\t\t\t const IPAOperationData &ipaConfig,\n> -\t\t\t IPAOperationData *result) = 0;\n> +\tvirtual int configure(const CameraSensorInfo &sensorInfo,\n> +\t\t\t const std::map &streamConfig,\n> +\t\t\t const std::map &entityControls,\n> +\t\t\t const IPAOperationData &ipaConfig,\n> +\t\t\t IPAOperationData *result) = 0;\n> \n> \tvirtual void mapBuffers(const std::vector &buffers) = 0;\n> \tvirtual void unmapBuffers(const std::vector &ids) = 0;\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> index cee532e3a747..74d7bc6e1c9b 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> @@ -115,12 +115,12 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,\n> \tctx->cb_ctx_ = cb_ctx;\n> }\n> \n> -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> -\t\t\t\t const struct ipa_sensor_info *sensor_info,\n> -\t\t\t\t const struct ipa_stream *streams,\n> -\t\t\t\t unsigned int num_streams,\n> -\t\t\t\t const struct ipa_control_info_map *maps,\n> -\t\t\t\t unsigned int num_maps)\n> +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> +\t\t\t\t const struct ipa_sensor_info *sensor_info,\n> +\t\t\t\t const struct ipa_stream *streams,\n> +\t\t\t\t unsigned int num_streams,\n> +\t\t\t\t const struct ipa_control_info_map *maps,\n> +\t\t\t\t unsigned int num_maps)\n> {\n> \tIPAInterfaceWrapper *ctx = static_cast(_ctx);\n> \n> @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> \n> \t/* \\todo Translate the ipaConfig and result. */\n> \tIPAOperationData ipaConfig;\n> -\tctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,\n> -\t\t\t nullptr);\n> +\treturn ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,\n> +\t\t\t\t ipaConfig, nullptr);\n> }\n> \n> void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h\n> index a1c701599b56..acd3160039b1 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> @@ -30,12 +30,12 @@ private:\n> \tstatic void register_callbacks(struct ipa_context *ctx,\n> \t\t\t\t const struct ipa_callback_ops *callbacks,\n> \t\t\t\t void *cb_ctx);\n> -\tstatic void configure(struct ipa_context *ctx,\n> -\t\t\t const struct ipa_sensor_info *sensor_info,\n> -\t\t\t const struct ipa_stream *streams,\n> -\t\t\t unsigned int num_streams,\n> -\t\t\t const struct ipa_control_info_map *maps,\n> -\t\t\t unsigned int num_maps);\n> +\tstatic int configure(struct ipa_context *ctx,\n> +\t\t\t const struct ipa_sensor_info *sensor_info,\n> +\t\t\t const struct ipa_stream *streams,\n> +\t\t\t unsigned int num_streams,\n> +\t\t\t const struct ipa_control_info_map *maps,\n> +\t\t\t unsigned int num_maps);\n> \tstatic void map_buffers(struct ipa_context *ctx,\n> \t\t\t\tconst struct ipa_buffer *c_buffers,\n> \t\t\t\tsize_t num_buffers);\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 9853a343c892..75f7af3430ef 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -81,11 +81,11 @@ public:\n> \tint start() override { return 0; }\n> \tvoid stop() override {}\n> \n> -\tvoid configure(const CameraSensorInfo &sensorInfo,\n> -\t\t const std::map &streamConfig,\n> -\t\t const std::map &entityControls,\n> -\t\t const IPAOperationData &data,\n> -\t\t IPAOperationData *response) override;\n> +\tint configure(const CameraSensorInfo &sensorInfo,\n> +\t\t const std::map &streamConfig,\n> +\t\t const std::map &entityControls,\n> +\t\t const IPAOperationData &data,\n> +\t\t IPAOperationData *response) override;\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> \tvoid processEvent(const IPAOperationData &event) override;\n> @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> \tmode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;\n> }\n> \n> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> -\t\t [[maybe_unused]] const std::map &streamConfig,\n> -\t\t const std::map &entityControls,\n> -\t\t const IPAOperationData &ipaConfig,\n> -\t\t IPAOperationData *result)\n> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> +\t\t [[maybe_unused]] const std::map &streamConfig,\n> +\t\t const std::map &entityControls,\n> +\t\t const IPAOperationData &ipaConfig,\n> +\t\t IPAOperationData *result)\n> {\n> \tif (entityControls.empty())\n> -\t\treturn;\n> +\t\treturn -EINVAL;\n> \n> \tresult->operation = 0;\n> \n> @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t}\n> \n> \tlastMode_ = mode_;\n> +\treturn 0;\n> }\n> \n> void IPARPi::mapBuffers(const std::vector &buffers)\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 07d7f1b2ddd8..78d78c5ac521 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -40,11 +40,11 @@ public:\n> \tint start() override { return 0; }\n> \tvoid stop() override {}\n> \n> -\tvoid configure(const CameraSensorInfo &info,\n> -\t\t const std::map &streamConfig,\n> -\t\t const std::map &entityControls,\n> -\t\t const IPAOperationData &ipaConfig,\n> -\t\t IPAOperationData *response) override;\n> +\tint configure(const CameraSensorInfo &info,\n> +\t\t const std::map &streamConfig,\n> +\t\t const std::map &entityControls,\n> +\t\t const IPAOperationData &ipaConfig,\n> +\t\t IPAOperationData *response) override;\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> \tvoid processEvent(const IPAOperationData &event) override;\n> @@ -79,27 +79,27 @@ private:\n> * assemble one. Make sure the reported sensor information are relevant\n> * before accessing them.\n> */\n> -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> -\t\t\t [[maybe_unused]] const std::map &streamConfig,\n> -\t\t\t const std::map &entityControls,\n> -\t\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n> -\t\t\t [[maybe_unused]] IPAOperationData *result)\n> +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> +\t\t\t [[maybe_unused]] const std::map &streamConfig,\n> +\t\t\t const std::map &entityControls,\n> +\t\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n> +\t\t\t [[maybe_unused]] IPAOperationData *result)\n> {\n> \tif (entityControls.empty())\n> -\t\treturn;\n> +\t\treturn -EINVAL;\n> \n> \tctrls_ = entityControls.at(0);\n> \n> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> \tif (itExp == ctrls_.end()) {\n> \t\tLOG(IPARkISP1, Error) << \"Can't find exposure control\";\n> -\t\treturn;\n> +\t\treturn -EINVAL;\n> \t}\n> \n> \tconst auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> \tif (itGain == ctrls_.end()) {\n> \t\tLOG(IPARkISP1, Error) << \"Can't find gain control\";\n> -\t\treturn;\n> +\t\treturn -EINVAL;\n> \t}\n> \n> \tautoExposure_ = true;\n> @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> \t\t<< \" Gain: \" << minGain_ << \"-\" << maxGain_;\n> \n> \tsetControls(0);\n> +\treturn 0;\n> }\n> \n> void IPARkISP1::mapBuffers(const std::vector &buffers)\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index cf8411359e40..79c8c2fb8927 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -37,11 +37,11 @@ public:\n> \tint start() override;\n> \tvoid stop() override;\n> \n> -\tvoid configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> -\t\t [[maybe_unused]] const std::map &streamConfig,\n> -\t\t [[maybe_unused]] const std::map &entityControls,\n> -\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n> -\t\t [[maybe_unused]] IPAOperationData *result) override {}\n> +\tint configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> +\t\t [[maybe_unused]] const std::map &streamConfig,\n> +\t\t [[maybe_unused]] const std::map &entityControls,\n> +\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n> +\t\t [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> \tvoid mapBuffers([[maybe_unused]] const std::vector &buffers) override {}\n> \tvoid unmapBuffers([[maybe_unused]] const std::vector &ids) override {}\n> \tvoid processEvent([[maybe_unused]] const IPAOperationData &event) override {}\n> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> index 231300ce0bec..f63ad830c003 100644\n> --- a/src/libcamera/ipa_context_wrapper.cpp\n> +++ b/src/libcamera/ipa_context_wrapper.cpp\n> @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()\n> \tctx_->ops->stop(ctx_);\n> }\n> \n> -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> -\t\t\t\t const std::map &streamConfig,\n> -\t\t\t\t const std::map &entityControls,\n> -\t\t\t\t const IPAOperationData &ipaConfig,\n> -\t\t\t\t IPAOperationData *result)\n> +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> +\t\t\t\t const std::map &streamConfig,\n> +\t\t\t\t const std::map &entityControls,\n> +\t\t\t\t const IPAOperationData &ipaConfig,\n> +\t\t\t\t IPAOperationData *result)\n> {\n> \tif (intf_)\n> \t\treturn intf_->configure(sensorInfo, streamConfig,\n> \t\t\t\t\tentityControls, ipaConfig, result);\n> \n> \tif (!ctx_)\n> -\t\treturn;\n> +\t\treturn 0;\n> \n> \tserializer_.reset();\n> \n> @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> \t}\n> \n> \t/* \\todo Translate the ipaConfig and reponse */\n> -\tctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),\n> -\t\t\t c_info_maps, entityControls.size());\n> +\treturn ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n> +\t\t\t\t streamConfig.size(), c_info_maps,\n> +\t\t\t\t entityControls.size());\n> }\n> \n> void IPAContextWrapper::mapBuffers(const std::vector &buffers)\n> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> index 23fc56d7d48e..516a8ecd4b53 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n> @@ -347,6 +347,8 @@\n> * \\param[in] num_maps The number of entries in the \\a maps array\n> *\n> * \\sa libcamera::IPAInterface::configure()\n> + *\n> + * \\return 0 on success or a negative error code on failure\n> */\n> \n> /**\n> @@ -573,6 +575,8 @@ namespace libcamera {\n> * pipeline handler to the IPA and back. The pipeline handler may set the \\a\n> * result parameter to null if the IPA protocol doesn't need to pass a result\n> * back through the configure() function.\n> + *\n> + * \\return 0 on success or a negative error code on failure\n> */\n> \n> /**\n> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> index b78a0e4535f5..ed250ce79c17 100644\n> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> @@ -32,11 +32,11 @@ public:\n> \t}\n> \tint start() override { return 0; }\n> \tvoid stop() override {}\n> -\tvoid configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> -\t\t [[maybe_unused]] const std::map &streamConfig,\n> -\t\t [[maybe_unused]] const std::map &entityControls,\n> -\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n> -\t\t [[maybe_unused]] IPAOperationData *result) override {}\n> +\tint configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> +\t\t [[maybe_unused]] const std::map &streamConfig,\n> +\t\t [[maybe_unused]] const std::map &entityControls,\n> +\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n> +\t\t [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> \tvoid mapBuffers([[maybe_unused]] const std::vector &buffers) override {}\n> \tvoid unmapBuffers([[maybe_unused]] const std::vector &ids) override {}\n> \tvoid processEvent([[maybe_unused]] const IPAOperationData &event) override {}\n> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> index eead2883708d..fd91726c4840 100644\n> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> @@ -29,11 +29,11 @@ public:\n> \tint start() override;\n> \tvoid stop() override;\n> \n> -\tvoid configure(const CameraSensorInfo &sensorInfo,\n> -\t\t const std::map &streamConfig,\n> -\t\t const std::map &entityControls,\n> -\t\t const IPAOperationData &ipaConfig,\n> -\t\t IPAOperationData *result) override;\n> +\tint configure(const CameraSensorInfo &sensorInfo,\n> +\t\t const std::map &streamConfig,\n> +\t\t const std::map &entityControls,\n> +\t\t const IPAOperationData &ipaConfig,\n> +\t\t IPAOperationData *result) override;\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n> \tvoid processEvent(const IPAOperationData &event) override;\n> @@ -132,14 +132,14 @@ void IPAProxyThread::stop()\n> \tthread_.wait();\n> }\n> \n> -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> -\t\t\t const std::map &streamConfig,\n> -\t\t\t const std::map &entityControls,\n> -\t\t\t const IPAOperationData &ipaConfig,\n> -\t\t\t IPAOperationData *result)\n> +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> +\t\t\t const std::map &streamConfig,\n> +\t\t\t const std::map &entityControls,\n> +\t\t\t const IPAOperationData &ipaConfig,\n> +\t\t\t IPAOperationData *result)\n> {\n> -\tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> -\t\t\tresult);\n> +\treturn ipa_->configure(sensorInfo, streamConfig, entityControls,\n> +\t\t\t ipaConfig, result);\n> }\n> \n> void IPAProxyThread::mapBuffers(const std::vector &buffers)\n> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\n> index 59d991cbbf6a..ad0fb0386865 100644\n> --- a/test/ipa/ipa_wrappers_test.cpp\n> +++ b/test/ipa/ipa_wrappers_test.cpp\n> @@ -67,55 +67,63 @@ public:\n> \t\treport(Op_stop, TestPass);\n> \t}\n> \n> -\tvoid configure(const CameraSensorInfo &sensorInfo,\n> -\t\t const std::map &streamConfig,\n> -\t\t const std::map &entityControls,\n> -\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n> -\t\t [[maybe_unused]] IPAOperationData *result) override\n> +\tint configure(const CameraSensorInfo &sensorInfo,\n> +\t\t const std::map &streamConfig,\n> +\t\t const std::map &entityControls,\n> +\t\t [[maybe_unused]] const IPAOperationData &ipaConfig,\n> +\t\t [[maybe_unused]] IPAOperationData *result) override\n> \t{\n> \t\t/* Verify sensorInfo. */\n> \t\tif (sensorInfo.outputSize.width != 2560 ||\n> \t\t sensorInfo.outputSize.height != 1940) {\n> \t\t\tcerr << \"configure(): Invalid sensor info size \"\n> \t\t\t << sensorInfo.outputSize.toString();\n> +\t\t\treport(Op_configure, TestFail);\n> +\t\t\treturn -EINVAL;\n> \t\t}\n> \n> \t\t/* Verify streamConfig. */\n> \t\tif (streamConfig.size() != 2) {\n> \t\t\tcerr << \"configure(): Invalid number of streams \"\n> \t\t\t << streamConfig.size() << endl;\n> -\t\t\treturn report(Op_configure, TestFail);\n> +\t\t\treport(Op_configure, TestFail);\n> +\t\t\treturn -EINVAL;\n> \t\t}\n> \n> \t\tauto iter = streamConfig.find(1);\n> \t\tif (iter == streamConfig.end()) {\n> \t\t\tcerr << \"configure(): No configuration for stream 1\" << endl;\n> -\t\t\treturn report(Op_configure, TestFail);\n> +\t\t\treport(Op_configure, TestFail);\n> +\t\t\treturn -EINVAL;\n> \t\t}\n> \t\tconst IPAStream *stream = &iter->second;\n> \t\tif (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||\n> \t\t stream->size != Size{ 1024, 768 }) {\n> \t\t\tcerr << \"configure(): Invalid configuration for stream 1\" << endl;\n> -\t\t\treturn report(Op_configure, TestFail);\n> +\t\t\treport(Op_configure, TestFail);\n> +\t\t\treturn -EINVAL;\n> \t\t}\n> \n> \t\titer = streamConfig.find(2);\n> \t\tif (iter == streamConfig.end()) {\n> \t\t\tcerr << \"configure(): No configuration for stream 2\" << endl;\n> -\t\t\treturn report(Op_configure, TestFail);\n> +\t\t\treport(Op_configure, TestFail);\n> +\t\t\treturn -EINVAL;\n> \t\t}\n> \t\tstream = &iter->second;\n> \t\tif (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||\n> \t\t stream->size != Size{ 800, 600 }) {\n> \t\t\tcerr << \"configure(): Invalid configuration for stream 2\" << endl;\n> -\t\t\treturn report(Op_configure, TestFail);\n> +\t\t\treport(Op_configure, TestFail);\n> +\t\t\treturn -EINVAL;\n> \t\t}\n> \n> \t\t/* Verify entityControls. */\n> \t\tauto ctrlIter = entityControls.find(42);\n> \t\tif (ctrlIter == entityControls.end()) {\n> \t\t\tcerr << \"configure(): Controls not found\" << endl;\n> -\t\t\treturn report(Op_configure, TestFail);\n> +\t\t\treport(Op_configure, TestFail);\n> +\t\t\treturn -EINVAL;\n> \t\t}\n> \n> \t\tconst ControlInfoMap &infoMap = ctrlIter->second;\n> @@ -124,10 +132,12 @@ public:\n> \t\t infoMap.count(V4L2_CID_CONTRAST) != 1 ||\n> \t\t infoMap.count(V4L2_CID_SATURATION) != 1) {\n> \t\t\tcerr << \"configure(): Invalid control IDs\" << endl;\n> -\t\t\treturn report(Op_configure, TestFail);\n> +\t\t\treport(Op_configure, TestFail);\n> +\t\t\treturn -EINVAL;\n> \t\t}\n> \n> \t\treport(Op_configure, TestPass);\n> +\t\treturn 0;\n> \t}\n> \n> \tvoid mapBuffers(const std::vector &buffers) override\n> \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 1 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++++++-\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++++\n> > 3 files changed, 17 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index 2b55dbfc..86c97ffa 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -21,6 +21,7 @@ enum ConfigParameters {\n> > \tIPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> > \tIPA_CONFIG_SENSOR = (1 << 2),\n> > \tIPA_CONFIG_DROP_FRAMES = (1 << 3),\n> > +\tIPA_CONFIG_FAILED = (1 << 4),\n> > };\n> > \n> > enum Operations {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 9853a343..57dd9c61 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \t\t const IPAOperationData &ipaConfig,\n> > \t\t IPAOperationData *result)\n> > {\n> > -\tif (entityControls.empty())\n> > +\tif (entityControls.size() != 2) {\n> > +\t\tLOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > +\t\tresult->operation = RPi::IPA_CONFIG_FAILED;\n> > \t\treturn;\n> > +\t}\n> > \n> > \tresult->operation = 0;\n> > \n> > @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > \tif (!helper_) {\n> > \t\thelper_ = std::unique_ptr(RPiController::CamHelper::Create(cameraName));\n> > \n> > +\t\tif (!helper_) {\n> > +\t\t\tLOG(IPARPI, Error) << \"Could not create camera helper for \"\n> > +\t\t\t\t\t << cameraName;\n> > +\t\t\tresult->operation = RPi::IPA_CONFIG_FAILED;\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> > \t\t/*\n> > \t\t * Pass out the sensor config to the pipeline handler in order\n> > \t\t * to setup the staggered writer class.\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 6fcdf557..76252806 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > \tipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > \t\t\t&result);\n> > \n> > +\tif (result.operation & RPi::IPA_CONFIG_FAILED) {\n> > +\t\tLOG(RPI, Error) << \"IPA configuration failed!\";\n> > +\t\treturn -EPIPE;\n> > +\t}\n> > +\n> > \tunsigned int resultIdx = 0;\n> > \tif (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {\n> > \t\t/*","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 33202BE176\n\tfor ;\n\tFri, 4 Dec 2020 10:39:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7ECD635EE;\n\tFri, 4 Dec 2020 11:39:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16AE5615AF\n\tfor ;\n\tFri, 4 Dec 2020 11:39:36 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ABDAE99A;\n\tFri, 4 Dec 2020 11:39:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dTrh3JVx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607078375;\n\tbh=ErhXhQYYJbthFys+/GY9upAcbEXbKi7Mf1W9vQXxyEE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dTrh3JVx1QTUcwtmAz6SeZZb9oYUge0reGUqR85WZhrTsBwrDHKU+wbKb1FQY+DO2\n\tiKMg8iLv8IecrUr5s4aRmFDyWVT4k2jAcRbz5cIdjLpiJocOo4TOnlbw9YuutY/VmX\n\tPWGxy8ngaoveL/rVuVg4m69/ElXWYZDlaXxDRAB0=","Date":"Fri, 4 Dec 2020 19:39:27 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20201204103927.GD2050@pyrite.rasen.tech>","References":"<20201127145612.1105770-1-naush@raspberrypi.com>\n\t<20201201015543.GF4315@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201201015543.GF4315@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14100,"web_url":"https://patchwork.libcamera.org/comment/14100/","msgid":"","date":"2020-12-07T13:11:15","subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Paul,\n\nThank you for your review.\n\nOn Fri, 4 Dec 2020 at 10:39, wrote:\n\n> Hi Naush,\n>\n> On Tue, Dec 01, 2020 at 03:55:43AM +0200, Laurent Pinchart wrote:\n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:\n> > > If the IPA fails during configuration, return an error flag to the\n> > > pipeline handler and fail the use case gracefully.\n> > >\n> > > At present, the IPA configuration can fail for the following reasons:\n> > > - The sensor is not recognised, and fails to open a CamHelper object.\n> > > - The pipeline handler did not pass in controls for the ISP and sensor.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: Jacopo Mondi \n> >\n> > Wouldn't it be simpler to modify the configure() function to return an\n> > int ? How about the following ? If it works for you I'll submit a proper\n> > patch.\n>\n> From the perspective of the IPC changes, Naush's solution is more easily\n> translatable. When the IPA interface will become customizable, there\n> isn't a way to specify which output parameters should be return values\n> vs return parameters. Thus keeping them all as return parameters is the\n> better solution here.\n>\n> So imo this is fine to merge as-is.\n>\n> Reviewed-by: Paul Elder \n>\n\nIn which case, I think this is ready to be merged :)\n\nRegards,\nNaush\n\n\n>\n> > Author: Laurent Pinchart \n> > Date: Tue Dec 1 03:54:18 2020 +0200\n> >\n> > libcamera: ipa_interface: Make configure() return an int\n> >\n> > It's useful for the IPAInterface::configure() function to be able to\n> > return a status. Make it return an int, to avoid forcing IPAs to\n> return\n> > a status encoded in the IPAOperationData in a custom way.\n> >\n> > Signed-off-by: Laurent Pinchart \n> >\n> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n> b/include/libcamera/internal/ipa_context_wrapper.h\n> > index 8f767e844221..a00b5e7b92eb 100644\n> > --- a/include/libcamera/internal/ipa_context_wrapper.h\n> > +++ b/include/libcamera/internal/ipa_context_wrapper.h\n> > @@ -22,11 +22,11 @@ public:\n> > int init(const IPASettings &settings) override;\n> > int start() override;\n> > void stop() override;\n> > - void configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> &streamConfig,\n> > - const std::map &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result) override;\n> > + int configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> &streamConfig,\n> > + const std::map\n> &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result) override;\n> >\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > diff --git a/include/libcamera/ipa/ipa_interface.h\n> b/include/libcamera/ipa/ipa_interface.h\n> > index 322b7079070e..1d8cf8dc1c56 100644\n> > --- a/include/libcamera/ipa/ipa_interface.h\n> > +++ b/include/libcamera/ipa/ipa_interface.h\n> > @@ -95,12 +95,12 @@ struct ipa_context_ops {\n> > void (*register_callbacks)(struct ipa_context *ctx,\n> > const struct ipa_callback_ops\n> *callbacks,\n> > void *cb_ctx);\n> > - void (*configure)(struct ipa_context *ctx,\n> > - const struct ipa_sensor_info *sensor_info,\n> > - const struct ipa_stream *streams,\n> > - unsigned int num_streams,\n> > - const struct ipa_control_info_map *maps,\n> > - unsigned int num_maps);\n> > + int (*configure)(struct ipa_context *ctx,\n> > + const struct ipa_sensor_info *sensor_info,\n> > + const struct ipa_stream *streams,\n> > + unsigned int num_streams,\n> > + const struct ipa_control_info_map *maps,\n> > + unsigned int num_maps);\n> > void (*map_buffers)(struct ipa_context *ctx,\n> > const struct ipa_buffer *buffers,\n> > size_t num_buffers);\n> > @@ -156,11 +156,11 @@ public:\n> > virtual int start() = 0;\n> > virtual void stop() = 0;\n> >\n> > - virtual void configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> &streamConfig,\n> > - const std::map ControlInfoMap &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result) = 0;\n> > + virtual int configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> &streamConfig,\n> > + const std::map ControlInfoMap &> &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result) = 0;\n> >\n> > virtual void mapBuffers(const std::vector &buffers) = 0;\n> > virtual void unmapBuffers(const std::vector &ids) =\n> 0;\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > index cee532e3a747..74d7bc6e1c9b 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > @@ -115,12 +115,12 @@ void\n> IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,\n> > ctx->cb_ctx_ = cb_ctx;\n> > }\n> >\n> > -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> > - const struct ipa_sensor_info\n> *sensor_info,\n> > - const struct ipa_stream *streams,\n> > - unsigned int num_streams,\n> > - const struct ipa_control_info_map\n> *maps,\n> > - unsigned int num_maps)\n> > +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> > + const struct ipa_sensor_info\n> *sensor_info,\n> > + const struct ipa_stream *streams,\n> > + unsigned int num_streams,\n> > + const struct ipa_control_info_map *maps,\n> > + unsigned int num_maps)\n> > {\n> > IPAInterfaceWrapper *ctx = static_cast *>(_ctx);\n> >\n> > @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct\n> ipa_context *_ctx,\n> >\n> > /* \\todo Translate the ipaConfig and result. */\n> > IPAOperationData ipaConfig;\n> > - ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,\n> ipaConfig,\n> > - nullptr);\n> > + return ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,\n> > + ipaConfig, nullptr);\n> > }\n> >\n> > void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h\n> b/src/ipa/libipa/ipa_interface_wrapper.h\n> > index a1c701599b56..acd3160039b1 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> > @@ -30,12 +30,12 @@ private:\n> > static void register_callbacks(struct ipa_context *ctx,\n> > const struct ipa_callback_ops\n> *callbacks,\n> > void *cb_ctx);\n> > - static void configure(struct ipa_context *ctx,\n> > - const struct ipa_sensor_info *sensor_info,\n> > - const struct ipa_stream *streams,\n> > - unsigned int num_streams,\n> > - const struct ipa_control_info_map *maps,\n> > - unsigned int num_maps);\n> > + static int configure(struct ipa_context *ctx,\n> > + const struct ipa_sensor_info *sensor_info,\n> > + const struct ipa_stream *streams,\n> > + unsigned int num_streams,\n> > + const struct ipa_control_info_map *maps,\n> > + unsigned int num_maps);\n> > static void map_buffers(struct ipa_context *ctx,\n> > const struct ipa_buffer *c_buffers,\n> > size_t num_buffers);\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 9853a343c892..75f7af3430ef 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -81,11 +81,11 @@ public:\n> > int start() override { return 0; }\n> > void stop() override {}\n> >\n> > - void configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> &streamConfig,\n> > - const std::map &> &entityControls,\n> > - const IPAOperationData &data,\n> > - IPAOperationData *response) override;\n> > + int configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> &streamConfig,\n> > + const std::map\n> &entityControls,\n> > + const IPAOperationData &data,\n> > + IPAOperationData *response) override;\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > void processEvent(const IPAOperationData &event) override;\n> > @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> > mode_.line_length = 1e9 * sensorInfo.lineLength /\n> sensorInfo.pixelRate;\n> > }\n> >\n> > -void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > - [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > - const std::map &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result)\n> > +int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > + [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > + const std::map\n> &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result)\n> > {\n> > if (entityControls.empty())\n> > - return;\n> > + return -EINVAL;\n> >\n> > result->operation = 0;\n> >\n> > @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > }\n> >\n> > lastMode_ = mode_;\n> > + return 0;\n> > }\n> >\n> > void IPARPi::mapBuffers(const std::vector &buffers)\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 07d7f1b2ddd8..78d78c5ac521 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -40,11 +40,11 @@ public:\n> > int start() override { return 0; }\n> > void stop() override {}\n> >\n> > - void configure(const CameraSensorInfo &info,\n> > - const std::map\n> &streamConfig,\n> > - const std::map &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *response) override;\n> > + int configure(const CameraSensorInfo &info,\n> > + const std::map\n> &streamConfig,\n> > + const std::map\n> &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *response) override;\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > void processEvent(const IPAOperationData &event) override;\n> > @@ -79,27 +79,27 @@ private:\n> > * assemble one. Make sure the reported sensor information are relevant\n> > * before accessing them.\n> > */\n> > -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> > - [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > - const std::map ControlInfoMap &> &entityControls,\n> > - [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> > - [[maybe_unused]] IPAOperationData *result)\n> > +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,\n> > + [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > + const std::map &> &entityControls,\n> > + [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> > + [[maybe_unused]] IPAOperationData *result)\n> > {\n> > if (entityControls.empty())\n> > - return;\n> > + return -EINVAL;\n> >\n> > ctrls_ = entityControls.at(0);\n> >\n> > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> > if (itExp == ctrls_.end()) {\n> > LOG(IPARkISP1, Error) << \"Can't find exposure control\";\n> > - return;\n> > + return -EINVAL;\n> > }\n> >\n> > const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> > if (itGain == ctrls_.end()) {\n> > LOG(IPARkISP1, Error) << \"Can't find gain control\";\n> > - return;\n> > + return -EINVAL;\n> > }\n> >\n> > autoExposure_ = true;\n> > @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]] const\n> CameraSensorInfo &info,\n> > << \" Gain: \" << minGain_ << \"-\" << maxGain_;\n> >\n> > setControls(0);\n> > + return 0;\n> > }\n> >\n> > void IPARkISP1::mapBuffers(const std::vector &buffers)\n> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> > index cf8411359e40..79c8c2fb8927 100644\n> > --- a/src/ipa/vimc/vimc.cpp\n> > +++ b/src/ipa/vimc/vimc.cpp\n> > @@ -37,11 +37,11 @@ public:\n> > int start() override;\n> > void stop() override;\n> >\n> > - void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> > - [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > - [[maybe_unused]] const std::map ControlInfoMap &> &entityControls,\n> > - [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > - [[maybe_unused]] IPAOperationData *result) override\n> {}\n> > + int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> > + [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > + [[maybe_unused]] const std::map ControlInfoMap &> &entityControls,\n> > + [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > + [[maybe_unused]] IPAOperationData *result) override\n> { return 0; }\n> > void mapBuffers([[maybe_unused]] const std::vector\n> &buffers) override {}\n> > void unmapBuffers([[maybe_unused]] const std::vector\n> &ids) override {}\n> > void processEvent([[maybe_unused]] const IPAOperationData &event)\n> override {}\n> > diff --git a/src/libcamera/ipa_context_wrapper.cpp\n> b/src/libcamera/ipa_context_wrapper.cpp\n> > index 231300ce0bec..f63ad830c003 100644\n> > --- a/src/libcamera/ipa_context_wrapper.cpp\n> > +++ b/src/libcamera/ipa_context_wrapper.cpp\n> > @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()\n> > ctx_->ops->stop(ctx_);\n> > }\n> >\n> > -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> &streamConfig,\n> > - const std::map ControlInfoMap &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result)\n> > +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> &streamConfig,\n> > + const std::map ControlInfoMap &> &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result)\n> > {\n> > if (intf_)\n> > return intf_->configure(sensorInfo, streamConfig,\n> > entityControls, ipaConfig, result);\n> >\n> > if (!ctx_)\n> > - return;\n> > + return 0;\n> >\n> > serializer_.reset();\n> >\n> > @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const\n> CameraSensorInfo &sensorInfo,\n> > }\n> >\n> > /* \\todo Translate the ipaConfig and reponse */\n> > - ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n> streamConfig.size(),\n> > - c_info_maps, entityControls.size());\n> > + return ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n> > + streamConfig.size(), c_info_maps,\n> > + entityControls.size());\n> > }\n> >\n> > void IPAContextWrapper::mapBuffers(const std::vector\n> &buffers)\n> > diff --git a/src/libcamera/ipa_interface.cpp\n> b/src/libcamera/ipa_interface.cpp\n> > index 23fc56d7d48e..516a8ecd4b53 100644\n> > --- a/src/libcamera/ipa_interface.cpp\n> > +++ b/src/libcamera/ipa_interface.cpp\n> > @@ -347,6 +347,8 @@\n> > * \\param[in] num_maps The number of entries in the \\a maps array\n> > *\n> > * \\sa libcamera::IPAInterface::configure()\n> > + *\n> > + * \\return 0 on success or a negative error code on failure\n> > */\n> >\n> > /**\n> > @@ -573,6 +575,8 @@ namespace libcamera {\n> > * pipeline handler to the IPA and back. The pipeline handler may set\n> the \\a\n> > * result parameter to null if the IPA protocol doesn't need to pass a\n> result\n> > * back through the configure() function.\n> > + *\n> > + * \\return 0 on success or a negative error code on failure\n> > */\n> >\n> > /**\n> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > index b78a0e4535f5..ed250ce79c17 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > @@ -32,11 +32,11 @@ public:\n> > }\n> > int start() override { return 0; }\n> > void stop() override {}\n> > - void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> > - [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > - [[maybe_unused]] const std::map ControlInfoMap &> &entityControls,\n> > - [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > - [[maybe_unused]] IPAOperationData *result) override\n> {}\n> > + int configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> > + [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > + [[maybe_unused]] const std::map ControlInfoMap &> &entityControls,\n> > + [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > + [[maybe_unused]] IPAOperationData *result) override\n> { return 0; }\n> > void mapBuffers([[maybe_unused]] const std::vector\n> &buffers) override {}\n> > void unmapBuffers([[maybe_unused]] const std::vector\n> &ids) override {}\n> > void processEvent([[maybe_unused]] const IPAOperationData &event)\n> override {}\n> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > index eead2883708d..fd91726c4840 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > @@ -29,11 +29,11 @@ public:\n> > int start() override;\n> > void stop() override;\n> >\n> > - void configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> &streamConfig,\n> > - const std::map &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result) override;\n> > + int configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> &streamConfig,\n> > + const std::map\n> &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result) override;\n> > void mapBuffers(const std::vector &buffers) override;\n> > void unmapBuffers(const std::vector &ids) override;\n> > void processEvent(const IPAOperationData &event) override;\n> > @@ -132,14 +132,14 @@ void IPAProxyThread::stop()\n> > thread_.wait();\n> > }\n> >\n> > -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> &streamConfig,\n> > - const std::map ControlInfoMap &> &entityControls,\n> > - const IPAOperationData &ipaConfig,\n> > - IPAOperationData *result)\n> > +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> &streamConfig,\n> > + const std::map ControlInfoMap &> &entityControls,\n> > + const IPAOperationData &ipaConfig,\n> > + IPAOperationData *result)\n> > {\n> > - ipa_->configure(sensorInfo, streamConfig, entityControls,\n> ipaConfig,\n> > - result);\n> > + return ipa_->configure(sensorInfo, streamConfig, entityControls,\n> > + ipaConfig, result);\n> > }\n> >\n> > void IPAProxyThread::mapBuffers(const std::vector &buffers)\n> > diff --git a/test/ipa/ipa_wrappers_test.cpp\n> b/test/ipa/ipa_wrappers_test.cpp\n> > index 59d991cbbf6a..ad0fb0386865 100644\n> > --- a/test/ipa/ipa_wrappers_test.cpp\n> > +++ b/test/ipa/ipa_wrappers_test.cpp\n> > @@ -67,55 +67,63 @@ public:\n> > report(Op_stop, TestPass);\n> > }\n> >\n> > - void configure(const CameraSensorInfo &sensorInfo,\n> > - const std::map\n> &streamConfig,\n> > - const std::map &> &entityControls,\n> > - [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > - [[maybe_unused]] IPAOperationData *result) override\n> > + int configure(const CameraSensorInfo &sensorInfo,\n> > + const std::map\n> &streamConfig,\n> > + const std::map\n> &entityControls,\n> > + [[maybe_unused]] const IPAOperationData &ipaConfig,\n> > + [[maybe_unused]] IPAOperationData *result) override\n> > {\n> > /* Verify sensorInfo. */\n> > if (sensorInfo.outputSize.width != 2560 ||\n> > sensorInfo.outputSize.height != 1940) {\n> > cerr << \"configure(): Invalid sensor info size \"\n> > << sensorInfo.outputSize.toString();\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > /* Verify streamConfig. */\n> > if (streamConfig.size() != 2) {\n> > cerr << \"configure(): Invalid number of streams \"\n> > << streamConfig.size() << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > auto iter = streamConfig.find(1);\n> > if (iter == streamConfig.end()) {\n> > cerr << \"configure(): No configuration for stream\n> 1\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> > const IPAStream *stream = &iter->second;\n> > if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||\n> > stream->size != Size{ 1024, 768 }) {\n> > cerr << \"configure(): Invalid configuration for\n> stream 1\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > iter = streamConfig.find(2);\n> > if (iter == streamConfig.end()) {\n> > cerr << \"configure(): No configuration for stream\n> 2\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> > stream = &iter->second;\n> > if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||\n> > stream->size != Size{ 800, 600 }) {\n> > cerr << \"configure(): Invalid configuration for\n> stream 2\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > /* Verify entityControls. */\n> > auto ctrlIter = entityControls.find(42);\n> > if (ctrlIter == entityControls.end()) {\n> > cerr << \"configure(): Controls not found\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > const ControlInfoMap &infoMap = ctrlIter->second;\n> > @@ -124,10 +132,12 @@ public:\n> > infoMap.count(V4L2_CID_CONTRAST) != 1 ||\n> > infoMap.count(V4L2_CID_SATURATION) != 1) {\n> > cerr << \"configure(): Invalid control IDs\" << endl;\n> > - return report(Op_configure, TestFail);\n> > + report(Op_configure, TestFail);\n> > + return -EINVAL;\n> > }\n> >\n> > report(Op_configure, TestPass);\n> > + return 0;\n> > }\n> >\n> > void mapBuffers(const std::vector &buffers) override\n> >\n> > > ---\n> > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 12 +++++++++++-\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 5 +++++\n> > > 3 files changed, 17 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > > index 2b55dbfc..86c97ffa 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -21,6 +21,7 @@ enum ConfigParameters {\n> > > IPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> > > IPA_CONFIG_SENSOR = (1 << 2),\n> > > IPA_CONFIG_DROP_FRAMES = (1 << 3),\n> > > + IPA_CONFIG_FAILED = (1 << 4),\n> > > };\n> > >\n> > > enum Operations {\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 9853a343..57dd9c61 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -197,8 +197,11 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > > const IPAOperationData &ipaConfig,\n> > > IPAOperationData *result)\n> > > {\n> > > - if (entityControls.empty())\n> > > + if (entityControls.size() != 2) {\n> > > + LOG(IPARPI, Error) << \"No ISP or sensor controls found.\";\n> > > + result->operation = RPi::IPA_CONFIG_FAILED;\n> > > return;\n> > > + }\n> > >\n> > > result->operation = 0;\n> > >\n> > > @@ -217,6 +220,13 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > > if (!helper_) {\n> > > helper_ =\n> std::unique_ptr(RPiController::CamHelper::Create(cameraName));\n> > >\n> > > + if (!helper_) {\n> > > + LOG(IPARPI, Error) << \"Could not create camera\n> helper for \"\n> > > + << cameraName;\n> > > + result->operation = RPi::IPA_CONFIG_FAILED;\n> > > + return;\n> > > + }\n> > > +\n> > > /*\n> > > * Pass out the sensor config to the pipeline handler in\n> order\n> > > * to setup the staggered writer class.\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 6fcdf557..76252806 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > > ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> ipaConfig,\n> > > &result);\n> > >\n> > > + if (result.operation & RPi::IPA_CONFIG_FAILED) {\n> > > + LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > + return -EPIPE;\n> > > + }\n> > > +\n> > > unsigned int resultIdx = 0;\n> > > if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {\n> > > /*\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 49E7EBDB1F\n\tfor ;\n\tMon, 7 Dec 2020 13:11:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8EE9B67E18;\n\tMon, 7 Dec 2020 14:11:34 +0100 (CET)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 641AB60325\n\tfor ;\n\tMon, 7 Dec 2020 14:11:33 +0100 (CET)","by mail-lj1-x22f.google.com with SMTP id y16so14917290ljk.1\n\tfor ;\n\tMon, 07 Dec 2020 05:11:33 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"gGRwa90w\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Zedmi9LyJ5kgSblrs42kqvtK6+dQGG9GZIKvU++SaQA=;\n\tb=gGRwa90wQlXabDezBDru1i2Bjq4FYYENOCKRVP2uUair49GMKn/ZABi8G269aIFV0O\n\t/14yvGHioneI8+0GpAUTpbltTe3ybFpzZouHAQiYsOgUYmqsqHach+jf7TPzyjmN7L72\n\tCZJ4E+A+yxaYI49z3Ommxax+LNdnHRyme9P+gZmcXK7TG3sOFUvtTB8KQWeY9XyGWUJo\n\tuGjgwrMltrv9Te7oLaB4dm4FLX8TSZKET4mjHOk9lZyyvIwNxOQy3SpaImOCGA6l2lpa\n\ttXjfSeM4S7wOUSgpg4dWPTlP0MZXDsKO7EpOcQ8Se1KEPXO8tFhlxWDVWFwDmqiiSrVz\n\tkZCg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Zedmi9LyJ5kgSblrs42kqvtK6+dQGG9GZIKvU++SaQA=;\n\tb=uINMEmswkKAYdzX+cDesTiacTlfGkVTsEy3aQau23bTMaaM/rurhZHQbsfOJjK4vsa\n\tYuxhFY2ht4boBL8DP3XqEO3nAnCShn5qc1O2D2kAIpkunwGp1H9SAEErNz+99/I/aD0+\n\tpZR3F1/VpFd9fPwyZY2cQzFr/1hj4u9uPXnKLFai6g+1+oXdvg9hG2bsXFuH+ijI2u5R\n\tiQ6409M+dbECi1UaQzhKpvBhRW50v9u8hyKrz6VBfW81l61WC3a4Fmzm//oEWwvGKx3q\n\t2L2KhmRcEAVGHJRu9aJVtt0lx419wtSdqnq3W+wuiUBuVq1ndxCEivrAm7VnDS9dxcCG\n\taR0g==","X-Gm-Message-State":"AOAM533KyR2MPSNcI/xOFLkVio5DZaA7F1isHSdniuqDG+SJe9pFVIYD\n\tMnEByjXV3QGN/NEOzpGlcUSk88+AqtaXLH4pdyY7arFcFCI=","X-Google-Smtp-Source":"ABdhPJykB7SIb4JuCwfIaZfJQi55E+o+mNyB/+27/I5UWvKZ0p/M0YlNKLKGjbyftB6qQ5aSgm/sC1OMyJ6MxKkZQh4=","X-Received":"by 2002:a2e:a377:: with SMTP id\n\ti23mr4214551ljn.103.1607346692495; \n\tMon, 07 Dec 2020 05:11:32 -0800 (PST)","MIME-Version":"1.0","References":"<20201127145612.1105770-1-naush@raspberrypi.com>\n\t<20201201015543.GF4315@pendragon.ideasonboard.com>\n\t<20201204103927.GD2050@pyrite.rasen.tech>","In-Reply-To":"<20201204103927.GD2050@pyrite.rasen.tech>","From":"Naushir Patuck ","Date":"Mon, 7 Dec 2020 13:11:15 +0000","Message-ID":"","To":"Paul Elder ","Subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============0282886136091168353==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14102,"web_url":"https://patchwork.libcamera.org/comment/14102/","msgid":"<17256d0d-57ce-6502-a157-c7d56deef917@ideasonboard.com>","date":"2020-12-07T15:14:16","subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush, Paul,\n\nOn 07/12/2020 13:11, Naushir Patuck wrote:\n> Hi Paul,\n> \n> Thank you for your review.\n> \n> On Fri, 4 Dec 2020 at 10:39, > wrote:\n> \n> Hi Naush,\n> \n> On Tue, Dec 01, 2020 at 03:55:43AM +0200, Laurent Pinchart wrote:\n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Nov 27, 2020 at 02:56:12PM +0000, Naushir Patuck wrote:\n> > > If the IPA fails during configuration, return an error flag to the\n> > > pipeline handler and fail the use case gracefully.\n> > >\n> > > At present, the IPA configuration can fail for the following\n> reasons:\n> > > - The sensor is not recognised, and fails to open a CamHelper\n> object.\n> > > - The pipeline handler did not pass in controls for the ISP and\n> sensor.\n> > >\n> > > Signed-off-by: Naushir Patuck >\n> > > Reviewed-by: Jacopo Mondi >\n> >\n> > Wouldn't it be simpler to modify the configure() function to return an\n> > int ? How about the following ? If it works for you I'll submit a\n> proper\n> > patch.\n> \n> From the perspective of the IPC changes, Naush's solution is more easily\n> translatable. When the IPA interface will become customizable, there\n> isn't a way to specify which output parameters should be return values\n> vs return parameters. Thus keeping them all as return parameters is the\n> better solution here.\n> \n> So imo this is fine to merge as-is.\n> \n> Reviewed-by: Paul Elder >\n> \n> \n> In which case, I think this is ready to be merged :)\n\nPushed.\n\nRegards\n--\nKieran\n\n\n> Regards,\n> Naush\n>  \n> \n> \n> > Author: Laurent Pinchart >\n> > Date:   Tue Dec 1 03:54:18 2020 +0200\n> >\n> >     libcamera: ipa_interface: Make configure() return an int\n> >\n> >     It's useful for the IPAInterface::configure() function to be\n> able to\n> >     return a status. Make it return an int, to avoid forcing IPAs\n> to return\n> >     a status encoded in the IPAOperationData in a custom way.\n> >\n> >     Signed-off-by: Laurent Pinchart\n> >\n> >\n> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n> b/include/libcamera/internal/ipa_context_wrapper.h\n> > index 8f767e844221..a00b5e7b92eb 100644\n> > --- a/include/libcamera/internal/ipa_context_wrapper.h\n> > +++ b/include/libcamera/internal/ipa_context_wrapper.h\n> > @@ -22,11 +22,11 @@ public:\n> >       int init(const IPASettings &settings) override;\n> >       int start() override;\n> >       void stop() override;\n> > -     void configure(const CameraSensorInfo &sensorInfo,\n> > -                    const std::map\n> &streamConfig,\n> > -                    const std::map ControlInfoMap &> &entityControls,\n> > -                    const IPAOperationData &ipaConfig,\n> > -                    IPAOperationData *result) override;\n> > +     int configure(const CameraSensorInfo &sensorInfo,\n> > +                   const std::map\n> &streamConfig,\n> > +                   const std::map ControlInfoMap &> &entityControls,\n> > +                   const IPAOperationData &ipaConfig,\n> > +                   IPAOperationData *result) override;\n> >\n> >       void mapBuffers(const std::vector &buffers) override;\n> >       void unmapBuffers(const std::vector &ids)\n> override;\n> > diff --git a/include/libcamera/ipa/ipa_interface.h\n> b/include/libcamera/ipa/ipa_interface.h\n> > index 322b7079070e..1d8cf8dc1c56 100644\n> > --- a/include/libcamera/ipa/ipa_interface.h\n> > +++ b/include/libcamera/ipa/ipa_interface.h\n> > @@ -95,12 +95,12 @@ struct ipa_context_ops {\n> >       void (*register_callbacks)(struct ipa_context *ctx,\n> >                                  const struct ipa_callback_ops\n> *callbacks,\n> >                                  void *cb_ctx);\n> > -     void (*configure)(struct ipa_context *ctx,\n> > -                       const struct ipa_sensor_info *sensor_info,\n> > -                       const struct ipa_stream *streams,\n> > -                       unsigned int num_streams,\n> > -                       const struct ipa_control_info_map *maps,\n> > -                       unsigned int num_maps);\n> > +     int (*configure)(struct ipa_context *ctx,\n> > +                      const struct ipa_sensor_info *sensor_info,\n> > +                      const struct ipa_stream *streams,\n> > +                      unsigned int num_streams,\n> > +                      const struct ipa_control_info_map *maps,\n> > +                      unsigned int num_maps);\n> >       void (*map_buffers)(struct ipa_context *ctx,\n> >                           const struct ipa_buffer *buffers,\n> >                           size_t num_buffers);\n> > @@ -156,11 +156,11 @@ public:\n> >       virtual int start() = 0;\n> >       virtual void stop() = 0;\n> >\n> > -     virtual void configure(const CameraSensorInfo &sensorInfo,\n> > -                            const std::map IPAStream> &streamConfig,\n> > -                            const std::map ControlInfoMap &> &entityControls,\n> > -                            const IPAOperationData &ipaConfig,\n> > -                            IPAOperationData *result) = 0;\n> > +     virtual int configure(const CameraSensorInfo &sensorInfo,\n> > +                           const std::map IPAStream> &streamConfig,\n> > +                           const std::map ControlInfoMap &> &entityControls,\n> > +                           const IPAOperationData &ipaConfig,\n> > +                           IPAOperationData *result) = 0;\n> >\n> >       virtual void mapBuffers(const std::vector\n> &buffers) = 0;\n> >       virtual void unmapBuffers(const std::vector\n> &ids) = 0;\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > index cee532e3a747..74d7bc6e1c9b 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > @@ -115,12 +115,12 @@ void\n> IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,\n> >       ctx->cb_ctx_ = cb_ctx;\n> >  }\n> >\n> > -void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> > -                                 const struct ipa_sensor_info\n> *sensor_info,\n> > -                                 const struct ipa_stream *streams,\n> > -                                 unsigned int num_streams,\n> > -                                 const struct\n> ipa_control_info_map *maps,\n> > -                                 unsigned int num_maps)\n> > +int IPAInterfaceWrapper::configure(struct ipa_context *_ctx,\n> > +                                const struct ipa_sensor_info\n> *sensor_info,\n> > +                                const struct ipa_stream *streams,\n> > +                                unsigned int num_streams,\n> > +                                const struct ipa_control_info_map\n> *maps,\n> > +                                unsigned int num_maps)\n> >  {\n> >       IPAInterfaceWrapper *ctx = static_cast *>(_ctx);\n> >\n> > @@ -168,8 +168,8 @@ void IPAInterfaceWrapper::configure(struct\n> ipa_context *_ctx,\n> >\n> >       /* \\todo Translate the ipaConfig and result. */\n> >       IPAOperationData ipaConfig;\n> > -     ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls,\n> ipaConfig,\n> > -                          nullptr);\n> > +     return ctx->ipa_->configure(sensorInfo, ipaStreams,\n> entityControls,\n> > +                                 ipaConfig, nullptr);\n> >  }\n> >\n> >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,\n> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h\n> b/src/ipa/libipa/ipa_interface_wrapper.h\n> > index a1c701599b56..acd3160039b1 100644\n> > --- a/src/ipa/libipa/ipa_interface_wrapper.h\n> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h\n> > @@ -30,12 +30,12 @@ private:\n> >       static void register_callbacks(struct ipa_context *ctx,\n> >                                      const struct ipa_callback_ops\n> *callbacks,\n> >                                      void *cb_ctx);\n> > -     static void configure(struct ipa_context *ctx,\n> > -                           const struct ipa_sensor_info *sensor_info,\n> > -                           const struct ipa_stream *streams,\n> > -                           unsigned int num_streams,\n> > -                           const struct ipa_control_info_map *maps,\n> > -                           unsigned int num_maps);\n> > +     static int configure(struct ipa_context *ctx,\n> > +                          const struct ipa_sensor_info *sensor_info,\n> > +                          const struct ipa_stream *streams,\n> > +                          unsigned int num_streams,\n> > +                          const struct ipa_control_info_map *maps,\n> > +                          unsigned int num_maps);\n> >       static void map_buffers(struct ipa_context *ctx,\n> >                               const struct ipa_buffer *c_buffers,\n> >                               size_t num_buffers);\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 9853a343c892..75f7af3430ef 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -81,11 +81,11 @@ public:\n> >       int start() override { return 0; }\n> >       void stop() override {}\n> >\n> > -     void configure(const CameraSensorInfo &sensorInfo,\n> > -                    const std::map\n> &streamConfig,\n> > -                    const std::map ControlInfoMap &> &entityControls,\n> > -                    const IPAOperationData &data,\n> > -                    IPAOperationData *response) override;\n> > +     int configure(const CameraSensorInfo &sensorInfo,\n> > +                   const std::map\n> &streamConfig,\n> > +                   const std::map ControlInfoMap &> &entityControls,\n> > +                   const IPAOperationData &data,\n> > +                   IPAOperationData *response) override;\n> >       void mapBuffers(const std::vector &buffers) override;\n> >       void unmapBuffers(const std::vector &ids)\n> override;\n> >       void processEvent(const IPAOperationData &event) override;\n> > @@ -191,14 +191,14 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> >       mode_.line_length = 1e9 * sensorInfo.lineLength /\n> sensorInfo.pixelRate;\n> >  }\n> >\n> > -void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > -                    [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > -                    const std::map ControlInfoMap &> &entityControls,\n> > -                    const IPAOperationData &ipaConfig,\n> > -                    IPAOperationData *result)\n> > +int IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > +                   [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > +                   const std::map ControlInfoMap &> &entityControls,\n> > +                   const IPAOperationData &ipaConfig,\n> > +                   IPAOperationData *result)\n> >  {\n> >       if (entityControls.empty())\n> > -             return;\n> > +             return -EINVAL;\n> >\n> >       result->operation = 0;\n> >\n> > @@ -314,6 +314,7 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> >       }\n> >\n> >       lastMode_ = mode_;\n> > +     return 0;\n> >  }\n> >\n> >  void IPARPi::mapBuffers(const std::vector &buffers)\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 07d7f1b2ddd8..78d78c5ac521 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -40,11 +40,11 @@ public:\n> >       int start() override { return 0; }\n> >       void stop() override {}\n> >\n> > -     void configure(const CameraSensorInfo &info,\n> > -                    const std::map\n> &streamConfig,\n> > -                    const std::map ControlInfoMap &> &entityControls,\n> > -                    const IPAOperationData &ipaConfig,\n> > -                    IPAOperationData *response) override;\n> > +     int configure(const CameraSensorInfo &info,\n> > +                   const std::map\n> &streamConfig,\n> > +                   const std::map ControlInfoMap &> &entityControls,\n> > +                   const IPAOperationData &ipaConfig,\n> > +                   IPAOperationData *response) override;\n> >       void mapBuffers(const std::vector &buffers) override;\n> >       void unmapBuffers(const std::vector &ids)\n> override;\n> >       void processEvent(const IPAOperationData &event) override;\n> > @@ -79,27 +79,27 @@ private:\n> >   * assemble one. Make sure the reported sensor information are\n> relevant\n> >   * before accessing them.\n> >   */\n> > -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo\n> &info,\n> > -                       [[maybe_unused]] const std::map int, IPAStream> &streamConfig,\n> > -                       const std::map ControlInfoMap &> &entityControls,\n> > -                       [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> > -                       [[maybe_unused]] IPAOperationData *result)\n> > +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo\n> &info,\n> > +                      [[maybe_unused]] const std::map int, IPAStream> &streamConfig,\n> > +                      const std::map ControlInfoMap &> &entityControls,\n> > +                      [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> > +                      [[maybe_unused]] IPAOperationData *result)\n> >  {\n> >       if (entityControls.empty())\n> > -             return;\n> > +             return -EINVAL;\n> >\n> >       ctrls_ = entityControls.at(0);\n> >\n> >       const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> >       if (itExp == ctrls_.end()) {\n> >               LOG(IPARkISP1, Error) << \"Can't find exposure control\";\n> > -             return;\n> > +             return -EINVAL;\n> >       }\n> >\n> >       const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);\n> >       if (itGain == ctrls_.end()) {\n> >               LOG(IPARkISP1, Error) << \"Can't find gain control\";\n> > -             return;\n> > +             return -EINVAL;\n> >       }\n> >\n> >       autoExposure_ = true;\n> > @@ -117,6 +117,7 @@ void IPARkISP1::configure([[maybe_unused]]\n> const CameraSensorInfo &info,\n> >               << \" Gain: \" << minGain_ << \"-\" << maxGain_;\n> >\n> >       setControls(0);\n> > +     return 0;\n> >  }\n> >\n> >  void IPARkISP1::mapBuffers(const std::vector &buffers)\n> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> > index cf8411359e40..79c8c2fb8927 100644\n> > --- a/src/ipa/vimc/vimc.cpp\n> > +++ b/src/ipa/vimc/vimc.cpp\n> > @@ -37,11 +37,11 @@ public:\n> >       int start() override;\n> >       void stop() override;\n> >\n> > -     void configure([[maybe_unused]] const CameraSensorInfo\n> &sensorInfo,\n> > -                    [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > -                    [[maybe_unused]] const std::map const ControlInfoMap &> &entityControls,\n> > -                    [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> > -                    [[maybe_unused]] IPAOperationData *result)\n> override {}\n> > +     int configure([[maybe_unused]] const CameraSensorInfo\n> &sensorInfo,\n> > +                   [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > +                   [[maybe_unused]] const std::map const ControlInfoMap &> &entityControls,\n> > +                   [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> > +                   [[maybe_unused]] IPAOperationData *result)\n> override { return 0; }\n> >       void mapBuffers([[maybe_unused]] const\n> std::vector &buffers) override {}\n> >       void unmapBuffers([[maybe_unused]] const\n> std::vector &ids) override {}\n> >       void processEvent([[maybe_unused]] const IPAOperationData\n> &event) override {}\n> > diff --git a/src/libcamera/ipa_context_wrapper.cpp\n> b/src/libcamera/ipa_context_wrapper.cpp\n> > index 231300ce0bec..f63ad830c003 100644\n> > --- a/src/libcamera/ipa_context_wrapper.cpp\n> > +++ b/src/libcamera/ipa_context_wrapper.cpp\n> > @@ -108,18 +108,18 @@ void IPAContextWrapper::stop()\n> >       ctx_->ops->stop(ctx_);\n> >  }\n> >\n> > -void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> > -                               const std::map IPAStream> &streamConfig,\n> > -                               const std::map ControlInfoMap &> &entityControls,\n> > -                               const IPAOperationData &ipaConfig,\n> > -                               IPAOperationData *result)\n> > +int IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,\n> > +                              const std::map IPAStream> &streamConfig,\n> > +                              const std::map ControlInfoMap &> &entityControls,\n> > +                              const IPAOperationData &ipaConfig,\n> > +                              IPAOperationData *result)\n> >  {\n> >       if (intf_)\n> >               return intf_->configure(sensorInfo, streamConfig,\n> >                                       entityControls, ipaConfig,\n> result);\n> >\n> >       if (!ctx_)\n> > -             return;\n> > +             return 0;\n> >\n> >       serializer_.reset();\n> >\n> > @@ -178,8 +178,9 @@ void IPAContextWrapper::configure(const\n> CameraSensorInfo &sensorInfo,\n> >       }\n> >\n> >       /* \\todo Translate the ipaConfig and reponse */\n> > -     ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n> streamConfig.size(),\n> > -                          c_info_maps, entityControls.size());\n> > +     return ctx_->ops->configure(ctx_, &sensor_info, c_streams,\n> > +                                 streamConfig.size(), c_info_maps,\n> > +                                 entityControls.size());\n> >  }\n> >\n> >  void IPAContextWrapper::mapBuffers(const std::vector\n> &buffers)\n> > diff --git a/src/libcamera/ipa_interface.cpp\n> b/src/libcamera/ipa_interface.cpp\n> > index 23fc56d7d48e..516a8ecd4b53 100644\n> > --- a/src/libcamera/ipa_interface.cpp\n> > +++ b/src/libcamera/ipa_interface.cpp\n> > @@ -347,6 +347,8 @@\n> >   * \\param[in] num_maps The number of entries in the \\a maps array\n> >   *\n> >   * \\sa libcamera::IPAInterface::configure()\n> > + *\n> > + * \\return 0 on success or a negative error code on failure\n> >   */\n> >\n> >  /**\n> > @@ -573,6 +575,8 @@ namespace libcamera {\n> >   * pipeline handler to the IPA and back. The pipeline handler may\n> set the \\a\n> >   * result parameter to null if the IPA protocol doesn't need to\n> pass a result\n> >   * back through the configure() function.\n> > + *\n> > + * \\return 0 on success or a negative error code on failure\n> >   */\n> >\n> >  /**\n> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > index b78a0e4535f5..ed250ce79c17 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> > @@ -32,11 +32,11 @@ public:\n> >       }\n> >       int start() override { return 0; }\n> >       void stop() override {}\n> > -     void configure([[maybe_unused]] const CameraSensorInfo\n> &sensorInfo,\n> > -                    [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > -                    [[maybe_unused]] const std::map const ControlInfoMap &> &entityControls,\n> > -                    [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> > -                    [[maybe_unused]] IPAOperationData *result)\n> override {}\n> > +     int configure([[maybe_unused]] const CameraSensorInfo\n> &sensorInfo,\n> > +                   [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > +                   [[maybe_unused]] const std::map const ControlInfoMap &> &entityControls,\n> > +                   [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> > +                   [[maybe_unused]] IPAOperationData *result)\n> override { return 0; }\n> >       void mapBuffers([[maybe_unused]] const\n> std::vector &buffers) override {}\n> >       void unmapBuffers([[maybe_unused]] const\n> std::vector &ids) override {}\n> >       void processEvent([[maybe_unused]] const IPAOperationData\n> &event) override {}\n> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > index eead2883708d..fd91726c4840 100644\n> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > @@ -29,11 +29,11 @@ public:\n> >       int start() override;\n> >       void stop() override;\n> >\n> > -     void configure(const CameraSensorInfo &sensorInfo,\n> > -                    const std::map\n> &streamConfig,\n> > -                    const std::map ControlInfoMap &> &entityControls,\n> > -                    const IPAOperationData &ipaConfig,\n> > -                    IPAOperationData *result) override;\n> > +     int configure(const CameraSensorInfo &sensorInfo,\n> > +                   const std::map\n> &streamConfig,\n> > +                   const std::map ControlInfoMap &> &entityControls,\n> > +                   const IPAOperationData &ipaConfig,\n> > +                   IPAOperationData *result) override;\n> >       void mapBuffers(const std::vector &buffers) override;\n> >       void unmapBuffers(const std::vector &ids)\n> override;\n> >       void processEvent(const IPAOperationData &event) override;\n> > @@ -132,14 +132,14 @@ void IPAProxyThread::stop()\n> >       thread_.wait();\n> >  }\n> >\n> > -void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> > -                            const std::map IPAStream> &streamConfig,\n> > -                            const std::map ControlInfoMap &> &entityControls,\n> > -                            const IPAOperationData &ipaConfig,\n> > -                            IPAOperationData *result)\n> > +int IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,\n> > +                           const std::map IPAStream> &streamConfig,\n> > +                           const std::map ControlInfoMap &> &entityControls,\n> > +                           const IPAOperationData &ipaConfig,\n> > +                           IPAOperationData *result)\n> >  {\n> > -     ipa_->configure(sensorInfo, streamConfig, entityControls,\n> ipaConfig,\n> > -                     result);\n> > +     return ipa_->configure(sensorInfo, streamConfig, entityControls,\n> > +                            ipaConfig, result);\n> >  }\n> >\n> >  void IPAProxyThread::mapBuffers(const std::vector\n> &buffers)\n> > diff --git a/test/ipa/ipa_wrappers_test.cpp\n> b/test/ipa/ipa_wrappers_test.cpp\n> > index 59d991cbbf6a..ad0fb0386865 100644\n> > --- a/test/ipa/ipa_wrappers_test.cpp\n> > +++ b/test/ipa/ipa_wrappers_test.cpp\n> > @@ -67,55 +67,63 @@ public:\n> >               report(Op_stop, TestPass);\n> >       }\n> >\n> > -     void configure(const CameraSensorInfo &sensorInfo,\n> > -                    const std::map\n> &streamConfig,\n> > -                    const std::map ControlInfoMap &> &entityControls,\n> > -                    [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> > -                    [[maybe_unused]] IPAOperationData *result)\n> override\n> > +     int configure(const CameraSensorInfo &sensorInfo,\n> > +                   const std::map\n> &streamConfig,\n> > +                   const std::map ControlInfoMap &> &entityControls,\n> > +                   [[maybe_unused]] const IPAOperationData\n> &ipaConfig,\n> > +                   [[maybe_unused]] IPAOperationData *result)\n> override\n> >       {\n> >               /* Verify sensorInfo. */\n> >               if (sensorInfo.outputSize.width != 2560 ||\n> >                   sensorInfo.outputSize.height != 1940) {\n> >                       cerr << \"configure(): Invalid sensor info size \"\n> >                            << sensorInfo.outputSize.toString();\n> > +                     report(Op_configure, TestFail);\n> > +                     return -EINVAL;\n> >               }\n> >\n> >               /* Verify streamConfig. */\n> >               if (streamConfig.size() != 2) {\n> >                       cerr << \"configure(): Invalid number of\n> streams \"\n> >                            << streamConfig.size() << endl;\n> > -                     return report(Op_configure, TestFail);\n> > +                     report(Op_configure, TestFail);\n> > +                     return -EINVAL;\n> >               }\n> >\n> >               auto iter = streamConfig.find(1);\n> >               if (iter == streamConfig.end()) {\n> >                       cerr << \"configure(): No configuration for\n> stream 1\" << endl;\n> > -                     return report(Op_configure, TestFail);\n> > +                     report(Op_configure, TestFail);\n> > +                     return -EINVAL;\n> >               }\n> >               const IPAStream *stream = &iter->second;\n> >               if (stream->pixelFormat != V4L2_PIX_FMT_YUYV ||\n> >                   stream->size != Size{ 1024, 768 }) {\n> >                       cerr << \"configure(): Invalid configuration\n> for stream 1\" << endl;\n> > -                     return report(Op_configure, TestFail);\n> > +                     report(Op_configure, TestFail);\n> > +                     return -EINVAL;\n> >               }\n> >\n> >               iter = streamConfig.find(2);\n> >               if (iter == streamConfig.end()) {\n> >                       cerr << \"configure(): No configuration for\n> stream 2\" << endl;\n> > -                     return report(Op_configure, TestFail);\n> > +                     report(Op_configure, TestFail);\n> > +                     return -EINVAL;\n> >               }\n> >               stream = &iter->second;\n> >               if (stream->pixelFormat != V4L2_PIX_FMT_NV12 ||\n> >                   stream->size != Size{ 800, 600 }) {\n> >                       cerr << \"configure(): Invalid configuration\n> for stream 2\" << endl;\n> > -                     return report(Op_configure, TestFail);\n> > +                     report(Op_configure, TestFail);\n> > +                     return -EINVAL;\n> >               }\n> >\n> >               /* Verify entityControls. */\n> >               auto ctrlIter = entityControls.find(42);\n> >               if (ctrlIter == entityControls.end()) {\n> >                       cerr << \"configure(): Controls not found\" <<\n> endl;\n> > -                     return report(Op_configure, TestFail);\n> > +                     report(Op_configure, TestFail);\n> > +                     return -EINVAL;\n> >               }\n> >\n> >               const ControlInfoMap &infoMap = ctrlIter->second;\n> > @@ -124,10 +132,12 @@ public:\n> >                   infoMap.count(V4L2_CID_CONTRAST) != 1 ||\n> >                   infoMap.count(V4L2_CID_SATURATION) != 1) {\n> >                       cerr << \"configure(): Invalid control IDs\"\n> << endl;\n> > -                     return report(Op_configure, TestFail);\n> > +                     report(Op_configure, TestFail);\n> > +                     return -EINVAL;\n> >               }\n> >\n> >               report(Op_configure, TestPass);\n> > +             return 0;\n> >       }\n> >\n> >       void mapBuffers(const std::vector &buffers) override\n> >\n> > > ---\n> > >  include/libcamera/ipa/raspberrypi.h                |  1 +\n> > >  src/ipa/raspberrypi/raspberrypi.cpp                | 12\n> +++++++++++-\n> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  5 +++++\n> > >  3 files changed, 17 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > > index 2b55dbfc..86c97ffa 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -21,6 +21,7 @@ enum ConfigParameters {\n> > >     IPA_CONFIG_STAGGERED_WRITE = (1 << 1),\n> > >     IPA_CONFIG_SENSOR = (1 << 2),\n> > >     IPA_CONFIG_DROP_FRAMES = (1 << 3),\n> > > +   IPA_CONFIG_FAILED = (1 << 4),\n> > >  };\n> > > \n> > >  enum Operations {\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 9853a343..57dd9c61 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -197,8 +197,11 @@ void IPARPi::configure(const\n> CameraSensorInfo &sensorInfo,\n> > >                    const IPAOperationData &ipaConfig,\n> > >                    IPAOperationData *result)\n> > >  {\n> > > -   if (entityControls.empty())\n> > > +   if (entityControls.size() != 2) {\n> > > +           LOG(IPARPI, Error) << \"No ISP or sensor controls\n> found.\";\n> > > +           result->operation = RPi::IPA_CONFIG_FAILED;\n> > >             return;\n> > > +   }\n> > > \n> > >     result->operation = 0;\n> > > \n> > > @@ -217,6 +220,13 @@ void IPARPi::configure(const\n> CameraSensorInfo &sensorInfo,\n> > >     if (!helper_) {\n> > >             helper_ =\n> std::unique_ptr(RPiController::CamHelper::Create(cameraName));\n> > > \n> > > +           if (!helper_) {\n> > > +                   LOG(IPARPI, Error) << \"Could not create\n> camera helper for \"\n> > > +                                      << cameraName;\n> > > +                   result->operation = RPi::IPA_CONFIG_FAILED;\n> > > +                   return;\n> > > +           }\n> > > +\n> > >             /*\n> > >              * Pass out the sensor config to the pipeline\n> handler in order\n> > >              * to setup the staggered writer class.\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 6fcdf557..76252806 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > >     ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> ipaConfig,\n> > >                     &result);\n> > > \n> > > +   if (result.operation & RPi::IPA_CONFIG_FAILED) {\n> > > +           LOG(RPI, Error) << \"IPA configuration failed!\";\n> > > +           return -EPIPE;\n> > > +   }\n> > > +\n> > >     unsigned int resultIdx = 0;\n> > >     if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {\n> > >             /*\n> \n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 496BBBDB1F\n\tfor ;\n\tMon, 7 Dec 2020 15:14:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A924667E6E;\n\tMon, 7 Dec 2020 16:14:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2D2A635A0\n\tfor ;\n\tMon, 7 Dec 2020 16:14:19 +0100 (CET)","from [192.168.0.217]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0E3218D;\n\tMon, 7 Dec 2020 16:14:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JfcarIWG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607354059;\n\tbh=TY+0t580ZJpVjCgiRjs7twE5+rbWyTQXCOc9O9eH+ek=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=JfcarIWGuE+mSbSmVKfW7SBm+Z7m+kJpP36zk7rOpQ+QqDzGaQR0FtiqjkIGSebpF\n\tD5za0oF9IkA/Ks7Wz9Lvs3JN9cgCeQjjytcTdEJNPLLj8r+OOsqBq5sbXyNPT3wFeB\n\tEdJqCnF0R+tB3A+6XJhrDsLhsK7a8jtu0dMJqi94=","To":"Naushir Patuck ,\n\tPaul Elder ","References":"<20201127145612.1105770-1-naush@raspberrypi.com>\n\t<20201201015543.GF4315@pendragon.ideasonboard.com>\n\t<20201204103927.GD2050@pyrite.rasen.tech>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<17256d0d-57ce-6502-a157-c7d56deef917@ideasonboard.com>","Date":"Mon, 7 Dec 2020 15:14:16 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2] pipeline: ipa: raspberrypi: Handle\n\tfailures during IPA configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]