[{"id":13761,"web_url":"https://patchwork.libcamera.org/comment/13761/","msgid":"","date":"2020-11-17T16:34:54","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: ipa: Pass a set of\n\tcontrols and return results from ipa::start()","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the patch. Only some minor nitpicks...\n\nOn Thu, 12 Nov 2020 at 08:59, Naushir Patuck wrote:\n>\n> This change allows controls passed into pipeline_hanlder::start to be\n> forwarded onto ipa::start(). We also add a return channel if the\n> pipeline handle must action any of these controls, e.g. setting the\n> analogue gain or shutter speed in the sensor device.\n\ns/hanlder/handler/ (though strictly the class is PipelineHandler)\n\nIs IPAContextWrapper::start more accurate, instead of ipa::start?\n\ns/pipeline handle/pipeline handler/\n\n(sorry!)\n\n>\n> todo: The IPA interface wrapper needs a translation for passing\n> IPAOperationData into start() and configure()\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/internal/ipa_context_wrapper.h | 3 ++-\n> include/libcamera/ipa/ipa_interface.h | 3 ++-\n> src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 3 ++-\n> src/ipa/rkisp1/rkisp1.cpp | 3 ++-\n> src/ipa/vimc/vimc.cpp | 6 ++++--\n> src/libcamera/ipa_context_wrapper.cpp | 6 ++++--\n> src/libcamera/ipa_interface.cpp | 7 +++++++\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 +++-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++--\n> src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-\n> src/libcamera/proxy/ipa_proxy_linux.cpp | 3 ++-\n> src/libcamera/proxy/ipa_proxy_thread.cpp | 13 ++++++++-----\n> test/ipa/ipa_interface_test.cpp | 3 ++-\n> test/ipa/ipa_wrappers_test.cpp | 5 +++--\n> 15 files changed, 49 insertions(+), 22 deletions(-)\n>\n> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h\n> index 8f767e84..1878a615 100644\n> --- a/include/libcamera/internal/ipa_context_wrapper.h\n> +++ b/include/libcamera/internal/ipa_context_wrapper.h\n> @@ -20,7 +20,8 @@ public:\n> ~IPAContextWrapper();\n>\n> int init(const IPASettings &settings) override;\n> - int start() override;\n> + int start(const IPAOperationData &ipaConfig,\n> + IPAOperationData *result) override;\n> void stop() override;\n> void configure(const CameraSensorInfo &sensorInfo,\n> const std::map &streamConfig,\n> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h\n> index 322b7079..b44e2538 100644\n> --- a/include/libcamera/ipa/ipa_interface.h\n> +++ b/include/libcamera/ipa/ipa_interface.h\n> @@ -153,7 +153,8 @@ public:\n> virtual ~IPAInterface() = default;\n>\n> virtual int init(const IPASettings &settings) = 0;\n> - virtual int start() = 0;\n> + virtual int start(const IPAOperationData &ipaConfig,\n> + IPAOperationData *result) = 0;\n> virtual void stop() = 0;\n>\n> virtual void configure(const CameraSensorInfo &sensorInfo,\n> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> index cee532e3..fb2da9d3 100644\n> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp\n> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context *_ctx)\n> {\n> IPAInterfaceWrapper *ctx = static_cast(_ctx);\n>\n> - return ctx->ipa_->start();\n> + /* \\todo Translate the ipaConfig and result. */\n> + IPAOperationData ipaConfig;\n\nIn other places (though not always) you've tended to add \"= {}\" to the\nipaConfig object. As far as I can see, it's only the \"operation\" field\nthat might be uninitialised. Does this matter?\n\n> + return ctx->ipa_->start(ipaConfig, nullptr);\n> }\n>\n> void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f338ff8b..7a07b477 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -77,7 +77,8 @@ public:\n> }\n>\n> int init(const IPASettings &settings) override;\n> - int start() override { return 0; }\n> + int start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> + [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> void stop() override {}\n>\n> void configure(const CameraSensorInfo &sensorInfo,\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 07d7f1b2..0b8a9096 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -37,7 +37,8 @@ public:\n> {\n> return 0;\n> }\n> - int start() override { return 0; }\n> + int start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> + [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> void stop() override {}\n>\n> void configure(const CameraSensorInfo &info,\n> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp\n> index cf841135..ed26331d 100644\n> --- a/src/ipa/vimc/vimc.cpp\n> +++ b/src/ipa/vimc/vimc.cpp\n> @@ -34,7 +34,8 @@ public:\n>\n> int init(const IPASettings &settings) override;\n>\n> - int start() override;\n> + int start(const IPAOperationData &ipaConfig,\n> + IPAOperationData *result) override;\n> void stop() override;\n>\n> void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)\n> return 0;\n> }\n>\n> -int IPAVimc::start()\n> +int IPAVimc::start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> + [[maybe_unused]] IPAOperationData *result)\n> {\n> trace(IPAOperationStart);\n>\n> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> index 231300ce..de96a606 100644\n> --- a/src/libcamera/ipa_context_wrapper.cpp\n> +++ b/src/libcamera/ipa_context_wrapper.cpp\n> @@ -86,14 +86,16 @@ int IPAContextWrapper::init(const IPASettings &settings)\n> return 0;\n> }\n>\n> -int IPAContextWrapper::start()\n> +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,\n> + IPAOperationData *result)\n> {\n> if (intf_)\n> - return intf_->start();\n> + return intf_->start(ipaConfig, result);\n>\n> if (!ctx_)\n> return 0;\n>\n> + /* \\todo Translate the ipaConfig and reponse */\n> return ctx_->ops->start(ctx_);\n> }\n\ns/reponse/response/\n\n>\n> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp\n> index 23fc56d7..282c3c0f 100644\n> --- a/src/libcamera/ipa_interface.cpp\n> +++ b/src/libcamera/ipa_interface.cpp\n> @@ -536,10 +536,17 @@ namespace libcamera {\n> /**\n> * \\fn IPAInterface::start()\n> * \\brief Start the IPA\n> + * \\param[in] ipaConfig Pipeline-handler-specific configuration data\n> + * \\param[out] result Pipeline-handler-specific configuration result\n> *\n> * This method informs the IPA module that the camera is about to be started.\n> * The IPA module shall prepare any resources it needs to operate.\n> *\n> + * The \\a ipaConfig and \\a result parameters carry custom data passed by the\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 otherwise\n> */\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index ddb30e49..a8e4997a 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -747,7 +747,9 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> }\n>\n> /* Start the IPA. */\n> - ret = data->ipa_->start();\n> + IPAOperationData ipaConfig, result;\n\nAnother IPAOperationData with undefined \"operation\" field. Does it matter?\n\n> + ipaConfig.controls.emplace_back(*controls);\n> + ret = data->ipa_->start(ipaConfig, &result);\n> if (ret) {\n> LOG(RPI, Error)\n> << \"Failed to start IPA for \" << camera->id();\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 2e8d2930..a6c82a48 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -832,7 +832,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> if (ret)\n> return ret;\n>\n> - ret = data->ipa_->start();\n> + IPAOperationData ipaConfig = {};\n> + ret = data->ipa_->start(ipaConfig, nullptr);\n> if (ret) {\n> freeBuffers(camera);\n> LOG(RkISP1, Error)\n> @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> std::map entityControls;\n> entityControls.emplace(0, data->sensor_->controls());\n>\n> - IPAOperationData ipaConfig;\n> + ipaConfig = {};\n> data->ipa_->configure(sensorInfo, streamConfig, entityControls,\n> ipaConfig, nullptr);\n>\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index d81b8598..b4773cf5 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] ControlList *con\n> if (ret < 0)\n> return ret;\n>\n> - ret = data->ipa_->start();\n> + IPAOperationData ipaConfig = {};\n> + ret = data->ipa_->start(ipaConfig, nullptr);\n> if (ret) {\n> data->video_->releaseBuffers();\n> return ret;\n> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> index b78a0e45..619976e5 100644\n> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp\n> @@ -30,7 +30,8 @@ public:\n> {\n> return 0;\n> }\n> - int start() override { return 0; }\n> + int start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> + [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> void stop() override {}\n> void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> [[maybe_unused]] const std::map &streamConfig,\n> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> index eead2883..9a81b6e7 100644\n> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp\n> @@ -26,7 +26,8 @@ public:\n> IPAProxyThread(IPAModule *ipam);\n>\n> int init(const IPASettings &settings) override;\n> - int start() override;\n> + int start(const IPAOperationData &ipaConfig,\n> + IPAOperationData *result) override;\n> void stop() override;\n>\n> void configure(const CameraSensorInfo &sensorInfo,\n> @@ -50,9 +51,9 @@ private:\n> ipa_ = ipa;\n> }\n>\n> - int start()\n> + int start(const IPAOperationData &ipaConfig, IPAOperationData *result)\n> {\n> - return ipa_->start();\n> + return ipa_->start(ipaConfig, result);\n> }\n>\n> void stop()\n> @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings &settings)\n> return 0;\n> }\n>\n> -int IPAProxyThread::start()\n> +int IPAProxyThread::start(const IPAOperationData &ipaConfig,\n> + IPAOperationData *result)\n> {\n> running_ = true;\n> thread_.start();\n>\n> - return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);\n> + return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking,\n> + ipaConfig, result);\n> }\n>\n> void IPAProxyThread::stop()\n> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp\n> index 67488409..6222938f 100644\n> --- a/test/ipa/ipa_interface_test.cpp\n> +++ b/test/ipa/ipa_interface_test.cpp\n> @@ -120,7 +120,8 @@ protected:\n> }\n>\n> /* Test start of IPA module. */\n> - ipa_->start();\n> + IPAOperationData ipaConfig;\n\nAnother uninitialised \"operation\" field. Does it matter? Should we\nmake them all look the same?\n\nOther than these nitpicks:\n\nReviewed-by: David Plowman \nTested-by: David Plowman \n\nBest regards\nDavid\n\n> + ipa_->start(ipaConfig, nullptr);\n> timer.start(1000);\n> while (timer.isRunning() && trace_ != IPAOperationStart)\n> dispatcher->processEvents();\n> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp\n> index 59d991cb..7b8ae77b 100644\n> --- a/test/ipa/ipa_wrappers_test.cpp\n> +++ b/test/ipa/ipa_wrappers_test.cpp\n> @@ -56,7 +56,8 @@ public:\n> return 0;\n> }\n>\n> - int start() override\n> + int start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> + [[maybe_unused]] IPAOperationData *result) override\n> {\n> report(Op_start, TestPass);\n> return 0;\n> @@ -376,7 +377,7 @@ protected:\n> return TestFail;\n> }\n>\n> - ret = INVOKE(start);\n> + ret = INVOKE(start, ipaConfig, nullptr);\n> if (ret == TestFail) {\n> cerr << \"Failed to run start()\";\n> return TestFail;\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 E3773BE082\n\tfor ;\n\tTue, 17 Nov 2020 16:35:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFED56331E;\n\tTue, 17 Nov 2020 17:35:07 +0100 (CET)","from mail-ot1-x335.google.com (mail-ot1-x335.google.com\n\t[IPv6:2607:f8b0:4864:20::335])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8513C6033B\n\tfor ;\n\tTue, 17 Nov 2020 17:35:06 +0100 (CET)","by mail-ot1-x335.google.com with SMTP id f16so19969578otl.11\n\tfor ;\n\tTue, 17 Nov 2020 08:35:06 -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=\"e8i+8MXS\"; 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=6eaWRMIkcK3pKlbeLYOot/V5BwWrMXgd7QPrDLPEBbM=;\n\tb=e8i+8MXS9mMJw3YUET2n29xQC9nnJwOynBfUTzCM1v+7JtqXlGifoQ+vWc+9z17+jf\n\toOQPympwyK0WFLHmC1iye8G0g4FEG3CGQxSR3oOl4vGWAeXkj5lZRJERVir9HBvFHolP\n\tl8YVsJMbNsClsw9mBDa5CrP2Vo1c1www+n0LIQrrHwQzBSXBuSyv1NXiXDzlDqz8wAqD\n\t5qGnKPiaPIHqOKm8jEqi/DXdrzGHgoPMGFNxYSe5Y8BtwxPMXQC0d/MkAFx/M1BotB7x\n\tCaSpl+CyrY5of/28ppzpYVhRPJxtfdLIzsiBzlzcSLrcUj4NzldxmmK6r9Upj8xHqVMn\n\t5xOw==","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=6eaWRMIkcK3pKlbeLYOot/V5BwWrMXgd7QPrDLPEBbM=;\n\tb=sIwgPVNms7j9VRojMJOKVUPa79gw/kEg/HMMYHZkr1BxJJjW2UHIYzZCttNhHXyMyE\n\trCqZ6nzOx+U6RZOdt8xKaHx48HkXnfKoWcJRLYNiAlVE7WfUYNIYoV5QfvMUy1j4XiEx\n\tyg6JQpwLayAppjUXwoNBoqdp6eqW0g74fkIpRpqgMLSgxWWIZ6ATyv2B69Wohf0URkxg\n\tIlJJDrflpiXDzFtPvYDA8j2p8W1Jgrj1GkbUTSMZWHtzHfP8gwZg16tuwLSb2IFScgxD\n\tFYVZnMaoL87t9ZM+ampXy0wxrAh/wkH+6FkqpSM6Ca7ywEBjktlmqtBDt3ZfEqh+XUmB\n\t1DWg==","X-Gm-Message-State":"AOAM5323x41GG5AKyycTlLRAfkkoJDobgnCer4YsnEkfyT8PT45KAyox\n\ta6nmKpCkrCx+0WS+Cg4+CrxnaW+6YdgzjfvCJl2wog==","X-Google-Smtp-Source":"ABdhPJzsMJB/Ziwq2KIDsyZIaz6cggRSkohRS9EliD0Bdy19VRKqpVA9NrwPGt2pkD8cSesbaL/m87+ZY9Z+U53UbL4=","X-Received":"by 2002:a05:6830:1ad0:: with SMTP id\n\tr16mr3735019otc.160.1605630905159; \n\tTue, 17 Nov 2020 08:35:05 -0800 (PST)","MIME-Version":"1.0","References":"<20201112085915.3053-1-naush@raspberrypi.com>\n\t<20201112085915.3053-3-naush@raspberrypi.com>","In-Reply-To":"<20201112085915.3053-3-naush@raspberrypi.com>","From":"David Plowman ","Date":"Tue, 17 Nov 2020 16:34:54 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: ipa: Pass a set of\n\tcontrols and return results from ipa::start()","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\" "}}]