[libcamera-devel] [RFCv2 1/7] ipa: Add start() and stop() operations
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Mar 26 22:25:12 CET 2020
Hi Niklas,
Thank you for the patch.
On Thu, Mar 26, 2020 at 05:08:13PM +0100, Niklas Söderlund wrote:
> Add two new operations to the IPA interface to start and stop it. The
> intention is that these functions shall be used by the IPA to perform
> in actions in conjunction with the camera start and stop.
"in actions in conjunction" ?
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/ipa/ipa_interface.h | 4 ++++
> include/ipa/ipa_vimc.h | 2 ++
> src/ipa/libipa/ipa_interface_wrapper.cpp | 16 +++++++++++++
> src/ipa/libipa/ipa_interface_wrapper.h | 2 ++
> src/ipa/rkisp1/rkisp1.cpp | 2 ++
> src/ipa/vimc/vimc.cpp | 20 ++++++++++++++++
> src/libcamera/include/ipa_context_wrapper.h | 2 ++
> src/libcamera/ipa_context_wrapper.cpp | 26 +++++++++++++++++++++
> src/libcamera/proxy/ipa_proxy_linux.cpp | 2 ++
> test/ipa/ipa_interface_test.cpp | 24 +++++++++++++++++++
> test/ipa/ipa_wrappers_test.cpp | 25 ++++++++++++++++++--
> 11 files changed, 123 insertions(+), 2 deletions(-)
>
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index 229d1124b19ec1c9..9a62208cc80084c8 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -64,6 +64,8 @@ struct ipa_context_ops {
> void (*destroy)(struct ipa_context *ctx);
> void *(*get_interface)(struct ipa_context *ctx);
> void (*init)(struct ipa_context *ctx);
> + void (*start)(struct ipa_context *ctx);
> + void (*stop)(struct ipa_context *ctx);
> void (*register_callbacks)(struct ipa_context *ctx,
> const struct ipa_callback_ops *callbacks,
> void *cb_ctx);
> @@ -120,6 +122,8 @@ public:
> virtual ~IPAInterface() {}
>
> virtual int init() = 0;
> + virtual int start() = 0;
> + virtual void stop() = 0;
>
> virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> diff --git a/include/ipa/ipa_vimc.h b/include/ipa/ipa_vimc.h
> index 9add122cf598aa6f..8e82dd94bf479e35 100644
> --- a/include/ipa/ipa_vimc.h
> +++ b/include/ipa/ipa_vimc.h
> @@ -15,6 +15,8 @@ namespace libcamera {
> enum IPAOperationCode {
> IPAOperationNone,
> IPAOperationInit,
> + IPAOperationStart,
> + IPAOperationStop,
> };
>
> } /* namespace libcamera */
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index b93c1c1f1c1a2018..ef074ba17316ed97 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -86,6 +86,20 @@ void IPAInterfaceWrapper::init(struct ipa_context *_ctx)
> ctx->ipa_->init();
> }
>
> +void IPAInterfaceWrapper::start(struct ipa_context *_ctx)
> +{
> + IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> +
> + ctx->ipa_->start();
> +}
> +
> +void IPAInterfaceWrapper::stop(struct ipa_context *_ctx)
> +{
> + IPAInterfaceWrapper *ctx = static_cast<IPAInterfaceWrapper *>(_ctx);
> +
> + ctx->ipa_->stop();
> +}
> +
> void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
> const struct ipa_callback_ops *callbacks,
> void *cb_ctx)
> @@ -234,6 +248,8 @@ const struct ipa_context_ops IPAInterfaceWrapper::operations_ = {
> .destroy = &IPAInterfaceWrapper::destroy,
> .get_interface = &IPAInterfaceWrapper::get_interface,
> .init = &IPAInterfaceWrapper::init,
> + .start = &IPAInterfaceWrapper::start,
> + .stop = &IPAInterfaceWrapper::stop,
> .register_callbacks = &IPAInterfaceWrapper::register_callbacks,
> .configure = &IPAInterfaceWrapper::configure,
> .map_buffers = &IPAInterfaceWrapper::map_buffers,
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> index 3fb7b447643953ff..9ee4fe15b2738cf4 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.h
> +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> @@ -24,6 +24,8 @@ private:
> static void destroy(struct ipa_context *ctx);
> static void *get_interface(struct ipa_context *ctx);
> static void init(struct ipa_context *ctx);
> + static void start(struct ipa_context *ctx);
> + static void stop(struct ipa_context *ctx);
> static void register_callbacks(struct ipa_context *ctx,
> const struct ipa_callback_ops *callbacks,
> void *cb_ctx);
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 438b3c66f77a1e57..af38e3299f2ff045 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -33,6 +33,8 @@ class IPARkISP1 : public IPAInterface
> {
> public:
> int init() override { return 0; }
> + int start() override { return 0; }
> + void stop() override {}
>
> void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index 6e2095b56bbcdaa3..566a66e404affbd5 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -32,6 +32,10 @@ public:
> ~IPAVimc();
>
> int init() override;
> +
> + int start() override;
> + void stop() override;
> +
> void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> @@ -66,6 +70,22 @@ int IPAVimc::init()
> return 0;
> }
>
> +int IPAVimc::start()
> +{
> + trace(IPAOperationStart);
> +
> + LOG(IPAVimc, Debug) << "start vimc IPA!";
> +
> + return 0;
> +}
> +
> +void IPAVimc::stop()
> +{
> + trace(IPAOperationStop);
> +
> + LOG(IPAVimc, Debug) << "stop vimc IPA!";
> +}
> +
> void IPAVimc::initTrace()
> {
> struct stat fifoStat;
> diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> index c9e194120de6b69c..0a48bfe732c831d7 100644
> --- a/src/libcamera/include/ipa_context_wrapper.h
> +++ b/src/libcamera/include/ipa_context_wrapper.h
> @@ -20,6 +20,8 @@ public:
> ~IPAContextWrapper();
>
> int init() override;
> + int start() override;
> + void stop() override;
> void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
>
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index 946a2fd8cab1782a..c40e56169512a1ba 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -82,6 +82,32 @@ int IPAContextWrapper::init()
> return 0;
> }
>
> +int IPAContextWrapper::start()
> +{
> + if (intf_)
> + return intf_->start();
> +
> + if (!ctx_)
> + return 0;
> +
> + ctx_->ops->start(ctx_);
> +
> + return 0;
> +}
> +
> +void IPAContextWrapper::stop()
> +{
> + if (intf_)
> + return intf_->stop();
> +
> + if (!ctx_)
> + return;
> +
> + ctx_->ops->stop(ctx_);
> +
> + return;
> +}
> +
> void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> {
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index c7218fb472490cc8..2aa80b9467044fbf 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -27,6 +27,8 @@ public:
> ~IPAProxyLinux();
>
> int init() override { return 0; }
> + int start() override { return 0; }
> + void stop() override {}
> void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index cafc249bbbc96cf8..f861c10f16925d78 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -109,6 +109,30 @@ protected:
> return TestFail;
> }
>
> + /* Test start of IPA module. */
> + ipa_->start();
> + timer.start(1000);
> + while (timer.isRunning() && trace_ != IPAOperationStart)
> + dispatcher->processEvents();
> +
> + if (trace_ != IPAOperationStart) {
> + cerr << "Failed to test IPA start sequence"
> + << endl;
> + return TestFail;
> + }
> +
> + /* Test start of IPA module. */
> + ipa_->stop();
> + timer.start(1000);
> + while (timer.isRunning() && trace_ != IPAOperationStop)
> + dispatcher->processEvents();
> +
> + if (trace_ != IPAOperationStop) {
> + cerr << "Failed to test IPA stop sequence"
> + << endl;
> + return TestFail;
> + }
> +
> return TestPass;
> }
>
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index 1ae1781169c85e6c..ac56498c40e597c9 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -27,6 +27,8 @@ using namespace std;
>
> enum Operation {
> Op_init,
> + Op_start,
> + Op_stop,
> Op_configure,
> Op_mapBuffers,
> Op_unmapBuffers,
> @@ -47,6 +49,17 @@ public:
> return 0;
> }
>
> + int start() override
> + {
> + report(Op_start, TestPass);
> + return 0;
> + }
> +
> + void stop() override
> + {
> + report(Op_stop, TestPass);
> + }
> +
> void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> {
> @@ -323,13 +336,21 @@ protected:
> return TestFail;
>
> /*
> - * Test init() last to ensure nothing in the wrappers or
> - * serializer depends on init() being called first.
> + * Test init(), start() and stop() last to ensure nothing in the
> + * wrappers or serializer depends on them being called first.
> */
> ret = INVOKE(init);
> if (ret == TestFail)
> return TestFail;
>
> + ret = INVOKE(start);
> + if (ret == TestFail)
> + return TestFail;
> +
> + ret = INVOKE(stop);
> + if (ret == TestFail)
> + return TestFail;
While at it, cerr messages for failures ?
Apart from the lack of documentation (which I understand is due to RFC
status), this looks good to me.
> +
> return TestPass;
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list