[{"id":18658,"web_url":"https://patchwork.libcamera.org/comment/18658/","msgid":"<20210810073502.GS2167@pyrite.rasen.tech>","date":"2021-08-10T07:35:02","subject":"Re: [libcamera-devel] [RFC PATCH] test: camera: Camera\n\treconfiguration and fd-leak test","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Aug 06, 2021 at 05:21:32PM +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.\n\nThe test looks good!\n\n> \n> Currently, it uses vimc, but can be easily changed to using another\n> platform (for e.g. IPU3) by changing CAM_ID and IPA_PROXY_NAME defines.\n> \n> This test is geared towards reproducing [1], issue noticed on IPU3.\n> \n> Ensure that test is run in isolated mode.\n> Running standalone:\n> $ LIBCAMERA_IPA_FORCE_ISOLATION=1 ./build/test/camera/camera_reconfigure\n> \n> [1] https://bugs.libcamera.org/show_bug.cgi?id=63\n> \n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> ---\n> - still RFC because it's based on \"Pass buffers to VIMC IPA\" series\n> - We probably need to set LIBCAMERA_IPA_FORCE_ISOLATION=1 from inside\n>   the test. Setting at run-time with setenv(), sets the env var but still\n>   the test is unable to run in isolated mode. For now, I only have it\n\nThis doesn't work because the IPAs are created very early on, usually at\nmatch() time.\n\n>   working, by running it standalone as mentioned in the commit message.\n>   OR any other ideas enforcing isolated mode on the test?\n\nMaybe you could extend the CameraTest class to add an additional extra-early\noverridden init() (earlyInit() ?) before cm_ = new CameraManager?\n\n> - Some sprinkled prints are intentionally present, will be reworked in\n>   v1. Please ignore those for review.\n\nI'll withold my rb-tag then :D\n\n\nThanks,\n\nPaul\n\n>   (log snippet: https://paste.debian.net/1206765/)\n> ---\n>  test/camera/camera_reconfigure.cpp | 250 +++++++++++++++++++++++++++++\n>  test/camera/meson.build            |   1 +\n>  2 files changed, 251 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..20a4a6a4\n> --- /dev/null\n> +++ b/test/camera/camera_reconfigure.cpp\n> @@ -0,0 +1,250 @@\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 <dirent.h>\n> +#include <iostream>\n> +#include <fstream>\n> +\n> +#include <libcamera/framebuffer_allocator.h>\n> +\n> +#include <libcamera/base/event_dispatcher.h>\n> +#include <libcamera/base/file.h>\n> +#include <libcamera/base/thread.h>\n> +#include <libcamera/base/timer.h>\n> +\n> +#include \"camera_test.h\"\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +\n> +#define CAM_ID\t\t\t\"platform/vimc.0 Sensor B\"\n> +#define IPA_PROXY_NAME\t\t\"vimc_ipa_proxy\"\n> +#define NUM_OF_RECONFIGURES\t20\n> +\n> +namespace {\n> +\n> +class CameraReconfigure : public CameraTest, public Test\n> +{\n> +public:\n> +\tCameraReconfigure()\n> +\t\t: CameraTest(CAM_ID)\n> +\t{\n> +\t}\n> +\n> +protected:\n> +\tunsigned int configureRuns_;\n> +\tunsigned int openFdsAtStart_;\n> +\tstring vimcProxyName_;\n> +\tbool allocated_;\n> +\n> +\tvector<unique_ptr<Request>> requests_;\n> +\n> +\tunique_ptr<CameraConfiguration> config_;\n> +\tFrameBufferAllocator *allocator_;\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\tcout << \"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\tcout << \"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<FrameBuffer> &buffer : allocator_->buffers(stream)) {\n> +\t\t\tunique_ptr<Request> request = camera_->createRequest();\n> +\t\t\tif (!request) {\n> +\t\t\t\tcout << \"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\tcout << \"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\tcout << \"Failed to start camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tfor (unique_ptr<Request> &request : requests_) {\n> +\t\t\tif (camera_->queueRequest(request.get())) {\n> +\t\t\t\tcout << \"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(1000);\n> +\t\twhile (timer.isRunning())\n> +\t\t\tdispatcher->processEvents();\n> +\n> +\t\tif (camera_->stop()) {\n> +\t\t\tcout << \"Failed to stop camera\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (camera_->release()) {\n> +\t\t\tcout << \"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(string pid)\n> +\t{\n> +\t\tstring proxyFdPath = \"/proc/\" + 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\twhile((ptr = readdir(dir)) != nullptr) {\n> +\t\t\tif ((strcmp(ptr->d_name, \".\") == 0) || (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> +\tstring findProxyPid()\n> +\t{\n> +\t\tstring proxyPid;\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> +\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 == vimcProxyName_ ?\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\treturn proxyPid;\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\tcout << \"Failed to generate default configuration\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tallocator_ = new FrameBufferAllocator(camera_);\n> +\n> +\t\topenFdsAtStart_ = 0;\n> +\t\tconfigureRuns_ = NUM_OF_RECONFIGURES;\n> +\t\tvimcProxyName_ = IPA_PROXY_NAME;\n> +\t\tallocated_ = false;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tvoid cleanup() override\n> +\t{\n> +\t\tdelete allocator_;\n> +\t}\n> +\n> +\tint run() override\n> +\t{\n> +\t\tunsigned int openFds = 0;\n> +\t\t/* Find pid of vimc_ipa_proxy */\n> +\t\tstring proxyPid = findProxyPid();\n> +\t\tif (proxyPid.empty()) {\n> +\t\t\tcout << \"Not running in isolated mode, exiting\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/* Find number of open fds  by vimc_ipa_proxy */\n> +\t\topenFdsAtStart_ = fdsOpen(proxyPid);\n> +\t\tcout << IPA_PROXY_NAME << \" PID:\" << proxyPid\n> +\t\t\t<< \" has open fds at start:\"  << openFdsAtStart_ << endl;\n> +\n> +\t\tfor (unsigned int i = 0; i < configureRuns_; i++)\n> +\t\t{\n> +\t\t\tstartAndStop();\n> +\t\t\topenFds = fdsOpen(proxyPid);\n> +\t\t\tcout << \"No. of open fds after #\" << i << \" runs: \" << openFds << endl;\n> +\t\t}\n> +\n> +\t\tif (openFds == openFdsAtStart_)\n> +\t\t\treturn TestPass;\n> +\t\telse\n> +\t\t\treturn TestFail;\n> +\t}\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\n> -- \n> 2.31.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A6C55C3240\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 10 Aug 2021 07:35:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2726368826;\n\tTue, 10 Aug 2021 09:35:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3659C687DE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 10 Aug 2021 09:35:11 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC723EE;\n\tTue, 10 Aug 2021 09:35:09 +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=\"LpfGjtFk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628580910;\n\tbh=EwVYlOOstdhBUpdbYj4iem/zL5wj6G+9PqLU8C5tIwc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LpfGjtFksVsTcFgcOGYoF+NcMBeeFJ34HjxkkvpH0/BIV6a2FpzMjXg9X/17dRML0\n\tn0ZggaakRnB2pxrGlPv9td3YFuDqLWfFlReUGJRGYVjGoCsf3RZUWbWlMWxqvQZacd\n\tq/hYk4zjKcZf36tDzrOo0vWggUp5+CBqDN4a3Ooc=","Date":"Tue, 10 Aug 2021 16:35:02 +0900","From":"paul.elder@ideasonboard.com","To":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<20210810073502.GS2167@pyrite.rasen.tech>","References":"<20210806115132.353831-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210806115132.353831-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18802,"web_url":"https://patchwork.libcamera.org/comment/18802/","msgid":"<YRgybSFRJn6mSPJT@pendragon.ideasonboard.com>","date":"2021-08-14T21:15:25","subject":"Re: [libcamera-devel] [RFC PATCH] 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":"Hello,\n\nOn Tue, Aug 10, 2021 at 04:35:02PM +0900, paul.elder@ideasonboard.com wrote:\n> On Fri, Aug 06, 2021 at 05:21:32PM +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.\n> \n> The test looks good!\n> \n> > Currently, it uses vimc, but can be easily changed to using another\n> > platform (for e.g. IPU3) by changing CAM_ID and IPA_PROXY_NAME defines.\n> > \n> > This test is geared towards reproducing [1], issue noticed on IPU3.\n> > \n> > Ensure that test is run in isolated mode.\n> > Running standalone:\n> > $ LIBCAMERA_IPA_FORCE_ISOLATION=1 ./build/test/camera/camera_reconfigure\n> > \n> > [1] https://bugs.libcamera.org/show_bug.cgi?id=63\n> > \n> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> > ---\n> > - still RFC because it's based on \"Pass buffers to VIMC IPA\" series\n> > - We probably need to set LIBCAMERA_IPA_FORCE_ISOLATION=1 from inside\n> >   the test. Setting at run-time with setenv(), sets the env var but still\n> >   the test is unable to run in isolated mode. For now, I only have it\n> \n> This doesn't work because the IPAs are created very early on, usually at\n> match() time.\n\nWhich happens when CameraManager::start() is called, so it should be\npossible to set the environment variable before that.\n\n> >   working, by running it standalone as mentioned in the commit message.\n> >   OR any other ideas enforcing isolated mode on the test?\n> \n> Maybe you could extend the CameraTest class to add an additional extra-early\n> overridden init() (earlyInit() ?) before cm_ = new CameraManager?\n\nOr add a parameter to the constructor.\n\n> > - Some sprinkled prints are intentionally present, will be reworked in\n> >   v1. Please ignore those for review.\n> \n> I'll withold my rb-tag then :D\n> \n> >   (log snippet: https://paste.debian.net/1206765/)\n> > ---\n> >  test/camera/camera_reconfigure.cpp | 250 +++++++++++++++++++++++++++++\n> >  test/camera/meson.build            |   1 +\n> >  2 files changed, 251 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..20a4a6a4\n> > --- /dev/null\n> > +++ b/test/camera/camera_reconfigure.cpp\n> > @@ -0,0 +1,250 @@\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 <dirent.h>\n> > +#include <iostream>\n> > +#include <fstream>\n> > +\n> > +#include <libcamera/framebuffer_allocator.h>\n> > +\n> > +#include <libcamera/base/event_dispatcher.h>\n> > +#include <libcamera/base/file.h>\n> > +#include <libcamera/base/thread.h>\n> > +#include <libcamera/base/timer.h>\n> > +\n> > +#include \"camera_test.h\"\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> > +\n> > +#define CAM_ID\t\t\t\"platform/vimc.0 Sensor B\"\n> > +#define IPA_PROXY_NAME\t\t\"vimc_ipa_proxy\"\n> > +#define NUM_OF_RECONFIGURES\t20\n\n20 runs of one second each is long compared to the time taken to run the\ntest suite today. Can this be sped up ?\n\nThe C++ way would be\n\nstatic constexpr char *kCamId = \"platform/vimc.0 Sensor B\";\nstatic constexpr char *kIpaProxyName = \"vimc_ipa_proxy\";\nstatic constexpr unsigned int kNumOfReconfigures = 20;\n\nThey can also be made members of the CameraReconfigure class.\n\n> > +\n> > +namespace {\n> > +\n> > +class CameraReconfigure : public CameraTest, public Test\n> > +{\n> > +public:\n> > +\tCameraReconfigure()\n> > +\t\t: CameraTest(CAM_ID)\n> > +\t{\n> > +\t}\n> > +\n> > +protected:\n> > +\tunsigned int configureRuns_;\n> > +\tunsigned int openFdsAtStart_;\n> > +\tstring vimcProxyName_;\n> > +\tbool allocated_;\n> > +\n> > +\tvector<unique_ptr<Request>> requests_;\n> > +\n> > +\tunique_ptr<CameraConfiguration> config_;\n> > +\tFrameBufferAllocator *allocator_;\n\nVariables afer functions please. And these variables should be private.\n\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\nExtra space.\n\n> > +\t{\n> > +\t\tStreamConfiguration &cfg = config_->at(0);\n> > +\n> > +\t\tif (camera_->acquire()) {\n> > +\t\t\tcout << \"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\tcout << \"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<FrameBuffer> &buffer : allocator_->buffers(stream)) {\n> > +\t\t\tunique_ptr<Request> request = camera_->createRequest();\n> > +\t\t\tif (!request) {\n> > +\t\t\t\tcout << \"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\tcout << \"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\tcout << \"Failed to start camera\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tfor (unique_ptr<Request> &request : requests_) {\n> > +\t\t\tif (camera_->queueRequest(request.get())) {\n> > +\t\t\t\tcout << \"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(1000);\n> > +\t\twhile (timer.isRunning())\n> > +\t\t\tdispatcher->processEvents();\n> > +\n> > +\t\tif (camera_->stop()) {\n> > +\t\t\tcout << \"Failed to stop camera\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (camera_->release()) {\n> > +\t\t\tcout << \"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(string pid)\n> > +\t{\n> > +\t\tstring proxyFdPath = \"/proc/\" + 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\nError checking ?\n\n> > +\t\twhile((ptr = readdir(dir)) != nullptr) {\n\ns/while/while /\n\nThere are other missing spaces below.\n\n> > +\t\t\tif ((strcmp(ptr->d_name, \".\") == 0) || (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> > +\tstring findProxyPid()\n\nA pid is a number, I'd return a pid_t.\n\n> > +\t{\n> > +\t\tstring proxyPid;\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> > +\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 == vimcProxyName_ ?\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\treturn proxyPid;\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\tcout << \"Failed to generate default configuration\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tallocator_ = new FrameBufferAllocator(camera_);\n> > +\n> > +\t\topenFdsAtStart_ = 0;\n> > +\t\tconfigureRuns_ = NUM_OF_RECONFIGURES;\n> > +\t\tvimcProxyName_ = IPA_PROXY_NAME;\n> > +\t\tallocated_ = false;\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tvoid cleanup() override\n> > +\t{\n> > +\t\tdelete allocator_;\n\nYou can store it in an std::unique_ptr<>.\n\n> > +\t}\n> > +\n> > +\tint run() override\n> > +\t{\n> > +\t\tunsigned int openFds = 0;\n> > +\t\t/* Find pid of vimc_ipa_proxy */\n> > +\t\tstring proxyPid = findProxyPid();\n> > +\t\tif (proxyPid.empty()) {\n> > +\t\t\tcout << \"Not running in isolated mode, exiting\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/* Find number of open fds  by vimc_ipa_proxy */\n> > +\t\topenFdsAtStart_ = fdsOpen(proxyPid);\n> > +\t\tcout << IPA_PROXY_NAME << \" PID:\" << proxyPid\n> > +\t\t\t<< \" has open fds at start:\"  << openFdsAtStart_ << endl;\n> > +\n> > +\t\tfor (unsigned int i = 0; i < configureRuns_; i++)\n\nYou can drop the configureRuns_ member variable and use\nkNumOfReconfigures directly here. Same for the proxy name I think.\n\n> > +\t\t{\n\nThis should be at the end of the previous line.\n\n> > +\t\t\tstartAndStop();\n> > +\t\t\topenFds = fdsOpen(proxyPid);\n> > +\t\t\tcout << \"No. of open fds after #\" << i << \" runs: \" << openFds << endl;\n> > +\t\t}\n> > +\n> > +\t\tif (openFds == openFdsAtStart_)\n\nShouldn't this be checked for every run of the loop ? I'd then print the\nmessage with the number of fds only on failure.\n\n> > +\t\t\treturn TestPass;\n> > +\t\telse\n> > +\t\t\treturn TestFail;\n\nI wonder if counting the number of open fds at exit time isn't something\nthat the VIMC IPA should implement. That way, we would have the check in\nevery test, not just this one.\n\nPaul, what do you think ?\n\n> > +\t}\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":"<libcamera-devel-bounces@lists.libcamera.org>","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 66423BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 14 Aug 2021 21:15:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AEED668855;\n\tSat, 14 Aug 2021 23:15:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 228D960261\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 14 Aug 2021 23:15:31 +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 7D7AF3F0;\n\tSat, 14 Aug 2021 23:15:30 +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=\"n2lvwQbn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628975730;\n\tbh=IE964twCC+Rw3ZhMWj6kZ66mExMCe+uF5gxDjxuLcxU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n2lvwQbnCoypLBf6mAzW/H7ZVdF9TA6CS6pQxNjdg4JVlVlzd23oBdyZk61F0peID\n\txg/ibdvyNUSebXIGeJ65xY0okbfFpaWwmLLpghHnywSuDLNikUd0za9xhhOHexHtS/\n\toHFCj2ib5zNy5yfyQm3FWlbjTOxkNwQLCqZ1oDuI=","Date":"Sun, 15 Aug 2021 00:15:25 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"paul.elder@ideasonboard.com","Message-ID":"<YRgybSFRJn6mSPJT@pendragon.ideasonboard.com>","References":"<20210806115132.353831-1-umang.jain@ideasonboard.com>\n\t<20210810073502.GS2167@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210810073502.GS2167@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [RFC PATCH] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18816,"web_url":"https://patchwork.libcamera.org/comment/18816/","msgid":"<20210816065422.GB1733965@pyrite.rasen.tech>","date":"2021-08-16T06:54:22","subject":"Re: [libcamera-devel] [RFC PATCH] test: camera: Camera\n\treconfiguration and fd-leak test","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Sun, Aug 15, 2021 at 12:15:25AM +0300, Laurent Pinchart wrote:\n> Hello,\n> \n> On Tue, Aug 10, 2021 at 04:35:02PM +0900, paul.elder@ideasonboard.com wrote:\n> > On Fri, Aug 06, 2021 at 05:21:32PM +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.\n> > \n> > The test looks good!\n> > \n> > > Currently, it uses vimc, but can be easily changed to using another\n> > > platform (for e.g. IPU3) by changing CAM_ID and IPA_PROXY_NAME defines.\n> > > \n> > > This test is geared towards reproducing [1], issue noticed on IPU3.\n> > > \n> > > Ensure that test is run in isolated mode.\n> > > Running standalone:\n> > > $ LIBCAMERA_IPA_FORCE_ISOLATION=1 ./build/test/camera/camera_reconfigure\n> > > \n> > > [1] https://bugs.libcamera.org/show_bug.cgi?id=63\n> > > \n> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> > > ---\n> > > - still RFC because it's based on \"Pass buffers to VIMC IPA\" series\n> > > - We probably need to set LIBCAMERA_IPA_FORCE_ISOLATION=1 from inside\n> > >   the test. Setting at run-time with setenv(), sets the env var but still\n> > >   the test is unable to run in isolated mode. For now, I only have it\n> > \n> > This doesn't work because the IPAs are created very early on, usually at\n> > match() time.\n> \n> Which happens when CameraManager::start() is called, so it should be\n> possible to set the environment variable before that.\n> \n> > >   working, by running it standalone as mentioned in the commit message.\n> > >   OR any other ideas enforcing isolated mode on the test?\n> > \n> > Maybe you could extend the CameraTest class to add an additional extra-early\n> > overridden init() (earlyInit() ?) before cm_ = new CameraManager?\n> \n> Or add a parameter to the constructor.\n> \n> > > - Some sprinkled prints are intentionally present, will be reworked in\n> > >   v1. Please ignore those for review.\n> > \n> > I'll withold my rb-tag then :D\n> > \n> > >   (log snippet: https://paste.debian.net/1206765/)\n> > > ---\n> > >  test/camera/camera_reconfigure.cpp | 250 +++++++++++++++++++++++++++++\n> > >  test/camera/meson.build            |   1 +\n> > >  2 files changed, 251 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..20a4a6a4\n> > > --- /dev/null\n> > > +++ b/test/camera/camera_reconfigure.cpp\n> > > @@ -0,0 +1,250 @@\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 <dirent.h>\n> > > +#include <iostream>\n> > > +#include <fstream>\n> > > +\n> > > +#include <libcamera/framebuffer_allocator.h>\n> > > +\n> > > +#include <libcamera/base/event_dispatcher.h>\n> > > +#include <libcamera/base/file.h>\n> > > +#include <libcamera/base/thread.h>\n> > > +#include <libcamera/base/timer.h>\n> > > +\n> > > +#include \"camera_test.h\"\n> > > +#include \"test.h\"\n> > > +\n> > > +using namespace std;\n> > > +\n> > > +#define CAM_ID\t\t\t\"platform/vimc.0 Sensor B\"\n> > > +#define IPA_PROXY_NAME\t\t\"vimc_ipa_proxy\"\n> > > +#define NUM_OF_RECONFIGURES\t20\n> \n> 20 runs of one second each is long compared to the time taken to run the\n> test suite today. Can this be sped up ?\n> \n> The C++ way would be\n> \n> static constexpr char *kCamId = \"platform/vimc.0 Sensor B\";\n> static constexpr char *kIpaProxyName = \"vimc_ipa_proxy\";\n> static constexpr unsigned int kNumOfReconfigures = 20;\n> \n> They can also be made members of the CameraReconfigure class.\n> \n> > > +\n> > > +namespace {\n> > > +\n> > > +class CameraReconfigure : public CameraTest, public Test\n> > > +{\n> > > +public:\n> > > +\tCameraReconfigure()\n> > > +\t\t: CameraTest(CAM_ID)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +protected:\n> > > +\tunsigned int configureRuns_;\n> > > +\tunsigned int openFdsAtStart_;\n> > > +\tstring vimcProxyName_;\n> > > +\tbool allocated_;\n> > > +\n> > > +\tvector<unique_ptr<Request>> requests_;\n> > > +\n> > > +\tunique_ptr<CameraConfiguration> config_;\n> > > +\tFrameBufferAllocator *allocator_;\n> \n> Variables afer functions please. And these variables should be private.\n> \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> \n> Extra space.\n> \n> > > +\t{\n> > > +\t\tStreamConfiguration &cfg = config_->at(0);\n> > > +\n> > > +\t\tif (camera_->acquire()) {\n> > > +\t\t\tcout << \"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\tcout << \"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<FrameBuffer> &buffer : allocator_->buffers(stream)) {\n> > > +\t\t\tunique_ptr<Request> request = camera_->createRequest();\n> > > +\t\t\tif (!request) {\n> > > +\t\t\t\tcout << \"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\tcout << \"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\tcout << \"Failed to start camera\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tfor (unique_ptr<Request> &request : requests_) {\n> > > +\t\t\tif (camera_->queueRequest(request.get())) {\n> > > +\t\t\t\tcout << \"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(1000);\n> > > +\t\twhile (timer.isRunning())\n> > > +\t\t\tdispatcher->processEvents();\n> > > +\n> > > +\t\tif (camera_->stop()) {\n> > > +\t\t\tcout << \"Failed to stop camera\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (camera_->release()) {\n> > > +\t\t\tcout << \"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(string pid)\n> > > +\t{\n> > > +\t\tstring proxyFdPath = \"/proc/\" + 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> \n> Error checking ?\n> \n> > > +\t\twhile((ptr = readdir(dir)) != nullptr) {\n> \n> s/while/while /\n> \n> There are other missing spaces below.\n> \n> > > +\t\t\tif ((strcmp(ptr->d_name, \".\") == 0) || (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> > > +\tstring findProxyPid()\n> \n> A pid is a number, I'd return a pid_t.\n> \n> > > +\t{\n> > > +\t\tstring proxyPid;\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> > > +\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 == vimcProxyName_ ?\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\treturn proxyPid;\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\tcout << \"Failed to generate default configuration\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tallocator_ = new FrameBufferAllocator(camera_);\n> > > +\n> > > +\t\topenFdsAtStart_ = 0;\n> > > +\t\tconfigureRuns_ = NUM_OF_RECONFIGURES;\n> > > +\t\tvimcProxyName_ = IPA_PROXY_NAME;\n> > > +\t\tallocated_ = false;\n> > > +\n> > > +\t\treturn TestPass;\n> > > +\t}\n> > > +\n> > > +\tvoid cleanup() override\n> > > +\t{\n> > > +\t\tdelete allocator_;\n> \n> You can store it in an std::unique_ptr<>.\n> \n> > > +\t}\n> > > +\n> > > +\tint run() override\n> > > +\t{\n> > > +\t\tunsigned int openFds = 0;\n> > > +\t\t/* Find pid of vimc_ipa_proxy */\n> > > +\t\tstring proxyPid = findProxyPid();\n> > > +\t\tif (proxyPid.empty()) {\n> > > +\t\t\tcout << \"Not running in isolated mode, exiting\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\t/* Find number of open fds  by vimc_ipa_proxy */\n> > > +\t\topenFdsAtStart_ = fdsOpen(proxyPid);\n> > > +\t\tcout << IPA_PROXY_NAME << \" PID:\" << proxyPid\n> > > +\t\t\t<< \" has open fds at start:\"  << openFdsAtStart_ << endl;\n> > > +\n> > > +\t\tfor (unsigned int i = 0; i < configureRuns_; i++)\n> \n> You can drop the configureRuns_ member variable and use\n> kNumOfReconfigures directly here. Same for the proxy name I think.\n> \n> > > +\t\t{\n> \n> This should be at the end of the previous line.\n> \n> > > +\t\t\tstartAndStop();\n> > > +\t\t\topenFds = fdsOpen(proxyPid);\n> > > +\t\t\tcout << \"No. of open fds after #\" << i << \" runs: \" << openFds << endl;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (openFds == openFdsAtStart_)\n> \n> Shouldn't this be checked for every run of the loop ? I'd then print the\n> message with the number of fds only on failure.\n> \n> > > +\t\t\treturn TestPass;\n> > > +\t\telse\n> > > +\t\t\treturn TestFail;\n> \n> I wonder if counting the number of open fds at exit time isn't something\n> that the VIMC IPA should implement. That way, we would have the check in\n> every test, not just this one.\n> \n> Paul, what do you think ?\n\nYeah, I think that would be useful.\n\n\nPaul\n\n> \n> > > +\t}\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":"<libcamera-devel-bounces@lists.libcamera.org>","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 773A4BD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 16 Aug 2021 06:54:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE24368892;\n\tMon, 16 Aug 2021 08:54:32 +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 9E0FA6025C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Aug 2021 08:54:30 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CF8703E5;\n\tMon, 16 Aug 2021 08:54:28 +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=\"g6zw9xKM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629096870;\n\tbh=8P845vQaTgCR6QXvw33mSjSaHDbxd4WOv3D7gogRWWg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g6zw9xKMj1pRNMbpN2PBCHC80mZdMmsiqCcol9GRcUA9blf6HBghft3GyD0b8EgU7\n\tUAbXNExCDYZhuqzRMBYhKgs0x71Qua+e6GBOuMErZuvC1lwavhGKCMRcb/LK0dQZ+d\n\tvy2QNxFciCmjBIPc/6j5uOqLQhqiQqTE2O84AJCU=","Date":"Mon, 16 Aug 2021 15:54:22 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210816065422.GB1733965@pyrite.rasen.tech>","References":"<20210806115132.353831-1-umang.jain@ideasonboard.com>\n\t<20210810073502.GS2167@pyrite.rasen.tech>\n\t<YRgybSFRJn6mSPJT@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<YRgybSFRJn6mSPJT@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]