[{"id":14024,"web_url":"https://patchwork.libcamera.org/comment/14024/","msgid":"","date":"2020-12-01T18:47:27","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_interface: Make\n\tconfigure() return an int","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for the patch. This does exactly what I need!\n\nOn Tue, 1 Dec 2020 at 09:59, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\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\nReviewed-by: Naushir Patuck \nTested-by: Naushir Patuck \n\n---\n> .../libcamera/internal/ipa_context_wrapper.h | 10 +++---\n> include/libcamera/ipa/ipa_interface.h | 22 ++++++------\n> src/ipa/libipa/ipa_interface_wrapper.cpp | 16 ++++-----\n> src/ipa/libipa/ipa_interface_wrapper.h | 12 +++----\n> src/ipa/raspberrypi/raspberrypi.cpp | 23 +++++++------\n> src/ipa/rkisp1/rkisp1.cpp | 27 ++++++++-------\n> src/ipa/vimc/vimc.cpp | 10 +++---\n> src/libcamera/ipa_context_wrapper.cpp | 17 +++++-----\n> src/libcamera/ipa_interface.cpp | 4 +++\n> src/libcamera/proxy/ipa_proxy_linux.cpp | 10 +++---\n> src/libcamera/proxy/ipa_proxy_thread.cpp | 24 ++++++-------\n> test/ipa/ipa_wrappers_test.cpp | 34 ++++++++++++-------\n> 12 files changed, 113 insertions(+), 96 deletions(-)\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> 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 D4DD5BE176\n\tfor ;\n\tTue, 1 Dec 2020 18:47:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67E8663505;\n\tTue, 1 Dec 2020 19:47:46 +0100 (CET)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE17163460\n\tfor ;\n\tTue, 1 Dec 2020 19:47:44 +0100 (CET)","by mail-lf1-x12e.google.com with SMTP id d8so6425804lfa.1\n\tfor ;\n\tTue, 01 Dec 2020 10:47:44 -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=\"iXbj87qs\"; 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=UwqMph9buucMR0Mg/2U/dC1My8hmmzkFbsiUMY/nLxk=;\n\tb=iXbj87qsjpY/Cy5qrlMK8NL0o7MMd463IiPJU2wlPM/GoxxvckfiAfyubFwRHdcPyT\n\t95XtrFMvxvZ19wfoTAlSIVtFgsg8MCNgpg8GvLmwUgB0CrkhKTRQfcz39I8O3zvEn8ew\n\tWIBcmNl9wRISalMDXS4Q4ZFIQAiqpWEukpIqHeCJzl2Kk9nuYQbiXlOuqnPs4vyPlNFM\n\tFfXFk2KKVWpcCTSZ49BHYEQI2ezhF26co2R4DvrngpnMjJdCTwPvPjwEpPA+SJiQGK5P\n\t2t5ch61jNVISp6zQv8DIq9PwLohQlo4qgV0R5cBNI5A5fnxRwSBfe0L6L+34mIpnXZRV\n\tobVw==","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=UwqMph9buucMR0Mg/2U/dC1My8hmmzkFbsiUMY/nLxk=;\n\tb=n2USVd3qC3RBjSoPTrR2R5T8HzHHNXyC3voUCvykWe1a12tWjHuEPBFEpOikxkxQMu\n\txIHBLcgFDWlRo/fLo9O37cucNVeegjrTCQ9hodR9ofFFd70s7XlW4/8DNvkpSIf7WPty\n\tHwqdIoLWK/WEQrPpBmtCPG3nYq5DvrGrrLFTMzDU+9eY9AaN8fpXPPv12NRI2B0c6NvT\n\ttCaD+NB7Cpt6Hd0xQVk3348IU89ZrfOzW9GqhPJMiBFSLu1pPbBdXkbTRoiyYgY4tYoM\n\tK09aTeDLaAf7F60JxSJXHom6sPny4OsEs8yaLMYqJyALQbchd3OMTq5v4i2WaBvWalsh\n\t0Z9A==","X-Gm-Message-State":"AOAM533QbPKYNKt6KqBPBOiXe9AzuYgSSNFyxBS0YAg9UCgJpGGjdMNJ\n\twulRZLTEI+ItFac/jh8UygRNuejdhawpV8hvj3EhgYUXIEhAmQ==","X-Google-Smtp-Source":"ABdhPJx7CBRzygCdwPu6/wJpv93bVH4moP9y9Fm1KK05MhcWBqzpZkTx3tkCUs6lTjrebX1v/KzG161VjOmMo2OfxhI=","X-Received":"by 2002:a05:6512:3e6:: with SMTP id\n\tn6mr1872935lfq.413.1606848464197; \n\tTue, 01 Dec 2020 10:47:44 -0800 (PST)","MIME-Version":"1.0","References":"<20201201095942.7280-1-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20201201095942.7280-1-laurent.pinchart@ideasonboard.com>","From":"Naushir Patuck ","Date":"Tue, 1 Dec 2020 18:47:27 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_interface: Make\n\tconfigure() return an int","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=\"===============5459566849281546829==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14040,"web_url":"https://patchwork.libcamera.org/comment/14040/","msgid":"","date":"2020-12-04T10:17:44","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_interface: Make\n\tconfigure() return an int","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 18:47, Naushir Patuck wrote:\n\n> Hi Laurent,\n>\n> Thank you for the patch. This does exactly what I need!\n>\n> On Tue, 1 Dec 2020 at 09:59, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\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>\n> Reviewed-by: Naushir Patuck \n> Tested-by: Naushir Patuck \n>\n\nDid you reach any conclusion on whether this is the preferred approach to\nuse for this?\n\nNaush\n\n\n\n> ---\n>> .../libcamera/internal/ipa_context_wrapper.h | 10 +++---\n>> include/libcamera/ipa/ipa_interface.h | 22 ++++++------\n>> src/ipa/libipa/ipa_interface_wrapper.cpp | 16 ++++-----\n>> src/ipa/libipa/ipa_interface_wrapper.h | 12 +++----\n>> src/ipa/raspberrypi/raspberrypi.cpp | 23 +++++++------\n>> src/ipa/rkisp1/rkisp1.cpp | 27 ++++++++-------\n>> src/ipa/vimc/vimc.cpp | 10 +++---\n>> src/libcamera/ipa_context_wrapper.cpp | 17 +++++-----\n>> src/libcamera/ipa_interface.cpp | 4 +++\n>> src/libcamera/proxy/ipa_proxy_linux.cpp | 10 +++---\n>> src/libcamera/proxy/ipa_proxy_thread.cpp | 24 ++++++-------\n>> test/ipa/ipa_wrappers_test.cpp | 34 ++++++++++++-------\n>> 12 files changed, 113 insertions(+), 96 deletions(-)\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>> 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 C148EBE177\n\tfor ;\n\tFri, 4 Dec 2020 10:18:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DD0D635D8;\n\tFri, 4 Dec 2020 11:18:03 +0100 (CET)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A53B3635D2\n\tfor ;\n\tFri, 4 Dec 2020 11:18:01 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id t22so6013405ljk.0\n\tfor ;\n\tFri, 04 Dec 2020 02:18: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=\"ROHin44/\"; 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=2Qa925Dohl5iHecvXoo7/hYUD8ZyS4PQiKZnMl8jBNI=;\n\tb=ROHin44/PKdTza/TV4MvcXiqJ91jNsXoDd6acAbY2NKjyW+i3XIq/P6stQmUDRC5Mo\n\tws5EJbhoP1POHYTlqXm5YOx3xVxOlVuo7rnkevQ0UBbA5rZ+45tl5k4paCfZMJfxnIZJ\n\tjtZ2wY9QdtP0VAzEQC3Ygdfqe9EiLlLPOFOyrWcx3TwLQvLyD5ZPhOr8+yBJXdwwT0p+\n\thUdID7ycu81xeLO32FdIc6H81VQlS9rjjGsCcrwzoIRBmdBxaSrOXhKXgD4QLWXfKc6Q\n\tEPrsVzc5z/BremnUjvB6tEng9YkoyK9tfjCp0Q1PTWyjMKTjLqHExtkCr4RLbNqNySEQ\n\t6SIA==","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=2Qa925Dohl5iHecvXoo7/hYUD8ZyS4PQiKZnMl8jBNI=;\n\tb=fp46JrbK9YRImVbaDwU91AftTtCqPC/Z+0fxJ+hKXxl64DQ7XrP37EMSNg6eTa7SoO\n\tkH38w/xTMqGaUpLV8QPbULwRhldhEDkbBotAwBjsWmCOsRTIyXt66POOlPyi1QjBaUhx\n\tZLcxcRZGI4ZVmoH7rxol4NrflFKY5LridjhbWWz7NI6Q8jSZyak1C8epPZhUddEzO/l+\n\tKKKOvq9NyoJfKzbF9T14A0hAgM+r5ynq+eF8x52N7iRlv9g8BsAGNGTtcWwWFmX7KxWD\n\tRRiWBvQ7WBFv7GdqcNAB9wuJ26SA2ALLKrApko8I73iwOEaaFL67pWDXvBpiece39YgO\n\tjpwQ==","X-Gm-Message-State":"AOAM530QH/HIoANtxE4DZDCQJVJd8AP3Et6pliICmJkG5BsRvOoZNvpL\n\tcUlLYosUIROFnlPHES/hpfT4rIOWtKKtrub20o5bUEFtszM=","X-Google-Smtp-Source":"ABdhPJywr6J4PiGHk4fu7Z1F9SOTxZLhnbakQbSMdzP7NTje3TMYL10G8LSgFiWrb0x7QlMW3nh/TnVYzKLJ3onsa+g=","X-Received":"by 2002:a05:651c:152:: with SMTP id\n\tc18mr3082993ljd.228.1607077080926; \n\tFri, 04 Dec 2020 02:18:00 -0800 (PST)","MIME-Version":"1.0","References":"<20201201095942.7280-1-laurent.pinchart@ideasonboard.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 4 Dec 2020 10:17:44 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_interface: Make\n\tconfigure() return an int","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=\"===============4119545563744212707==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14093,"web_url":"https://patchwork.libcamera.org/comment/14093/","msgid":"<20201207004300.GG2050@pyrite.rasen.tech>","date":"2020-12-07T00:43:00","subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_interface: Make\n\tconfigure() return an int","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Dec 04, 2020 at 10:17:44AM +0000, Naushir Patuck wrote:\n> Hi Laurent,\n> \n> On Tue, 1 Dec 2020 at 18:47, Naushir Patuck wrote:\n> \n> Hi Laurent,\n> \n> Thank you for the patch.  This does exactly what I need!\n> \n> On Tue, 1 Dec 2020 at 09:59, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\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> \n> Reviewed-by: Naushir Patuck \n> Tested-by: Naushir Patuck \n> \n> \n> Did you reach any conclusion on whether this is the preferred approach to use\n> for this?\n\nYeah, your approach is better, and I've sent my rb tag for it.\n\n\nPaul\n\n>  \n> \n> ---\n>  .../libcamera/internal/ipa_context_wrapper.h  | 10 +++---\n>  include/libcamera/ipa/ipa_interface.h         | 22 ++++++------\n>  src/ipa/libipa/ipa_interface_wrapper.cpp      | 16 ++++-----\n>  src/ipa/libipa/ipa_interface_wrapper.h        | 12 +++----\n>  src/ipa/raspberrypi/raspberrypi.cpp           | 23 +++++++------\n>  src/ipa/rkisp1/rkisp1.cpp                     | 27 ++++++++-------\n>  src/ipa/vimc/vimc.cpp                         | 10 +++---\n>  src/libcamera/ipa_context_wrapper.cpp         | 17 +++++-----\n>  src/libcamera/ipa_interface.cpp               |  4 +++\n>  src/libcamera/proxy/ipa_proxy_linux.cpp       | 10 +++---\n>  src/libcamera/proxy/ipa_proxy_thread.cpp      | 24 ++++++-------\n>  test/ipa/ipa_wrappers_test.cpp                | 34 ++++++++++++-------\n>  12 files changed, 113 insertions(+), 96 deletions(-)\n> \n> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include\n> /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 &> &entityControls,\n> +                     const IPAOperationData &ipaConfig,\n> +                     IPAOperationData *result) override;\n> \n>         void mapBuffers(const std::vector &buffers)\n> override;\n>         void unmapBuffers(const std::vector &ids)\n> override;\n> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/\n> 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 b/src/ipa/libipa/\n> 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\n> (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\n> *maps,\n> +                                  unsigned int num_maps)\n>  {\n>         IPAInterfaceWrapper *ctx = static_cast\n> (_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 b/src/ipa/libipa/\n> 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\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> +       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 b/src/ipa/raspberrypi/\n> 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 &> &entityControls,\n> +                     const IPAOperationData &data,\n> +                     IPAOperationData *response) override;\n>         void mapBuffers(const std::vector &buffers)\n> 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 &> &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 &> &entityControls,\n> +                     const IPAOperationData &ipaConfig,\n> +                     IPAOperationData *response) override;\n>         void mapBuffers(const std::vector &buffers)\n> 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 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 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 &\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 std::vector &\n> buffers) override {}\n>         void unmapBuffers([[maybe_unused]] const std::vector int> &ids) override {}\n>         void processEvent([[maybe_unused]] const IPAOperationData &\n> event) override {}\n> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/\n> 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 b/src/libcamera/\n> 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 b/src/libcamera/\n> 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 std::vector &\n> buffers) override {}\n>         void unmapBuffers([[maybe_unused]] const std::vector int> &ids) override {}\n>         void processEvent([[maybe_unused]] const IPAOperationData &\n> event) override {}\n> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/\n> 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 &> &entityControls,\n> +                     const IPAOperationData &ipaConfig,\n> +                     IPAOperationData *result) override;\n>         void mapBuffers(const std::vector &buffers)\n> 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\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,\n> entityControls,\n> +                              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/\n> 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 &> &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> \"\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> \"\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 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\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 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\" <<\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> 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 8E5C2BDB1F\n\tfor ;\n\tMon, 7 Dec 2020 00:43:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A85A67E17;\n\tMon, 7 Dec 2020 01:43:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ADA5D67E12\n\tfor ;\n\tMon, 7 Dec 2020 01:43:08 +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 8DA788D;\n\tMon, 7 Dec 2020 01:43:06 +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=\"uybcPXsV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607301788;\n\tbh=AtEXKf0avdZ8eoiEX3K2slr9InIdWvl/00fPf2N5Lwg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uybcPXsVg8rdTv/W+xSKxHD5wAkYobPDmDwJjhJTG3EK30uop4u+rnyMk40oQFBCD\n\t6N/WS9xFzxZ4uNIjGCUE6zhw1PuFUc0gve8sgw/EizZnRU18lhSDwJMitlv6U/s28s\n\tsNaRyFzwUK6fp3lDN49wGGqBzNUl97EW2j2ic148=","Date":"Mon, 7 Dec 2020 09:43:00 +0900","From":"paul.elder@ideasonboard.com","To":"Naushir Patuck ","Message-ID":"<20201207004300.GG2050@pyrite.rasen.tech>","References":"<20201201095942.7280-1-laurent.pinchart@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: ipa_interface: Make\n\tconfigure() return an int","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]