[libcamera-devel] [PATCH v4 2/2] test: pipeline: IPU3: Add IPU3 pipeline test

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Jan 21, 2019 at 04:07:56PM +0100, Jacopo Mondi wrote:
> Add test for the Intel IPU3 pipeline that lists all the cameras
> registered in the system and verifies the result matches the expected.
> 
> This test is meant to be run on IPU3 platforms, it gets skipped
> otherwise.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  test/meson.build                          |   3 +
>  test/pipeline/ipu3/ipu3_pipeline_test.cpp | 140 ++++++++++++++++++++++
>  test/pipeline/ipu3/meson.build            |  11 ++
>  test/pipeline/meson.build                 |   1 +
>  4 files changed, 155 insertions(+)
>  create mode 100644 test/pipeline/ipu3/ipu3_pipeline_test.cpp
>  create mode 100644 test/pipeline/ipu3/meson.build
>  create mode 100644 test/pipeline/meson.build
> 
> diff --git a/test/meson.build b/test/meson.build
> index fb6b115..594082a 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -1,6 +1,9 @@
>  subdir('libtest')
>  
>  subdir('media_device')
> +
> +subdir('pipeline')
> +

I think Kieran mentioned that blank are not needed here.

>  subdir('v4l2_device')
>  
>  public_tests = [
> diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> new file mode 100644
> index 0000000..deaee40
> --- /dev/null
> +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
> @@ -0,0 +1,140 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.

Happy new year :-)

> + *
> + * ipu3_pipeline_test.cpp - Intel IPU3 pipeline test
> + */
> +
> +#include <iostream>
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>

t comes after s

> +#include <unistd.h>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
> +
> +#include "media_device.h"
> +#include "media_object.h"
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;

When don't make use of the using directive in libcamera, do you think we
should skip it from tests too ? "using namespace libcamera" makes sense
as it would get pretty tedious to type otherwise, but I wonder if we
shouldn't write std::string explicitly instead of string.

> +
> +/*
> + * Verify that the Intel IPU3 pipeline handler gets matched and cameras
> + * are enumerated correctly.
> + *
> + * The test is supposed to be run on an IPU3 platform, otherwise it gets
> + * skipped.
> + *
> + * The test lists all cameras registered in the system, if any camera is
> + * available at all.
> + */
> +class IPU3PipelineTest : public Test
> +{
> +protected:
> +	int init();
> +	int run();
> +	void cleanup();
> +
> +private:
> +	CameraManager *cameraManager_;
> +	unsigned int sensors_;
> +};
> +
> +int IPU3PipelineTest::init()
> +{
> +	const string devnode("/dev/media");
> +	bool cio2 = false;
> +	bool imgu = false;
> +	unsigned int i;
> +	int ret;
> +
> +	sensors_ = 0;
> +
> +	/*
> +	 * Test all media devices from /dev/media0 to /dev/media256.
> +	 * Media device numbering might not be continue, and we cannot stop
> +	 * as soon as we hit a non accessible media device.
> +	 */

How about using the device enumerator instead, as in
media_device_link_test.cpp ? Make sure to delete it before starting the
camera manager though, otherwise there could possibly be conflicts.

> +	for (i = 0; i < 256; i++) {
> +		string mediadev = devnode + to_string(i);
> +		struct stat pstat = { };
> +
> +		if (stat(mediadev.c_str(), &pstat))
> +			continue;
> +
> +		MediaDevice dev(mediadev);
> +		if (dev.open()) {
> +			cerr << "Failed to open media device " << mediadev << endl;
> +			return TestFail;
> +		}
> +
> +		if (dev.driver() == "ipu3-imgu") {
> +			imgu = true;
> +		} else if (dev.driver() == "ipu3-cio2") {
> +			/*
> +			 * Camera sensor are connected to the CIO2 unit.
> +			 * Count how many sensors are connected in the system
> +			 * and later verify this matches the number of registered
> +			 * cameras.
> +			 */
> +			ret = dev.populate();
> +			if (ret) {
> +				cerr << "Failed to populate media device " << dev.devnode() << endl;
> +				return TestFail;
> +			}
> +
> +			vector<MediaEntity *> entities = dev.entities();
> +			for (MediaEntity *entity : entities) {
> +				if (entity->function() == MEDIA_ENT_F_CAM_SENSOR)
> +					sensors_++;
> +			}
> +
> +			cio2 = true;
> +		}
> +
> +		dev.close();
> +
> +		if (imgu && cio2)
> +			break;
> +	}
> +
> +	/* Not running on an IPU3 system: skip test. */
> +	if (!(imgu && cio2))

You may want to log a message.

> +		return TestSkip;
> +
> +	cameraManager_ = CameraManager::instance();
> +	ret = cameraManager_->start();
> +	if (ret) {
> +		cerr << "Failed to start the CameraManager" << endl;
> +		return TestFail;
> +	}
> +
> +	return 0;
> +}
> +
> +int IPU3PipelineTest::run()
> +{
> +	unsigned int cameras = 0;
> +	for (const shared_ptr<Camera> &cam : cameraManager_->cameras()) {
> +		cout << "Found camera '" << cam->name() << "'" << endl;
> +		cameras++;
> +	}

A possible simplification:

	const std::vector<std::shared_ptr<Camera>> &cameras = cameraManager_->cameras();
	for (const std::shared_ptr<Camera> &cam : cameras)
		cout << "Found camera '" << cam->name() << "'" << endl;

	if (cameras.size() != sensors_) {
		cerr << cameras.size() << ...

And I think some of that code could be a good candidate for auto
variables (I don't mind them in test code as much as I do in the library
itself). It's up to you, your code works fine too.

Apart from this,

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

> +
> +	if (cameras != sensors_) {
> +		cerr << cameras << " cameras registered, but " << sensors_
> +		     << " were expected" << endl;
> +		return TestFail;
> +	}
> +
> +	return TestPass;
> +}
> +
> +void IPU3PipelineTest::cleanup()
> +{
> +	cameraManager_->stop();
> +}
> +
> +TEST_REGISTER(IPU3PipelineTest)
> diff --git a/test/pipeline/ipu3/meson.build b/test/pipeline/ipu3/meson.build
> new file mode 100644
> index 0000000..caba5c7
> --- /dev/null
> +++ b/test/pipeline/ipu3/meson.build
> @@ -0,0 +1,11 @@
> +ipu3_test = [
> +    ['ipu3_pipeline_test',	      'ipu3_pipeline_test.cpp'],
> +]
> +
> +foreach t : ipu3_test
> +    exe = executable(t[0], t[1],
> +                     link_with : test_libraries,
> +                     include_directories : test_includes_internal)
> +
> +    test(t[0], exe, suite: 'ipu3', is_parallel: false)
> +endforeach
> diff --git a/test/pipeline/meson.build b/test/pipeline/meson.build
> new file mode 100644
> index 0000000..f434c79
> --- /dev/null
> +++ b/test/pipeline/meson.build
> @@ -0,0 +1 @@
> +subdir('ipu3')

-- 
Regards,

Laurent Pinchart