[libcamera-devel] [PATCH v4 2/2] test: pipeline: IPU3: Add IPU3 pipeline test
Jacopo Mondi
jacopo at jmondi.org
Tue Jan 22 10:07:08 CET 2019
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 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.
>
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190122/fbc4c13f/attachment.sig>
More information about the libcamera-devel
mailing list