[{"id":18871,"web_url":"https://patchwork.libcamera.org/comment/18871/","msgid":"","date":"2021-08-17T13:04:10","subject":"Re: [libcamera-devel] [PATCH 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Tue, Aug 17, 2021 at 05:56:46PM +0530, Umang Jain wrote:\n> This tests basically checks for two things:\n> - Camera reconfigurations without stopping CameraManager\n> - Fd leaks across IPA IPC boundary [1]\n> \n> Currently, it uses vimc, but can be easily changed to using another\n> platform (for e.g. IPU3) by changing kCamId_ and kIpaProxyName_.\n> \n> The test performs kNumOfReconfigures_ (currently set to 10)\n> reconfigurations of the camera. Each reconfiguration runs\n> start(), capture(100ms), stop() of the camera. Hence, the test\n> runs approximately for 1 second.\n> \n> For checking the fd leaks, the test monitors the /proc/$PROXY_PID/fd\n> directory for open fds. It compares the number of open fds after each\n> run to the number of open fds before the first run. If those two are\n> found to be mis-matched, the test shall report failure.\n> \n> Since, the test needs to test the IPA IPC code paths, it is meant to\n> always run with LIBCAMERA_IPA_FORCE_ISOLATION environment variable.\n> \n> [1] https://bugs.libcamera.org/show_bug.cgi?id=63\n> \n> Signed-off-by: Umang Jain \n> ---\n> test/camera/camera_reconfigure.cpp | 257 +++++++++++++++++++++++++++++\n> test/camera/meson.build | 1 +\n> 2 files changed, 258 insertions(+)\n> create mode 100644 test/camera/camera_reconfigure.cpp\n> \n> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp\n> new file mode 100644\n> index 00000000..b32264b1\n> --- /dev/null\n> +++ b/test/camera/camera_reconfigure.cpp\n> @@ -0,0 +1,257 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * Test:\n> + * - Camera's multiple reconfigurations without stopping CameraManager\n> + * - leaking fds across IPA IPC boundary\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"camera_test.h\"\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +\n> +namespace {\n> +\n> +class CameraReconfigure : public CameraTest, public Test\n> +{\n> +public:\n> +\tCameraReconfigure()\n> +\t\t: CameraTest(kCamId_, true)\n> +\t{\n> +\t}\n> +\n> +private:\n> +\tstatic constexpr const char *kCamId_ = \"platform/vimc.0 Sensor B\";\n> +\tstatic constexpr const char *kIpaProxyName_ = \"vimc_ipa_proxy\";\n> +\tstatic constexpr unsigned int kNumOfReconfigures_ = 10;\n> +\n> +\tvoid requestComplete(Request *request)\n> +\t{\n> +\t\tif (request->status() != Request::RequestComplete)\n> +\t\t\treturn;\n> +\n> +\t\tconst Request::BufferMap &buffers = request->buffers();\n> +\n> +\t\t/* Create a new request. */\n> +\t\tconst Stream *stream = buffers.begin()->first;\n> +\t\tFrameBuffer *buffer = buffers.begin()->second;\n> +\n> +\t\trequest->reuse();\n> +\t\trequest->addBuffer(stream, buffer);\n> +\t\tcamera_->queueRequest(request);\n> +\t}\n> +\n> +\tint startAndStop()\n> +\t{\n> +\t\tStreamConfiguration &cfg = config_->at(0);\n> +\n> +\t\tif (camera_->acquire()) {\n> +\t\t\tcerr << \"Failed to acquire the camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (camera_->configure(config_.get())) {\n> +\t\t\tcerr << \"Failed to set default configuration\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tStream *stream = cfg.stream();\n> +\n> +\t\tif (!allocated_) {\n> +\t\t\tint ret = allocator_->allocate(stream);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\treturn TestFail;\n> +\t\t\tallocated_ = true;\n> +\t\t}\n> +\n> +\t\tfor (const unique_ptr &buffer : allocator_->buffers(stream)) {\n> +\t\t\tunique_ptr request = camera_->createRequest();\n> +\t\t\tif (!request) {\n> +\t\t\t\tcerr << \"Failed to create request\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\tif (request->addBuffer(stream, buffer.get())) {\n> +\t\t\t\tcerr << \"Failed to associating buffer with request\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\trequests_.push_back(move(request));\n> +\t\t}\n> +\n> +\t\tcamera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete);\n> +\n> +\t\tif (camera_->start()) {\n> +\t\t\tcerr << \"Failed to start camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tfor (unique_ptr &request : requests_) {\n> +\t\t\tif (camera_->queueRequest(request.get())) {\n> +\t\t\t\tcerr << \"Failed to queue request\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tEventDispatcher *dispatcher = Thread::current()->eventDispatcher();\n> +\n> +\t\tTimer timer;\n> +\t\ttimer.start(100);\n> +\t\twhile (timer.isRunning())\n> +\t\t\tdispatcher->processEvents();\n> +\n> +\t\tif (camera_->stop()) {\n> +\t\t\tcerr << \"Failed to stop camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (camera_->release()) {\n> +\t\t\tcerr << \"Failed to release camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tcamera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete);\n> +\n> +\t\trequests_.clear();\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tint fdsOpen(pid_t pid)\n> +\t{\n> +\t\tstring proxyFdPath = \"/proc/\" + to_string(pid) + \"/fd\";\n> +\t\tDIR *dir;\n> +\t\tstruct dirent *ptr;\n> +\t\tunsigned int openFds = 0;\n> +\n> +\t\tdir = opendir(proxyFdPath.c_str());\n> +\t\tif (dir == nullptr) {\n> +\t\t\tint err = errno;\n> +\t\t\tcerr << \"Error opening \" << proxyFdPath << \": \"\n> +\t\t\t << strerror(-err) << endl;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\n> +\t\twhile ((ptr = readdir(dir)) != nullptr) {\n> +\t\t\tif ((strcmp(ptr->d_name, \".\") == 0) ||\n> +\t\t\t (strcmp(ptr->d_name, \"..\") == 0))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\topenFds++;\n> +\t\t}\n> +\t\tclosedir(dir);\n> +\n> +\t\treturn openFds;\n> +\t}\n> +\n> +\tpid_t findProxyPid()\n> +\t{\n> +\t\tstring proxyPid;\n> +\t\tstring proxyName(kIpaProxyName_);\n> +\t\tDIR *dir;\n> +\t\tstruct dirent *ptr;\n> +\n> +\t\tdir = opendir(\"/proc\");\n> +\t\twhile((ptr = readdir(dir)) != nullptr) {\n> +\t\t\tif(ptr->d_type != DT_DIR)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tstring pname(\"/proc/\" + string(ptr->d_name) + \"/comm\");\n> +\t\t\tif(File::exists(pname.c_str())) {\n\nMissing space.\n\nI may have missed it, but I haven't seen an answer to my review of the\nprevious version that proposed checking the number of open file\ndescriptors in the VIMC IPA module itself. That would test for fd leaks\nin all tests using vimc, and wouldn't require finding child processes.\n\n> +\t\t\t\tifstream pfile(pname.c_str());\n> +\t\t\t\tstring comm;\n> +\t\t\t\tgetline (pfile, comm);\n> +\t\t\t\tpfile.close();\n> +\n> +\t\t\t\tproxyPid = comm == proxyName ?\n> +\t\t\t\t\t string(ptr->d_name) :\n> +\t\t\t\t\t \"\";\n> +\t\t\t}\n> +\n> +\t\t\tif (!proxyPid.empty())\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t\tclosedir(dir);\n> +\n> +\t\tif (!proxyPid.empty())\n> +\t\t\treturn atoi(proxyPid.c_str());\n> +\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tint init() override\n> +\t{\n> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n> +\n> +\t\tconfig_ = camera_->generateConfiguration({ StreamRole::StillCapture });\n> +\t\tif (!config_ || config_->size() != 1) {\n> +\t\t\tcerr << \"Failed to generate default configuration\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tallocator_ = make_unique(camera_);\n> +\t\tallocated_ = false;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\tunsigned int openFdsAtStart = 0;\n> +\t\tunsigned int openFds = 0;\n> +\n> +\t\tpid_t proxyPid = findProxyPid();\n> +\t\tif (proxyPid < 0) {\n> +\t\t\tcerr << \"Cannot find \" << kIpaProxyName_\n> +\t\t\t << \" pid, exiting\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\topenFdsAtStart = fdsOpen(proxyPid);\n> +\t\tfor (unsigned int i = 0; i < kNumOfReconfigures_; i++) {\n> +\t\t\tstartAndStop();\n> +\t\t\topenFds = fdsOpen(proxyPid);\n> +\t\t\tif (openFds == 0) {\n> +\t\t\t\tcerr << \"No open fds found whereas \"\n> +\t\t\t\t << \"open fds at start: \" << openFdsAtStart\n> +\t\t\t\t << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\n> +\t\t\tif (openFds != openFdsAtStart) {\n> +\t\t\t\tcerr << \"Leaking fds for \" << kIpaProxyName_\n> +\t\t\t\t << \" - Open fds: \" << openFds << \" vs \"\n> +\t\t\t\t << \"Open fds at start: \" << openFdsAtStart\n> +\t\t\t\t << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tbool allocated_;\n> +\n> +\tvector> requests_;\n> +\n> +\tunique_ptr config_;\n> +\tunique_ptr allocator_;\n> +};\n> +\n> +} /* namespace */\n> +\n> +TEST_REGISTER(CameraReconfigure)\n> diff --git a/test/camera/meson.build b/test/camera/meson.build\n> index 002a87b5..668d5c03 100644\n> --- a/test/camera/meson.build\n> +++ b/test/camera/meson.build\n> @@ -8,6 +8,7 @@ camera_tests = [\n> ['buffer_import', 'buffer_import.cpp'],\n> ['statemachine', 'statemachine.cpp'],\n> ['capture', 'capture.cpp'],\n> + ['camera_reconfigure', 'camera_reconfigure.cpp'],\n> ]\n> \n> foreach t : camera_tests","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 88649BD87C\n\tfor ;\n\tTue, 17 Aug 2021 13:04:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF56468894;\n\tTue, 17 Aug 2021 15:04:18 +0200 (CEST)","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 86A3A6025C\n\tfor ;\n\tTue, 17 Aug 2021 15:04:17 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E67D72C5;\n\tTue, 17 Aug 2021 15:04:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"k3XmxCKj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629205457;\n\tbh=WrjZ0Mt8g81BabQhDxDjxks1FW2cS4LUxk0q6PSrdIA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k3XmxCKjjYDQA+xcAcTPoRidUVxhTCmymfrDl4nWmhPZKTDIhUyYwjnTUZSPHDAC2\n\tFoOaG+HRfzmOMFOkf9thM53Kcw7BAaLhkTxjZp8jA908kW/7rKOgsjc0ulvkEB2BWA\n\tzLQVMiIqO1TcTi5Cw/92hzhuVTKclI0pBHhDdlpE=","Date":"Tue, 17 Aug 2021 16:04:10 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210817122646.185978-1-umang.jain@ideasonboard.com>\n\t<20210817122646.185978-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210817122646.185978-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18872,"web_url":"https://patchwork.libcamera.org/comment/18872/","msgid":"<3a519f6f-2a61-deb0-561b-4c127460d299@ideasonboard.com>","date":"2021-08-17T13:57:24","subject":"Re: [libcamera-devel] [PATCH 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 17/08/2021 14:04, Laurent Pinchart wrote:\n> Hi Umang,\n> >> +\tpid_t findProxyPid()\n>> +\t{\n>> +\t\tstring proxyPid;\n>> +\t\tstring proxyName(kIpaProxyName_);\n>> +\t\tDIR *dir;\n>> +\t\tstruct dirent *ptr;\n>> +\n>> +\t\tdir = opendir(\"/proc\");\n>> +\t\twhile((ptr = readdir(dir)) != nullptr) {\n>> +\t\t\tif(ptr->d_type != DT_DIR)\n>> +\t\t\t\tcontinue;\n>> +\n>> +\t\t\tstring pname(\"/proc/\" + string(ptr->d_name) + \"/comm\");\n>> +\t\t\tif(File::exists(pname.c_str())) {\n> \n> Missing space.\n> \n> I may have missed it, but I haven't seen an answer to my review of the\n> previous version that proposed checking the number of open file\n> descriptors in the VIMC IPA module itself. That would test for fd leaks\n> in all tests using vimc, and wouldn't require finding child processes.\n\n\nI presume this would just fire an ASSERT() if it was detected. (Unless\nyou have another way to report the failure up to the test?)\n\nI'm a bit weary that this would be hard to track down when it fires as\nthe failure would occur in a process which we don't (easily) get logs\nfrom. The failure condition might get a bit 'lost'.\n\nThe assert would cause the IPA to 'crash', and the tests would fail -\nbut they would look like they were failing because the IPA had\n'disappeared' and may not be obvious to diagnose..\n\n\nI wonder if we need to tackle how we log from isolated processes already\n... :-S\n\n\nI guess the alternative is to have some extended interface callbacks\nfrom the IPA to allow it to message the VIMC pipeline handler to report\nsomething ... and ... cause a failure and shutdown on the pipeline side ?\n\n--\nKieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A1571BD87C\n\tfor ;\n\tTue, 17 Aug 2021 13:57:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 326C768894;\n\tTue, 17 Aug 2021 15:57:28 +0200 (CEST)","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 E56806025C\n\tfor ;\n\tTue, 17 Aug 2021 15:57:26 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 49A662C5;\n\tTue, 17 Aug 2021 15:57:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"S/OPuVRa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629208646;\n\tbh=1HPNZLF6qd3B6oNh+xOrVv9JG6yOaEQRRtBtZCSNDHw=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=S/OPuVRaYHYREVLve/aH+JTphzUgXZyPrtnQbzQJH1iQurvnTiqu1jlGoRs1JYFWU\n\tnd4Ot5bFmvKXhtODMIKDVEKcpgfpppqLTv9a5HmlYzZ7OkNXwTWklh9zzWbS3ddDtU\n\tlHjz5JJEIPk4CtrKF1Z+bwrdrnRQr7NwivxsXHxY=","To":"Laurent Pinchart ,\n\tUmang Jain ","References":"<20210817122646.185978-1-umang.jain@ideasonboard.com>\n\t<20210817122646.185978-3-umang.jain@ideasonboard.com>\n\t","From":"Kieran Bingham ","Message-ID":"<3a519f6f-2a61-deb0-561b-4c127460d299@ideasonboard.com>","Date":"Tue, 17 Aug 2021 14:57:24 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18874,"web_url":"https://patchwork.libcamera.org/comment/18874/","msgid":"<2ef3631c-2b85-c457-fbb0-3024fe06302b@ideasonboard.com>","date":"2021-08-17T15:00:34","subject":"Re: [libcamera-devel] [PATCH 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 8/17/21 6:34 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Tue, Aug 17, 2021 at 05:56:46PM +0530, Umang Jain wrote:\n>> This tests basically checks for two things:\n>> - Camera reconfigurations without stopping CameraManager\n>> - Fd leaks across IPA IPC boundary [1]\n>>\n>> Currently, it uses vimc, but can be easily changed to using another\n>> platform (for e.g. IPU3) by changing kCamId_ and kIpaProxyName_.\n>>\n>> The test performs kNumOfReconfigures_ (currently set to 10)\n>> reconfigurations of the camera. Each reconfiguration runs\n>> start(), capture(100ms), stop() of the camera. Hence, the test\n>> runs approximately for 1 second.\n>>\n>> For checking the fd leaks, the test monitors the /proc/$PROXY_PID/fd\n>> directory for open fds. It compares the number of open fds after each\n>> run to the number of open fds before the first run. If those two are\n>> found to be mis-matched, the test shall report failure.\n>>\n>> Since, the test needs to test the IPA IPC code paths, it is meant to\n>> always run with LIBCAMERA_IPA_FORCE_ISOLATION environment variable.\n>>\n>> [1] https://bugs.libcamera.org/show_bug.cgi?id=63\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> test/camera/camera_reconfigure.cpp | 257 +++++++++++++++++++++++++++++\n>> test/camera/meson.build | 1 +\n>> 2 files changed, 258 insertions(+)\n>> create mode 100644 test/camera/camera_reconfigure.cpp\n>>\n>> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp\n>> new file mode 100644\n>> index 00000000..b32264b1\n>> --- /dev/null\n>> +++ b/test/camera/camera_reconfigure.cpp\n>> @@ -0,0 +1,257 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * Test:\n>> + * - Camera's multiple reconfigurations without stopping CameraManager\n>> + * - leaking fds across IPA IPC boundary\n>> + */\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \"camera_test.h\"\n>> +#include \"test.h\"\n>> +\n>> +using namespace std;\n>> +\n>> +namespace {\n>> +\n>> +class CameraReconfigure : public CameraTest, public Test\n>> +{\n>> +public:\n>> +\tCameraReconfigure()\n>> +\t\t: CameraTest(kCamId_, true)\n>> +\t{\n>> +\t}\n>> +\n>> +private:\n>> +\tstatic constexpr const char *kCamId_ = \"platform/vimc.0 Sensor B\";\n>> +\tstatic constexpr const char *kIpaProxyName_ = \"vimc_ipa_proxy\";\n>> +\tstatic constexpr unsigned int kNumOfReconfigures_ = 10;\n>> +\n>> +\tvoid requestComplete(Request *request)\n>> +\t{\n>> +\t\tif (request->status() != Request::RequestComplete)\n>> +\t\t\treturn;\n>> +\n>> +\t\tconst Request::BufferMap &buffers = request->buffers();\n>> +\n>> +\t\t/* Create a new request. */\n>> +\t\tconst Stream *stream = buffers.begin()->first;\n>> +\t\tFrameBuffer *buffer = buffers.begin()->second;\n>> +\n>> +\t\trequest->reuse();\n>> +\t\trequest->addBuffer(stream, buffer);\n>> +\t\tcamera_->queueRequest(request);\n>> +\t}\n>> +\n>> +\tint startAndStop()\n>> +\t{\n>> +\t\tStreamConfiguration &cfg = config_->at(0);\n>> +\n>> +\t\tif (camera_->acquire()) {\n>> +\t\t\tcerr << \"Failed to acquire the camera\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tif (camera_->configure(config_.get())) {\n>> +\t\t\tcerr << \"Failed to set default configuration\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tStream *stream = cfg.stream();\n>> +\n>> +\t\tif (!allocated_) {\n>> +\t\t\tint ret = allocator_->allocate(stream);\n>> +\t\t\tif (ret < 0)\n>> +\t\t\t\treturn TestFail;\n>> +\t\t\tallocated_ = true;\n>> +\t\t}\n>> +\n>> +\t\tfor (const unique_ptr &buffer : allocator_->buffers(stream)) {\n>> +\t\t\tunique_ptr request = camera_->createRequest();\n>> +\t\t\tif (!request) {\n>> +\t\t\t\tcerr << \"Failed to create request\" << endl;\n>> +\t\t\t\treturn TestFail;\n>> +\t\t\t}\n>> +\n>> +\t\t\tif (request->addBuffer(stream, buffer.get())) {\n>> +\t\t\t\tcerr << \"Failed to associating buffer with request\" << endl;\n>> +\t\t\t\treturn TestFail;\n>> +\t\t\t}\n>> +\n>> +\t\t\trequests_.push_back(move(request));\n>> +\t\t}\n>> +\n>> +\t\tcamera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete);\n>> +\n>> +\t\tif (camera_->start()) {\n>> +\t\t\tcerr << \"Failed to start camera\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tfor (unique_ptr &request : requests_) {\n>> +\t\t\tif (camera_->queueRequest(request.get())) {\n>> +\t\t\t\tcerr << \"Failed to queue request\" << endl;\n>> +\t\t\t\treturn TestFail;\n>> +\t\t\t}\n>> +\t\t}\n>> +\n>> +\t\tEventDispatcher *dispatcher = Thread::current()->eventDispatcher();\n>> +\n>> +\t\tTimer timer;\n>> +\t\ttimer.start(100);\n>> +\t\twhile (timer.isRunning())\n>> +\t\t\tdispatcher->processEvents();\n>> +\n>> +\t\tif (camera_->stop()) {\n>> +\t\t\tcerr << \"Failed to stop camera\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tif (camera_->release()) {\n>> +\t\t\tcerr << \"Failed to release camera\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tcamera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete);\n>> +\n>> +\t\trequests_.clear();\n>> +\n>> +\t\treturn 0;\n>> +\t}\n>> +\n>> +\tint fdsOpen(pid_t pid)\n>> +\t{\n>> +\t\tstring proxyFdPath = \"/proc/\" + to_string(pid) + \"/fd\";\n>> +\t\tDIR *dir;\n>> +\t\tstruct dirent *ptr;\n>> +\t\tunsigned int openFds = 0;\n>> +\n>> +\t\tdir = opendir(proxyFdPath.c_str());\n>> +\t\tif (dir == nullptr) {\n>> +\t\t\tint err = errno;\n>> +\t\t\tcerr << \"Error opening \" << proxyFdPath << \": \"\n>> +\t\t\t << strerror(-err) << endl;\n>> +\t\t\treturn 0;\n>> +\t\t}\n>> +\n>> +\t\twhile ((ptr = readdir(dir)) != nullptr) {\n>> +\t\t\tif ((strcmp(ptr->d_name, \".\") == 0) ||\n>> +\t\t\t (strcmp(ptr->d_name, \"..\") == 0))\n>> +\t\t\t\tcontinue;\n>> +\n>> +\t\t\topenFds++;\n>> +\t\t}\n>> +\t\tclosedir(dir);\n>> +\n>> +\t\treturn openFds;\n>> +\t}\n>> +\n>> +\tpid_t findProxyPid()\n>> +\t{\n>> +\t\tstring proxyPid;\n>> +\t\tstring proxyName(kIpaProxyName_);\n>> +\t\tDIR *dir;\n>> +\t\tstruct dirent *ptr;\n>> +\n>> +\t\tdir = opendir(\"/proc\");\n>> +\t\twhile((ptr = readdir(dir)) != nullptr) {\n>> +\t\t\tif(ptr->d_type != DT_DIR)\n>> +\t\t\t\tcontinue;\n>> +\n>> +\t\t\tstring pname(\"/proc/\" + string(ptr->d_name) + \"/comm\");\n>> +\t\t\tif(File::exists(pname.c_str())) {\n> Missing space.\n\n\nI failed to run checkstyle.py before sending, apologies. I hit -ENOSPC \non my primary device few days agp and I am ended up working on a adhoc \nclone which didn't had the checkstyle.py as post-commit hook\n\n>\n> I may have missed it, but I haven't seen an answer to my review of the\n> previous version that proposed checking the number of open file\n> descriptors in the VIMC IPA module itself. That would test for fd leaks\n> in all tests using vimc, and wouldn't require finding child processes.\n\n\nComing to your request, the reply sounded to me as an enhancement in the \ntest-able VIMC in general, to increase our test coverage and IPC code \npaths. I am assuming you don't intend to dis-regard this particular test \nin favor of having it plumbed down to VIMC IPA, or is it? I am assuming \non my part that both can(or should) co-exist, latter being developed \nalong the lines and out of scope of this patch. Our priorities are \nwell-addressed by this test for now, so maybe can I track it in bugzilla \nand come back to it in coming weeks?\n\n\n>\n>> +\t\t\t\tifstream pfile(pname.c_str());\n>> +\t\t\t\tstring comm;\n>> +\t\t\t\tgetline (pfile, comm);\n>> +\t\t\t\tpfile.close();\n>> +\n>> +\t\t\t\tproxyPid = comm == proxyName ?\n>> +\t\t\t\t\t string(ptr->d_name) :\n>> +\t\t\t\t\t \"\";\n>> +\t\t\t}\n>> +\n>> +\t\t\tif (!proxyPid.empty())\n>> +\t\t\t\tbreak;\n>> +\t\t}\n>> +\t\tclosedir(dir);\n>> +\n>> +\t\tif (!proxyPid.empty())\n>> +\t\t\treturn atoi(proxyPid.c_str());\n>> +\n>> +\t\treturn -1;\n>> +\t}\n>> +\n>> +\tint init() override\n>> +\t{\n>> +\t\tif (status_ != TestPass)\n>> +\t\t\treturn status_;\n>> +\n>> +\t\tconfig_ = camera_->generateConfiguration({ StreamRole::StillCapture });\n>> +\t\tif (!config_ || config_->size() != 1) {\n>> +\t\t\tcerr << \"Failed to generate default configuration\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\tallocator_ = make_unique(camera_);\n>> +\t\tallocated_ = false;\n>> +\n>> +\t\treturn TestPass;\n>> +\t}\n>> +\n>> +\tint run() override\n>> +\t{\n>> +\t\tunsigned int openFdsAtStart = 0;\n>> +\t\tunsigned int openFds = 0;\n>> +\n>> +\t\tpid_t proxyPid = findProxyPid();\n>> +\t\tif (proxyPid < 0) {\n>> +\t\t\tcerr << \"Cannot find \" << kIpaProxyName_\n>> +\t\t\t << \" pid, exiting\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\topenFdsAtStart = fdsOpen(proxyPid);\n>> +\t\tfor (unsigned int i = 0; i < kNumOfReconfigures_; i++) {\n>> +\t\t\tstartAndStop();\n>> +\t\t\topenFds = fdsOpen(proxyPid);\n>> +\t\t\tif (openFds == 0) {\n>> +\t\t\t\tcerr << \"No open fds found whereas \"\n>> +\t\t\t\t << \"open fds at start: \" << openFdsAtStart\n>> +\t\t\t\t << endl;\n>> +\t\t\t\treturn TestFail;\n>> +\t\t\t}\n>> +\n>> +\t\t\tif (openFds != openFdsAtStart) {\n>> +\t\t\t\tcerr << \"Leaking fds for \" << kIpaProxyName_\n>> +\t\t\t\t << \" - Open fds: \" << openFds << \" vs \"\n>> +\t\t\t\t << \"Open fds at start: \" << openFdsAtStart\n>> +\t\t\t\t << endl;\n>> +\t\t\t\treturn TestFail;\n>> +\t\t\t}\n>> +\t\t}\n>> +\n>> +\t\treturn TestPass;\n>> +\t}\n>> +\n>> +\tbool allocated_;\n>> +\n>> +\tvector> requests_;\n>> +\n>> +\tunique_ptr config_;\n>> +\tunique_ptr allocator_;\n>> +};\n>> +\n>> +} /* namespace */\n>> +\n>> +TEST_REGISTER(CameraReconfigure)\n>> diff --git a/test/camera/meson.build b/test/camera/meson.build\n>> index 002a87b5..668d5c03 100644\n>> --- a/test/camera/meson.build\n>> +++ b/test/camera/meson.build\n>> @@ -8,6 +8,7 @@ camera_tests = [\n>> ['buffer_import', 'buffer_import.cpp'],\n>> ['statemachine', 'statemachine.cpp'],\n>> ['capture', 'capture.cpp'],\n>> + ['camera_reconfigure', 'camera_reconfigure.cpp'],\n>> ]\n>> \n>> foreach t : camera_tests","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A7F71BD87C\n\tfor ;\n\tTue, 17 Aug 2021 15:00:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2BD9F6025F;\n\tTue, 17 Aug 2021 17:00:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B46E46025C\n\tfor ;\n\tTue, 17 Aug 2021 17:00:39 +0200 (CEST)","from [192.168.1.104] (unknown [103.238.109.15])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D140B2C5;\n\tTue, 17 Aug 2021 17:00:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jvfdPqfw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629212439;\n\tbh=FZy4kAu68lw9E0YRKps9tOmcshXvL3MX8b/zjQl4CbE=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=jvfdPqfwNW6As05hWryif5N5m716fuOl3Zjd6LS8c7me9uhgWo+x+rnJVN7wvMkI8\n\tKS3ax2ra9MoxoxI1R30NA05pA8QeYv6KVAUjka79nx6QDJqXQxD66Gb1KNVoF4bvVr\n\timXEKl/+dkeQZTdvudSaVpQ9ZouVcOT0WK/7Y49k=","To":"Laurent Pinchart ","References":"<20210817122646.185978-1-umang.jain@ideasonboard.com>\n\t<20210817122646.185978-3-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<2ef3631c-2b85-c457-fbb0-3024fe06302b@ideasonboard.com>","Date":"Tue, 17 Aug 2021 20:30:34 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18878,"web_url":"https://patchwork.libcamera.org/comment/18878/","msgid":"","date":"2021-08-17T18:44:16","subject":"Re: [libcamera-devel] [PATCH 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Aug 17, 2021 at 02:57:24PM +0100, Kieran Bingham wrote:\n> On 17/08/2021 14:04, Laurent Pinchart wrote:\n> > Hi Umang,\n> > >> +\tpid_t findProxyPid()\n> >> +\t{\n> >> +\t\tstring proxyPid;\n> >> +\t\tstring proxyName(kIpaProxyName_);\n> >> +\t\tDIR *dir;\n> >> +\t\tstruct dirent *ptr;\n> >> +\n> >> +\t\tdir = opendir(\"/proc\");\n> >> +\t\twhile((ptr = readdir(dir)) != nullptr) {\n> >> +\t\t\tif(ptr->d_type != DT_DIR)\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\tstring pname(\"/proc/\" + string(ptr->d_name) + \"/comm\");\n> >> +\t\t\tif(File::exists(pname.c_str())) {\n> > \n> > Missing space.\n> > \n> > I may have missed it, but I haven't seen an answer to my review of the\n> > previous version that proposed checking the number of open file\n> > descriptors in the VIMC IPA module itself. That would test for fd leaks\n> > in all tests using vimc, and wouldn't require finding child processes.\n> \n> \n> I presume this would just fire an ASSERT() if it was detected. (Unless\n> you have another way to report the failure up to the test?)\n> \n> I'm a bit weary that this would be hard to track down when it fires as\n> the failure would occur in a process which we don't (easily) get logs\n> from. The failure condition might get a bit 'lost'.\n> \n> The assert would cause the IPA to 'crash', and the tests would fail -\n> but they would look like they were failing because the IPA had\n> 'disappeared' and may not be obvious to diagnose..\n> \n> \n> I wonder if we need to tackle how we log from isolated processes already\n> ... :-S\n> \n> \n> I guess the alternative is to have some extended interface callbacks\n> from the IPA to allow it to message the VIMC pipeline handler to report\n> something ... and ... cause a failure and shutdown on the pipeline side ?\n\nWe do have a tracing mechanism already, with a FIFO used to pass\ninformation from the IPA to tests. It could be leveraged, possibly just\nusing the existing infrastructure to start with (it's limited to passing\n32-bit operation codes for tracing), extending it later with support for\nmore data.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E5F19BD87C\n\tfor ;\n\tTue, 17 Aug 2021 18:44:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B5A5688A2;\n\tTue, 17 Aug 2021 20:44:25 +0200 (CEST)","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 8C9476025F\n\tfor ;\n\tTue, 17 Aug 2021 20:44:24 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B2102C5;\n\tTue, 17 Aug 2021 20:44:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tonP+JYV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629225864;\n\tbh=5K5MxMj3JkzWjNwN3XPvgcEXDuvHymHgTsGv8Ok/ccc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tonP+JYViEOh4dz2v0C8j3lemU9TN7O+lIJwrDWDCZj+H7oLvmQZoLMoAgIKjxByQ\n\t4YlM4itk0NR4s6gxqpck2az0vn+sspUpBa4+FGTBHveCPrFjTgCASt7FDPInEbc26O\n\tVuYWaCnlLOEI5tyF96CRfbEx0n4+5hmXRy4+hsNY=","Date":"Tue, 17 Aug 2021 21:44:16 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210817122646.185978-1-umang.jain@ideasonboard.com>\n\t<20210817122646.185978-3-umang.jain@ideasonboard.com>\n\t\n\t<3a519f6f-2a61-deb0-561b-4c127460d299@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<3a519f6f-2a61-deb0-561b-4c127460d299@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18891,"web_url":"https://patchwork.libcamera.org/comment/18891/","msgid":"","date":"2021-08-18T02:14:54","subject":"Re: [libcamera-devel] [PATCH 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Tue, Aug 17, 2021 at 08:30:34PM +0530, Umang Jain wrote:\n> On 8/17/21 6:34 PM, Laurent Pinchart wrote:\n> > On Tue, Aug 17, 2021 at 05:56:46PM +0530, Umang Jain wrote:\n> >> This tests basically checks for two things:\n> >> - Camera reconfigurations without stopping CameraManager\n> >> - Fd leaks across IPA IPC boundary [1]\n> >>\n> >> Currently, it uses vimc, but can be easily changed to using another\n> >> platform (for e.g. IPU3) by changing kCamId_ and kIpaProxyName_.\n> >>\n> >> The test performs kNumOfReconfigures_ (currently set to 10)\n> >> reconfigurations of the camera. Each reconfiguration runs\n> >> start(), capture(100ms), stop() of the camera. Hence, the test\n> >> runs approximately for 1 second.\n> >>\n> >> For checking the fd leaks, the test monitors the /proc/$PROXY_PID/fd\n> >> directory for open fds. It compares the number of open fds after each\n> >> run to the number of open fds before the first run. If those two are\n> >> found to be mis-matched, the test shall report failure.\n> >>\n> >> Since, the test needs to test the IPA IPC code paths, it is meant to\n> >> always run with LIBCAMERA_IPA_FORCE_ISOLATION environment variable.\n> >>\n> >> [1] https://bugs.libcamera.org/show_bug.cgi?id=63\n> >>\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> test/camera/camera_reconfigure.cpp | 257 +++++++++++++++++++++++++++++\n> >> test/camera/meson.build | 1 +\n> >> 2 files changed, 258 insertions(+)\n> >> create mode 100644 test/camera/camera_reconfigure.cpp\n> >>\n> >> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp\n> >> new file mode 100644\n> >> index 00000000..b32264b1\n> >> --- /dev/null\n> >> +++ b/test/camera/camera_reconfigure.cpp\n> >> @@ -0,0 +1,257 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2021, Google Inc.\n> >> + *\n> >> + * Test:\n> >> + * - Camera's multiple reconfigurations without stopping CameraManager\n> >> + * - leaking fds across IPA IPC boundary\n> >> + */\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \"camera_test.h\"\n> >> +#include \"test.h\"\n> >> +\n> >> +using namespace std;\n> >> +\n> >> +namespace {\n> >> +\n> >> +class CameraReconfigure : public CameraTest, public Test\n> >> +{\n> >> +public:\n> >> +\tCameraReconfigure()\n> >> +\t\t: CameraTest(kCamId_, true)\n> >> +\t{\n> >> +\t}\n> >> +\n> >> +private:\n> >> +\tstatic constexpr const char *kCamId_ = \"platform/vimc.0 Sensor B\";\n> >> +\tstatic constexpr const char *kIpaProxyName_ = \"vimc_ipa_proxy\";\n> >> +\tstatic constexpr unsigned int kNumOfReconfigures_ = 10;\n> >> +\n> >> +\tvoid requestComplete(Request *request)\n> >> +\t{\n> >> +\t\tif (request->status() != Request::RequestComplete)\n> >> +\t\t\treturn;\n> >> +\n> >> +\t\tconst Request::BufferMap &buffers = request->buffers();\n> >> +\n> >> +\t\t/* Create a new request. */\n> >> +\t\tconst Stream *stream = buffers.begin()->first;\n> >> +\t\tFrameBuffer *buffer = buffers.begin()->second;\n> >> +\n> >> +\t\trequest->reuse();\n> >> +\t\trequest->addBuffer(stream, buffer);\n> >> +\t\tcamera_->queueRequest(request);\n> >> +\t}\n> >> +\n> >> +\tint startAndStop()\n> >> +\t{\n> >> +\t\tStreamConfiguration &cfg = config_->at(0);\n> >> +\n> >> +\t\tif (camera_->acquire()) {\n> >> +\t\t\tcerr << \"Failed to acquire the camera\" << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tif (camera_->configure(config_.get())) {\n> >> +\t\t\tcerr << \"Failed to set default configuration\" << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tStream *stream = cfg.stream();\n> >> +\n> >> +\t\tif (!allocated_) {\n> >> +\t\t\tint ret = allocator_->allocate(stream);\n> >> +\t\t\tif (ret < 0)\n> >> +\t\t\t\treturn TestFail;\n> >> +\t\t\tallocated_ = true;\n> >> +\t\t}\n> >> +\n> >> +\t\tfor (const unique_ptr &buffer : allocator_->buffers(stream)) {\n> >> +\t\t\tunique_ptr request = camera_->createRequest();\n> >> +\t\t\tif (!request) {\n> >> +\t\t\t\tcerr << \"Failed to create request\" << endl;\n> >> +\t\t\t\treturn TestFail;\n> >> +\t\t\t}\n> >> +\n> >> +\t\t\tif (request->addBuffer(stream, buffer.get())) {\n> >> +\t\t\t\tcerr << \"Failed to associating buffer with request\" << endl;\n> >> +\t\t\t\treturn TestFail;\n> >> +\t\t\t}\n> >> +\n> >> +\t\t\trequests_.push_back(move(request));\n> >> +\t\t}\n> >> +\n> >> +\t\tcamera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete);\n> >> +\n> >> +\t\tif (camera_->start()) {\n> >> +\t\t\tcerr << \"Failed to start camera\" << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tfor (unique_ptr &request : requests_) {\n> >> +\t\t\tif (camera_->queueRequest(request.get())) {\n> >> +\t\t\t\tcerr << \"Failed to queue request\" << endl;\n> >> +\t\t\t\treturn TestFail;\n> >> +\t\t\t}\n> >> +\t\t}\n> >> +\n> >> +\t\tEventDispatcher *dispatcher = Thread::current()->eventDispatcher();\n> >> +\n> >> +\t\tTimer timer;\n> >> +\t\ttimer.start(100);\n> >> +\t\twhile (timer.isRunning())\n> >> +\t\t\tdispatcher->processEvents();\n> >> +\n> >> +\t\tif (camera_->stop()) {\n> >> +\t\t\tcerr << \"Failed to stop camera\" << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tif (camera_->release()) {\n> >> +\t\t\tcerr << \"Failed to release camera\" << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tcamera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete);\n> >> +\n> >> +\t\trequests_.clear();\n> >> +\n> >> +\t\treturn 0;\n> >> +\t}\n> >> +\n> >> +\tint fdsOpen(pid_t pid)\n> >> +\t{\n> >> +\t\tstring proxyFdPath = \"/proc/\" + to_string(pid) + \"/fd\";\n> >> +\t\tDIR *dir;\n> >> +\t\tstruct dirent *ptr;\n> >> +\t\tunsigned int openFds = 0;\n> >> +\n> >> +\t\tdir = opendir(proxyFdPath.c_str());\n> >> +\t\tif (dir == nullptr) {\n> >> +\t\t\tint err = errno;\n> >> +\t\t\tcerr << \"Error opening \" << proxyFdPath << \": \"\n> >> +\t\t\t << strerror(-err) << endl;\n> >> +\t\t\treturn 0;\n> >> +\t\t}\n> >> +\n> >> +\t\twhile ((ptr = readdir(dir)) != nullptr) {\n> >> +\t\t\tif ((strcmp(ptr->d_name, \".\") == 0) ||\n> >> +\t\t\t (strcmp(ptr->d_name, \"..\") == 0))\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\topenFds++;\n> >> +\t\t}\n> >> +\t\tclosedir(dir);\n> >> +\n> >> +\t\treturn openFds;\n> >> +\t}\n> >> +\n> >> +\tpid_t findProxyPid()\n> >> +\t{\n> >> +\t\tstring proxyPid;\n> >> +\t\tstring proxyName(kIpaProxyName_);\n> >> +\t\tDIR *dir;\n> >> +\t\tstruct dirent *ptr;\n> >> +\n> >> +\t\tdir = opendir(\"/proc\");\n> >> +\t\twhile((ptr = readdir(dir)) != nullptr) {\n> >> +\t\t\tif(ptr->d_type != DT_DIR)\n> >> +\t\t\t\tcontinue;\n> >> +\n> >> +\t\t\tstring pname(\"/proc/\" + string(ptr->d_name) + \"/comm\");\n> >> +\t\t\tif(File::exists(pname.c_str())) {\n> > Missing space.\n> \n> I failed to run checkstyle.py before sending, apologies. I hit -ENOSPC \n> on my primary device few days agp and I am ended up working on a adhoc \n> clone which didn't had the checkstyle.py as post-commit hook\n\nNo worries.\n\n> > I may have missed it, but I haven't seen an answer to my review of the\n> > previous version that proposed checking the number of open file\n> > descriptors in the VIMC IPA module itself. That would test for fd leaks\n> > in all tests using vimc, and wouldn't require finding child processes.\n> \n> Coming to your request, the reply sounded to me as an enhancement in the \n> test-able VIMC in general, to increase our test coverage and IPC code \n> paths. I am assuming you don't intend to dis-regard this particular test \n> in favor of having it plumbed down to VIMC IPA, or is it? I am assuming \n> on my part that both can(or should) co-exist, latter being developed \n> along the lines and out of scope of this patch.\n\nI was thinking that if we compute the number of open fds in the IPA\nitself, then we could drop it from here. It would simplify the\nimplementation of this test, we wouldn't have to loop over /proc here\n(which is the part that bothered me a bit, as I tried to see if there\nwas a C API to do this, and couldn't find any :-)), but only receive the\nnumber of fds from the VIMC IPA through the FIFO that it uses to\ncommunicate with tests.\n\nAnother option btw, now that I think about it, would be keep counting\nfds here, but pass the VIMC IPA PID through the FIFO.\n\n> Our priorities are well-addressed by this test for now, so maybe can I\n> track it in bugzilla and come back to it in coming weeks?\n\nThat's OK with me.\n\n> >> +\t\t\t\tifstream pfile(pname.c_str());\n> >> +\t\t\t\tstring comm;\n> >> +\t\t\t\tgetline (pfile, comm);\n> >> +\t\t\t\tpfile.close();\n> >> +\n> >> +\t\t\t\tproxyPid = comm == proxyName ?\n> >> +\t\t\t\t\t string(ptr->d_name) :\n> >> +\t\t\t\t\t \"\";\n> >> +\t\t\t}\n> >> +\n> >> +\t\t\tif (!proxyPid.empty())\n> >> +\t\t\t\tbreak;\n> >> +\t\t}\n> >> +\t\tclosedir(dir);\n> >> +\n> >> +\t\tif (!proxyPid.empty())\n> >> +\t\t\treturn atoi(proxyPid.c_str());\n> >> +\n> >> +\t\treturn -1;\n> >> +\t}\n> >> +\n> >> +\tint init() override\n> >> +\t{\n> >> +\t\tif (status_ != TestPass)\n> >> +\t\t\treturn status_;\n> >> +\n> >> +\t\tconfig_ = camera_->generateConfiguration({ StreamRole::StillCapture });\n> >> +\t\tif (!config_ || config_->size() != 1) {\n> >> +\t\t\tcerr << \"Failed to generate default configuration\" << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\tallocator_ = make_unique(camera_);\n> >> +\t\tallocated_ = false;\n> >> +\n> >> +\t\treturn TestPass;\n> >> +\t}\n> >> +\n> >> +\tint run() override\n> >> +\t{\n> >> +\t\tunsigned int openFdsAtStart = 0;\n> >> +\t\tunsigned int openFds = 0;\n> >> +\n> >> +\t\tpid_t proxyPid = findProxyPid();\n> >> +\t\tif (proxyPid < 0) {\n> >> +\t\t\tcerr << \"Cannot find \" << kIpaProxyName_\n> >> +\t\t\t << \" pid, exiting\" << endl;\n> >> +\t\t\treturn TestFail;\n> >> +\t\t}\n> >> +\n> >> +\t\topenFdsAtStart = fdsOpen(proxyPid);\n> >> +\t\tfor (unsigned int i = 0; i < kNumOfReconfigures_; i++) {\n> >> +\t\t\tstartAndStop();\n> >> +\t\t\topenFds = fdsOpen(proxyPid);\n> >> +\t\t\tif (openFds == 0) {\n> >> +\t\t\t\tcerr << \"No open fds found whereas \"\n> >> +\t\t\t\t << \"open fds at start: \" << openFdsAtStart\n> >> +\t\t\t\t << endl;\n> >> +\t\t\t\treturn TestFail;\n> >> +\t\t\t}\n> >> +\n> >> +\t\t\tif (openFds != openFdsAtStart) {\n> >> +\t\t\t\tcerr << \"Leaking fds for \" << kIpaProxyName_\n> >> +\t\t\t\t << \" - Open fds: \" << openFds << \" vs \"\n> >> +\t\t\t\t << \"Open fds at start: \" << openFdsAtStart\n> >> +\t\t\t\t << endl;\n> >> +\t\t\t\treturn TestFail;\n> >> +\t\t\t}\n> >> +\t\t}\n> >> +\n> >> +\t\treturn TestPass;\n> >> +\t}\n> >> +\n> >> +\tbool allocated_;\n> >> +\n> >> +\tvector> requests_;\n> >> +\n> >> +\tunique_ptr config_;\n> >> +\tunique_ptr allocator_;\n> >> +};\n> >> +\n> >> +} /* namespace */\n> >> +\n> >> +TEST_REGISTER(CameraReconfigure)\n> >> diff --git a/test/camera/meson.build b/test/camera/meson.build\n> >> index 002a87b5..668d5c03 100644\n> >> --- a/test/camera/meson.build\n> >> +++ b/test/camera/meson.build\n> >> @@ -8,6 +8,7 @@ camera_tests = [\n> >> ['buffer_import', 'buffer_import.cpp'],\n> >> ['statemachine', 'statemachine.cpp'],\n> >> ['capture', 'capture.cpp'],\n> >> + ['camera_reconfigure', 'camera_reconfigure.cpp'],\n> >> ]\n> >> \n> >> foreach t : camera_tests","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B5320BD87C\n\tfor ;\n\tWed, 18 Aug 2021 02:15:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 07281688A3;\n\tWed, 18 Aug 2021 04:15:04 +0200 (CEST)","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 6D4B76025D\n\tfor ;\n\tWed, 18 Aug 2021 04:15:02 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DF7A5466;\n\tWed, 18 Aug 2021 04:15:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EcqLmWXr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629252902;\n\tbh=FJk34VsfacTMV62yZioaO1y/2IfGPXPK5aeddGz/GMg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EcqLmWXrSX4J+S/Us1JgrJuVoSPbxKg1/95LfN+dUOHBs7ueVw4uoMRYkDYeStRSx\n\t9VKB6KjANxl4/qBjjmTecuOOxjQW+AOLyAWRmGV9BRVFt5iUEoLdOf1qhk4cx+Vigm\n\tb3U2wOoFWXrHiinmyt7XmS2qOKuhemUYwUbFciTw=","Date":"Wed, 18 Aug 2021 05:14:54 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210817122646.185978-1-umang.jain@ideasonboard.com>\n\t<20210817122646.185978-3-umang.jain@ideasonboard.com>\n\t\n\t<2ef3631c-2b85-c457-fbb0-3024fe06302b@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<2ef3631c-2b85-c457-fbb0-3024fe06302b@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] test: camera: Camera\n\treconfiguration and fd-leak test","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]