<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your comments.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 17 Nov 2020 at 16:35, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
Thanks for the patch. Only some minor nitpicks...<br>
<br>
On Thu, 12 Nov 2020 at 08:59, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> This change allows controls passed into pipeline_hanlder::start to be<br>
> forwarded onto ipa::start(). We also add a return channel if the<br>
> pipeline handle must action any of these controls, e.g. setting the<br>
> analogue gain or shutter speed in the sensor device.<br>
<br>
s/hanlder/handler/ (though strictly the class is PipelineHandler)<br>
<br>
Is IPAContextWrapper::start more accurate, instead of ipa::start?<br>
<br>
s/pipeline handle/pipeline handler/<br>
<br>
(sorry!)<br></blockquote><div><br></div><div>Ack.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> todo: The IPA interface wrapper needs a translation for passing<br>
> IPAOperationData into start() and configure()<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/internal/ipa_context_wrapper.h | 3 ++-<br>
> include/libcamera/ipa/ipa_interface.h | 3 ++-<br>
> src/ipa/libipa/ipa_interface_wrapper.cpp | 4 +++-<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 3 ++-<br>
> src/ipa/rkisp1/rkisp1.cpp | 3 ++-<br>
> src/ipa/vimc/vimc.cpp | 6 ++++--<br>
> src/libcamera/ipa_context_wrapper.cpp | 6 ++++--<br>
> src/libcamera/ipa_interface.cpp | 7 +++++++<br>
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 +++-<br>
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++--<br>
> src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-<br>
> src/libcamera/proxy/ipa_proxy_linux.cpp | 3 ++-<br>
> src/libcamera/proxy/ipa_proxy_thread.cpp | 13 ++++++++-----<br>
> test/ipa/ipa_interface_test.cpp | 3 ++-<br>
> test/ipa/ipa_wrappers_test.cpp | 5 +++--<br>
> 15 files changed, 49 insertions(+), 22 deletions(-)<br>
><br>
> diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h<br>
> index 8f767e84..1878a615 100644<br>
> --- a/include/libcamera/internal/ipa_context_wrapper.h<br>
> +++ b/include/libcamera/internal/ipa_context_wrapper.h<br>
> @@ -20,7 +20,8 @@ public:<br>
> ~IPAContextWrapper();<br>
><br>
> int init(const IPASettings &settings) override;<br>
> - int start() override;<br>
> + int start(const IPAOperationData &ipaConfig,<br>
> + IPAOperationData *result) override;<br>
> void stop() override;<br>
> void configure(const CameraSensorInfo &sensorInfo,<br>
> const std::map<unsigned int, IPAStream> &streamConfig,<br>
> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h<br>
> index 322b7079..b44e2538 100644<br>
> --- a/include/libcamera/ipa/ipa_interface.h<br>
> +++ b/include/libcamera/ipa/ipa_interface.h<br>
> @@ -153,7 +153,8 @@ public:<br>
> virtual ~IPAInterface() = default;<br>
><br>
> virtual int init(const IPASettings &settings) = 0;<br>
> - virtual int start() = 0;<br>
> + virtual int start(const IPAOperationData &ipaConfig,<br>
> + IPAOperationData *result) = 0;<br>
> virtual void stop() = 0;<br>
><br>
> virtual void configure(const CameraSensorInfo &sensorInfo,<br>
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp<br>
> index cee532e3..fb2da9d3 100644<br>
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp<br>
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp<br>
> @@ -95,7 +95,9 @@ int IPAInterfaceWrapper::start(struct ipa_context *_ctx)<br>
> {<br>
> IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);<br>
><br>
> - return ctx->ipa_->start();<br>
> + /* \todo Translate the ipaConfig and result. */<br>
> + IPAOperationData ipaConfig;<br>
<br>
In other places (though not always) you've tended to add "= {}" to the<br>
ipaConfig object. As far as I can see, it's only the "operation" field<br>
that might be uninitialised. Does this matter?<br></blockquote><div><br></div><div>It does not matter *now*, but things can change under us, and I really should initialise it with = {} to ensure the .operation field is cleared. Will fix this one.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + return ctx->ipa_->start(ipaConfig, nullptr);<br>
> }<br>
><br>
> void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index f338ff8b..7a07b477 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -77,7 +77,8 @@ public:<br>
> }<br>
><br>
> int init(const IPASettings &settings) override;<br>
> - int start() override { return 0; }<br>
> + int start([[maybe_unused]] const IPAOperationData &ipaConfig,<br>
> + [[maybe_unused]] IPAOperationData *result) override { return 0; }<br>
> void stop() override {}<br>
><br>
> void configure(const CameraSensorInfo &sensorInfo,<br>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp<br>
> index 07d7f1b2..0b8a9096 100644<br>
> --- a/src/ipa/rkisp1/rkisp1.cpp<br>
> +++ b/src/ipa/rkisp1/rkisp1.cpp<br>
> @@ -37,7 +37,8 @@ public:<br>
> {<br>
> return 0;<br>
> }<br>
> - int start() override { return 0; }<br>
> + int start([[maybe_unused]] const IPAOperationData &ipaConfig,<br>
> + [[maybe_unused]] IPAOperationData *result) override { return 0; }<br>
> void stop() override {}<br>
><br>
> void configure(const CameraSensorInfo &info,<br>
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp<br>
> index cf841135..ed26331d 100644<br>
> --- a/src/ipa/vimc/vimc.cpp<br>
> +++ b/src/ipa/vimc/vimc.cpp<br>
> @@ -34,7 +34,8 @@ public:<br>
><br>
> int init(const IPASettings &settings) override;<br>
><br>
> - int start() override;<br>
> + int start(const IPAOperationData &ipaConfig,<br>
> + IPAOperationData *result) override;<br>
> void stop() override;<br>
><br>
> void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,<br>
> @@ -82,7 +83,8 @@ int IPAVimc::init(const IPASettings &settings)<br>
> return 0;<br>
> }<br>
><br>
> -int IPAVimc::start()<br>
> +int IPAVimc::start([[maybe_unused]] const IPAOperationData &ipaConfig,<br>
> + [[maybe_unused]] IPAOperationData *result)<br>
> {<br>
> trace(IPAOperationStart);<br>
><br>
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp<br>
> index 231300ce..de96a606 100644<br>
> --- a/src/libcamera/ipa_context_wrapper.cpp<br>
> +++ b/src/libcamera/ipa_context_wrapper.cpp<br>
> @@ -86,14 +86,16 @@ int IPAContextWrapper::init(const IPASettings &settings)<br>
> return 0;<br>
> }<br>
><br>
> -int IPAContextWrapper::start()<br>
> +int IPAContextWrapper::start(const IPAOperationData &ipaConfig,<br>
> + IPAOperationData *result)<br>
> {<br>
> if (intf_)<br>
> - return intf_->start();<br>
> + return intf_->start(ipaConfig, result);<br>
><br>
> if (!ctx_)<br>
> return 0;<br>
><br>
> + /* \todo Translate the ipaConfig and reponse */<br>
> return ctx_->ops->start(ctx_);<br>
> }<br>
<br>
s/reponse/response/<br></blockquote><div><br></div><div>Ack.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp<br>
> index 23fc56d7..282c3c0f 100644<br>
> --- a/src/libcamera/ipa_interface.cpp<br>
> +++ b/src/libcamera/ipa_interface.cpp<br>
> @@ -536,10 +536,17 @@ namespace libcamera {<br>
> /**<br>
> * \fn IPAInterface::start()<br>
> * \brief Start the IPA<br>
> + * \param[in] ipaConfig Pipeline-handler-specific configuration data<br>
> + * \param[out] result Pipeline-handler-specific configuration result<br>
> *<br>
> * This method informs the IPA module that the camera is about to be started.<br>
> * The IPA module shall prepare any resources it needs to operate.<br>
> *<br>
> + * The \a ipaConfig and \a result parameters carry custom data passed by the<br>
> + * pipeline handler to the IPA and back. The pipeline handler may set the \a<br>
> + * result parameter to null if the IPA protocol doesn't need to pass a result<br>
> + * back through the configure() function.<br>
> + *<br>
> * \return 0 on success or a negative error code otherwise<br>
> */<br>
><br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index ddb30e49..a8e4997a 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -747,7 +747,9 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont<br>
> }<br>
><br>
> /* Start the IPA. */<br>
> - ret = data->ipa_->start();<br>
> + IPAOperationData ipaConfig, result;<br>
<br>
Another IPAOperationData with undefined "operation" field. Does it matter?<br></blockquote><div><br></div><div>Will fix this.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + ipaConfig.controls.emplace_back(*controls);<br>
> + ret = data->ipa_->start(ipaConfig, &result);<br>
> if (ret) {<br>
> LOG(RPI, Error)<br>
> << "Failed to start IPA for " << camera->id();<br>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> index 2e8d2930..a6c82a48 100644<br>
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> @@ -832,7 +832,8 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c<br>
> if (ret)<br>
> return ret;<br>
><br>
> - ret = data->ipa_->start();<br>
> + IPAOperationData ipaConfig = {};<br>
> + ret = data->ipa_->start(ipaConfig, nullptr);<br>
> if (ret) {<br>
> freeBuffers(camera);<br>
> LOG(RkISP1, Error)<br>
> @@ -911,7 +912,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] ControlList *c<br>
> std::map<unsigned int, const ControlInfoMap &> entityControls;<br>
> entityControls.emplace(0, data->sensor_->controls());<br>
><br>
> - IPAOperationData ipaConfig;<br>
> + ipaConfig = {};<br>
> data->ipa_->configure(sensorInfo, streamConfig, entityControls,<br>
> ipaConfig, nullptr);<br>
><br>
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp<br>
> index d81b8598..b4773cf5 100644<br>
> --- a/src/libcamera/pipeline/vimc/vimc.cpp<br>
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp<br>
> @@ -322,7 +322,8 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] ControlList *con<br>
> if (ret < 0)<br>
> return ret;<br>
><br>
> - ret = data->ipa_->start();<br>
> + IPAOperationData ipaConfig = {};<br>
> + ret = data->ipa_->start(ipaConfig, nullptr);<br>
> if (ret) {<br>
> data->video_->releaseBuffers();<br>
> return ret;<br>
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp<br>
> index b78a0e45..619976e5 100644<br>
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp<br>
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp<br>
> @@ -30,7 +30,8 @@ public:<br>
> {<br>
> return 0;<br>
> }<br>
> - int start() override { return 0; }<br>
> + int start([[maybe_unused]] const IPAOperationData &ipaConfig,<br>
> + [[maybe_unused]] IPAOperationData *result) override { return 0; }<br>
> void stop() override {}<br>
> void configure([[maybe_unused]] const CameraSensorInfo &sensorInfo,<br>
> [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,<br>
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp<br>
> index eead2883..9a81b6e7 100644<br>
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp<br>
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp<br>
> @@ -26,7 +26,8 @@ public:<br>
> IPAProxyThread(IPAModule *ipam);<br>
><br>
> int init(const IPASettings &settings) override;<br>
> - int start() override;<br>
> + int start(const IPAOperationData &ipaConfig,<br>
> + IPAOperationData *result) override;<br>
> void stop() override;<br>
><br>
> void configure(const CameraSensorInfo &sensorInfo,<br>
> @@ -50,9 +51,9 @@ private:<br>
> ipa_ = ipa;<br>
> }<br>
><br>
> - int start()<br>
> + int start(const IPAOperationData &ipaConfig, IPAOperationData *result)<br>
> {<br>
> - return ipa_->start();<br>
> + return ipa_->start(ipaConfig, result);<br>
> }<br>
><br>
> void stop()<br>
> @@ -111,12 +112,14 @@ int IPAProxyThread::init(const IPASettings &settings)<br>
> return 0;<br>
> }<br>
><br>
> -int IPAProxyThread::start()<br>
> +int IPAProxyThread::start(const IPAOperationData &ipaConfig,<br>
> + IPAOperationData *result)<br>
> {<br>
> running_ = true;<br>
> thread_.start();<br>
><br>
> - return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);<br>
> + return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking,<br>
> + ipaConfig, result);<br>
> }<br>
><br>
> void IPAProxyThread::stop()<br>
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp<br>
> index 67488409..6222938f 100644<br>
> --- a/test/ipa/ipa_interface_test.cpp<br>
> +++ b/test/ipa/ipa_interface_test.cpp<br>
> @@ -120,7 +120,8 @@ protected:<br>
> }<br>
><br>
> /* Test start of IPA module. */<br>
> - ipa_->start();<br>
> + IPAOperationData ipaConfig;<br>
<br>
Another uninitialised "operation" field. Does it matter? Should we<br>
make them all look the same?<br></blockquote><div><br></div><div>Will fix this. I think wherever we explicitly set .operation, it does not matter, but everywhere else, I should initilaise explicitly with = {}.</div><div> </div><div>Regards,</div><div>Naush</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Other than these nitpicks:<br>
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Best regards<br>
David<br>
<br>
> + ipa_->start(ipaConfig, nullptr);<br>
> timer.start(1000);<br>
> while (timer.isRunning() && trace_ != IPAOperationStart)<br>
> dispatcher->processEvents();<br>
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp<br>
> index 59d991cb..7b8ae77b 100644<br>
> --- a/test/ipa/ipa_wrappers_test.cpp<br>
> +++ b/test/ipa/ipa_wrappers_test.cpp<br>
> @@ -56,7 +56,8 @@ public:<br>
> return 0;<br>
> }<br>
><br>
> - int start() override<br>
> + int start([[maybe_unused]] const IPAOperationData &ipaConfig,<br>
> + [[maybe_unused]] IPAOperationData *result) override<br>
> {<br>
> report(Op_start, TestPass);<br>
> return 0;<br>
> @@ -376,7 +377,7 @@ protected:<br>
> return TestFail;<br>
> }<br>
><br>
> - ret = INVOKE(start);<br>
> + ret = INVOKE(start, ipaConfig, nullptr);<br>
> if (ret == TestFail) {<br>
> cerr << "Failed to run start()";<br>
> return TestFail;<br>
> --<br>
> 2.25.1<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>