[libcamera-devel] [PATCH v2 6/6] test: ipa: Add test for the IPA Interface

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Oct 04, 2019 at 06:37:34PM +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

s/a communication channel/an out-of-band feedback communication channel/
?

> 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>
> ---
>  src/ipa/meson.build             |   2 +
>  test/ipa/ipa_interface_test.cpp | 136 ++++++++++++++++++++++++++++++++
>  test/ipa/meson.build            |  10 ++-
>  3 files changed, 146 insertions(+), 2 deletions(-)
>  create mode 100644 test/ipa/ipa_interface_test.cpp
> 
> diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> index 2827dc0303b2..3e11e166f283 100644
> --- a/src/ipa/meson.build
> +++ b/src/ipa/meson.build
> @@ -5,6 +5,8 @@ ipa_vimc_sources = [
>  
>  ipa_install_dir = join_paths(get_option('libdir'), 'libcamera')
>  
> +ipa_internal_includes =  include_directories('.')

I think you won't need this if you move the vimc IPA header to
include/ipa/

> +
>  ipa_includes = [
>      libcamera_includes,
>      libcamera_internal_includes,
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> new file mode 100644
> index 000000000000..b6d0a4e7de20
> --- /dev/null
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -0,0 +1,136 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * ipa_interface_test.cpp - Test the IPA interface
> + */
> +
> +#include <iostream>
> +
> +#include <fcntl.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +

Here too the C headers should come before the C++ headers if you want to
separate them.

> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/event_notifier.h>
> +#include <libcamera/timer.h>
> +
> +#include "device_enumerator.h"
> +#include "ipa_manager.h"
> +#include "ipa_module.h"
> +#include "pipeline_handler.h"
> +#include "test.h"
> +#include "thread.h"
> +
> +#include "ipa_vimc.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);

Not something that you need to address now (but feel free to still do so
:-)), what will happen if this test is interrupted for any reason
(including a crash) ?

> +		if (ret) {
> +			cerr << "Failed to create IPA test fifo at: "

"... test FIFO at '" << path << "': " ...

> +			     << vimcFifoPath << ": " << strerror(errno) << endl;

You need to store -errno in ret as usual.

> +			return TestFail;
> +		}
> +
> +		ret = open(vimcFifoPath, O_RDONLY | O_NONBLOCK);
> +		if (ret < 0) {
> +			cerr << "Failed to open IPA test fifo at: "
> +			     << vimcFifoPath << ": " << strerror(errno) << endl;

Same two comments.

> +			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) {
> +			cerr << "Failed to read from IPA test fifo: "
> +			     << strerror(errno) << endl;

Same here.

> +			trace_ = IPAOperationNone;
> +		}
> +	}
> +
> +	std::shared_ptr<PipelineHandler> pipe_;
> +	std::unique_ptr<IPAInterface> ipa_;
> +	enum IPAOperationCodes trace_;
> +	EventNotifier *notifier_;
> +	int fd_;
> +};
> +
> +TEST_REGISTER(IPAInterfaceTest)
> diff --git a/test/ipa/meson.build b/test/ipa/meson.build
> index e174671d05e1..d19de18ab97e 100644
> --- a/test/ipa/meson.build
> +++ b/test/ipa/meson.build
> @@ -1,12 +1,18 @@
>  ipa_test = [
> -    ['ipa_module_test', 'ipa_module_test.cpp'],
> +    ['ipa_module_test',     'ipa_module_test.cpp'],
> +    ['ipa_interface_test',  'ipa_interface_test.cpp'],
> +]
> +
> +test_includes_ipa = [
> +    test_includes_internal,
> +    ipa_internal_includes,
>  ]
>  
>  foreach t : ipa_test
>      exe = executable(t[0], t[1],
>                       dependencies : libcamera_dep,
>                       link_with : test_libraries,
> -                     include_directories : test_includes_internal)
> +                     include_directories : test_includes_ipa)
>  
>      test(t[0], exe, suite : 'ipa')
>  endforeach

-- 
Regards,

Laurent Pinchart