Message ID | 20191003152037.74617-5-jacopo@jmondi.org |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Oct 03, 2019 at 05:20:36PM +0200, Jacopo Mondi wrote: > Implementing a basic test for IPA Interface using the VIMC dummy IPA > module. The test implements a communication channel between the test and > the dummy IPA module to verify the success of the IPA interactions. > > Test the only available operation defined by the IPA interface by > receiving a confirmation code on the fifo communication channel. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > test/ipa/ipa_interface_test.cpp | 147 ++++++++++++++++++++++++++++++++ > test/ipa/meson.build | 3 +- > 2 files changed, 149 insertions(+), 1 deletion(-) > create mode 100644 test/ipa/ipa_interface_test.cpp > > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp > new file mode 100644 > index 000000000000..83eef7440fa4 > --- /dev/null > +++ b/test/ipa/ipa_interface_test.cpp > @@ -0,0 +1,147 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * ipa_interface_test.cpp - Test the IPA interface communication using the VIMC > + * dummy IPA module That's getting a bit long for a short description. How about "Test the IPA interface" ? > + */ > + > +#include <iostream> > + No need for a blank line. > +#include <fcntl.h> > +#include <string.h> > +#include <sys/types.h> > +#include <sys/stat.h> stat comes before types. > +#include <unistd.h> > + > +#include <libcamera/event_dispatcher.h> > +#include <libcamera/event_notifier.h> > +#include <libcamera/timer.h> > + > +#include "ipa_module.h" > + > +#include "test.h" > +#include "thread.h" > + > +using namespace std; > +using namespace libcamera; > + > +/* Keep with in sync with the VIMC IPA module. */ s/Keep with/Keep/ > +static const char *ipaFifoPath = "/tmp/vimc_ipa_fifo"; > +#define IPA_INIT_CODE 0x01 > + > +static const char *vimcModulePath = "src/ipa/ipa_vimc.so"; > + > +class IPAInterfaceTest : public Test, public Object > +{ > +public: > + IPAInterfaceTest() > + : fd_(0), data_(0) You should initialise ipaModule_ and notifier_ to nullptr. > + { > + } > + > + ~IPAInterfaceTest() > + { > + /* > + * Delete the IPA Interface instance -before- deleting the > + * module used to create it, as the IPAModule destructor closes > + * the IPA shared object, resulting in segfault at interface > + * deletion time. > + */ > + IPAInterface *ipa = ipa_.release(); > + delete ipa; You can write this ipa_.reset(); > + > + if (ipaModule_) > + delete ipaModule_; > + > + delete notifier_; Shouldn't this be moved to cleanup() ? > + } > + > +protected: > + int init() override > + { > + int ret = mkfifo(ipaFifoPath, S_IRUSR | S_IWUSR); > + if (ret) { > + cerr << "Failed to create IPA test fifo at: " > + << ipaFifoPath << ": " << strerror(errno) << endl; > + return TestFail; > + } > + > + ret = open(ipaFifoPath, O_RDONLY | O_NONBLOCK); > + if (ret < 0) { > + cerr << "Failed to open IPA test fifo at: " > + << ipaFifoPath << ": " << strerror(errno) << endl; > + unlink(ipaFifoPath); > + return TestFail; > + } > + fd_ = ret; > + > + notifier_ = new EventNotifier(fd_, EventNotifier::Read, this); > + notifier_->activated.connect(this, &IPAInterfaceTest::readFifo); > + > + return TestPass; > + } > + > + int run() override > + { > + EventDispatcher *dispatcher = thread()->eventDispatcher(); > + Timer timer; > + > + ipaModule_ = new IPAModule(vimcModulePath); > + if (!ipaModule_ || !ipaModule_->isValid()) { > + cerr << "Failed to create VIMC IPA module at: " > + << vimcModulePath << endl; > + return TestFail; > + } > + > + if (!ipaModule_->load()) { > + cerr << "Failed to load VIMC IPA module" << endl; > + return TestFail; > + } > + > + ipa_ = ipaModule_->createInstance(); > + if (!ipa_) { > + cerr << "Failed to create VIMC IPA interface" << endl; > + return TestFail; > + } I think we should use the IPA manager to create the instance, in order to pull in IPC and proxy. Otherwise you will end up with a plain IPAInterface that will directly call the IPA, emptying the init() call test below from its substance. > + > + /* Test initialization of IPA module. */ > + ipa_->init(); > + timer.start(1000); > + while (timer.isRunning() && data_ != IPA_INIT_CODE) > + dispatcher->processEvents(); > + > + if (data_ != IPA_INIT_CODE) { > + cerr << "Failed to test IPA initialization sequence" > + << endl; > + return TestFail; > + } > + > + return TestPass; > + } > + > + void cleanup() override > + { > + close(fd_); > + unlink(ipaFifoPath); > + } > + > +private: > + void readFifo(EventNotifier *notifier) > + { > + size_t s = read(notifier->fd(), &data_, 1); > + if (s < 0) { > + cerr << "Failed to to read from IPA test fifo at: " s/to to/to/ s/fifo/FIFO/ > + << ipaFifoPath << strerror(errno) << endl; I think It's remove the FIFO path. > + data_ = -1; > + } > + } > + > + std::unique_ptr<IPAInterface> ipa_; > + EventNotifier *notifier_; > + IPAModule *ipaModule_; > + unsigned int fd_; > + int8_t data_; How about naming this status_ and making it an enum ? > +}; > + > +TEST_REGISTER(IPAInterfaceTest) > diff --git a/test/ipa/meson.build b/test/ipa/meson.build > index e174671d05e1..c501fcf99aed 100644 > --- a/test/ipa/meson.build > +++ b/test/ipa/meson.build > @@ -1,5 +1,6 @@ > ipa_test = [ > - ['ipa_module_test', 'ipa_module_test.cpp'], > + ['ipa_module_test', 'ipa_module_test.cpp'], > + ['ipa_interface_test', 'ipa_interface_test.cpp'], > ] > > foreach t : ipa_test
Hi Jacopo, Another comment. On Thu, Oct 03, 2019 at 09:34:09PM +0300, Laurent Pinchart wrote: > On Thu, Oct 03, 2019 at 05:20:36PM +0200, Jacopo Mondi wrote: > > Implementing a basic test for IPA Interface using the VIMC dummy IPA > > module. The test implements a communication channel between the test and > > the dummy IPA module to verify the success of the IPA interactions. > > > > Test the only available operation defined by the IPA interface by > > receiving a confirmation code on the fifo communication channel. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > test/ipa/ipa_interface_test.cpp | 147 ++++++++++++++++++++++++++++++++ > > test/ipa/meson.build | 3 +- > > 2 files changed, 149 insertions(+), 1 deletion(-) > > create mode 100644 test/ipa/ipa_interface_test.cpp > > > > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp > > new file mode 100644 > > index 000000000000..83eef7440fa4 > > --- /dev/null > > +++ b/test/ipa/ipa_interface_test.cpp > > @@ -0,0 +1,147 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * ipa_interface_test.cpp - Test the IPA interface communication using the VIMC > > + * dummy IPA module > > That's getting a bit long for a short description. How about "Test the > IPA interface" ? > > > + */ > > + > > +#include <iostream> > > + > > No need for a blank line. > > > +#include <fcntl.h> > > +#include <string.h> > > +#include <sys/types.h> > > +#include <sys/stat.h> > > stat comes before types. > > > +#include <unistd.h> > > + > > +#include <libcamera/event_dispatcher.h> > > +#include <libcamera/event_notifier.h> > > +#include <libcamera/timer.h> > > + > > +#include "ipa_module.h" > > + > > +#include "test.h" > > +#include "thread.h" > > + > > +using namespace std; > > +using namespace libcamera; > > + > > +/* Keep with in sync with the VIMC IPA module. */ > > s/Keep with/Keep/ > > > +static const char *ipaFifoPath = "/tmp/vimc_ipa_fifo"; > > +#define IPA_INIT_CODE 0x01 > > + > > +static const char *vimcModulePath = "src/ipa/ipa_vimc.so"; > > + > > +class IPAInterfaceTest : public Test, public Object > > +{ > > +public: > > + IPAInterfaceTest() > > + : fd_(0), data_(0) > > You should initialise ipaModule_ and notifier_ to nullptr. > > > + { > > + } > > + > > + ~IPAInterfaceTest() > > + { > > + /* > > + * Delete the IPA Interface instance -before- deleting the > > + * module used to create it, as the IPAModule destructor closes > > + * the IPA shared object, resulting in segfault at interface > > + * deletion time. > > + */ > > + IPAInterface *ipa = ipa_.release(); > > + delete ipa; > > You can write this > > ipa_.reset(); > > > + > > + if (ipaModule_) > > + delete ipaModule_; > > + > > + delete notifier_; > > Shouldn't this be moved to cleanup() ? > > > + } > > + > > +protected: > > + int init() override > > + { > > + int ret = mkfifo(ipaFifoPath, S_IRUSR | S_IWUSR); > > + if (ret) { > > + cerr << "Failed to create IPA test fifo at: " > > + << ipaFifoPath << ": " << strerror(errno) << endl; > > + return TestFail; > > + } > > + > > + ret = open(ipaFifoPath, O_RDONLY | O_NONBLOCK); > > + if (ret < 0) { > > + cerr << "Failed to open IPA test fifo at: " > > + << ipaFifoPath << ": " << strerror(errno) << endl; > > + unlink(ipaFifoPath); > > + return TestFail; > > + } > > + fd_ = ret; > > + > > + notifier_ = new EventNotifier(fd_, EventNotifier::Read, this); > > + notifier_->activated.connect(this, &IPAInterfaceTest::readFifo); > > + > > + return TestPass; > > + } > > + > > + int run() override > > + { > > + EventDispatcher *dispatcher = thread()->eventDispatcher(); > > + Timer timer; > > + > > + ipaModule_ = new IPAModule(vimcModulePath); > > + if (!ipaModule_ || !ipaModule_->isValid()) { > > + cerr << "Failed to create VIMC IPA module at: " > > + << vimcModulePath << endl; > > + return TestFail; > > + } > > + > > + if (!ipaModule_->load()) { > > + cerr << "Failed to load VIMC IPA module" << endl; > > + return TestFail; > > + } > > + > > + ipa_ = ipaModule_->createInstance(); > > + if (!ipa_) { > > + cerr << "Failed to create VIMC IPA interface" << endl; > > + return TestFail; > > + } > > I think we should use the IPA manager to create the instance, in order > to pull in IPC and proxy. Otherwise you will end up with a plain > IPAInterface that will directly call the IPA, emptying the init() call > test below from its substance. > > > + > > + /* Test initialization of IPA module. */ > > + ipa_->init(); > > + timer.start(1000); > > + while (timer.isRunning() && data_ != IPA_INIT_CODE) > > + dispatcher->processEvents(); > > + > > + if (data_ != IPA_INIT_CODE) { > > + cerr << "Failed to test IPA initialization sequence" > > + << endl; > > + return TestFail; > > + } > > + > > + return TestPass; > > + } > > + > > + void cleanup() override > > + { > > + close(fd_); > > + unlink(ipaFifoPath); > > + } > > + > > +private: > > + void readFifo(EventNotifier *notifier) > > + { > > + size_t s = read(notifier->fd(), &data_, 1); > > + if (s < 0) { size_t is always positive. You should use ssize_t. > > + cerr << "Failed to to read from IPA test fifo at: " > > s/to to/to/ > s/fifo/FIFO/ > > > + << ipaFifoPath << strerror(errno) << endl; > > I think It's remove the FIFO path. > > > + data_ = -1; > > + } > > + } > > + > > + std::unique_ptr<IPAInterface> ipa_; > > + EventNotifier *notifier_; > > + IPAModule *ipaModule_; > > + unsigned int fd_; > > + int8_t data_; > > How about naming this status_ and making it an enum ? > > > +}; > > + > > +TEST_REGISTER(IPAInterfaceTest) > > diff --git a/test/ipa/meson.build b/test/ipa/meson.build > > index e174671d05e1..c501fcf99aed 100644 > > --- a/test/ipa/meson.build > > +++ b/test/ipa/meson.build > > @@ -1,5 +1,6 @@ > > ipa_test = [ > > - ['ipa_module_test', 'ipa_module_test.cpp'], > > + ['ipa_module_test', 'ipa_module_test.cpp'], > > + ['ipa_interface_test', 'ipa_interface_test.cpp'], > > ] > > > > foreach t : ipa_test
diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp new file mode 100644 index 000000000000..83eef7440fa4 --- /dev/null +++ b/test/ipa/ipa_interface_test.cpp @@ -0,0 +1,147 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * ipa_interface_test.cpp - Test the IPA interface communication using the VIMC + * dummy IPA module + */ + +#include <iostream> + +#include <fcntl.h> +#include <string.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> + +#include <libcamera/event_dispatcher.h> +#include <libcamera/event_notifier.h> +#include <libcamera/timer.h> + +#include "ipa_module.h" + +#include "test.h" +#include "thread.h" + +using namespace std; +using namespace libcamera; + +/* Keep with in sync with the VIMC IPA module. */ +static const char *ipaFifoPath = "/tmp/vimc_ipa_fifo"; +#define IPA_INIT_CODE 0x01 + +static const char *vimcModulePath = "src/ipa/ipa_vimc.so"; + +class IPAInterfaceTest : public Test, public Object +{ +public: + IPAInterfaceTest() + : fd_(0), data_(0) + { + } + + ~IPAInterfaceTest() + { + /* + * Delete the IPA Interface instance -before- deleting the + * module used to create it, as the IPAModule destructor closes + * the IPA shared object, resulting in segfault at interface + * deletion time. + */ + IPAInterface *ipa = ipa_.release(); + delete ipa; + + if (ipaModule_) + delete ipaModule_; + + delete notifier_; + } + +protected: + int init() override + { + int ret = mkfifo(ipaFifoPath, S_IRUSR | S_IWUSR); + if (ret) { + cerr << "Failed to create IPA test fifo at: " + << ipaFifoPath << ": " << strerror(errno) << endl; + return TestFail; + } + + ret = open(ipaFifoPath, O_RDONLY | O_NONBLOCK); + if (ret < 0) { + cerr << "Failed to open IPA test fifo at: " + << ipaFifoPath << ": " << strerror(errno) << endl; + unlink(ipaFifoPath); + return TestFail; + } + fd_ = ret; + + notifier_ = new EventNotifier(fd_, EventNotifier::Read, this); + notifier_->activated.connect(this, &IPAInterfaceTest::readFifo); + + return TestPass; + } + + int run() override + { + EventDispatcher *dispatcher = thread()->eventDispatcher(); + Timer timer; + + ipaModule_ = new IPAModule(vimcModulePath); + if (!ipaModule_ || !ipaModule_->isValid()) { + cerr << "Failed to create VIMC IPA module at: " + << vimcModulePath << endl; + return TestFail; + } + + if (!ipaModule_->load()) { + cerr << "Failed to load VIMC IPA module" << endl; + return TestFail; + } + + ipa_ = ipaModule_->createInstance(); + if (!ipa_) { + cerr << "Failed to create VIMC IPA interface" << endl; + return TestFail; + } + + /* Test initialization of IPA module. */ + ipa_->init(); + timer.start(1000); + while (timer.isRunning() && data_ != IPA_INIT_CODE) + dispatcher->processEvents(); + + if (data_ != IPA_INIT_CODE) { + cerr << "Failed to test IPA initialization sequence" + << endl; + return TestFail; + } + + return TestPass; + } + + void cleanup() override + { + close(fd_); + unlink(ipaFifoPath); + } + +private: + void readFifo(EventNotifier *notifier) + { + size_t s = read(notifier->fd(), &data_, 1); + if (s < 0) { + cerr << "Failed to to read from IPA test fifo at: " + << ipaFifoPath << strerror(errno) << endl; + data_ = -1; + } + } + + std::unique_ptr<IPAInterface> ipa_; + EventNotifier *notifier_; + IPAModule *ipaModule_; + unsigned int fd_; + int8_t data_; +}; + +TEST_REGISTER(IPAInterfaceTest) diff --git a/test/ipa/meson.build b/test/ipa/meson.build index e174671d05e1..c501fcf99aed 100644 --- a/test/ipa/meson.build +++ b/test/ipa/meson.build @@ -1,5 +1,6 @@ ipa_test = [ - ['ipa_module_test', 'ipa_module_test.cpp'], + ['ipa_module_test', 'ipa_module_test.cpp'], + ['ipa_interface_test', 'ipa_interface_test.cpp'], ] foreach t : ipa_test
Implementing a basic test for IPA Interface using the VIMC dummy IPA module. The test implements a communication channel between the test and the dummy IPA module to verify the success of the IPA interactions. Test the only available operation defined by the IPA interface by receiving a confirmation code on the fifo communication channel. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- test/ipa/ipa_interface_test.cpp | 147 ++++++++++++++++++++++++++++++++ test/ipa/meson.build | 3 +- 2 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 test/ipa/ipa_interface_test.cpp