[{"id":2957,"web_url":"https://patchwork.libcamera.org/comment/2957/","msgid":"<20191023174827.GG1904@pendragon.ideasonboard.com>","date":"2019-10-23T17:48:27","subject":"Re: [libcamera-devel] [PATCH 1/1] rkisp1: add pipeline test for\n\trkisp1","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Show,\n\nThank you for the patch.\n\nOn Wed, Oct 23, 2019 at 05:52:54PM +0800, Show Liu wrote:\n\nA commit message would be nice here, maybe just copied from the cover\nletter.\n\n> Signed-off-by: Show Liu \n> ---\n> test/pipeline/meson.build | 1 +\n> test/pipeline/rkisp1/meson.build | 12 ++\n> test/pipeline/rkisp1/rkisp1_pipeline_test.cpp | 109 ++++++++++++++++++\n> 3 files changed, 122 insertions(+)\n> create mode 100644 test/pipeline/rkisp1/meson.build\n> create mode 100644 test/pipeline/rkisp1/rkisp1_pipeline_test.cpp\n> \n> diff --git a/test/pipeline/meson.build b/test/pipeline/meson.build\n> index f434c79..157f789 100644\n> --- a/test/pipeline/meson.build\n> +++ b/test/pipeline/meson.build\n> @@ -1 +1,2 @@\n> subdir('ipu3')\n> +subdir('rkisp1')\n> diff --git a/test/pipeline/rkisp1/meson.build b/test/pipeline/rkisp1/meson.build\n> new file mode 100644\n> index 0000000..d3f9749\n> --- /dev/null\n> +++ b/test/pipeline/rkisp1/meson.build\n> @@ -0,0 +1,12 @@\n> +rkisp1_test = [\n> + ['rkisp1_pipeline_test', 'rkisp1_pipeline_test.cpp'],\n> +]\n> +\n> +foreach t : rkisp1_test\n> + exe = executable(t[0], t[1],\n> + dependencies : libcamera_dep,\n> + link_with : test_libraries,\n> + include_directories : test_includes_internal)\n> +\n> + test(t[0], exe, suite : 'rkisp1', is_parallel : false)\n> +endforeach\n> diff --git a/test/pipeline/rkisp1/rkisp1_pipeline_test.cpp b/test/pipeline/rkisp1/rkisp1_pipeline_test.cpp\n> new file mode 100644\n> index 0000000..a463309\n> --- /dev/null\n> +++ b/test/pipeline/rkisp1/rkisp1_pipeline_test.cpp\n> @@ -0,0 +1,109 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n\nThis is missing a copyright line.\n\n> + * rkisp1_pipeline_test.cpp - Rockchip RK3399 pipeline test\n> + */\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"device_enumerator.h\"\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> +/*\n> + * Verify that the RK3399 pipeline handler gets matched and cameras\n> + * are enumerated correctly.\n> + *\n> + * The test is supposed to be run on rockchip platform.\n> + *\n> + * The test lists all cameras registered in the system, if any camera is\n> + * available at all.\n> + */\n> +class RKISP1PipelineTest : 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 RKISP1PipelineTest::init()\n> +{\n> +\tunique_ptr enumerator = DeviceEnumerator::create();\n> +\tif (!enumerator) {\n> +\t\tcerr << \"Failed to create device enumerator\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tif (enumerator->enumerate()) {\n> +\t\tcerr << \"Failed to enumerate media devices\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tDeviceMatch dm(\"rkisp1\");\n> +\n> +\tstd::shared_ptr dm_rkisp1 = enumerator->search(dm);\n\nI don't think the prefer dm_ is right here. The dm above stands for\nDeviceMatch. I would call this variable just rkisp1.\n\n> +\tif (!dm_rkisp1) {\n> +\t\tcerr << \"Failed to find rkisp1: test skip\" << endl;\n> +\t\treturn TestSkip;\n> +\t}\n> +\n> +\tint ret = dm_rkisp1->populate();\n> +\tif (ret) {\n> +\t\tcerr << \"Failed to populate media device \" << dm_rkisp1->deviceNode() << endl;\n\nCould you wrap this line ? We try to keep lines under the 80 columns\nlimit when possible, with a hard limit at 120 when wrapping isn't\npossible at all.\n\n\t\tcerr << \"Failed to populate media device \"\n\t\t << dm_rkisp1->deviceNode() << endl;\n\nOther than that the patch looks good to me. It duplicates code from the\nIPU3 test, and it would be nice to share the code instead, but we plan\nto work on pipeline-specific tests in the near future, so we'll handle\nthat refactoring.\n\nCould you please send a v2 with those small issues fixed ?\n\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tsensors_ = 0;\n> +\tconst vector &entities = dm_rkisp1->entities();\n> +\tfor (MediaEntity *entity : entities) {\n> +\t\tif (entity->function() == MEDIA_ENT_F_CAM_SENSOR)\n> +\t\t\tsensors_++;\n> +\t}\n> +\n> +\tcameraManager_ = new CameraManager();\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 RKISP1PipelineTest::run()\n> +{\n> +\tauto 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() << \" cameras registered, but \" << sensors_\n> +\t\t << \" were expected\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\treturn TestPass;\n> +}\n> +\n> +void RKISP1PipelineTest::cleanup()\n> +{\n> +\tcameraManager_->stop();\n> +\tdelete cameraManager_;\n> +}\n> +\n> +TEST_REGISTER(RKISP1PipelineTest)","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 22B5F61383\n\tfor ;\n\tWed, 23 Oct 2019 19:48:34 +0200 (CEST)","from pendragon.ideasonboard.com (143.121.2.93.rev.sfr.net\n\t[93.2.121.143])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 944514F7;\n\tWed, 23 Oct 2019 19:48:33 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1571852913;\n\tbh=tvkeRJEIumTqP66Fb4dezD4TWf93vxzRhQRhczYbIwA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UtmJ4DeieMZedBDHNrrO414KGJo1l48WqJPN9IcP+KRH5ll1jTVJg8Xcs7/ycIuuH\n\t+Gx5/NS40MeD9SN4/FevXo6F4CNjnHL71XMka/peOXuiB1404LBpaS8Ta1YBXafqF3\n\thGFBJ7XvwcLmF53d/E+QB+84kOfi0yOVBH25WnEQ=","Date":"Wed, 23 Oct 2019 20:48:27 +0300","From":"Laurent Pinchart ","To":"Show Liu ","Cc":"libcamera-devel@lists.libcamera.org, peter.griffin@linaro.org","Message-ID":"<20191023174827.GG1904@pendragon.ideasonboard.com>","References":"<20191023095254.1479-1-show.liu@linaro.org>\n\t<20191023095254.1479-2-show.liu@linaro.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191023095254.1479-2-show.liu@linaro.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/1] rkisp1: add pipeline test for\n\trkisp1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 23 Oct 2019 17:48:34 -0000"}}]