[libcamera-devel,v4,2/2] test: pipeline: IPU3: Add IPU3 pipeline test
diff mbox series

Message ID 20190121150756.14982-3-jacopo@jmondi.org
State Accepted
Headers show
Series
  • libcamera: pipeline: Add Intel IPU3 pipeline handler
Related show

Commit Message

Jacopo Mondi Jan. 21, 2019, 3:07 p.m. UTC 
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@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

Comments

Laurent Pinchart Jan. 21, 2019, 8:22 p.m. UTC | #1 
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@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@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')
Jacopo Mondi Jan. 22, 2019, 9:07 a.m. UTC | #2 
Hi Laurent,

On Mon, Jan 21, 2019 at 10:22:40PM +0200, Laurent Pinchart wrote:
> 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@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.
>

Ok!


> >  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 :-)
>

eh :/

> > + *
> > + * 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.
>

Most, but not all, tests use namespace std.
Personally, I'm fine with both, with a slight preference for std::
but I would like to have all tests doing the same.

> > +
> > +/*
> > + * 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.
>

I'll try, it is surely nicer and I should be able to obtain the same
behavior

> > +	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@ideasonboard.com>

Thanks, I'll fix the above comments and push

Thanks
   j

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

Patch
diff mbox series 

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')
+
 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.
+ *
+ * ipu3_pipeline_test.cpp - Intel IPU3 pipeline test
+ */
+
+#include <iostream>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#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;
+
+/*
+ * 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.
+	 */
+	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))
+		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++;
+	}
+
+	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')