[libcamera-devel] [PATCH v2 2/3] libcamera: ipa: Pass a set of controls and return results from ipa::start()
Naushir Patuck
naush at raspberrypi.com
Fri Dec 4 14:38:58 CET 2020
Hi Jacopo,
On Fri, 4 Dec 2020 at 13:20, Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Nuash,
>
> On Fri, Dec 04, 2020 at 01:06:09PM +0000, Naushir Patuck wrote:
> > Hi Jacopo,
> >
> > Thank you for your review feedback.
> >
> > On Fri, 4 Dec 2020 at 11:26, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > > Hi Naush,
> > >
> > > On Thu, Nov 26, 2020 at 09:51:25AM +0000, Naushir Patuck wrote:
> > > > This change allows controls passed into PipelineHandler::start to be
> > > > forwarded onto IPAInterface::start(). We also add a return channel
> if the
> > > > pipeline handler must action any of these controls, e.g. setting the
> > > > analogue gain or shutter speed in the sensor device.
> > > >
> > > > todo: The IPA interface wrapper needs a translation for passing
> > > > IPAOperationData into start() and configure()
> > >
> > > This is 'problematic' as it makes isolated IPAs deviate from other
> > > ones. We have the same issue with configure() which has a very similar
> > > \todo item :/
> > >
> > > Now, this is not an issue right now, and I think with Paul's IPC
> > > we'll get it for free.
> > >
> > > Other comments below:
> > >
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > Tested-by: David Plowman <david.plowman 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 | 6 ++++--
> > > > 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, 50 insertions(+), 23 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,
> > >
> > > 'ipaConfig' is only used in configure.
> > > It's generally just called 'data' and I would use it here too.
> > >
> >
> > Sure, will fix. I will also s/ ipaConfig/data/ in all instances of
> start()
> > that have been changed in this patch.
>
> Thanks! Other names are good to, just drop 'Config'
>
Could you clarify the above comment please? I was intending to use "data"
for all IPAOperationData variables that have been introduced in the
start() method in this patch. Is your suggestion to use "ipa" instead of
"data"?
Regards,
Naush
>
> >
> >
> > > > + 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..2b305b56 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 = {};
> > > > + 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)
> > >
> > > ^ is this mis-aligned or is it my email client ?
> > > There are more in this patch if that's the case.
> > >
> >
> > I think this may be an email client rendering thing. They do look
> correct
> > to me, and I would expect the style checker to complain about
> > misalignment. However, I will check though once again.
> >
>
> Might be, I just pointed it out as I usually see indentation off 1
> space with my client and here I see 3. I haven't applied the patches,
> so I trust your opinion!
>
> >
> > > > {
> > > > trace(IPAOperationStart);
> > > >
> > > > diff --git a/src/libcamera/ipa_context_wrapper.cpp
> > > b/src/libcamera/ipa_context_wrapper.cpp
> > > > index 231300ce..8c4ec40a 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 response */
> > > > return ctx_->ops->start(ctx_);
> > > > }
> > > >
> > > > 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
> > >
> > > These come from the 'configuration' operation
> > > I would:
> > >
> > > \param[in] data Protocol-specific data for the start operation
> > > \param[out] result Result of the start operation
> > >
> >
> > Fixed.
>
> Thanks
>
> >
> >
> > >
> > > > *
> > > > * 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.
> > > > + *
> > >
> > > Copied from configure too, replace 'configure()' at least.
> > >
> >
> > Oops, replaced with start()!
> >
> >
> > >
> > > > * \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..29bcef07 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 = {};
> > > > + ipaConfig.controls.emplace_back(*controls);
> > >
> > > What if controls is nullptr ?
> > >
> >
> > Hmmm.... I am a bit confused. My version of the code has:
> >
> > if (controls) {
> > ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
> > ipaConfig.controls.emplace_back(*controls);
> > }
> >
> > But clearly my patch in the email does not :( Once I complete all your
> > suggested fixups, I will send an updated patchset!
> >
>
> Thanks
>
> >
> > > > + ret = data->ipa_->start(ipaConfig, &result);
> > > > if (ret) {
> > > > LOG(RPI, Error)
> > > > << "Failed to start IPA for " << camera->id();
> > > > @@ -1176,7 +1178,7 @@ int RPiCameraData::configureIPA(const
> > > CameraConfiguration *config)
> > > > }
> > > >
> > > > /* Ready the IPA - it must know about the sensor resolution. */
> > > > - IPAOperationData result;
> > > > + IPAOperationData result = {};
> > >
> > > Unrelated but I think it's ok
> > >
> >
> > This was an update requested by David in his review. Just to be safe :)
> >
>
> I agree it is an opportune fix, maybe not strictly part of this patch.
> But it's ok :)
>
> >
> > >
> > > >
> > > > ipa_->configure(sensorInfo_, streamConfig, entityControls,
> > > ipaConfig,
> > > > &result);
> > > > 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 you want to use a single IPAOperationData for start and configure,
> > > please rename it (here and in other pipeline handlers).
> > >
> >
> > I would use data (as with the other IPAOperationData parameters passed
> into
> > start), but we already have a declaration of RkISP1CameraData *data. Any
> > objections to renaming it ipaData?
> >
>
> Sounds good to me!
> Thanks for addressing all comments!
>
> > Regards,
> > Naush
> >
> >
> >
> > >
> > > Thanks
> > > j
> > >
> > >
> > > > 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..03b7f0ad 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 = {};
> > > > + 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/20201204/fc85d38c/attachment-0001.htm>
More information about the libcamera-devel
mailing list