| Message ID | 20200326160819.4088361-2-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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; > } >
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; + return TestPass; }
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. Signed-off-by: Niklas Söderlund <niklas.soderlund@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(-)