[libcamera-devel] [PATCH v1 3/9] libcamera: ipa_interface: Add support for custom IPA data to configure()
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Jun 29 22:51:43 CEST 2020
Hi Laurent,
On 2020-06-29 18:25:38 +0200, Niklas Söderlund wrote:
> Hi Laurent,
>
> On 2020-06-29 17:57:43 +0300, Laurent Pinchart wrote:
> > Hi Niklas,
> >
> > On Mon, Jun 29, 2020 at 04:31:13PM +0200, Niklas Söderlund wrote:
> > > On 2020-06-29 02:19:28 +0300, Laurent Pinchart wrote:
> > > > Add two new parameters, ipaConfig and result, to the
> > > > IPAInterface::configure() function to allow pipeline handlers to pass
> > > > custom data to their IPA, and receive data back. Wire this through the
> > > > code base. The C API interface will be addressed separately, likely
> > > > through automation of the C <-> C++ translation.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > > include/libcamera/internal/ipa_context_wrapper.h | 4 +++-
> > > > include/libcamera/ipa/ipa_interface.h | 4 +++-
> > > > src/ipa/libipa/ipa_interface_wrapper.cpp | 5 ++++-
> > > > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++++--
> > > > src/ipa/rkisp1/rkisp1.cpp | 8 ++++++--
> > > > src/ipa/vimc/vimc.cpp | 4 +++-
> > > > src/libcamera/ipa_context_wrapper.cpp | 8 ++++++--
> > > > src/libcamera/ipa_interface.cpp | 7 +++++++
> > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 +++-
> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +++-
> > > > src/libcamera/proxy/ipa_proxy_linux.cpp | 4 +++-
> > > > src/libcamera/proxy/ipa_proxy_thread.cpp | 11 ++++++++---
> > > > test/ipa/ipa_wrappers_test.cpp | 8 ++++++--
> > > > 13 files changed, 61 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/ipa_context_wrapper.h b/include/libcamera/internal/ipa_context_wrapper.h
> > > > index 4e6f791d18e6..8f767e844221 100644
> > > > --- a/include/libcamera/internal/ipa_context_wrapper.h
> > > > +++ b/include/libcamera/internal/ipa_context_wrapper.h
> > > > @@ -24,7 +24,9 @@ public:
> > > > void stop() override;
> > > > void configure(const CameraSensorInfo &sensorInfo,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *result) override;
> > > >
> > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > > > index dc9fc714d564..5016ec25ea9c 100644
> > > > --- a/include/libcamera/ipa/ipa_interface.h
> > > > +++ b/include/libcamera/ipa/ipa_interface.h
> > > > @@ -158,7 +158,9 @@ public:
> > > >
> > > > virtual void configure(const CameraSensorInfo &sensorInfo,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *result) = 0;
> > > >
> > > > virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > > > virtual void unmapBuffers(const std::vector<unsigned int> &ids) = 0;
> > > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > index 2a2e43abc708..47ce5a704851 100644
> > > > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > > @@ -166,7 +166,10 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > > > entityControls.emplace(id, infoMaps[id]);
> > > > }
> > > >
> > > > - ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
> > > > + /* \todo Translate the ipaConfig and reponse */
> > >
> > > I wonder if we should add a FATAL LOG statement here to make it clear
> > > that we break the wrapper and this issue should be solved before anyone
> > > attempts to use the feature, which we know won't work.
> >
> > That would break existing tests though, as they use this function.
>
> Since all this is going away I wont stress it :-)
I forgot to add,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> >
> > > > + IPAOperationData ipaConfig;
> > > > + ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls, ipaConfig,
> > > > + nullptr);
> > > > }
> > > >
> > > > void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index bc89ab58d03a..860be22ddb5d 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -78,7 +78,9 @@ public:
> > > >
> > > > void configure(const CameraSensorInfo &sensorInfo,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &data,
> > > > + IPAOperationData *response) override;
> > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > void processEvent(const IPAOperationData &event) override;
> > > > @@ -186,7 +188,9 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> > > >
> > > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *result)
> > > > {
> > > > if (entityControls.empty())
> > > > return;
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index fbdc908fc816..34d6f63a7ff4 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -39,7 +39,9 @@ public:
> > > >
> > > > void configure(const CameraSensorInfo &info,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *response) override;
> > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > void processEvent(const IPAOperationData &event) override;
> > > > @@ -76,7 +78,9 @@ private:
> > > > */
> > > > void IPARkISP1::configure(const CameraSensorInfo &info,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *result)
> > > > {
> > > > if (entityControls.empty())
> > > > return;
> > > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > > > index af278a482b8a..1593c92d80f3 100644
> > > > --- a/src/ipa/vimc/vimc.cpp
> > > > +++ b/src/ipa/vimc/vimc.cpp
> > > > @@ -39,7 +39,9 @@ public:
> > > >
> > > > void configure(const CameraSensorInfo &sensorInfo,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *result) override {}
> > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > > > void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > > > void processEvent(const IPAOperationData &event) override {}
> > > > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > > > index 471118f57cdc..231300ce0bec 100644
> > > > --- a/src/libcamera/ipa_context_wrapper.cpp
> > > > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > > > @@ -110,10 +110,13 @@ void IPAContextWrapper::stop()
> > > >
> > > > void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *result)
> > > > {
> > > > if (intf_)
> > > > - return intf_->configure(sensorInfo, streamConfig, entityControls);
> > > > + return intf_->configure(sensorInfo, streamConfig,
> > > > + entityControls, ipaConfig, result);
> > > >
> > > > if (!ctx_)
> > > > return;
> > > > @@ -174,6 +177,7 @@ void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > > > ++i;
> > > > }
> > > >
> > > > + /* \todo Translate the ipaConfig and reponse */
> > > > ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
> > > > c_info_maps, entityControls.size());
> > > > }
> > > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > > > index ebe47e1233a5..23fc56d7d48e 100644
> > > > --- a/src/libcamera/ipa_interface.cpp
> > > > +++ b/src/libcamera/ipa_interface.cpp
> > > > @@ -557,6 +557,8 @@ namespace libcamera {
> > > > * \param[in] sensorInfo Camera sensor information
> > > > * \param[in] streamConfig Configuration of all active streams
> > > > * \param[in] entityControls Controls provided by the pipeline entities
> > > > + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > > + * \param[out] result Pipeline-handler-specific configuration result
> > > > *
> > > > * This method shall be called when the camera is started to inform the IPA of
> > > > * the camera's streams and the sensor settings. The meaning of the numerical
> > > > @@ -566,6 +568,11 @@ namespace libcamera {
> > > > * The \a sensorInfo conveys information about the camera sensor settings that
> > > > * the pipeline handler has selected for the configuration. The IPA may use
> > > > * that information to tune its algorithms.
> > > > + *
> > > > + * 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.
> > > > */
> > > >
> > > > /**
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index dcd737a1d1a0..3b5cdf1e1849 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -1017,7 +1017,9 @@ int PipelineHandlerRPi::configureIPA(Camera *camera)
> > > > }
> > > >
> > > > /* Ready the IPA - it must know about the sensor resolution. */
> > > > - data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > > > + IPAOperationData ipaConfig;
> > > > + data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > > > + ipaConfig, nullptr);
> > > >
> > > > return 0;
> > > > }
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 3c01821135f8..4fde5539e667 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -843,7 +843,9 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > > > std::map<unsigned int, const ControlInfoMap &> entityControls;
> > > > entityControls.emplace(0, data->sensor_->controls());
> > > >
> > > > - data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > > > + IPAOperationData ipaConfig;
> > > > + data->ipa_->configure(sensorInfo, streamConfig, entityControls,
> > > > + ipaConfig, nullptr);
> > > >
> > > > return ret;
> > > > }
> > > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > index be34f20aa857..68eafb307b2a 100644
> > > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > > @@ -31,7 +31,9 @@ public:
> > > > void stop() override {}
> > > > void configure(const CameraSensorInfo &sensorInfo,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *result) override {}
> > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > > > void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > > > void processEvent(const IPAOperationData &event) override {}
> > > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > index 6fbebed2ba72..aa403e22f297 100644
> > > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > > @@ -31,7 +31,9 @@ public:
> > > >
> > > > void configure(const CameraSensorInfo &sensorInfo,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *result) override;
> > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > void processEvent(const IPAOperationData &event) override;
> > > > @@ -129,9 +131,12 @@ void IPAProxyThread::stop()
> > > >
> > > > void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *result)
> > > > {
> > > > - ipa_->configure(sensorInfo, streamConfig, entityControls);
> > > > + ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > > > + result);
> > > > }
> > > >
> > > > void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> > > > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> > > > index aa7a9dcc6050..23c799da0663 100644
> > > > --- a/test/ipa/ipa_wrappers_test.cpp
> > > > +++ b/test/ipa/ipa_wrappers_test.cpp
> > > > @@ -69,7 +69,9 @@ public:
> > > >
> > > > void configure(const CameraSensorInfo &sensorInfo,
> > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > - const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> > > > + const std::map<unsigned int, const ControlInfoMap &> &entityControls,
> > > > + const IPAOperationData &ipaConfig,
> > > > + IPAOperationData *result) override
> > > > {
> > > > /* Verify sensorInfo. */
> > > > if (sensorInfo.outputSize.width != 2560 ||
> > > > @@ -317,7 +319,9 @@ protected:
> > > > };
> > > > std::map<unsigned int, const ControlInfoMap &> controlInfo;
> > > > controlInfo.emplace(42, subdev_->controls());
> > > > - ret = INVOKE(configure, sensorInfo, config, controlInfo);
> > > > + IPAOperationData ipaConfig;
> > > > + ret = INVOKE(configure, sensorInfo, config, controlInfo,
> > > > + ipaConfig, nullptr);
> > > > if (ret == TestFail)
> > > > return TestFail;
> > > >
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> Regards,
> Niklas Söderlund
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list