[libcamera-devel] [PATCH 4/5] test: ipa: Add test for the IPA Interface

[Laurent Pinchart]

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 at 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

-- 
Regards,

Laurent Pinchart