Message ID | 20210817122646.185978-3-umang.jain@ideasonboard.com |
---|---|
State | Superseded |
Delegated to: | Umang Jain |
Headers | show |
Series |
|
Related | show |
Hi Umang, Thank you for the patch. On Tue, Aug 17, 2021 at 05:56:46PM +0530, Umang Jain wrote: > This tests basically checks for two things: > - Camera reconfigurations without stopping CameraManager > - Fd leaks across IPA IPC boundary [1] > > Currently, it uses vimc, but can be easily changed to using another > platform (for e.g. IPU3) by changing kCamId_ and kIpaProxyName_. > > The test performs kNumOfReconfigures_ (currently set to 10) > reconfigurations of the camera. Each reconfiguration runs > start(), capture(100ms), stop() of the camera. Hence, the test > runs approximately for 1 second. > > For checking the fd leaks, the test monitors the /proc/$PROXY_PID/fd > directory for open fds. It compares the number of open fds after each > run to the number of open fds before the first run. If those two are > found to be mis-matched, the test shall report failure. > > Since, the test needs to test the IPA IPC code paths, it is meant to > always run with LIBCAMERA_IPA_FORCE_ISOLATION environment variable. > > [1] https://bugs.libcamera.org/show_bug.cgi?id=63 > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > test/camera/camera_reconfigure.cpp | 257 +++++++++++++++++++++++++++++ > test/camera/meson.build | 1 + > 2 files changed, 258 insertions(+) > create mode 100644 test/camera/camera_reconfigure.cpp > > diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp > new file mode 100644 > index 00000000..b32264b1 > --- /dev/null > +++ b/test/camera/camera_reconfigure.cpp > @@ -0,0 +1,257 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2021, Google Inc. > + * > + * Test: > + * - Camera's multiple reconfigurations without stopping CameraManager > + * - leaking fds across IPA IPC boundary > + */ > + > +#include <dirent.h> > +#include <iostream> > +#include <fstream> > + > +#include <libcamera/framebuffer_allocator.h> > + > +#include <libcamera/base/event_dispatcher.h> > +#include <libcamera/base/file.h> > +#include <libcamera/base/thread.h> > +#include <libcamera/base/timer.h> > + > +#include "camera_test.h" > +#include "test.h" > + > +using namespace std; > + > +namespace { > + > +class CameraReconfigure : public CameraTest, public Test > +{ > +public: > + CameraReconfigure() > + : CameraTest(kCamId_, true) > + { > + } > + > +private: > + static constexpr const char *kCamId_ = "platform/vimc.0 Sensor B"; > + static constexpr const char *kIpaProxyName_ = "vimc_ipa_proxy"; > + static constexpr unsigned int kNumOfReconfigures_ = 10; > + > + void requestComplete(Request *request) > + { > + if (request->status() != Request::RequestComplete) > + return; > + > + const Request::BufferMap &buffers = request->buffers(); > + > + /* Create a new request. */ > + const Stream *stream = buffers.begin()->first; > + FrameBuffer *buffer = buffers.begin()->second; > + > + request->reuse(); > + request->addBuffer(stream, buffer); > + camera_->queueRequest(request); > + } > + > + int startAndStop() > + { > + StreamConfiguration &cfg = config_->at(0); > + > + if (camera_->acquire()) { > + cerr << "Failed to acquire the camera" << endl; > + return TestFail; > + } > + > + if (camera_->configure(config_.get())) { > + cerr << "Failed to set default configuration" << endl; > + return TestFail; > + } > + > + Stream *stream = cfg.stream(); > + > + if (!allocated_) { > + int ret = allocator_->allocate(stream); > + if (ret < 0) > + return TestFail; > + allocated_ = true; > + } > + > + for (const unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) { > + unique_ptr<Request> request = camera_->createRequest(); > + if (!request) { > + cerr << "Failed to create request" << endl; > + return TestFail; > + } > + > + if (request->addBuffer(stream, buffer.get())) { > + cerr << "Failed to associating buffer with request" << endl; > + return TestFail; > + } > + > + requests_.push_back(move(request)); > + } > + > + camera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete); > + > + if (camera_->start()) { > + cerr << "Failed to start camera" << endl; > + return TestFail; > + } > + > + for (unique_ptr<Request> &request : requests_) { > + if (camera_->queueRequest(request.get())) { > + cerr << "Failed to queue request" << endl; > + return TestFail; > + } > + } > + > + EventDispatcher *dispatcher = Thread::current()->eventDispatcher(); > + > + Timer timer; > + timer.start(100); > + while (timer.isRunning()) > + dispatcher->processEvents(); > + > + if (camera_->stop()) { > + cerr << "Failed to stop camera" << endl; > + return TestFail; > + } > + > + if (camera_->release()) { > + cerr << "Failed to release camera" << endl; > + return TestFail; > + } > + > + camera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete); > + > + requests_.clear(); > + > + return 0; > + } > + > + int fdsOpen(pid_t pid) > + { > + string proxyFdPath = "/proc/" + to_string(pid) + "/fd"; > + DIR *dir; > + struct dirent *ptr; > + unsigned int openFds = 0; > + > + dir = opendir(proxyFdPath.c_str()); > + if (dir == nullptr) { > + int err = errno; > + cerr << "Error opening " << proxyFdPath << ": " > + << strerror(-err) << endl; > + return 0; > + } > + > + while ((ptr = readdir(dir)) != nullptr) { > + if ((strcmp(ptr->d_name, ".") == 0) || > + (strcmp(ptr->d_name, "..") == 0)) > + continue; > + > + openFds++; > + } > + closedir(dir); > + > + return openFds; > + } > + > + pid_t findProxyPid() > + { > + string proxyPid; > + string proxyName(kIpaProxyName_); > + DIR *dir; > + struct dirent *ptr; > + > + dir = opendir("/proc"); > + while((ptr = readdir(dir)) != nullptr) { > + if(ptr->d_type != DT_DIR) > + continue; > + > + string pname("/proc/" + string(ptr->d_name) + "/comm"); > + if(File::exists(pname.c_str())) { Missing space. I may have missed it, but I haven't seen an answer to my review of the previous version that proposed checking the number of open file descriptors in the VIMC IPA module itself. That would test for fd leaks in all tests using vimc, and wouldn't require finding child processes. > + ifstream pfile(pname.c_str()); > + string comm; > + getline (pfile, comm); > + pfile.close(); > + > + proxyPid = comm == proxyName ? > + string(ptr->d_name) : > + ""; > + } > + > + if (!proxyPid.empty()) > + break; > + } > + closedir(dir); > + > + if (!proxyPid.empty()) > + return atoi(proxyPid.c_str()); > + > + return -1; > + } > + > + int init() override > + { > + if (status_ != TestPass) > + return status_; > + > + config_ = camera_->generateConfiguration({ StreamRole::StillCapture }); > + if (!config_ || config_->size() != 1) { > + cerr << "Failed to generate default configuration" << endl; > + return TestFail; > + } > + > + allocator_ = make_unique<FrameBufferAllocator>(camera_); > + allocated_ = false; > + > + return TestPass; > + } > + > + int run() override > + { > + unsigned int openFdsAtStart = 0; > + unsigned int openFds = 0; > + > + pid_t proxyPid = findProxyPid(); > + if (proxyPid < 0) { > + cerr << "Cannot find " << kIpaProxyName_ > + << " pid, exiting" << endl; > + return TestFail; > + } > + > + openFdsAtStart = fdsOpen(proxyPid); > + for (unsigned int i = 0; i < kNumOfReconfigures_; i++) { > + startAndStop(); > + openFds = fdsOpen(proxyPid); > + if (openFds == 0) { > + cerr << "No open fds found whereas " > + << "open fds at start: " << openFdsAtStart > + << endl; > + return TestFail; > + } > + > + if (openFds != openFdsAtStart) { > + cerr << "Leaking fds for " << kIpaProxyName_ > + << " - Open fds: " << openFds << " vs " > + << "Open fds at start: " << openFdsAtStart > + << endl; > + return TestFail; > + } > + } > + > + return TestPass; > + } > + > + bool allocated_; > + > + vector<unique_ptr<Request>> requests_; > + > + unique_ptr<CameraConfiguration> config_; > + unique_ptr<FrameBufferAllocator> allocator_; > +}; > + > +} /* namespace */ > + > +TEST_REGISTER(CameraReconfigure) > diff --git a/test/camera/meson.build b/test/camera/meson.build > index 002a87b5..668d5c03 100644 > --- a/test/camera/meson.build > +++ b/test/camera/meson.build > @@ -8,6 +8,7 @@ camera_tests = [ > ['buffer_import', 'buffer_import.cpp'], > ['statemachine', 'statemachine.cpp'], > ['capture', 'capture.cpp'], > + ['camera_reconfigure', 'camera_reconfigure.cpp'], > ] > > foreach t : camera_tests
Hi Laurent, On 17/08/2021 14:04, Laurent Pinchart wrote: > Hi Umang, > >> + pid_t findProxyPid() >> + { >> + string proxyPid; >> + string proxyName(kIpaProxyName_); >> + DIR *dir; >> + struct dirent *ptr; >> + >> + dir = opendir("/proc"); >> + while((ptr = readdir(dir)) != nullptr) { >> + if(ptr->d_type != DT_DIR) >> + continue; >> + >> + string pname("/proc/" + string(ptr->d_name) + "/comm"); >> + if(File::exists(pname.c_str())) { > > Missing space. > > I may have missed it, but I haven't seen an answer to my review of the > previous version that proposed checking the number of open file > descriptors in the VIMC IPA module itself. That would test for fd leaks > in all tests using vimc, and wouldn't require finding child processes. I presume this would just fire an ASSERT() if it was detected. (Unless you have another way to report the failure up to the test?) I'm a bit weary that this would be hard to track down when it fires as the failure would occur in a process which we don't (easily) get logs from. The failure condition might get a bit 'lost'. The assert would cause the IPA to 'crash', and the tests would fail - but they would look like they were failing because the IPA had 'disappeared' and may not be obvious to diagnose.. I wonder if we need to tackle how we log from isolated processes already ... :-S I guess the alternative is to have some extended interface callbacks from the IPA to allow it to message the VIMC pipeline handler to report something ... and ... cause a failure and shutdown on the pipeline side ? -- Kieran
Hi Laurent, On 8/17/21 6:34 PM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Tue, Aug 17, 2021 at 05:56:46PM +0530, Umang Jain wrote: >> This tests basically checks for two things: >> - Camera reconfigurations without stopping CameraManager >> - Fd leaks across IPA IPC boundary [1] >> >> Currently, it uses vimc, but can be easily changed to using another >> platform (for e.g. IPU3) by changing kCamId_ and kIpaProxyName_. >> >> The test performs kNumOfReconfigures_ (currently set to 10) >> reconfigurations of the camera. Each reconfiguration runs >> start(), capture(100ms), stop() of the camera. Hence, the test >> runs approximately for 1 second. >> >> For checking the fd leaks, the test monitors the /proc/$PROXY_PID/fd >> directory for open fds. It compares the number of open fds after each >> run to the number of open fds before the first run. If those two are >> found to be mis-matched, the test shall report failure. >> >> Since, the test needs to test the IPA IPC code paths, it is meant to >> always run with LIBCAMERA_IPA_FORCE_ISOLATION environment variable. >> >> [1] https://bugs.libcamera.org/show_bug.cgi?id=63 >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> test/camera/camera_reconfigure.cpp | 257 +++++++++++++++++++++++++++++ >> test/camera/meson.build | 1 + >> 2 files changed, 258 insertions(+) >> create mode 100644 test/camera/camera_reconfigure.cpp >> >> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp >> new file mode 100644 >> index 00000000..b32264b1 >> --- /dev/null >> +++ b/test/camera/camera_reconfigure.cpp >> @@ -0,0 +1,257 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Copyright (C) 2021, Google Inc. >> + * >> + * Test: >> + * - Camera's multiple reconfigurations without stopping CameraManager >> + * - leaking fds across IPA IPC boundary >> + */ >> + >> +#include <dirent.h> >> +#include <iostream> >> +#include <fstream> >> + >> +#include <libcamera/framebuffer_allocator.h> >> + >> +#include <libcamera/base/event_dispatcher.h> >> +#include <libcamera/base/file.h> >> +#include <libcamera/base/thread.h> >> +#include <libcamera/base/timer.h> >> + >> +#include "camera_test.h" >> +#include "test.h" >> + >> +using namespace std; >> + >> +namespace { >> + >> +class CameraReconfigure : public CameraTest, public Test >> +{ >> +public: >> + CameraReconfigure() >> + : CameraTest(kCamId_, true) >> + { >> + } >> + >> +private: >> + static constexpr const char *kCamId_ = "platform/vimc.0 Sensor B"; >> + static constexpr const char *kIpaProxyName_ = "vimc_ipa_proxy"; >> + static constexpr unsigned int kNumOfReconfigures_ = 10; >> + >> + void requestComplete(Request *request) >> + { >> + if (request->status() != Request::RequestComplete) >> + return; >> + >> + const Request::BufferMap &buffers = request->buffers(); >> + >> + /* Create a new request. */ >> + const Stream *stream = buffers.begin()->first; >> + FrameBuffer *buffer = buffers.begin()->second; >> + >> + request->reuse(); >> + request->addBuffer(stream, buffer); >> + camera_->queueRequest(request); >> + } >> + >> + int startAndStop() >> + { >> + StreamConfiguration &cfg = config_->at(0); >> + >> + if (camera_->acquire()) { >> + cerr << "Failed to acquire the camera" << endl; >> + return TestFail; >> + } >> + >> + if (camera_->configure(config_.get())) { >> + cerr << "Failed to set default configuration" << endl; >> + return TestFail; >> + } >> + >> + Stream *stream = cfg.stream(); >> + >> + if (!allocated_) { >> + int ret = allocator_->allocate(stream); >> + if (ret < 0) >> + return TestFail; >> + allocated_ = true; >> + } >> + >> + for (const unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) { >> + unique_ptr<Request> request = camera_->createRequest(); >> + if (!request) { >> + cerr << "Failed to create request" << endl; >> + return TestFail; >> + } >> + >> + if (request->addBuffer(stream, buffer.get())) { >> + cerr << "Failed to associating buffer with request" << endl; >> + return TestFail; >> + } >> + >> + requests_.push_back(move(request)); >> + } >> + >> + camera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete); >> + >> + if (camera_->start()) { >> + cerr << "Failed to start camera" << endl; >> + return TestFail; >> + } >> + >> + for (unique_ptr<Request> &request : requests_) { >> + if (camera_->queueRequest(request.get())) { >> + cerr << "Failed to queue request" << endl; >> + return TestFail; >> + } >> + } >> + >> + EventDispatcher *dispatcher = Thread::current()->eventDispatcher(); >> + >> + Timer timer; >> + timer.start(100); >> + while (timer.isRunning()) >> + dispatcher->processEvents(); >> + >> + if (camera_->stop()) { >> + cerr << "Failed to stop camera" << endl; >> + return TestFail; >> + } >> + >> + if (camera_->release()) { >> + cerr << "Failed to release camera" << endl; >> + return TestFail; >> + } >> + >> + camera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete); >> + >> + requests_.clear(); >> + >> + return 0; >> + } >> + >> + int fdsOpen(pid_t pid) >> + { >> + string proxyFdPath = "/proc/" + to_string(pid) + "/fd"; >> + DIR *dir; >> + struct dirent *ptr; >> + unsigned int openFds = 0; >> + >> + dir = opendir(proxyFdPath.c_str()); >> + if (dir == nullptr) { >> + int err = errno; >> + cerr << "Error opening " << proxyFdPath << ": " >> + << strerror(-err) << endl; >> + return 0; >> + } >> + >> + while ((ptr = readdir(dir)) != nullptr) { >> + if ((strcmp(ptr->d_name, ".") == 0) || >> + (strcmp(ptr->d_name, "..") == 0)) >> + continue; >> + >> + openFds++; >> + } >> + closedir(dir); >> + >> + return openFds; >> + } >> + >> + pid_t findProxyPid() >> + { >> + string proxyPid; >> + string proxyName(kIpaProxyName_); >> + DIR *dir; >> + struct dirent *ptr; >> + >> + dir = opendir("/proc"); >> + while((ptr = readdir(dir)) != nullptr) { >> + if(ptr->d_type != DT_DIR) >> + continue; >> + >> + string pname("/proc/" + string(ptr->d_name) + "/comm"); >> + if(File::exists(pname.c_str())) { > Missing space. I failed to run checkstyle.py before sending, apologies. I hit -ENOSPC on my primary device few days agp and I am ended up working on a adhoc clone which didn't had the checkstyle.py as post-commit hook > > I may have missed it, but I haven't seen an answer to my review of the > previous version that proposed checking the number of open file > descriptors in the VIMC IPA module itself. That would test for fd leaks > in all tests using vimc, and wouldn't require finding child processes. Coming to your request, the reply sounded to me as an enhancement in the test-able VIMC in general, to increase our test coverage and IPC code paths. I am assuming you don't intend to dis-regard this particular test in favor of having it plumbed down to VIMC IPA, or is it? I am assuming on my part that both can(or should) co-exist, latter being developed along the lines and out of scope of this patch. Our priorities are well-addressed by this test for now, so maybe can I track it in bugzilla and come back to it in coming weeks? > >> + ifstream pfile(pname.c_str()); >> + string comm; >> + getline (pfile, comm); >> + pfile.close(); >> + >> + proxyPid = comm == proxyName ? >> + string(ptr->d_name) : >> + ""; >> + } >> + >> + if (!proxyPid.empty()) >> + break; >> + } >> + closedir(dir); >> + >> + if (!proxyPid.empty()) >> + return atoi(proxyPid.c_str()); >> + >> + return -1; >> + } >> + >> + int init() override >> + { >> + if (status_ != TestPass) >> + return status_; >> + >> + config_ = camera_->generateConfiguration({ StreamRole::StillCapture }); >> + if (!config_ || config_->size() != 1) { >> + cerr << "Failed to generate default configuration" << endl; >> + return TestFail; >> + } >> + >> + allocator_ = make_unique<FrameBufferAllocator>(camera_); >> + allocated_ = false; >> + >> + return TestPass; >> + } >> + >> + int run() override >> + { >> + unsigned int openFdsAtStart = 0; >> + unsigned int openFds = 0; >> + >> + pid_t proxyPid = findProxyPid(); >> + if (proxyPid < 0) { >> + cerr << "Cannot find " << kIpaProxyName_ >> + << " pid, exiting" << endl; >> + return TestFail; >> + } >> + >> + openFdsAtStart = fdsOpen(proxyPid); >> + for (unsigned int i = 0; i < kNumOfReconfigures_; i++) { >> + startAndStop(); >> + openFds = fdsOpen(proxyPid); >> + if (openFds == 0) { >> + cerr << "No open fds found whereas " >> + << "open fds at start: " << openFdsAtStart >> + << endl; >> + return TestFail; >> + } >> + >> + if (openFds != openFdsAtStart) { >> + cerr << "Leaking fds for " << kIpaProxyName_ >> + << " - Open fds: " << openFds << " vs " >> + << "Open fds at start: " << openFdsAtStart >> + << endl; >> + return TestFail; >> + } >> + } >> + >> + return TestPass; >> + } >> + >> + bool allocated_; >> + >> + vector<unique_ptr<Request>> requests_; >> + >> + unique_ptr<CameraConfiguration> config_; >> + unique_ptr<FrameBufferAllocator> allocator_; >> +}; >> + >> +} /* namespace */ >> + >> +TEST_REGISTER(CameraReconfigure) >> diff --git a/test/camera/meson.build b/test/camera/meson.build >> index 002a87b5..668d5c03 100644 >> --- a/test/camera/meson.build >> +++ b/test/camera/meson.build >> @@ -8,6 +8,7 @@ camera_tests = [ >> ['buffer_import', 'buffer_import.cpp'], >> ['statemachine', 'statemachine.cpp'], >> ['capture', 'capture.cpp'], >> + ['camera_reconfigure', 'camera_reconfigure.cpp'], >> ] >> >> foreach t : camera_tests
Hi Kieran, On Tue, Aug 17, 2021 at 02:57:24PM +0100, Kieran Bingham wrote: > On 17/08/2021 14:04, Laurent Pinchart wrote: > > Hi Umang, > > >> + pid_t findProxyPid() > >> + { > >> + string proxyPid; > >> + string proxyName(kIpaProxyName_); > >> + DIR *dir; > >> + struct dirent *ptr; > >> + > >> + dir = opendir("/proc"); > >> + while((ptr = readdir(dir)) != nullptr) { > >> + if(ptr->d_type != DT_DIR) > >> + continue; > >> + > >> + string pname("/proc/" + string(ptr->d_name) + "/comm"); > >> + if(File::exists(pname.c_str())) { > > > > Missing space. > > > > I may have missed it, but I haven't seen an answer to my review of the > > previous version that proposed checking the number of open file > > descriptors in the VIMC IPA module itself. That would test for fd leaks > > in all tests using vimc, and wouldn't require finding child processes. > > > I presume this would just fire an ASSERT() if it was detected. (Unless > you have another way to report the failure up to the test?) > > I'm a bit weary that this would be hard to track down when it fires as > the failure would occur in a process which we don't (easily) get logs > from. The failure condition might get a bit 'lost'. > > The assert would cause the IPA to 'crash', and the tests would fail - > but they would look like they were failing because the IPA had > 'disappeared' and may not be obvious to diagnose.. > > > I wonder if we need to tackle how we log from isolated processes already > ... :-S > > > I guess the alternative is to have some extended interface callbacks > from the IPA to allow it to message the VIMC pipeline handler to report > something ... and ... cause a failure and shutdown on the pipeline side ? We do have a tracing mechanism already, with a FIFO used to pass information from the IPA to tests. It could be leveraged, possibly just using the existing infrastructure to start with (it's limited to passing 32-bit operation codes for tracing), extending it later with support for more data.
Hi Umang, On Tue, Aug 17, 2021 at 08:30:34PM +0530, Umang Jain wrote: > On 8/17/21 6:34 PM, Laurent Pinchart wrote: > > On Tue, Aug 17, 2021 at 05:56:46PM +0530, Umang Jain wrote: > >> This tests basically checks for two things: > >> - Camera reconfigurations without stopping CameraManager > >> - Fd leaks across IPA IPC boundary [1] > >> > >> Currently, it uses vimc, but can be easily changed to using another > >> platform (for e.g. IPU3) by changing kCamId_ and kIpaProxyName_. > >> > >> The test performs kNumOfReconfigures_ (currently set to 10) > >> reconfigurations of the camera. Each reconfiguration runs > >> start(), capture(100ms), stop() of the camera. Hence, the test > >> runs approximately for 1 second. > >> > >> For checking the fd leaks, the test monitors the /proc/$PROXY_PID/fd > >> directory for open fds. It compares the number of open fds after each > >> run to the number of open fds before the first run. If those two are > >> found to be mis-matched, the test shall report failure. > >> > >> Since, the test needs to test the IPA IPC code paths, it is meant to > >> always run with LIBCAMERA_IPA_FORCE_ISOLATION environment variable. > >> > >> [1] https://bugs.libcamera.org/show_bug.cgi?id=63 > >> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> test/camera/camera_reconfigure.cpp | 257 +++++++++++++++++++++++++++++ > >> test/camera/meson.build | 1 + > >> 2 files changed, 258 insertions(+) > >> create mode 100644 test/camera/camera_reconfigure.cpp > >> > >> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp > >> new file mode 100644 > >> index 00000000..b32264b1 > >> --- /dev/null > >> +++ b/test/camera/camera_reconfigure.cpp > >> @@ -0,0 +1,257 @@ > >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ > >> +/* > >> + * Copyright (C) 2021, Google Inc. > >> + * > >> + * Test: > >> + * - Camera's multiple reconfigurations without stopping CameraManager > >> + * - leaking fds across IPA IPC boundary > >> + */ > >> + > >> +#include <dirent.h> > >> +#include <iostream> > >> +#include <fstream> > >> + > >> +#include <libcamera/framebuffer_allocator.h> > >> + > >> +#include <libcamera/base/event_dispatcher.h> > >> +#include <libcamera/base/file.h> > >> +#include <libcamera/base/thread.h> > >> +#include <libcamera/base/timer.h> > >> + > >> +#include "camera_test.h" > >> +#include "test.h" > >> + > >> +using namespace std; > >> + > >> +namespace { > >> + > >> +class CameraReconfigure : public CameraTest, public Test > >> +{ > >> +public: > >> + CameraReconfigure() > >> + : CameraTest(kCamId_, true) > >> + { > >> + } > >> + > >> +private: > >> + static constexpr const char *kCamId_ = "platform/vimc.0 Sensor B"; > >> + static constexpr const char *kIpaProxyName_ = "vimc_ipa_proxy"; > >> + static constexpr unsigned int kNumOfReconfigures_ = 10; > >> + > >> + void requestComplete(Request *request) > >> + { > >> + if (request->status() != Request::RequestComplete) > >> + return; > >> + > >> + const Request::BufferMap &buffers = request->buffers(); > >> + > >> + /* Create a new request. */ > >> + const Stream *stream = buffers.begin()->first; > >> + FrameBuffer *buffer = buffers.begin()->second; > >> + > >> + request->reuse(); > >> + request->addBuffer(stream, buffer); > >> + camera_->queueRequest(request); > >> + } > >> + > >> + int startAndStop() > >> + { > >> + StreamConfiguration &cfg = config_->at(0); > >> + > >> + if (camera_->acquire()) { > >> + cerr << "Failed to acquire the camera" << endl; > >> + return TestFail; > >> + } > >> + > >> + if (camera_->configure(config_.get())) { > >> + cerr << "Failed to set default configuration" << endl; > >> + return TestFail; > >> + } > >> + > >> + Stream *stream = cfg.stream(); > >> + > >> + if (!allocated_) { > >> + int ret = allocator_->allocate(stream); > >> + if (ret < 0) > >> + return TestFail; > >> + allocated_ = true; > >> + } > >> + > >> + for (const unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) { > >> + unique_ptr<Request> request = camera_->createRequest(); > >> + if (!request) { > >> + cerr << "Failed to create request" << endl; > >> + return TestFail; > >> + } > >> + > >> + if (request->addBuffer(stream, buffer.get())) { > >> + cerr << "Failed to associating buffer with request" << endl; > >> + return TestFail; > >> + } > >> + > >> + requests_.push_back(move(request)); > >> + } > >> + > >> + camera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete); > >> + > >> + if (camera_->start()) { > >> + cerr << "Failed to start camera" << endl; > >> + return TestFail; > >> + } > >> + > >> + for (unique_ptr<Request> &request : requests_) { > >> + if (camera_->queueRequest(request.get())) { > >> + cerr << "Failed to queue request" << endl; > >> + return TestFail; > >> + } > >> + } > >> + > >> + EventDispatcher *dispatcher = Thread::current()->eventDispatcher(); > >> + > >> + Timer timer; > >> + timer.start(100); > >> + while (timer.isRunning()) > >> + dispatcher->processEvents(); > >> + > >> + if (camera_->stop()) { > >> + cerr << "Failed to stop camera" << endl; > >> + return TestFail; > >> + } > >> + > >> + if (camera_->release()) { > >> + cerr << "Failed to release camera" << endl; > >> + return TestFail; > >> + } > >> + > >> + camera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete); > >> + > >> + requests_.clear(); > >> + > >> + return 0; > >> + } > >> + > >> + int fdsOpen(pid_t pid) > >> + { > >> + string proxyFdPath = "/proc/" + to_string(pid) + "/fd"; > >> + DIR *dir; > >> + struct dirent *ptr; > >> + unsigned int openFds = 0; > >> + > >> + dir = opendir(proxyFdPath.c_str()); > >> + if (dir == nullptr) { > >> + int err = errno; > >> + cerr << "Error opening " << proxyFdPath << ": " > >> + << strerror(-err) << endl; > >> + return 0; > >> + } > >> + > >> + while ((ptr = readdir(dir)) != nullptr) { > >> + if ((strcmp(ptr->d_name, ".") == 0) || > >> + (strcmp(ptr->d_name, "..") == 0)) > >> + continue; > >> + > >> + openFds++; > >> + } > >> + closedir(dir); > >> + > >> + return openFds; > >> + } > >> + > >> + pid_t findProxyPid() > >> + { > >> + string proxyPid; > >> + string proxyName(kIpaProxyName_); > >> + DIR *dir; > >> + struct dirent *ptr; > >> + > >> + dir = opendir("/proc"); > >> + while((ptr = readdir(dir)) != nullptr) { > >> + if(ptr->d_type != DT_DIR) > >> + continue; > >> + > >> + string pname("/proc/" + string(ptr->d_name) + "/comm"); > >> + if(File::exists(pname.c_str())) { > > Missing space. > > I failed to run checkstyle.py before sending, apologies. I hit -ENOSPC > on my primary device few days agp and I am ended up working on a adhoc > clone which didn't had the checkstyle.py as post-commit hook No worries. > > I may have missed it, but I haven't seen an answer to my review of the > > previous version that proposed checking the number of open file > > descriptors in the VIMC IPA module itself. That would test for fd leaks > > in all tests using vimc, and wouldn't require finding child processes. > > Coming to your request, the reply sounded to me as an enhancement in the > test-able VIMC in general, to increase our test coverage and IPC code > paths. I am assuming you don't intend to dis-regard this particular test > in favor of having it plumbed down to VIMC IPA, or is it? I am assuming > on my part that both can(or should) co-exist, latter being developed > along the lines and out of scope of this patch. I was thinking that if we compute the number of open fds in the IPA itself, then we could drop it from here. It would simplify the implementation of this test, we wouldn't have to loop over /proc here (which is the part that bothered me a bit, as I tried to see if there was a C API to do this, and couldn't find any :-)), but only receive the number of fds from the VIMC IPA through the FIFO that it uses to communicate with tests. Another option btw, now that I think about it, would be keep counting fds here, but pass the VIMC IPA PID through the FIFO. > Our priorities are well-addressed by this test for now, so maybe can I > track it in bugzilla and come back to it in coming weeks? That's OK with me. > >> + ifstream pfile(pname.c_str()); > >> + string comm; > >> + getline (pfile, comm); > >> + pfile.close(); > >> + > >> + proxyPid = comm == proxyName ? > >> + string(ptr->d_name) : > >> + ""; > >> + } > >> + > >> + if (!proxyPid.empty()) > >> + break; > >> + } > >> + closedir(dir); > >> + > >> + if (!proxyPid.empty()) > >> + return atoi(proxyPid.c_str()); > >> + > >> + return -1; > >> + } > >> + > >> + int init() override > >> + { > >> + if (status_ != TestPass) > >> + return status_; > >> + > >> + config_ = camera_->generateConfiguration({ StreamRole::StillCapture }); > >> + if (!config_ || config_->size() != 1) { > >> + cerr << "Failed to generate default configuration" << endl; > >> + return TestFail; > >> + } > >> + > >> + allocator_ = make_unique<FrameBufferAllocator>(camera_); > >> + allocated_ = false; > >> + > >> + return TestPass; > >> + } > >> + > >> + int run() override > >> + { > >> + unsigned int openFdsAtStart = 0; > >> + unsigned int openFds = 0; > >> + > >> + pid_t proxyPid = findProxyPid(); > >> + if (proxyPid < 0) { > >> + cerr << "Cannot find " << kIpaProxyName_ > >> + << " pid, exiting" << endl; > >> + return TestFail; > >> + } > >> + > >> + openFdsAtStart = fdsOpen(proxyPid); > >> + for (unsigned int i = 0; i < kNumOfReconfigures_; i++) { > >> + startAndStop(); > >> + openFds = fdsOpen(proxyPid); > >> + if (openFds == 0) { > >> + cerr << "No open fds found whereas " > >> + << "open fds at start: " << openFdsAtStart > >> + << endl; > >> + return TestFail; > >> + } > >> + > >> + if (openFds != openFdsAtStart) { > >> + cerr << "Leaking fds for " << kIpaProxyName_ > >> + << " - Open fds: " << openFds << " vs " > >> + << "Open fds at start: " << openFdsAtStart > >> + << endl; > >> + return TestFail; > >> + } > >> + } > >> + > >> + return TestPass; > >> + } > >> + > >> + bool allocated_; > >> + > >> + vector<unique_ptr<Request>> requests_; > >> + > >> + unique_ptr<CameraConfiguration> config_; > >> + unique_ptr<FrameBufferAllocator> allocator_; > >> +}; > >> + > >> +} /* namespace */ > >> + > >> +TEST_REGISTER(CameraReconfigure) > >> diff --git a/test/camera/meson.build b/test/camera/meson.build > >> index 002a87b5..668d5c03 100644 > >> --- a/test/camera/meson.build > >> +++ b/test/camera/meson.build > >> @@ -8,6 +8,7 @@ camera_tests = [ > >> ['buffer_import', 'buffer_import.cpp'], > >> ['statemachine', 'statemachine.cpp'], > >> ['capture', 'capture.cpp'], > >> + ['camera_reconfigure', 'camera_reconfigure.cpp'], > >> ] > >> > >> foreach t : camera_tests
diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp new file mode 100644 index 00000000..b32264b1 --- /dev/null +++ b/test/camera/camera_reconfigure.cpp @@ -0,0 +1,257 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2021, Google Inc. + * + * Test: + * - Camera's multiple reconfigurations without stopping CameraManager + * - leaking fds across IPA IPC boundary + */ + +#include <dirent.h> +#include <iostream> +#include <fstream> + +#include <libcamera/framebuffer_allocator.h> + +#include <libcamera/base/event_dispatcher.h> +#include <libcamera/base/file.h> +#include <libcamera/base/thread.h> +#include <libcamera/base/timer.h> + +#include "camera_test.h" +#include "test.h" + +using namespace std; + +namespace { + +class CameraReconfigure : public CameraTest, public Test +{ +public: + CameraReconfigure() + : CameraTest(kCamId_, true) + { + } + +private: + static constexpr const char *kCamId_ = "platform/vimc.0 Sensor B"; + static constexpr const char *kIpaProxyName_ = "vimc_ipa_proxy"; + static constexpr unsigned int kNumOfReconfigures_ = 10; + + void requestComplete(Request *request) + { + if (request->status() != Request::RequestComplete) + return; + + const Request::BufferMap &buffers = request->buffers(); + + /* Create a new request. */ + const Stream *stream = buffers.begin()->first; + FrameBuffer *buffer = buffers.begin()->second; + + request->reuse(); + request->addBuffer(stream, buffer); + camera_->queueRequest(request); + } + + int startAndStop() + { + StreamConfiguration &cfg = config_->at(0); + + if (camera_->acquire()) { + cerr << "Failed to acquire the camera" << endl; + return TestFail; + } + + if (camera_->configure(config_.get())) { + cerr << "Failed to set default configuration" << endl; + return TestFail; + } + + Stream *stream = cfg.stream(); + + if (!allocated_) { + int ret = allocator_->allocate(stream); + if (ret < 0) + return TestFail; + allocated_ = true; + } + + for (const unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) { + unique_ptr<Request> request = camera_->createRequest(); + if (!request) { + cerr << "Failed to create request" << endl; + return TestFail; + } + + if (request->addBuffer(stream, buffer.get())) { + cerr << "Failed to associating buffer with request" << endl; + return TestFail; + } + + requests_.push_back(move(request)); + } + + camera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete); + + if (camera_->start()) { + cerr << "Failed to start camera" << endl; + return TestFail; + } + + for (unique_ptr<Request> &request : requests_) { + if (camera_->queueRequest(request.get())) { + cerr << "Failed to queue request" << endl; + return TestFail; + } + } + + EventDispatcher *dispatcher = Thread::current()->eventDispatcher(); + + Timer timer; + timer.start(100); + while (timer.isRunning()) + dispatcher->processEvents(); + + if (camera_->stop()) { + cerr << "Failed to stop camera" << endl; + return TestFail; + } + + if (camera_->release()) { + cerr << "Failed to release camera" << endl; + return TestFail; + } + + camera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete); + + requests_.clear(); + + return 0; + } + + int fdsOpen(pid_t pid) + { + string proxyFdPath = "/proc/" + to_string(pid) + "/fd"; + DIR *dir; + struct dirent *ptr; + unsigned int openFds = 0; + + dir = opendir(proxyFdPath.c_str()); + if (dir == nullptr) { + int err = errno; + cerr << "Error opening " << proxyFdPath << ": " + << strerror(-err) << endl; + return 0; + } + + while ((ptr = readdir(dir)) != nullptr) { + if ((strcmp(ptr->d_name, ".") == 0) || + (strcmp(ptr->d_name, "..") == 0)) + continue; + + openFds++; + } + closedir(dir); + + return openFds; + } + + pid_t findProxyPid() + { + string proxyPid; + string proxyName(kIpaProxyName_); + DIR *dir; + struct dirent *ptr; + + dir = opendir("/proc"); + while((ptr = readdir(dir)) != nullptr) { + if(ptr->d_type != DT_DIR) + continue; + + string pname("/proc/" + string(ptr->d_name) + "/comm"); + if(File::exists(pname.c_str())) { + ifstream pfile(pname.c_str()); + string comm; + getline (pfile, comm); + pfile.close(); + + proxyPid = comm == proxyName ? + string(ptr->d_name) : + ""; + } + + if (!proxyPid.empty()) + break; + } + closedir(dir); + + if (!proxyPid.empty()) + return atoi(proxyPid.c_str()); + + return -1; + } + + int init() override + { + if (status_ != TestPass) + return status_; + + config_ = camera_->generateConfiguration({ StreamRole::StillCapture }); + if (!config_ || config_->size() != 1) { + cerr << "Failed to generate default configuration" << endl; + return TestFail; + } + + allocator_ = make_unique<FrameBufferAllocator>(camera_); + allocated_ = false; + + return TestPass; + } + + int run() override + { + unsigned int openFdsAtStart = 0; + unsigned int openFds = 0; + + pid_t proxyPid = findProxyPid(); + if (proxyPid < 0) { + cerr << "Cannot find " << kIpaProxyName_ + << " pid, exiting" << endl; + return TestFail; + } + + openFdsAtStart = fdsOpen(proxyPid); + for (unsigned int i = 0; i < kNumOfReconfigures_; i++) { + startAndStop(); + openFds = fdsOpen(proxyPid); + if (openFds == 0) { + cerr << "No open fds found whereas " + << "open fds at start: " << openFdsAtStart + << endl; + return TestFail; + } + + if (openFds != openFdsAtStart) { + cerr << "Leaking fds for " << kIpaProxyName_ + << " - Open fds: " << openFds << " vs " + << "Open fds at start: " << openFdsAtStart + << endl; + return TestFail; + } + } + + return TestPass; + } + + bool allocated_; + + vector<unique_ptr<Request>> requests_; + + unique_ptr<CameraConfiguration> config_; + unique_ptr<FrameBufferAllocator> allocator_; +}; + +} /* namespace */ + +TEST_REGISTER(CameraReconfigure) diff --git a/test/camera/meson.build b/test/camera/meson.build index 002a87b5..668d5c03 100644 --- a/test/camera/meson.build +++ b/test/camera/meson.build @@ -8,6 +8,7 @@ camera_tests = [ ['buffer_import', 'buffer_import.cpp'], ['statemachine', 'statemachine.cpp'], ['capture', 'capture.cpp'], + ['camera_reconfigure', 'camera_reconfigure.cpp'], ] foreach t : camera_tests
This tests basically checks for two things: - Camera reconfigurations without stopping CameraManager - Fd leaks across IPA IPC boundary [1] Currently, it uses vimc, but can be easily changed to using another platform (for e.g. IPU3) by changing kCamId_ and kIpaProxyName_. The test performs kNumOfReconfigures_ (currently set to 10) reconfigurations of the camera. Each reconfiguration runs start(), capture(100ms), stop() of the camera. Hence, the test runs approximately for 1 second. For checking the fd leaks, the test monitors the /proc/$PROXY_PID/fd directory for open fds. It compares the number of open fds after each run to the number of open fds before the first run. If those two are found to be mis-matched, the test shall report failure. Since, the test needs to test the IPA IPC code paths, it is meant to always run with LIBCAMERA_IPA_FORCE_ISOLATION environment variable. [1] https://bugs.libcamera.org/show_bug.cgi?id=63 Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- test/camera/camera_reconfigure.cpp | 257 +++++++++++++++++++++++++++++ test/camera/meson.build | 1 + 2 files changed, 258 insertions(+) create mode 100644 test/camera/camera_reconfigure.cpp