[libcamera-devel] [PATCH v2 2/3] libcamera: ipa: Pass a set of controls and return results from ipa::start()
Jacopo Mondi
jacopo at jmondi.org
Fri Dec 4 14:52:21 CET 2020
Hi Naush,
On Fri, Dec 04, 2020 at 01:38:58PM +0000, Naushir Patuck wrote:
> 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"?
Sorry for the confusion. What I meant was "If you have other (better)
names than 'data', that's also fine. What presses me is not to have
'Config' in the name"
>
> 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
> > > >
> >
More information about the libcamera-devel
mailing list