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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 06, 2019 at 10:08:52PM +0200, Jacopo Mondi wrote:
> Implementing a basic test for IPA Interface using the VIMC dummy IPA

s/Implementing/Implement/

> module. The test implements an out-of-band feedback 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 | 138 ++++++++++++++++++++++++++++++++
>  test/ipa/meson.build            |   3 +-
>  2 files changed, 140 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..71bdcc651392
> --- /dev/null
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -0,0 +1,138 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_interface_test.cpp - Test the IPA interface
> + */
> +
> +#include <fcntl.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <iostream>
> +
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/event_notifier.h>
> +#include <libcamera/timer.h>

I'd add a blank line here, or move it before the libcamera headers to
sort them alphabetically (blank line is my preference).

> +#include <ipa/ipa_vimc.h>
> +
> +#include "device_enumerator.h"
> +#include "ipa_manager.h"
> +#include "ipa_module.h"
> +#include "pipeline_handler.h"
> +#include "test.h"
> +#include "thread.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class IPAInterfaceTest : public Test, public Object
> +{
> +public:
> +	IPAInterfaceTest()
> +		: trace_(IPAOperationNone), notifier_(nullptr), fd_(-1)
> +	{
> +	}
> +
> +	~IPAInterfaceTest()
> +	{
> +		delete notifier_;
> +	}
> +
> +protected:
> +	int init() override
> +	{
> +		/* Create a pipeline handler for vimc. */
> +		std::vector<PipelineHandlerFactory *> &factories =
> +			PipelineHandlerFactory::factories();
> +		for (PipelineHandlerFactory *factory : factories) {
> +			if (factory->name() == "PipelineHandlerVimc") {
> +				pipe_ = factory->create(nullptr);
> +				break;
> +			}
> +		}
> +
> +		if (!pipe_) {
> +			cerr << "Vimc pipeline not found" << endl;
> +			return TestPass;
> +		}
> +
> +		/* Create and open the communication FIFO. */
> +		int ret = mkfifo(vimcFifoPath, S_IRUSR | S_IWUSR);
> +		if (ret) {
> +			ret = -errno;
> +			cerr << "Failed to create IPA test FIFO at: "
> +			     << vimcFifoPath << ": " << strerror(-ret) << endl;

Could you please re-read my comments on the previous version regarding
this message and the next one ? After addressing them,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +			return TestFail;
> +		}
> +
> +		ret = open(vimcFifoPath, O_RDONLY | O_NONBLOCK);
> +		if (ret < 0) {
> +			ret = -errno;
> +			cerr << "Failed to open IPA test FIFO at: "
> +			     << vimcFifoPath << ": " << strerror(-ret) << endl;
> +			unlink(vimcFifoPath);
> +			return TestFail;
> +		}
> +		fd_ = ret;
> +
> +		notifier_ = new EventNotifier(fd_, EventNotifier::Read, this);
> +		notifier_->activated.connect(this, &IPAInterfaceTest::readTrace);
> +
> +		return TestPass;
> +	}
> +
> +	int run() override
> +	{
> +		EventDispatcher *dispatcher = thread()->eventDispatcher();
> +		Timer timer;
> +
> +		ipa_ = IPAManager::instance()->createIPA(pipe_.get(), 0, 0);
> +		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() && trace_ != IPAOperationInit)
> +			dispatcher->processEvents();
> +
> +		if (trace_ != IPAOperationInit) {
> +			cerr << "Failed to test IPA initialization sequence"
> +			     << endl;
> +			return TestFail;
> +		}
> +
> +		return TestPass;
> +	}
> +
> +	void cleanup() override
> +	{
> +		close(fd_);
> +		unlink(vimcFifoPath);
> +	}
> +
> +private:
> +	void readTrace(EventNotifier *notifier)
> +	{
> +		ssize_t s = read(notifier->fd(), &trace_, sizeof(trace_));
> +		if (s < 0) {
> +			int ret = -errno;
> +			cerr << "Failed to read from IPA test FIFO: "
> +			     << strerror(-ret) << endl;
> +			trace_ = IPAOperationNone;
> +		}
> +	}
> +
> +	std::shared_ptr<PipelineHandler> pipe_;
> +	std::unique_ptr<IPAInterface> ipa_;
> +	enum IPAOperationCode trace_;
> +	EventNotifier *notifier_;
> +	int fd_;
> +};
> +
> +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