[{"id":458,"web_url":"https://patchwork.libcamera.org/comment/458/","msgid":"<20190121202240.GB4439@pendragon.ideasonboard.com>","date":"2019-01-21T20:22:40","subject":"Re: [libcamera-devel] [PATCH v4 2/2] test: pipeline: IPU3: Add IPU3\n\tpipeline test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Jan 21, 2019 at 04:07:56PM +0100, Jacopo Mondi wrote:\n> Add test for the Intel IPU3 pipeline that lists all the cameras\n> registered in the system and verifies the result matches the expected.\n> \n> This test is meant to be run on IPU3 platforms, it gets skipped\n> otherwise.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> test/meson.build | 3 +\n> test/pipeline/ipu3/ipu3_pipeline_test.cpp | 140 ++++++++++++++++++++++\n> test/pipeline/ipu3/meson.build | 11 ++\n> test/pipeline/meson.build | 1 +\n> 4 files changed, 155 insertions(+)\n> create mode 100644 test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> create mode 100644 test/pipeline/ipu3/meson.build\n> create mode 100644 test/pipeline/meson.build\n> \n> diff --git a/test/meson.build b/test/meson.build\n> index fb6b115..594082a 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -1,6 +1,9 @@\n> subdir('libtest')\n> \n> subdir('media_device')\n> +\n> +subdir('pipeline')\n> +\n\nI think Kieran mentioned that blank are not needed here.\n\n> subdir('v4l2_device')\n> \n> public_tests = [\n> diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> new file mode 100644\n> index 0000000..deaee40\n> --- /dev/null\n> +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> @@ -0,0 +1,140 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n\nHappy new year :-)\n\n> + *\n> + * ipu3_pipeline_test.cpp - Intel IPU3 pipeline test\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n\nt comes after s\n\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"media_device.h\"\n> +#include \"media_object.h\"\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n\nWhen don't make use of the using directive in libcamera, do you think we\nshould skip it from tests too ? \"using namespace libcamera\" makes sense\nas it would get pretty tedious to type otherwise, but I wonder if we\nshouldn't write std::string explicitly instead of string.\n\n> +\n> +/*\n> + * Verify that the Intel IPU3 pipeline handler gets matched and cameras\n> + * are enumerated correctly.\n> + *\n> + * The test is supposed to be run on an IPU3 platform, otherwise it gets\n> + * skipped.\n> + *\n> + * The test lists all cameras registered in the system, if any camera is\n> + * available at all.\n> + */\n> +class IPU3PipelineTest : public Test\n> +{\n> +protected:\n> +\tint init();\n> +\tint run();\n> +\tvoid cleanup();\n> +\n> +private:\n> +\tCameraManager *cameraManager_;\n> +\tunsigned int sensors_;\n> +};\n> +\n> +int IPU3PipelineTest::init()\n> +{\n> +\tconst string devnode(\"/dev/media\");\n> +\tbool cio2 = false;\n> +\tbool imgu = false;\n> +\tunsigned int i;\n> +\tint ret;\n> +\n> +\tsensors_ = 0;\n> +\n> +\t/*\n> +\t * Test all media devices from /dev/media0 to /dev/media256.\n> +\t * Media device numbering might not be continue, and we cannot stop\n> +\t * as soon as we hit a non accessible media device.\n> +\t */\n\nHow about using the device enumerator instead, as in\nmedia_device_link_test.cpp ? Make sure to delete it before starting the\ncamera manager though, otherwise there could possibly be conflicts.\n\n> +\tfor (i = 0; i < 256; i++) {\n> +\t\tstring mediadev = devnode + to_string(i);\n> +\t\tstruct stat pstat = { };\n> +\n> +\t\tif (stat(mediadev.c_str(), &pstat))\n> +\t\t\tcontinue;\n> +\n> +\t\tMediaDevice dev(mediadev);\n> +\t\tif (dev.open()) {\n> +\t\t\tcerr << \"Failed to open media device \" << mediadev << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (dev.driver() == \"ipu3-imgu\") {\n> +\t\t\timgu = true;\n> +\t\t} else if (dev.driver() == \"ipu3-cio2\") {\n> +\t\t\t/*\n> +\t\t\t * Camera sensor are connected to the CIO2 unit.\n> +\t\t\t * Count how many sensors are connected in the system\n> +\t\t\t * and later verify this matches the number of registered\n> +\t\t\t * cameras.\n> +\t\t\t */\n> +\t\t\tret = dev.populate();\n> +\t\t\tif (ret) {\n> +\t\t\t\tcerr << \"Failed to populate media device \" << dev.devnode() << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\tvector entities = dev.entities();\n> +\t\t\tfor (MediaEntity *entity : entities) {\n> +\t\t\t\tif (entity->function() == MEDIA_ENT_F_CAM_SENSOR)\n> +\t\t\t\t\tsensors_++;\n> +\t\t\t}\n> +\n> +\t\t\tcio2 = true;\n> +\t\t}\n> +\n> +\t\tdev.close();\n> +\n> +\t\tif (imgu && cio2)\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\t/* Not running on an IPU3 system: skip test. */\n> +\tif (!(imgu && cio2))\n\nYou may want to log a message.\n\n> +\t\treturn TestSkip;\n> +\n> +\tcameraManager_ = CameraManager::instance();\n> +\tret = cameraManager_->start();\n> +\tif (ret) {\n> +\t\tcerr << \"Failed to start the CameraManager\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int IPU3PipelineTest::run()\n> +{\n> +\tunsigned int cameras = 0;\n> +\tfor (const shared_ptr &cam : cameraManager_->cameras()) {\n> +\t\tcout << \"Found camera '\" << cam->name() << \"'\" << endl;\n> +\t\tcameras++;\n> +\t}\n\nA possible simplification:\n\n\tconst std::vector> &cameras = cameraManager_->cameras();\n\tfor (const std::shared_ptr &cam : cameras)\n\t\tcout << \"Found camera '\" << cam->name() << \"'\" << endl;\n\n\tif (cameras.size() != sensors_) {\n\t\tcerr << cameras.size() << ...\n\nAnd I think some of that code could be a good candidate for auto\nvariables (I don't mind them in test code as much as I do in the library\nitself). It's up to you, your code works fine too.\n\nApart from this,\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\tif (cameras != sensors_) {\n> +\t\tcerr << cameras << \" cameras registered, but \" << sensors_\n> +\t\t << \" were expected\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\treturn TestPass;\n> +}\n> +\n> +void IPU3PipelineTest::cleanup()\n> +{\n> +\tcameraManager_->stop();\n> +}\n> +\n> +TEST_REGISTER(IPU3PipelineTest)\n> diff --git a/test/pipeline/ipu3/meson.build b/test/pipeline/ipu3/meson.build\n> new file mode 100644\n> index 0000000..caba5c7\n> --- /dev/null\n> +++ b/test/pipeline/ipu3/meson.build\n> @@ -0,0 +1,11 @@\n> +ipu3_test = [\n> + ['ipu3_pipeline_test',\t 'ipu3_pipeline_test.cpp'],\n> +]\n> +\n> +foreach t : ipu3_test\n> + exe = executable(t[0], t[1],\n> + link_with : test_libraries,\n> + include_directories : test_includes_internal)\n> +\n> + test(t[0], exe, suite: 'ipu3', is_parallel: false)\n> +endforeach\n> diff --git a/test/pipeline/meson.build b/test/pipeline/meson.build\n> new file mode 100644\n> index 0000000..f434c79\n> --- /dev/null\n> +++ b/test/pipeline/meson.build\n> @@ -0,0 +1 @@\n> +subdir('ipu3')","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B83FE60B1D\n\tfor ;\n\tMon, 21 Jan 2019 21:22:41 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 25C4253E;\n\tMon, 21 Jan 2019 21:22:41 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548102161;\n\tbh=4pvyzScMg5771fJzhkhvtz+3EDcl32YKOJwS14D+QTo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NZl8sBzYkLmOAD78/BznyKH8XbRnEHNB0tQjPeajOlu6VH9Q+7clfIBnHfZStkm5+\n\twvlL9P3CfKvGAO4aqkBTjyIJbVyGFZgYymIZ+ZVA112Vqz6F65POX7lP8m1aCD4Rzl\n\tRpXb+cwo9cfLTHXhlsd6esgYl8DQzxYtawnPdZKI=","Date":"Mon, 21 Jan 2019 22:22:40 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190121202240.GB4439@pendragon.ideasonboard.com>","References":"<20190121150756.14982-1-jacopo@jmondi.org>\n\t<20190121150756.14982-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190121150756.14982-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 2/2] test: pipeline: IPU3: Add IPU3\n\tpipeline test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 21 Jan 2019 20:22:41 -0000"}},{"id":472,"web_url":"https://patchwork.libcamera.org/comment/472/","msgid":"<20190122090708.xlb2ddgbhhmyrnf7@uno.localdomain>","date":"2019-01-22T09:07:08","subject":"Re: [libcamera-devel] [PATCH v4 2/2] test: pipeline: IPU3: Add IPU3\n\tpipeline test","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jan 21, 2019 at 10:22:40PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Jan 21, 2019 at 04:07:56PM +0100, Jacopo Mondi wrote:\n> > Add test for the Intel IPU3 pipeline that lists all the cameras\n> > registered in the system and verifies the result matches the expected.\n> >\n> > This test is meant to be run on IPU3 platforms, it gets skipped\n> > otherwise.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > test/meson.build | 3 +\n> > test/pipeline/ipu3/ipu3_pipeline_test.cpp | 140 ++++++++++++++++++++++\n> > test/pipeline/ipu3/meson.build | 11 ++\n> > test/pipeline/meson.build | 1 +\n> > 4 files changed, 155 insertions(+)\n> > create mode 100644 test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > create mode 100644 test/pipeline/ipu3/meson.build\n> > create mode 100644 test/pipeline/meson.build\n> >\n> > diff --git a/test/meson.build b/test/meson.build\n> > index fb6b115..594082a 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -1,6 +1,9 @@\n> > subdir('libtest')\n> >\n> > subdir('media_device')\n> > +\n> > +subdir('pipeline')\n> > +\n>\n> I think Kieran mentioned that blank are not needed here.\n>\n\nOk!\n\n\n> > subdir('v4l2_device')\n> >\n> > public_tests = [\n> > diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > new file mode 100644\n> > index 0000000..deaee40\n> > --- /dev/null\n> > +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > @@ -0,0 +1,140 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n>\n> Happy new year :-)\n>\n\neh :/\n\n> > + *\n> > + * ipu3_pipeline_test.cpp - Intel IPU3 pipeline test\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n>\n> t comes after s\n>\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"media_device.h\"\n> > +#include \"media_object.h\"\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> > +using namespace libcamera;\n>\n> When don't make use of the using directive in libcamera, do you think we\n> should skip it from tests too ? \"using namespace libcamera\" makes sense\n> as it would get pretty tedious to type otherwise, but I wonder if we\n> shouldn't write std::string explicitly instead of string.\n>\n\nMost, but not all, tests use namespace std.\nPersonally, I'm fine with both, with a slight preference for std::\nbut I would like to have all tests doing the same.\n\n> > +\n> > +/*\n> > + * Verify that the Intel IPU3 pipeline handler gets matched and cameras\n> > + * are enumerated correctly.\n> > + *\n> > + * The test is supposed to be run on an IPU3 platform, otherwise it gets\n> > + * skipped.\n> > + *\n> > + * The test lists all cameras registered in the system, if any camera is\n> > + * available at all.\n> > + */\n> > +class IPU3PipelineTest : public Test\n> > +{\n> > +protected:\n> > +\tint init();\n> > +\tint run();\n> > +\tvoid cleanup();\n> > +\n> > +private:\n> > +\tCameraManager *cameraManager_;\n> > +\tunsigned int sensors_;\n> > +};\n> > +\n> > +int IPU3PipelineTest::init()\n> > +{\n> > +\tconst string devnode(\"/dev/media\");\n> > +\tbool cio2 = false;\n> > +\tbool imgu = false;\n> > +\tunsigned int i;\n> > +\tint ret;\n> > +\n> > +\tsensors_ = 0;\n> > +\n> > +\t/*\n> > +\t * Test all media devices from /dev/media0 to /dev/media256.\n> > +\t * Media device numbering might not be continue, and we cannot stop\n> > +\t * as soon as we hit a non accessible media device.\n> > +\t */\n>\n> How about using the device enumerator instead, as in\n> media_device_link_test.cpp ? Make sure to delete it before starting the\n> camera manager though, otherwise there could possibly be conflicts.\n>\n\nI'll try, it is surely nicer and I should be able to obtain the same\nbehavior\n\n> > +\tfor (i = 0; i < 256; i++) {\n> > +\t\tstring mediadev = devnode + to_string(i);\n> > +\t\tstruct stat pstat = { };\n> > +\n> > +\t\tif (stat(mediadev.c_str(), &pstat))\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tMediaDevice dev(mediadev);\n> > +\t\tif (dev.open()) {\n> > +\t\t\tcerr << \"Failed to open media device \" << mediadev << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (dev.driver() == \"ipu3-imgu\") {\n> > +\t\t\timgu = true;\n> > +\t\t} else if (dev.driver() == \"ipu3-cio2\") {\n> > +\t\t\t/*\n> > +\t\t\t * Camera sensor are connected to the CIO2 unit.\n> > +\t\t\t * Count how many sensors are connected in the system\n> > +\t\t\t * and later verify this matches the number of registered\n> > +\t\t\t * cameras.\n> > +\t\t\t */\n> > +\t\t\tret = dev.populate();\n> > +\t\t\tif (ret) {\n> > +\t\t\t\tcerr << \"Failed to populate media device \" << dev.devnode() << endl;\n> > +\t\t\t\treturn TestFail;\n> > +\t\t\t}\n> > +\n> > +\t\t\tvector entities = dev.entities();\n> > +\t\t\tfor (MediaEntity *entity : entities) {\n> > +\t\t\t\tif (entity->function() == MEDIA_ENT_F_CAM_SENSOR)\n> > +\t\t\t\t\tsensors_++;\n> > +\t\t\t}\n> > +\n> > +\t\t\tcio2 = true;\n> > +\t\t}\n> > +\n> > +\t\tdev.close();\n> > +\n> > +\t\tif (imgu && cio2)\n> > +\t\t\tbreak;\n> > +\t}\n> > +\n> > +\t/* Not running on an IPU3 system: skip test. */\n> > +\tif (!(imgu && cio2))\n>\n> You may want to log a message.\n>\n> > +\t\treturn TestSkip;\n> > +\n> > +\tcameraManager_ = CameraManager::instance();\n> > +\tret = cameraManager_->start();\n> > +\tif (ret) {\n> > +\t\tcerr << \"Failed to start the CameraManager\" << endl;\n> > +\t\treturn TestFail;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int IPU3PipelineTest::run()\n> > +{\n> > +\tunsigned int cameras = 0;\n> > +\tfor (const shared_ptr &cam : cameraManager_->cameras()) {\n> > +\t\tcout << \"Found camera '\" << cam->name() << \"'\" << endl;\n> > +\t\tcameras++;\n> > +\t}\n>\n> A possible simplification:\n>\n> \tconst std::vector> &cameras = cameraManager_->cameras();\n> \tfor (const std::shared_ptr &cam : cameras)\n> \t\tcout << \"Found camera '\" << cam->name() << \"'\" << endl;\n>\n> \tif (cameras.size() != sensors_) {\n> \t\tcerr << cameras.size() << ...\n>\n> And I think some of that code could be a good candidate for auto\n> variables (I don't mind them in test code as much as I do in the library\n> itself). It's up to you, your code works fine too.\n>\n> Apart from this,\n>\n> Reviewed-by: Laurent Pinchart \n\nThanks, I'll fix the above comments and push\n\nThanks\n j\n\n>\n> > +\n> > +\tif (cameras != sensors_) {\n> > +\t\tcerr << cameras << \" cameras registered, but \" << sensors_\n> > +\t\t << \" were expected\" << endl;\n> > +\t\treturn TestFail;\n> > +\t}\n> > +\n> > +\treturn TestPass;\n> > +}\n> > +\n> > +void IPU3PipelineTest::cleanup()\n> > +{\n> > +\tcameraManager_->stop();\n> > +}\n> > +\n> > +TEST_REGISTER(IPU3PipelineTest)\n> > diff --git a/test/pipeline/ipu3/meson.build b/test/pipeline/ipu3/meson.build\n> > new file mode 100644\n> > index 0000000..caba5c7\n> > --- /dev/null\n> > +++ b/test/pipeline/ipu3/meson.build\n> > @@ -0,0 +1,11 @@\n> > +ipu3_test = [\n> > + ['ipu3_pipeline_test',\t 'ipu3_pipeline_test.cpp'],\n> > +]\n> > +\n> > +foreach t : ipu3_test\n> > + exe = executable(t[0], t[1],\n> > + link_with : test_libraries,\n> > + include_directories : test_includes_internal)\n> > +\n> > + test(t[0], exe, suite: 'ipu3', is_parallel: false)\n> > +endforeach\n> > diff --git a/test/pipeline/meson.build b/test/pipeline/meson.build\n> > new file mode 100644\n> > index 0000000..f434c79\n> > --- /dev/null\n> > +++ b/test/pipeline/meson.build\n> > @@ -0,0 +1 @@\n> > +subdir('ipu3')\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D713860C81\n\tfor ;\n\tTue, 22 Jan 2019 10:06:56 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 4EE4A40011;\n\tTue, 22 Jan 2019 09:06:56 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 22 Jan 2019 10:07:08 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190122090708.xlb2ddgbhhmyrnf7@uno.localdomain>","References":"<20190121150756.14982-1-jacopo@jmondi.org>\n\t<20190121150756.14982-3-jacopo@jmondi.org>\n\t<20190121202240.GB4439@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"frt5lluhdpuj7mxp\"","Content-Disposition":"inline","In-Reply-To":"<20190121202240.GB4439@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 2/2] test: pipeline: IPU3: Add IPU3\n\tpipeline test","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 22 Jan 2019 09:06:57 -0000"}}]