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

[Laurent Pinchart]

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 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) {
> +			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