[{"id":14046,"web_url":"https://patchwork.libcamera.org/comment/14046/","msgid":"<20201204112620.baa4vuvd3ym2hsd5@uno.localdomain>","date":"2020-12-04T11:26:20","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: ipa: Pass a set of\n\tcontrols and return results from ipa::start()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:\n> This change allows controls passed into PipelineHandler::start to be\n> forwarded onto IPAInterface::start(). We also add a return channel if the\n> pipeline handler must action any of these controls, e.g. setting the\n> analogue gain or shutter speed in the sensor device.\n>\n> todo: The IPA interface wrapper needs a translation for passing\n> IPAOperationData into start() and configure()\n\nThis is 'problematic' as it makes isolated IPAs deviate from other\nones. We have the same issue with configure() which has a very similar\n\\todo item :/\n\nNow, this is not an issue right now, and I think with Paul's IPC\nwe'll get it for free.\n\nOther comments below:\n\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Tested-by: David Plowman \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 | 6 ++++--\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, 50 insertions(+), 23 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> \t~IPAContextWrapper();\n>\n> \tint init(const IPASettings &settings) override;\n> -\tint start() override;\n> +\tint start(const IPAOperationData &ipaConfig,\n\n'ipaConfig' is only used in configure.\nIt's generally just called 'data' and I would use it here too.\n\n> +\t\t IPAOperationData *result) override;\n> \tvoid stop() override;\n> \tvoid configure(const CameraSensorInfo &sensorInfo,\n> \t\t 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> \tvirtual ~IPAInterface() = default;\n>\n> \tvirtual int init(const IPASettings &settings) = 0;\n> -\tvirtual int start() = 0;\n> +\tvirtual int start(const IPAOperationData &ipaConfig,\n> +\t\t\t IPAOperationData *result) = 0;\n> \tvirtual void stop() = 0;\n>\n> \tvirtual 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..2b305b56 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> \tIPAInterfaceWrapper *ctx = static_cast(_ctx);\n>\n> -\treturn ctx->ipa_->start();\n> +\t/* \\todo Translate the ipaConfig and result. */\n> +\tIPAOperationData ipaConfig = {};\n> +\treturn 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> \t}\n>\n> \tint init(const IPASettings &settings) override;\n> -\tint start() override { return 0; }\n> +\tint start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> +\t\t [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> \tvoid stop() override {}\n>\n> \tvoid 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> \t{\n> \t\treturn 0;\n> \t}\n> -\tint start() override { return 0; }\n> +\tint start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> +\t\t [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> \tvoid stop() override {}\n>\n> \tvoid 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> \tint init(const IPASettings &settings) override;\n>\n> -\tint start() override;\n> +\tint start(const IPAOperationData &ipaConfig,\n> +\t\t IPAOperationData *result) override;\n> \tvoid stop() override;\n>\n> \tvoid configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)\n> \treturn 0;\n> }\n>\n> -int IPAVimc::start()\n> +int IPAVimc::start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> +\t\t [[maybe_unused]] IPAOperationData *result)\n\n ^ is this mis-aligned or is it my email client ?\n There are more in this patch if that's the case.\n> {\n> \ttrace(IPAOperationStart);\n>\n> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp\n> index 231300ce..8c4ec40a 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> \treturn 0;\n> }\n>\n> -int IPAContextWrapper::start()\n> +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,\n> +\t\t\t IPAOperationData *result)\n> {\n> \tif (intf_)\n> -\t\treturn intf_->start();\n> +\t\treturn intf_->start(ipaConfig, result);\n>\n> \tif (!ctx_)\n> \t\treturn 0;\n>\n> +\t/* \\todo Translate the ipaConfig and response */\n> \treturn ctx_->ops->start(ctx_);\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\nThese come from the 'configuration' operation\nI would:\n\n\\param[in] data Protocol-specific data for the start operation\n\\param[out] result Result of the start operation\n\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\nCopied from configure too, replace 'configure()' at least.\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..29bcef07 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> \t}\n>\n> \t/* Start the IPA. */\n> -\tret = data->ipa_->start();\n> +\tIPAOperationData ipaConfig = {}, result = {};\n> +\tipaConfig.controls.emplace_back(*controls);\n\nWhat if controls is nullptr ?\n\n> +\tret = data->ipa_->start(ipaConfig, &result);\n> \tif (ret) {\n> \t\tLOG(RPI, Error)\n> \t\t\t<< \"Failed to start IPA for \" << camera->id();\n> @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t}\n>\n> \t/* Ready the IPA - it must know about the sensor resolution. */\n> -\tIPAOperationData result;\n> +\tIPAOperationData result = {};\n\nUnrelated but I think it's ok\n\n>\n> \tipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> \t\t\t&result);\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> \tif (ret)\n> \t\treturn ret;\n>\n> -\tret = data->ipa_->start();\n> +\tIPAOperationData ipaConfig = {};\n> +\tret = data->ipa_->start(ipaConfig, nullptr);\n\nIf you want to use a single IPAOperationData for start and configure,\nplease rename it (here and in other pipeline handlers).\n\nThanks\n j\n\n\n> \tif (ret) {\n> \t\tfreeBuffers(camera);\n> \t\tLOG(RkISP1, Error)\n> @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c\n> \tstd::map entityControls;\n> \tentityControls.emplace(0, data->sensor_->controls());\n>\n> -\tIPAOperationData ipaConfig;\n> +\tipaConfig = {};\n> \tdata->ipa_->configure(sensorInfo, streamConfig, entityControls,\n> \t\t\t 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> \tif (ret < 0)\n> \t\treturn ret;\n>\n> -\tret = data->ipa_->start();\n> +\tIPAOperationData ipaConfig = {};\n> +\tret = data->ipa_->start(ipaConfig, nullptr);\n> \tif (ret) {\n> \t\tdata->video_->releaseBuffers();\n> \t\treturn 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> \t{\n> \t\treturn 0;\n> \t}\n> -\tint start() override { return 0; }\n> +\tint start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> +\t\t [[maybe_unused]] IPAOperationData *result) override { return 0; }\n> \tvoid stop() override {}\n> \tvoid configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> \t\t [[maybe_unused]] const std::map &streamConfig,\n> 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> \tIPAProxyThread(IPAModule *ipam);\n>\n> \tint init(const IPASettings &settings) override;\n> -\tint start() override;\n> +\tint start(const IPAOperationData &ipaConfig,\n> +\t\t IPAOperationData *result) override;\n> \tvoid stop() override;\n>\n> \tvoid configure(const CameraSensorInfo &sensorInfo,\n> @@ -50,9 +51,9 @@ private:\n> \t\t\tipa_ = ipa;\n> \t\t}\n>\n> -\t\tint start()\n> +\t\tint start(const IPAOperationData &ipaConfig, IPAOperationData *result)\n> \t\t{\n> -\t\t\treturn ipa_->start();\n> +\t\t\treturn ipa_->start(ipaConfig, result);\n> \t\t}\n>\n> \t\tvoid stop()\n> @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings &settings)\n> \treturn 0;\n> }\n>\n> -int IPAProxyThread::start()\n> +int IPAProxyThread::start(const IPAOperationData &ipaConfig,\n> +\t\t\t IPAOperationData *result)\n> {\n> \trunning_ = true;\n> \tthread_.start();\n>\n> -\treturn proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);\n> +\treturn proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking,\n> +\t\t\t\t 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..03b7f0ad 100644\n> --- a/test/ipa/ipa_interface_test.cpp\n> +++ b/test/ipa/ipa_interface_test.cpp\n> @@ -120,7 +120,8 @@ protected:\n> \t\t}\n>\n> \t\t/* Test start of IPA module. */\n> -\t\tipa_->start();\n> +\t\tIPAOperationData ipaConfig = {};\n> +\t\tipa_->start(ipaConfig, nullptr);\n> \t\ttimer.start(1000);\n> \t\twhile (timer.isRunning() && trace_ != IPAOperationStart)\n> \t\t\tdispatcher->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> \t\treturn 0;\n> \t}\n>\n> -\tint start() override\n> +\tint start([[maybe_unused]] const IPAOperationData &ipaConfig,\n> +\t\t [[maybe_unused]] IPAOperationData *result) override\n> \t{\n> \t\treport(Op_start, TestPass);\n> \t\treturn 0;\n> @@ -376,7 +377,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tret = INVOKE(start);\n> +\t\tret = INVOKE(start, ipaConfig, nullptr);\n> \t\tif (ret == TestFail) {\n> \t\t\tcerr << \"Failed to run start()\";\n> \t\t\treturn 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 1FA3ABE176\n\tfor ;\n\tFri, 4 Dec 2020 11:26:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6B95635EE;\n\tFri, 4 Dec 2020 12:26:14 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4217B634C6\n\tfor ;\n\tFri, 4 Dec 2020 12:26:14 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id A6847240008;\n\tFri, 4 Dec 2020 11:26:13 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 4 Dec 2020 12:26:20 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20201204112620.baa4vuvd3ym2hsd5@uno.localdomain>","References":"<20201126095126.997055-1-naush@raspberrypi.com>\n\t<20201126095126.997055-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201126095126.997055-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14048,"web_url":"https://patchwork.libcamera.org/comment/14048/","msgid":"","date":"2020-12-04T13:06:09","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: ipa: Pass a set of\n\tcontrols and return results from ipa::start()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nThank you for your review feedback.\n\nOn Fri, 4 Dec 2020 at 11:26, Jacopo Mondi wrote:\n\n> Hi Naush,\n>\n> On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:\n> > This change allows controls passed into PipelineHandler::start to be\n> > forwarded onto IPAInterface::start(). We also add a return channel if the\n> > pipeline handler must action any of these controls, e.g. setting the\n> > analogue gain or shutter speed in the sensor device.\n> >\n> > todo: The IPA interface wrapper needs a translation for passing\n> > IPAOperationData into start() and configure()\n>\n> This is 'problematic' as it makes isolated IPAs deviate from other\n> ones. We have the same issue with configure() which has a very similar\n> \\todo item :/\n>\n> Now, this is not an issue right now, and I think with Paul's IPC\n> we'll get it for free.\n>\n> Other comments below:\n>\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Tested-by: David Plowman \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 | 6 ++++--\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, 50 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n> 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>\n> 'ipaConfig' is only used in configure.\n> It's generally just called 'data' and I would use it here too.\n>\n\nSure, will fix. I will also s/ ipaConfig/data/ in all instances of start()\nthat have been changed in this patch.\n\n\n> > + IPAOperationData *result) override;\n> > void stop() override;\n> > void configure(const CameraSensorInfo &sensorInfo,\n> > const std::map\n> &streamConfig,\n> > diff --git a/include/libcamera/ipa/ipa_interface.h\n> 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\n> b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > index cee532e3..2b305b56 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\n> *_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> > + 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\n> 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 {\n> 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 {\n> 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> ^ is this mis-aligned or is it my email client ?\n> There are more in this patch if that's the case.\n>\n\nI think this may be an email client rendering thing. They do look correct\nto me, and I would expect the style checker to complain about\nmisalignment. However, I will check though once again.\n\n\n> > {\n> > trace(IPAOperationStart);\n> >\n> > diff --git a/src/libcamera/ipa_context_wrapper.cpp\n> b/src/libcamera/ipa_context_wrapper.cpp\n> > index 231300ce..8c4ec40a 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\n> &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 response */\n> > return ctx_->ops->start(ctx_);\n> > }\n> >\n> > diff --git a/src/libcamera/ipa_interface.cpp\n> 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> These come from the 'configuration' operation\n> I would:\n>\n> \\param[in] data Protocol-specific data for the start operation\n> \\param[out] result Result of the start operation\n>\n\nFixed.\n\n\n>\n> > *\n> > * This method informs the IPA module that the camera is about to be\n> 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\n> by the\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>\n> Copied from configure too, replace 'configure()' at least.\n>\n\nOops, replaced with start()!\n\n\n>\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index ddb30e49..29bcef07 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,\n> [[maybe_unused]] ControlList *cont\n> > }\n> >\n> > /* Start the IPA. */\n> > - ret = data->ipa_->start();\n> > + IPAOperationData ipaConfig = {}, result = {};\n> > + ipaConfig.controls.emplace_back(*controls);\n>\n> What if controls is nullptr ?\n>\n\nHmmm.... I am a bit confused. My version of the code has:\n\n if (controls) {\n ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;\n ipaConfig.controls.emplace_back(*controls);\n}\n\nBut clearly my patch in the email does not :( Once I complete all your\nsuggested fixups, I will send an updated patchset!\n\n\n> > + ret = data->ipa_->start(ipaConfig, &result);\n> > if (ret) {\n> > LOG(RPI, Error)\n> > << \"Failed to start IPA for \" << camera->id();\n> > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > }\n> >\n> > /* Ready the IPA - it must know about the sensor resolution. */\n> > - IPAOperationData result;\n> > + IPAOperationData result = {};\n>\n> Unrelated but I think it's ok\n>\n\nThis was an update requested by David in his review. Just to be safe :)\n\n\n>\n> >\n> > ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> ipaConfig,\n> > &result);\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> 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,\n> [[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>\n> If you want to use a single IPAOperationData for start and configure,\n> please rename it (here and in other pipeline handlers).\n>\n\nI would use data (as with the other IPAOperationData parameters passed into\nstart), but we already have a declaration of RkISP1CameraData *data. Any\nobjections to renaming it ipaData?\n\nRegards,\nNaush\n\n\n\n>\n> Thanks\n> j\n>\n>\n> > if (ret) {\n> > freeBuffers(camera);\n> > LOG(RkISP1, Error)\n> > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera,\n> [[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\n> 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,\n> [[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\n> 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 {\n> return 0; }\n> > void stop() override {}\n> > void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> > [[maybe_unused]] const std::map IPAStream> &streamConfig,\n> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> 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,\n> 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\n> &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,\n> ConnectionTypeBlocking);\n> > + return proxy_.invokeMethod(&ThreadProxy::start,\n> ConnectionTypeBlocking,\n> > + ipaConfig, result);\n> > }\n> >\n> > void IPAProxyThread::stop()\n> > diff --git a/test/ipa/ipa_interface_test.cpp\n> b/test/ipa/ipa_interface_test.cpp\n> > index 67488409..03b7f0ad 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> > + 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\n> 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\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 0A432BE176\n\tfor ;\n\tFri, 4 Dec 2020 13:06:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 610A0635EE;\n\tFri, 4 Dec 2020 14:06:28 +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 0657B634C6\n\tfor ;\n\tFri, 4 Dec 2020 14:06:26 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id 142so6499398ljj.10\n\tfor ;\n\tFri, 04 Dec 2020 05:06:26 -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=\"SGz3z+HE\"; 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=gq6ehEg9GPh45pOz0wV9O8UZOgxJq9iALomgPKDrJP4=;\n\tb=SGz3z+HEtfpDyusgg6yCFaYb78FnT/csZU5MIdgCgwurXnXSsRkrHnPHawss5SS5Ae\n\tWEvlJFl3x5mQIGMimBfH/g1QztmBTiKABXUwn9FzbJwDqddpD7utR7hP9EpE/3etmyEp\n\tZ6Ejz/XTvWv3itviRL/S9b3gdmgvYSX50NPcCUjVCQNPJrKgELhDmlX04IOisQZShsNX\n\t/hyTxTrYYLgZ7CBuUCMqZwH37S7FyUu8FrsUwNyKloLb5HL4sJAj5l9VAVJahbrUDhmy\n\tfH8njvx3gjf4LsyyzlrlT3EzySYplRsde/4S2r93k1P9uKB5uZP6KpODd6rQ5Dl5WsU/\n\t2orQ==","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=gq6ehEg9GPh45pOz0wV9O8UZOgxJq9iALomgPKDrJP4=;\n\tb=Xct/eblpwcxxlLYGOUGxVDve5HPFYXcJ0uTOa9ESJpvIYat2mUtf/Y7vS6jRysRdyA\n\tFloAIchWOfGpM0F3ufhnwp+XbMl6Zl1jzP3tJoQ/eZGk8LuvyWp7pI9kTESpUvmdnxUA\n\tGOoEW1DH7lPa3YMT8ao3W0IZbJlUghLfJoYOzrX5+sc1PnWn9FHF6bn3bfTWRJYg8ttV\n\tIOK9iFSQ2SxDsEzoDq0LPM1uoCysdPRbtvWPtmcvhTLg0Sj6/fRpzBMJX7BU1t6g2uhw\n\txqRO0JgDVGDh3dXSMQnsGBXFvX+7frcanqs6ATCr9nF3o1Skkn8JLNr9tk3rQIDMEPsc\n\tfvSQ==","X-Gm-Message-State":"AOAM5327brEQSmEE61Yslz4vVFZgW63PYeHrq6v1DA8nsoRwGtH8yPfj\n\tri6e126+vvivSOr14qvE7de4+UqGhvE+Df9SHebH2Y2eVJF5qQ==","X-Google-Smtp-Source":"ABdhPJwzHEO9BYLAVi7Ziiguj1+jVA65q/A5HIzaXRAeCoh2nqAc25E4fVOZOoeSa8v2vtqOanA9t3hSOrzoe6Ki4U8=","X-Received":"by 2002:a2e:9ad2:: with SMTP id\n\tp18mr3418262ljj.415.1607087186331; \n\tFri, 04 Dec 2020 05:06:26 -0800 (PST)","MIME-Version":"1.0","References":"<20201126095126.997055-1-naush@raspberrypi.com>\n\t<20201126095126.997055-2-naush@raspberrypi.com>\n\t<20201204112620.baa4vuvd3ym2hsd5@uno.localdomain>","In-Reply-To":"<20201204112620.baa4vuvd3ym2hsd5@uno.localdomain>","From":"Naushir Patuck ","Date":"Fri, 4 Dec 2020 13:06:09 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 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":"multipart/mixed;\n\tboundary=\"===============8044743618265383560==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14049,"web_url":"https://patchwork.libcamera.org/comment/14049/","msgid":"","date":"2020-12-04T13:16:50","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: ipa: Pass a set of\n\tcontrols and return results from ipa::start()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\n\nOn Fri, 4 Dec 2020 at 13:06, Naushir Patuck wrote:\n\n> Hi Jacopo,\n>\n> Thank you for your review feedback.\n>\n> On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi wrote:\n>\n>> Hi Naush,\n>>\n>> On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:\n>> > This change allows controls passed into PipelineHandler::start to be\n>> > forwarded onto IPAInterface::start(). We also add a return channel if\n>> the\n>> > pipeline handler must action any of these controls, e.g. setting the\n>> > analogue gain or shutter speed in the sensor device.\n>> >\n>> > todo: The IPA interface wrapper needs a translation for passing\n>> > IPAOperationData into start() and configure()\n>>\n>> This is 'problematic' as it makes isolated IPAs deviate from other\n>> ones. We have the same issue with configure() which has a very similar\n>> \\todo item :/\n>>\n>> Now, this is not an issue right now, and I think with Paul's IPC\n>> we'll get it for free.\n>>\n>> Other comments below:\n>>\n>> >\n>> > Signed-off-by: Naushir Patuck \n>> > Reviewed-by: David Plowman \n>> > Tested-by: David Plowman \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 | 6 ++++--\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, 50 insertions(+), 23 deletions(-)\n>> >\n>> > diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n>> 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>>\n>> 'ipaConfig' is only used in configure.\n>> It's generally just called 'data' and I would use it here too.\n>>\n>\n> Sure, will fix. I will also s/ ipaConfig/data/ in all instances of\n> start() that have been changed in this patch.\n>\n>\n>> > + IPAOperationData *result) override;\n>> > void stop() override;\n>> > void configure(const CameraSensorInfo &sensorInfo,\n>> > const std::map\n>> &streamConfig,\n>> > diff --git a/include/libcamera/ipa/ipa_interface.h\n>> 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\n>> b/src/ipa/libipa/ipa_interface_wrapper.cpp\n>> > index cee532e3..2b305b56 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\n>> *_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>> > + 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\n>> 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 {\n>> 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 {\n>> 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\n>> &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>> ^ is this mis-aligned or is it my email client ?\n>> There are more in this patch if that's the case.\n>>\n>\n> I think this may be an email client rendering thing. They do look correct\n> to me, and I would expect the style checker to complain about\n> misalignment. However, I will check though once again.\n>\n>\n>> > {\n>> > trace(IPAOperationStart);\n>> >\n>> > diff --git a/src/libcamera/ipa_context_wrapper.cpp\n>> b/src/libcamera/ipa_context_wrapper.cpp\n>> > index 231300ce..8c4ec40a 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\n>> &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 response */\n>> > return ctx_->ops->start(ctx_);\n>> > }\n>> >\n>> > diff --git a/src/libcamera/ipa_interface.cpp\n>> 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>> These come from the 'configuration' operation\n>> I would:\n>>\n>> \\param[in] data Protocol-specific data for the start operation\n>> \\param[out] result Result of the start operation\n>>\n>\n> Fixed.\n>\n>\n>>\n>> > *\n>> > * This method informs the IPA module that the camera is about to be\n>> 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\n>> by the\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>>\n>> Copied from configure too, replace 'configure()' at least.\n>>\n>\n> Oops, replaced with start()!\n>\n>\n>>\n>> > * \\return 0 on success or a negative error code otherwise\n>> > */\n>> >\n>> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > index ddb30e49..29bcef07 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,\n>> [[maybe_unused]] ControlList *cont\n>> > }\n>> >\n>> > /* Start the IPA. */\n>> > - ret = data->ipa_->start();\n>> > + IPAOperationData ipaConfig = {}, result = {};\n>> > + ipaConfig.controls.emplace_back(*controls);\n>>\n>> What if controls is nullptr ?\n>>\n>\n> Hmmm.... I am a bit confused. My version of the code has:\n>\n> if (controls) {\n> ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;\n> ipaConfig.controls.emplace_back(*controls);\n> }\n>\n> But clearly my patch in the email does not :( Once I complete all your\n> suggested fixups, I will send an updated patchset!\n>\n\nSorry, my bad, I was confusing myself. The above code (with the check on\ncontrols) is only introduced in patch 3/3. I will backport the test to\nthis patch 2/3.\n\nRegards,\nNaush\n\n\n>\n>\n>> > + ret = data->ipa_->start(ipaConfig, &result);\n>> > if (ret) {\n>> > LOG(RPI, Error)\n>> > << \"Failed to start IPA for \" << camera->id();\n>> > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const\n>> CameraConfiguration *config)\n>> > }\n>> >\n>> > /* Ready the IPA - it must know about the sensor resolution. */\n>> > - IPAOperationData result;\n>> > + IPAOperationData result = {};\n>>\n>> Unrelated but I think it's ok\n>>\n>\n> This was an update requested by David in his review. Just to be safe :)\n>\n>\n>>\n>> >\n>> > ipa_->configure(sensorInfo_, streamConfig, entityControls,\n>> ipaConfig,\n>> > &result);\n>> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> 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,\n>> [[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>>\n>> If you want to use a single IPAOperationData for start and configure,\n>> please rename it (here and in other pipeline handlers).\n>>\n>\n> I would use data (as with the other IPAOperationData parameters passed\n> into start), but we already have a declaration of RkISP1CameraData *data.\n> Any objections to renaming it ipaData?\n>\n> Regards,\n> Naush\n>\n>\n>\n>>\n>> Thanks\n>> j\n>>\n>>\n>> > if (ret) {\n>> > freeBuffers(camera);\n>> > LOG(RkISP1, Error)\n>> > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera,\n>> [[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\n>> 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,\n>> [[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\n>> 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 {\n>> return 0; }\n>> > void stop() override {}\n>> > void configure([[maybe_unused]] const CameraSensorInfo\n>> &sensorInfo,\n>> > [[maybe_unused]] const std::map> IPAStream> &streamConfig,\n>> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n>> 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,\n>> 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\n>> &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,\n>> ConnectionTypeBlocking);\n>> > + return proxy_.invokeMethod(&ThreadProxy::start,\n>> ConnectionTypeBlocking,\n>> > + ipaConfig, result);\n>> > }\n>> >\n>> > void IPAProxyThread::stop()\n>> > diff --git a/test/ipa/ipa_interface_test.cpp\n>> b/test/ipa/ipa_interface_test.cpp\n>> > index 67488409..03b7f0ad 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>> > + 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\n>> 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\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 3D57DBE177\n\tfor ;\n\tFri, 4 Dec 2020 13:17:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC22C635DE;\n\tFri, 4 Dec 2020 14:17:09 +0100 (CET)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 27C2F635D0\n\tfor ;\n\tFri, 4 Dec 2020 14:17:08 +0100 (CET)","by mail-lj1-x229.google.com with SMTP id f18so6550590ljg.9\n\tfor ;\n\tFri, 04 Dec 2020 05:17:08 -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=\"aKR3iuJM\"; 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=AFpCku5HAY729rZJRbZXfZeGfhDKfawDiZIoA0b29Bo=;\n\tb=aKR3iuJMT8/3be9exIl9mkcpf5fCiA8p6AFFIwZdfhukLqIowT4TGgfOdgsojh6DNd\n\tZJYsAfadXCwftsgKTa5FcBa4j7Z3e0rKGjn6PstDuAoxTFNGR8fdLGc+htoj95PcNAzy\n\tuT80F8KLDTKfB1fWKiu/99milrVe8sfFfIGmOYpFF+3mVRXw4A6qcl4wlqFQBA1Pv5W9\n\t0H90bZv09id94URneHMFowccj09S8bzGysy0XRnrXWj493u33qK+rJpVM4KcAVC01YWf\n\tZY2z/RdS+a/+eRTvMtj2zjI4uw9bPOZAjQ0WH5hsYtpi14t021ySw9BER6ByVj7/eU0J\n\tQWNQ==","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=AFpCku5HAY729rZJRbZXfZeGfhDKfawDiZIoA0b29Bo=;\n\tb=sxW9QA1mBk+7DZJQcXAegColmqITKrQSQF8MwPeia9bHekQaREtngE4FjLa0PZ9wsu\n\tapDiIGO8RqozfepgRPmTtN8nv8+B1XucTqc0YYHi5x6s8SoS2lm7bjnaf3Coa6s9dx0T\n\t/1mI64HrAByDfpRgTtBv8qf0qOf+ghaDwZCjgwuQhHm3NcFEhTwslVpyuYu/gOGEVNMs\n\tSbsQbMt3Ryg2CPPhrry8Obn3B8ztWEzsTwL39VBqNiZTWu5K75fx6f22B8o9KljPsuzn\n\thYRF/mJvFmLVqCzGowVdkDfcJMc3VkpCPxlVx22uAZ5LxmbKBwg86JfwtN9uzI2+EmB/\n\tR4Ow==","X-Gm-Message-State":"AOAM530XPUtCnj6hu6Z++lx4RSR9Dje0ZT5Plls09IU4Elmx2y+fs4Ej\n\tR8PCNrECt/T8M5ya8vdd80cbCU0TInvqu/eXgjTPJA==","X-Google-Smtp-Source":"ABdhPJwNRRMZcQkV2tmg6v/0BsK/Hc+SBWvkYJ+Z0uvxOMEo2taMz9r4AnQGKYiHi305mvLtG0jm6D4rgfF5N5V+clY=","X-Received":"by 2002:a2e:a58e:: with SMTP id m14mr3277498ljp.1.1607087827476; \n\tFri, 04 Dec 2020 05:17:07 -0800 (PST)","MIME-Version":"1.0","References":"<20201126095126.997055-1-naush@raspberrypi.com>\n\t<20201126095126.997055-2-naush@raspberrypi.com>\n\t<20201204112620.baa4vuvd3ym2hsd5@uno.localdomain>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 4 Dec 2020 13:16:50 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 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":"multipart/mixed;\n\tboundary=\"===============3013372953590933289==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14050,"web_url":"https://patchwork.libcamera.org/comment/14050/","msgid":"<20201204132029.djeo5brx5x5e4sy6@uno.localdomain>","date":"2020-12-04T13:20:29","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: ipa: Pass a set of\n\tcontrols and return results from ipa::start()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nuash,\n\nOn Fri, Dec 04, 2020 at 01:06:09PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> Thank you for your review feedback.\n>\n> On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi wrote:\n>\n> > Hi Naush,\n> >\n> > On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:\n> > > This change allows controls passed into PipelineHandler::start to be\n> > > forwarded onto IPAInterface::start(). We also add a return channel if the\n> > > pipeline handler must action any of these controls, e.g. setting the\n> > > analogue gain or shutter speed in the sensor device.\n> > >\n> > > todo: The IPA interface wrapper needs a translation for passing\n> > > IPAOperationData into start() and configure()\n> >\n> > This is 'problematic' as it makes isolated IPAs deviate from other\n> > ones. We have the same issue with configure() which has a very similar\n> > \\todo item :/\n> >\n> > Now, this is not an issue right now, and I think with Paul's IPC\n> > we'll get it for free.\n> >\n> > Other comments below:\n> >\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > Tested-by: David Plowman \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 | 6 ++++--\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, 50 insertions(+), 23 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n> > 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> >\n> > 'ipaConfig' is only used in configure.\n> > It's generally just called 'data' and I would use it here too.\n> >\n>\n> Sure, will fix. I will also s/ ipaConfig/data/ in all instances of start()\n> that have been changed in this patch.\n\nThanks! Other names are good to, just drop 'Config'\n\n>\n>\n> > > + IPAOperationData *result) override;\n> > > void stop() override;\n> > > void configure(const CameraSensorInfo &sensorInfo,\n> > > const std::map\n> > &streamConfig,\n> > > diff --git a/include/libcamera/ipa/ipa_interface.h\n> > 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\n> > b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > > index cee532e3..2b305b56 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\n> > *_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> > > + 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\n> > 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 {\n> > 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 {\n> > 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> > ^ is this mis-aligned or is it my email client ?\n> > There are more in this patch if that's the case.\n> >\n>\n> I think this may be an email client rendering thing. They do look correct\n> to me, and I would expect the style checker to complain about\n> misalignment. However, I will check though once again.\n>\n\nMight be, I just pointed it out as I usually see indentation off 1\nspace with my client and here I see 3. I haven't applied the patches,\nso I trust your opinion!\n\n>\n> > > {\n> > > trace(IPAOperationStart);\n> > >\n> > > diff --git a/src/libcamera/ipa_context_wrapper.cpp\n> > b/src/libcamera/ipa_context_wrapper.cpp\n> > > index 231300ce..8c4ec40a 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\n> > &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 response */\n> > > return ctx_->ops->start(ctx_);\n> > > }\n> > >\n> > > diff --git a/src/libcamera/ipa_interface.cpp\n> > 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> > These come from the 'configuration' operation\n> > I would:\n> >\n> > \\param[in] data Protocol-specific data for the start operation\n> > \\param[out] result Result of the start operation\n> >\n>\n> Fixed.\n\nThanks\n\n>\n>\n> >\n> > > *\n> > > * This method informs the IPA module that the camera is about to be\n> > 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\n> > by the\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> >\n> > Copied from configure too, replace 'configure()' at least.\n> >\n>\n> Oops, replaced with start()!\n>\n>\n> >\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index ddb30e49..29bcef07 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,\n> > [[maybe_unused]] ControlList *cont\n> > > }\n> > >\n> > > /* Start the IPA. */\n> > > - ret = data->ipa_->start();\n> > > + IPAOperationData ipaConfig = {}, result = {};\n> > > + ipaConfig.controls.emplace_back(*controls);\n> >\n> > What if controls is nullptr ?\n> >\n>\n> Hmmm.... I am a bit confused. My version of the code has:\n>\n> if (controls) {\n> ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;\n> ipaConfig.controls.emplace_back(*controls);\n> }\n>\n> But clearly my patch in the email does not :( Once I complete all your\n> suggested fixups, I will send an updated patchset!\n>\n\nThanks\n\n>\n> > > + ret = data->ipa_->start(ipaConfig, &result);\n> > > if (ret) {\n> > > LOG(RPI, Error)\n> > > << \"Failed to start IPA for \" << camera->id();\n> > > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const\n> > CameraConfiguration *config)\n> > > }\n> > >\n> > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > - IPAOperationData result;\n> > > + IPAOperationData result = {};\n> >\n> > Unrelated but I think it's ok\n> >\n>\n> This was an update requested by David in his review. Just to be safe :)\n>\n\nI agree it is an opportune fix, maybe not strictly part of this patch.\nBut it's ok :)\n\n>\n> >\n> > >\n> > > ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> > ipaConfig,\n> > > &result);\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > 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,\n> > [[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> >\n> > If you want to use a single IPAOperationData for start and configure,\n> > please rename it (here and in other pipeline handlers).\n> >\n>\n> I would use data (as with the other IPAOperationData parameters passed into\n> start), but we already have a declaration of RkISP1CameraData *data. Any\n> objections to renaming it ipaData?\n>\n\nSounds good to me!\nThanks for addressing all comments!\n\n> Regards,\n> Naush\n>\n>\n>\n> >\n> > Thanks\n> > j\n> >\n> >\n> > > if (ret) {\n> > > freeBuffers(camera);\n> > > LOG(RkISP1, Error)\n> > > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera,\n> > [[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\n> > 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,\n> > [[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\n> > 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 {\n> > return 0; }\n> > > void stop() override {}\n> > > void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,\n> > > [[maybe_unused]] const std::map > IPAStream> &streamConfig,\n> > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > 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,\n> > 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\n> > &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,\n> > ConnectionTypeBlocking);\n> > > + return proxy_.invokeMethod(&ThreadProxy::start,\n> > ConnectionTypeBlocking,\n> > > + ipaConfig, result);\n> > > }\n> > >\n> > > void IPAProxyThread::stop()\n> > > diff --git a/test/ipa/ipa_interface_test.cpp\n> > b/test/ipa/ipa_interface_test.cpp\n> > > index 67488409..03b7f0ad 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> > > + 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\n> > 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\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 EE58DBE177\n\tfor ;\n\tFri, 4 Dec 2020 13:20:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 886F4635DE;\n\tFri, 4 Dec 2020 14:20:24 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8F067635D0\n\tfor ;\n\tFri, 4 Dec 2020 14:20:23 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id EA168100002;\n\tFri, 4 Dec 2020 13:20:22 +0000 (UTC)"],"Date":"Fri, 4 Dec 2020 14:20:29 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20201204132029.djeo5brx5x5e4sy6@uno.localdomain>","References":"<20201126095126.997055-1-naush@raspberrypi.com>\n\t<20201126095126.997055-2-naush@raspberrypi.com>\n\t<20201204112620.baa4vuvd3ym2hsd5@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}},{"id":14052,"web_url":"https://patchwork.libcamera.org/comment/14052/","msgid":"","date":"2020-12-04T13:38:58","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: ipa: Pass a set of\n\tcontrols and return results from ipa::start()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\n\nOn Fri, 4 Dec 2020 at 13:20, Jacopo Mondi wrote:\n\n> Hi Nuash,\n>\n> On Fri, Dec 04, 2020 at 01:06:09PM +0000, Naushir Patuck wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for your review feedback.\n> >\n> > On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi wrote:\n> >\n> > > Hi Naush,\n> > >\n> > > On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:\n> > > > This change allows controls passed into PipelineHandler::start to be\n> > > > forwarded onto IPAInterface::start(). We also add a return channel\n> if the\n> > > > pipeline handler must action any of these controls, e.g. setting the\n> > > > analogue gain or shutter speed in the sensor device.\n> > > >\n> > > > todo: The IPA interface wrapper needs a translation for passing\n> > > > IPAOperationData into start() and configure()\n> > >\n> > > This is 'problematic' as it makes isolated IPAs deviate from other\n> > > ones. We have the same issue with configure() which has a very similar\n> > > \\todo item :/\n> > >\n> > > Now, this is not an issue right now, and I think with Paul's IPC\n> > > we'll get it for free.\n> > >\n> > > Other comments below:\n> > >\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > Tested-by: David Plowman \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 | 6 ++++--\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> ++++++++-----\n> > > > test/ipa/ipa_interface_test.cpp | 3 ++-\n> > > > test/ipa/ipa_wrappers_test.cpp | 5 +++--\n> > > > 15 files changed, 50 insertions(+), 23 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n> > > 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> > >\n> > > 'ipaConfig' is only used in configure.\n> > > It's generally just called 'data' and I would use it here too.\n> > >\n> >\n> > Sure, will fix. I will also s/ ipaConfig/data/ in all instances of\n> start()\n> > that have been changed in this patch.\n>\n> Thanks! Other names are good to, just drop 'Config'\n>\n\nCould you clarify the above comment please? I was intending to use \"data\"\nfor all IPAOperationData variables that have been introduced in the\nstart() method in this patch. Is your suggestion to use \"ipa\" instead of\n\"data\"?\n\nRegards,\nNaush\n\n\n\n>\n> >\n> >\n> > > > + IPAOperationData *result) override;\n> > > > void stop() override;\n> > > > void configure(const CameraSensorInfo &sensorInfo,\n> > > > const std::map\n> > > &streamConfig,\n> > > > diff --git a/include/libcamera/ipa/ipa_interface.h\n> > > 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\n> > > b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > > > index cee532e3..2b305b56 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\n> > > *_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> > > > + 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\n> > > 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 {\n> > > 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 {\n> > > 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\n> &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\n> &ipaConfig,\n> > > > + [[maybe_unused]] IPAOperationData *result)\n> > >\n> > > ^ is this mis-aligned or is it my email client ?\n> > > There are more in this patch if that's the case.\n> > >\n> >\n> > I think this may be an email client rendering thing. They do look\n> correct\n> > to me, and I would expect the style checker to complain about\n> > misalignment. However, I will check though once again.\n> >\n>\n> Might be, I just pointed it out as I usually see indentation off 1\n> space with my client and here I see 3. I haven't applied the patches,\n> so I trust your opinion!\n>\n> >\n> > > > {\n> > > > trace(IPAOperationStart);\n> > > >\n> > > > diff --git a/src/libcamera/ipa_context_wrapper.cpp\n> > > b/src/libcamera/ipa_context_wrapper.cpp\n> > > > index 231300ce..8c4ec40a 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\n> > > &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 response */\n> > > > return ctx_->ops->start(ctx_);\n> > > > }\n> > > >\n> > > > diff --git a/src/libcamera/ipa_interface.cpp\n> > > 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> > > These come from the 'configuration' operation\n> > > I would:\n> > >\n> > > \\param[in] data Protocol-specific data for the start operation\n> > > \\param[out] result Result of the start operation\n> > >\n> >\n> > Fixed.\n>\n> Thanks\n>\n> >\n> >\n> > >\n> > > > *\n> > > > * This method informs the IPA module that the camera is about to be\n> > > 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\n> passed\n> > > by the\n> > > > + * pipeline handler to the IPA and back. The pipeline handler may\n> set\n> > > the \\a\n> > > > + * result parameter to null if the IPA protocol doesn't need to\n> pass a\n> > > result\n> > > > + * back through the configure() function.\n> > > > + *\n> > >\n> > > Copied from configure too, replace 'configure()' at least.\n> > >\n> >\n> > Oops, replaced with start()!\n> >\n> >\n> > >\n> > > > * \\return 0 on success or a negative error code otherwise\n> > > > */\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index ddb30e49..29bcef07 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,\n> > > [[maybe_unused]] ControlList *cont\n> > > > }\n> > > >\n> > > > /* Start the IPA. */\n> > > > - ret = data->ipa_->start();\n> > > > + IPAOperationData ipaConfig = {}, result = {};\n> > > > + ipaConfig.controls.emplace_back(*controls);\n> > >\n> > > What if controls is nullptr ?\n> > >\n> >\n> > Hmmm.... I am a bit confused. My version of the code has:\n> >\n> > if (controls) {\n> > ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;\n> > ipaConfig.controls.emplace_back(*controls);\n> > }\n> >\n> > But clearly my patch in the email does not :( Once I complete all your\n> > suggested fixups, I will send an updated patchset!\n> >\n>\n> Thanks\n>\n> >\n> > > > + ret = data->ipa_->start(ipaConfig, &result);\n> > > > if (ret) {\n> > > > LOG(RPI, Error)\n> > > > << \"Failed to start IPA for \" << camera->id();\n> > > > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const\n> > > CameraConfiguration *config)\n> > > > }\n> > > >\n> > > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > > - IPAOperationData result;\n> > > > + IPAOperationData result = {};\n> > >\n> > > Unrelated but I think it's ok\n> > >\n> >\n> > This was an update requested by David in his review. Just to be safe :)\n> >\n>\n> I agree it is an opportune fix, maybe not strictly part of this patch.\n> But it's ok :)\n>\n> >\n> > >\n> > > >\n> > > > ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> > > ipaConfig,\n> > > > &result);\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > 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,\n> > > [[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> > >\n> > > If you want to use a single IPAOperationData for start and configure,\n> > > please rename it (here and in other pipeline handlers).\n> > >\n> >\n> > I would use data (as with the other IPAOperationData parameters passed\n> into\n> > start), but we already have a declaration of RkISP1CameraData *data. Any\n> > objections to renaming it ipaData?\n> >\n>\n> Sounds good to me!\n> Thanks for addressing all comments!\n>\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > >\n> > > > if (ret) {\n> > > > freeBuffers(camera);\n> > > > LOG(RkISP1, Error)\n> > > > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera,\n> > > [[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\n> > > 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,\n> > > [[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\n> > > 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 {\n> > > return 0; }\n> > > > void stop() override {}\n> > > > void configure([[maybe_unused]] const CameraSensorInfo\n> &sensorInfo,\n> > > > [[maybe_unused]] const std::map > > IPAStream> &streamConfig,\n> > > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > > 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,\n> > > 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\n> > > &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,\n> > > ConnectionTypeBlocking);\n> > > > + return proxy_.invokeMethod(&ThreadProxy::start,\n> > > ConnectionTypeBlocking,\n> > > > + ipaConfig, result);\n> > > > }\n> > > >\n> > > > void IPAProxyThread::stop()\n> > > > diff --git a/test/ipa/ipa_interface_test.cpp\n> > > b/test/ipa/ipa_interface_test.cpp\n> > > > index 67488409..03b7f0ad 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> > > > + ipa_->start(ipaConfig, nullptr);\n> > > > timer.start(1000);\n> > > > while (timer.isRunning() && trace_ !=\n> IPAOperationStart)\n> > > > dispatcher->processEvents();\n> > > > diff --git a/test/ipa/ipa_wrappers_test.cpp\n> > > 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\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 3E42EBE177\n\tfor ;\n\tFri, 4 Dec 2020 13:39:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1C2E635EF;\n\tFri, 4 Dec 2020 14:39:17 +0100 (CET)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F66A635D0\n\tfor ;\n\tFri, 4 Dec 2020 14:39:16 +0100 (CET)","by mail-lj1-x236.google.com with SMTP id t22so6689206ljk.0\n\tfor ;\n\tFri, 04 Dec 2020 05:39:16 -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=\"SP8dQwU9\"; 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=/O4Q+8JDeo7S6Rf9fuccOGnSJah/AHz+uNkZv/tLGc4=;\n\tb=SP8dQwU9eO6yC3r0AdEWdTiuF6KcZuTZCwHznaZWR2Rt2d5SK1dwZ2JlgzZZ3f4p/z\n\tGXYbpS3ddarog1kXd8mLF+WfN8Jn1gwSy8B8FL1e31WmfbyAdwEs4jK2KB8W38B9EKWS\n\t5SiqbsWeD1GNVd7jkbFdBWs7fnf4fdY3zLxceIR/DO2yZqN4w/CPfxWFgDDjKtPLFQsB\n\tsUFoIPumoBG5V438R3OH2YrnnVjD41vxyn3EAelsT970luz4Qzj6eQ3tfeHoT9YDN/tp\n\tWp9TpylzfoAb72i53/4EjTmV4fXGHCEgCrE+7Vu60kC1UQRnvUSBoHYu5azgVyYnZw7G\n\t/wWA==","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=/O4Q+8JDeo7S6Rf9fuccOGnSJah/AHz+uNkZv/tLGc4=;\n\tb=DBO6ILx2ck8vV/hmTIiLbEHh6VOq9ybt5C6UqecwYTqcOYVPkvmD8+oeRZRnZ6NhFI\n\t5qQfX39hR0f7w7tV8w076MtHuhRfXeph4/gLUiIwjux7baMg0GDHRVT4ZvzNoZsqRS7+\n\ta+Dpt1FYeVJh6DiXxf+cjGHnCDrW9+DaMeTraWlkVEmMotwum/zZYTdXLWMxYr5xOkJW\n\tXzqJGHVz8LjxDjcyYAhcTZWccxYV4lUR+zMscDzfr65Zt0UnOdC1OOYPxkjHc0rfzbaA\n\tEA68q2DakGYTUfK9NqtIYzQw/R0TL2nERVu0cKu/cLc8mWDnuacf6L25D8u+6pOiZngW\n\tzWaw==","X-Gm-Message-State":"AOAM532NDnYPEzN2cqd90bxf8vCT+HNtZfY/wGiPktTzj2CdkpAsSAJ5\n\td8fbHjLmZnNqhcEjXAychrHXRHtUSMub9f3cRTtPgwoE2sapiA==","X-Google-Smtp-Source":"ABdhPJybBPmAPkVDWbbgfI4gjQp3L0XUccKdwP2RzVQGEMNC0c7yel5I0xuRC+A4NUz96v0H3g4xMGA59VAoqaCSrv0=","X-Received":"by 2002:a2e:80c6:: with SMTP id r6mr3439907ljg.83.1607089155597; \n\tFri, 04 Dec 2020 05:39:15 -0800 (PST)","MIME-Version":"1.0","References":"<20201126095126.997055-1-naush@raspberrypi.com>\n\t<20201126095126.997055-2-naush@raspberrypi.com>\n\t<20201204112620.baa4vuvd3ym2hsd5@uno.localdomain>\n\t\n\t<20201204132029.djeo5brx5x5e4sy6@uno.localdomain>","In-Reply-To":"<20201204132029.djeo5brx5x5e4sy6@uno.localdomain>","From":"Naushir Patuck ","Date":"Fri, 4 Dec 2020 13:38:58 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 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":"multipart/mixed;\n\tboundary=\"===============5447910297655009005==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14053,"web_url":"https://patchwork.libcamera.org/comment/14053/","msgid":"<20201204135221.adiczcvewp2o5ihw@uno.localdomain>","date":"2020-12-04T13:52:21","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: ipa: Pass a set of\n\tcontrols and return results from ipa::start()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Fri, Dec 04, 2020 at 01:38:58PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n>\n> On Fri, 4 Dec 2020 at 13:20, Jacopo Mondi wrote:\n>\n> > Hi Nuash,\n> >\n> > On Fri, Dec 04, 2020 at 01:06:09PM +0000, Naushir Patuck wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thank you for your review feedback.\n> > >\n> > > On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi wrote:\n> > >\n> > > > Hi Naush,\n> > > >\n> > > > On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:\n> > > > > This change allows controls passed into PipelineHandler::start to be\n> > > > > forwarded onto IPAInterface::start(). We also add a return channel\n> > if the\n> > > > > pipeline handler must action any of these controls, e.g. setting the\n> > > > > analogue gain or shutter speed in the sensor device.\n> > > > >\n> > > > > todo: The IPA interface wrapper needs a translation for passing\n> > > > > IPAOperationData into start() and configure()\n> > > >\n> > > > This is 'problematic' as it makes isolated IPAs deviate from other\n> > > > ones. We have the same issue with configure() which has a very similar\n> > > > \\todo item :/\n> > > >\n> > > > Now, this is not an issue right now, and I think with Paul's IPC\n> > > > we'll get it for free.\n> > > >\n> > > > Other comments below:\n> > > >\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > Reviewed-by: David Plowman \n> > > > > Tested-by: David Plowman \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 | 6 ++++--\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> > ++++++++-----\n> > > > > test/ipa/ipa_interface_test.cpp | 3 ++-\n> > > > > test/ipa/ipa_wrappers_test.cpp | 5 +++--\n> > > > > 15 files changed, 50 insertions(+), 23 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n> > > > 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> > > >\n> > > > 'ipaConfig' is only used in configure.\n> > > > It's generally just called 'data' and I would use it here too.\n> > > >\n> > >\n> > > Sure, will fix. I will also s/ ipaConfig/data/ in all instances of\n> > start()\n> > > that have been changed in this patch.\n> >\n> > Thanks! Other names are good to, just drop 'Config'\n> >\n>\n> Could you clarify the above comment please? I was intending to use \"data\"\n> for all IPAOperationData variables that have been introduced in the\n> start() method in this patch. Is your suggestion to use \"ipa\" instead of\n> \"data\"?\n\nSorry for the confusion. What I meant was \"If you have other (better)\nnames than 'data', that's also fine. What presses me is not to have\n'Config' in the name\"\n\n>\n> Regards,\n> Naush\n>\n>\n>\n> >\n> > >\n> > >\n> > > > > + IPAOperationData *result) override;\n> > > > > void stop() override;\n> > > > > void configure(const CameraSensorInfo &sensorInfo,\n> > > > > const std::map\n> > > > &streamConfig,\n> > > > > diff --git a/include/libcamera/ipa/ipa_interface.h\n> > > > 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\n> > > > b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > > > > index cee532e3..2b305b56 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\n> > > > *_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> > > > > + 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\n> > > > 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 {\n> > > > 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 {\n> > > > 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\n> > &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\n> > &ipaConfig,\n> > > > > + [[maybe_unused]] IPAOperationData *result)\n> > > >\n> > > > ^ is this mis-aligned or is it my email client ?\n> > > > There are more in this patch if that's the case.\n> > > >\n> > >\n> > > I think this may be an email client rendering thing. They do look\n> > correct\n> > > to me, and I would expect the style checker to complain about\n> > > misalignment. However, I will check though once again.\n> > >\n> >\n> > Might be, I just pointed it out as I usually see indentation off 1\n> > space with my client and here I see 3. I haven't applied the patches,\n> > so I trust your opinion!\n> >\n> > >\n> > > > > {\n> > > > > trace(IPAOperationStart);\n> > > > >\n> > > > > diff --git a/src/libcamera/ipa_context_wrapper.cpp\n> > > > b/src/libcamera/ipa_context_wrapper.cpp\n> > > > > index 231300ce..8c4ec40a 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\n> > > > &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 response */\n> > > > > return ctx_->ops->start(ctx_);\n> > > > > }\n> > > > >\n> > > > > diff --git a/src/libcamera/ipa_interface.cpp\n> > > > 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> > > > These come from the 'configuration' operation\n> > > > I would:\n> > > >\n> > > > \\param[in] data Protocol-specific data for the start operation\n> > > > \\param[out] result Result of the start operation\n> > > >\n> > >\n> > > Fixed.\n> >\n> > Thanks\n> >\n> > >\n> > >\n> > > >\n> > > > > *\n> > > > > * This method informs the IPA module that the camera is about to be\n> > > > 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\n> > passed\n> > > > by the\n> > > > > + * pipeline handler to the IPA and back. The pipeline handler may\n> > set\n> > > > the \\a\n> > > > > + * result parameter to null if the IPA protocol doesn't need to\n> > pass a\n> > > > result\n> > > > > + * back through the configure() function.\n> > > > > + *\n> > > >\n> > > > Copied from configure too, replace 'configure()' at least.\n> > > >\n> > >\n> > > Oops, replaced with start()!\n> > >\n> > >\n> > > >\n> > > > > * \\return 0 on success or a negative error code otherwise\n> > > > > */\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index ddb30e49..29bcef07 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,\n> > > > [[maybe_unused]] ControlList *cont\n> > > > > }\n> > > > >\n> > > > > /* Start the IPA. */\n> > > > > - ret = data->ipa_->start();\n> > > > > + IPAOperationData ipaConfig = {}, result = {};\n> > > > > + ipaConfig.controls.emplace_back(*controls);\n> > > >\n> > > > What if controls is nullptr ?\n> > > >\n> > >\n> > > Hmmm.... I am a bit confused. My version of the code has:\n> > >\n> > > if (controls) {\n> > > ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;\n> > > ipaConfig.controls.emplace_back(*controls);\n> > > }\n> > >\n> > > But clearly my patch in the email does not :( Once I complete all your\n> > > suggested fixups, I will send an updated patchset!\n> > >\n> >\n> > Thanks\n> >\n> > >\n> > > > > + ret = data->ipa_->start(ipaConfig, &result);\n> > > > > if (ret) {\n> > > > > LOG(RPI, Error)\n> > > > > << \"Failed to start IPA for \" << camera->id();\n> > > > > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const\n> > > > CameraConfiguration *config)\n> > > > > }\n> > > > >\n> > > > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > > > - IPAOperationData result;\n> > > > > + IPAOperationData result = {};\n> > > >\n> > > > Unrelated but I think it's ok\n> > > >\n> > >\n> > > This was an update requested by David in his review. Just to be safe :)\n> > >\n> >\n> > I agree it is an opportune fix, maybe not strictly part of this patch.\n> > But it's ok :)\n> >\n> > >\n> > > >\n> > > > >\n> > > > > ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> > > > ipaConfig,\n> > > > > &result);\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > 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,\n> > > > [[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> > > >\n> > > > If you want to use a single IPAOperationData for start and configure,\n> > > > please rename it (here and in other pipeline handlers).\n> > > >\n> > >\n> > > I would use data (as with the other IPAOperationData parameters passed\n> > into\n> > > start), but we already have a declaration of RkISP1CameraData *data. Any\n> > > objections to renaming it ipaData?\n> > >\n> >\n> > Sounds good to me!\n> > Thanks for addressing all comments!\n> >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > >\n> > > > > if (ret) {\n> > > > > freeBuffers(camera);\n> > > > > LOG(RkISP1, Error)\n> > > > > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera,\n> > > > [[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\n> > > > 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,\n> > > > [[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\n> > > > 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 {\n> > > > return 0; }\n> > > > > void stop() override {}\n> > > > > void configure([[maybe_unused]] const CameraSensorInfo\n> > &sensorInfo,\n> > > > > [[maybe_unused]] const std::map > > > IPAStream> &streamConfig,\n> > > > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > > > 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,\n> > > > 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\n> > > > &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,\n> > > > ConnectionTypeBlocking);\n> > > > > + return proxy_.invokeMethod(&ThreadProxy::start,\n> > > > ConnectionTypeBlocking,\n> > > > > + ipaConfig, result);\n> > > > > }\n> > > > >\n> > > > > void IPAProxyThread::stop()\n> > > > > diff --git a/test/ipa/ipa_interface_test.cpp\n> > > > b/test/ipa/ipa_interface_test.cpp\n> > > > > index 67488409..03b7f0ad 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> > > > > + ipa_->start(ipaConfig, nullptr);\n> > > > > timer.start(1000);\n> > > > > while (timer.isRunning() && trace_ !=\n> > IPAOperationStart)\n> > > > > dispatcher->processEvents();\n> > > > > diff --git a/test/ipa/ipa_wrappers_test.cpp\n> > > > 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\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 8A9F9BE176\n\tfor ;\n\tFri, 4 Dec 2020 13:52:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C5AF635D6;\n\tFri, 4 Dec 2020 14:52:16 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9570635D0\n\tfor ;\n\tFri, 4 Dec 2020 14:52:14 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 79E69240005;\n\tFri, 4 Dec 2020 13:52:14 +0000 (UTC)"],"Date":"Fri, 4 Dec 2020 14:52:21 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20201204135221.adiczcvewp2o5ihw@uno.localdomain>","References":"<20201126095126.997055-1-naush@raspberrypi.com>\n\t<20201126095126.997055-2-naush@raspberrypi.com>\n\t<20201204112620.baa4vuvd3ym2hsd5@uno.localdomain>\n\t\n\t<20201204132029.djeo5brx5x5e4sy6@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}},{"id":14054,"web_url":"https://patchwork.libcamera.org/comment/14054/","msgid":"","date":"2020-12-04T14:07:30","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: ipa: Pass a set of\n\tcontrols and return results from ipa::start()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Fri, 4 Dec 2020 at 13:52, Jacopo Mondi wrote:\n\n> Hi Naush,\n>\n> On Fri, Dec 04, 2020 at 01:38:58PM +0000, Naushir Patuck wrote:\n> > Hi Jacopo,\n> >\n> >\n> > On Fri, 4 Dec 2020 at 13:20, Jacopo Mondi wrote:\n> >\n> > > Hi Nuash,\n> > >\n> > > On Fri, Dec 04, 2020 at 01:06:09PM +0000, Naushir Patuck wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thank you for your review feedback.\n> > > >\n> > > > On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi wrote:\n> > > >\n> > > > > Hi Naush,\n> > > > >\n> > > > > On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:\n> > > > > > This change allows controls passed into PipelineHandler::start\n> to be\n> > > > > > forwarded onto IPAInterface::start(). We also add a return\n> channel\n> > > if the\n> > > > > > pipeline handler must action any of these controls, e.g. setting\n> the\n> > > > > > analogue gain or shutter speed in the sensor device.\n> > > > > >\n> > > > > > todo: The IPA interface wrapper needs a translation for passing\n> > > > > > IPAOperationData into start() and configure()\n> > > > >\n> > > > > This is 'problematic' as it makes isolated IPAs deviate from other\n> > > > > ones. We have the same issue with configure() which has a very\n> similar\n> > > > > \\todo item :/\n> > > > >\n> > > > > Now, this is not an issue right now, and I think with Paul's IPC\n> > > > > we'll get it for free.\n> > > > >\n> > > > > Other comments below:\n> > > > >\n> > > > > >\n> > > > > > Signed-off-by: Naushir Patuck \n> > > > > > Reviewed-by: David Plowman \n> > > > > > Tested-by: David Plowman \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 | 6 ++++--\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> > > ++++++++-----\n> > > > > > test/ipa/ipa_interface_test.cpp | 3 ++-\n> > > > > > test/ipa/ipa_wrappers_test.cpp | 5 +++--\n> > > > > > 15 files changed, 50 insertions(+), 23 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h\n> > > > > 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> > > > >\n> > > > > 'ipaConfig' is only used in configure.\n> > > > > It's generally just called 'data' and I would use it here too.\n> > > > >\n> > > >\n> > > > Sure, will fix. I will also s/ ipaConfig/data/ in all instances of\n> > > start()\n> > > > that have been changed in this patch.\n> > >\n> > > Thanks! Other names are good to, just drop 'Config'\n> > >\n> >\n> > Could you clarify the above comment please? I was intending to use\n> \"data\"\n> > for all IPAOperationData variables that have been introduced in the\n> > start() method in this patch. Is your suggestion to use \"ipa\" instead of\n> > \"data\"?\n>\n> Sorry for the confusion. What I meant was \"If you have other (better)\n> names than 'data', that's also fine. What presses me is not to have\n> 'Config' in the name\"\n>\n\nGot it, thanks for clarifying!\n\n\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > >\n> > > >\n> > > >\n> > > > > > + IPAOperationData *result) override;\n> > > > > > void stop() override;\n> > > > > > void configure(const CameraSensorInfo &sensorInfo,\n> > > > > > const std::map\n> > > > > &streamConfig,\n> > > > > > diff --git a/include/libcamera/ipa/ipa_interface.h\n> > > > > 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\n> > > > > b/src/ipa/libipa/ipa_interface_wrapper.cpp\n> > > > > > index cee532e3..2b305b56 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\n> ipa_context\n> > > > > *_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> > > > > > + 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\n> > > > > 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\n> &ipaConfig,\n> > > > > > + [[maybe_unused]] IPAOperationData *result)\n> override {\n> > > > > return 0; }\n> > > > > > void stop() override {}\n> > > > > >\n> > > > > > void configure(const CameraSensorInfo &sensorInfo,\n> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp\n> 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\n> &ipaConfig,\n> > > > > > + [[maybe_unused]] IPAOperationData *result)\n> override {\n> > > > > 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\n> > > &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\n> > > &ipaConfig,\n> > > > > > + [[maybe_unused]] IPAOperationData *result)\n> > > > >\n> > > > > ^ is this mis-aligned or is it my email client ?\n> > > > > There are more in this patch if that's the case.\n> > > > >\n> > > >\n> > > > I think this may be an email client rendering thing. They do look\n> > > correct\n> > > > to me, and I would expect the style checker to complain about\n> > > > misalignment. However, I will check though once again.\n> > > >\n> > >\n> > > Might be, I just pointed it out as I usually see indentation off 1\n> > > space with my client and here I see 3. I haven't applied the patches,\n> > > so I trust your opinion!\n> > >\n> > > >\n> > > > > > {\n> > > > > > trace(IPAOperationStart);\n> > > > > >\n> > > > > > diff --git a/src/libcamera/ipa_context_wrapper.cpp\n> > > > > b/src/libcamera/ipa_context_wrapper.cpp\n> > > > > > index 231300ce..8c4ec40a 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\n> > > > > &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 response */\n> > > > > > return ctx_->ops->start(ctx_);\n> > > > > > }\n> > > > > >\n> > > > > > diff --git a/src/libcamera/ipa_interface.cpp\n> > > > > 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\n> data\n> > > > > > + * \\param[out] result Pipeline-handler-specific configuration\n> result\n> > > > >\n> > > > > These come from the 'configuration' operation\n> > > > > I would:\n> > > > >\n> > > > > \\param[in] data Protocol-specific data for the start operation\n> > > > > \\param[out] result Result of the start operation\n> > > > >\n> > > >\n> > > > Fixed.\n> > >\n> > > Thanks\n> > >\n> > > >\n> > > >\n> > > > >\n> > > > > > *\n> > > > > > * This method informs the IPA module that the camera is about\n> to be\n> > > > > started.\n> > > > > > * The IPA module shall prepare any resources it needs to\n> operate.\n> > > > > > *\n> > > > > > + * The \\a ipaConfig and \\a result parameters carry custom data\n> > > passed\n> > > > > by the\n> > > > > > + * pipeline handler to the IPA and back. The pipeline handler\n> may\n> > > set\n> > > > > the \\a\n> > > > > > + * result parameter to null if the IPA protocol doesn't need to\n> > > pass a\n> > > > > result\n> > > > > > + * back through the configure() function.\n> > > > > > + *\n> > > > >\n> > > > > Copied from configure too, replace 'configure()' at least.\n> > > > >\n> > > >\n> > > > Oops, replaced with start()!\n> > > >\n> > > >\n> > > > >\n> > > > > > * \\return 0 on success or a negative error code otherwise\n> > > > > > */\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > index ddb30e49..29bcef07 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,\n> > > > > [[maybe_unused]] ControlList *cont\n> > > > > > }\n> > > > > >\n> > > > > > /* Start the IPA. */\n> > > > > > - ret = data->ipa_->start();\n> > > > > > + IPAOperationData ipaConfig = {}, result = {};\n> > > > > > + ipaConfig.controls.emplace_back(*controls);\n> > > > >\n> > > > > What if controls is nullptr ?\n> > > > >\n> > > >\n> > > > Hmmm.... I am a bit confused. My version of the code has:\n> > > >\n> > > > if (controls) {\n> > > > ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;\n> > > > ipaConfig.controls.emplace_back(*controls);\n> > > > }\n> > > >\n> > > > But clearly my patch in the email does not :( Once I complete all\n> your\n> > > > suggested fixups, I will send an updated patchset!\n> > > >\n> > >\n> > > Thanks\n> > >\n> > > >\n> > > > > > + ret = data->ipa_->start(ipaConfig, &result);\n> > > > > > if (ret) {\n> > > > > > LOG(RPI, Error)\n> > > > > > << \"Failed to start IPA for \" <<\n> camera->id();\n> > > > > > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const\n> > > > > CameraConfiguration *config)\n> > > > > > }\n> > > > > >\n> > > > > > /* Ready the IPA - it must know about the sensor\n> resolution. */\n> > > > > > - IPAOperationData result;\n> > > > > > + IPAOperationData result = {};\n> > > > >\n> > > > > Unrelated but I think it's ok\n> > > > >\n> > > >\n> > > > This was an update requested by David in his review. Just to be\n> safe :)\n> > > >\n> > >\n> > > I agree it is an opportune fix, maybe not strictly part of this patch.\n> > > But it's ok :)\n> > >\n> > > >\n> > > > >\n> > > > > >\n> > > > > > ipa_->configure(sensorInfo_, streamConfig, entityControls,\n> > > > > ipaConfig,\n> > > > > > &result);\n> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > 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\n> *camera,\n> > > > > [[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> > > > >\n> > > > > If you want to use a single IPAOperationData for start and\n> configure,\n> > > > > please rename it (here and in other pipeline handlers).\n> > > > >\n> > > >\n> > > > I would use data (as with the other IPAOperationData parameters\n> passed\n> > > into\n> > > > start), but we already have a declaration of RkISP1CameraData\n> *data. Any\n> > > > objections to renaming it ipaData?\n> > > >\n> > >\n> > > Sounds good to me!\n> > > Thanks for addressing all comments!\n> > >\n> > > > Regards,\n> > > > Naush\n> > > >\n> > > >\n> > > >\n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > >\n> > > > > > if (ret) {\n> > > > > > freeBuffers(camera);\n> > > > > > LOG(RkISP1, Error)\n> > > > > > @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera\n> *camera,\n> > > > > [[maybe_unused]] ControlList *c\n> > > > > > std::map\n> entityControls;\n> > > > > > entityControls.emplace(0, data->sensor_->controls());\n> > > > > >\n> > > > > > - IPAOperationData ipaConfig;\n> > > > > > + ipaConfig = {};\n> > > > > > data->ipa_->configure(sensorInfo, streamConfig,\n> entityControls,\n> > > > > > ipaConfig, nullptr);\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > 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\n> *camera,\n> > > > > [[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\n> > > > > 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\n> &ipaConfig,\n> > > > > > + [[maybe_unused]] IPAOperationData *result)\n> override {\n> > > > > return 0; }\n> > > > > > void stop() override {}\n> > > > > > void configure([[maybe_unused]] const CameraSensorInfo\n> > > &sensorInfo,\n> > > > > > [[maybe_unused]] const std::map int,\n> > > > > IPAStream> &streamConfig,\n> > > > > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp\n> > > > > 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,\n> > > > > 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\n> > > > > &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,\n> > > > > ConnectionTypeBlocking);\n> > > > > > + return proxy_.invokeMethod(&ThreadProxy::start,\n> > > > > ConnectionTypeBlocking,\n> > > > > > + ipaConfig, result);\n> > > > > > }\n> > > > > >\n> > > > > > void IPAProxyThread::stop()\n> > > > > > diff --git a/test/ipa/ipa_interface_test.cpp\n> > > > > b/test/ipa/ipa_interface_test.cpp\n> > > > > > index 67488409..03b7f0ad 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> > > > > > + ipa_->start(ipaConfig, nullptr);\n> > > > > > timer.start(1000);\n> > > > > > while (timer.isRunning() && trace_ !=\n> > > IPAOperationStart)\n> > > > > > dispatcher->processEvents();\n> > > > > > diff --git a/test/ipa/ipa_wrappers_test.cpp\n> > > > > 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\n> &ipaConfig,\n> > > > > > + [[maybe_unused]] IPAOperationData *result)\n> 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\n> > > > >\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 003D5BE177\n\tfor ;\n\tFri, 4 Dec 2020 14:07:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 81B6D635DC;\n\tFri, 4 Dec 2020 15:07:50 +0100 (CET)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 826F6635D0\n\tfor ;\n\tFri, 4 Dec 2020 15:07:48 +0100 (CET)","by mail-lj1-x22e.google.com with SMTP id f18so6734169ljg.9\n\tfor ;\n\tFri, 04 Dec 2020 06:07:48 -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=\"M3WtJdg3\"; 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=AkbPODKjnOK3tZDhRf3fzJ7UfA90ZdQ10aPzcSFuJfU=;\n\tb=M3WtJdg3F7laY84QDpyYMKr3k/eRUwar/Vs0GwE8DPr0r4lkqPLuqgnh3TLWjf56Rc\n\tJRv3Mh3jIeZMqpHIyLSf69I9q3t+4J1PP0lWiGEP1CVq+6VvQYYJ5cnUC4ZH4YCD+bC2\n\toIm+yTiX42zFc8G1Ruhx5vitzOJPdR9/Hsl69pN1HyryOmRAV6TEZyD3dmgvAbDxaMyn\n\tr4mGVTfs+vcQgOPu+oY4dADutcgepQFp/qMukA5PjYD0zkxIvw7zvRVOZmHtrL+mcmpX\n\t8xcjhstV7cp3FzPviyLOx50iwKzDsRcz8yZsGjX3zvNxq4SezIriG9NSiy5XTdegBUSI\n\t/jdA==","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=AkbPODKjnOK3tZDhRf3fzJ7UfA90ZdQ10aPzcSFuJfU=;\n\tb=QTRirA8WkeI5lygPx6qA3NJI93TGxupG1kq6KMhLOTn159ELLZ30JBE+2yyggogDv7\n\tJnijsAtzLo70tB5sOHwS5NvAJgm/z3gLVpGsEfjVvM9Vizq76A4OwC0bYG2k6KXGw0Tp\n\tj7h0hqR9wbrmFR0rmYpQsIdrXor4XcIJEzQ45uhoKPhWAHAAZr8sBS6o7XI0j26wOrDk\n\tLnLup6nPB1Bouotk0gsQTWpWakj2N65DPlO2pD73jHYV9xzrFHb1Qpmab5X40OIJwiN+\n\tMtN1MC/WRItF4S1meqQD3ZPSnhtpB5bLogkSUwRlb6OfVvnASqQ0chTdl+rj9pY0U9AH\n\tj/lA==","X-Gm-Message-State":"AOAM533o4vkKtMemDOnHvcjEE3B879UZmikNuXy9E3SGVbRZN1YESnl1\n\tBa39fppDntMQPm7FncSGcISAAaBIl7unXvI3VE0n8Q==","X-Google-Smtp-Source":"ABdhPJyBduaFY0gQ7azVIINnpD1GvEX5Hiu/ozLdI65MPGxzFJ79gs7xFG291WXjEKrPIfaQYnn2uPvikvk259lP7pc=","X-Received":"by 2002:a2e:a58e:: with SMTP id m14mr3378034ljp.1.1607090867407; \n\tFri, 04 Dec 2020 06:07:47 -0800 (PST)","MIME-Version":"1.0","References":"<20201126095126.997055-1-naush@raspberrypi.com>\n\t<20201126095126.997055-2-naush@raspberrypi.com>\n\t<20201204112620.baa4vuvd3ym2hsd5@uno.localdomain>\n\t\n\t<20201204132029.djeo5brx5x5e4sy6@uno.localdomain>\n\t\n\t<20201204135221.adiczcvewp2o5ihw@uno.localdomain>","In-Reply-To":"<20201204135221.adiczcvewp2o5ihw@uno.localdomain>","From":"Naushir Patuck ","Date":"Fri, 4 Dec 2020 14:07:30 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 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":"multipart/mixed;\n\tboundary=\"===============6261121614207692324==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]