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