[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 16:31:13 CEST 2020
Hi Laurent,
Thanks for your work.
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.
> + 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list