[libcamera-devel] [RFC PATCH] test: camera: Camera reconfiguration and fd-leak test
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Mon Aug 16 08:54:22 CEST 2021
Hi Laurent,
On Sun, Aug 15, 2021 at 12:15:25AM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Tue, Aug 10, 2021 at 04:35:02PM +0900, paul.elder at ideasonboard.com wrote:
> > On Fri, Aug 06, 2021 at 05:21:32PM +0530, Umang Jain wrote:
> > > This tests basically checks for two things:
> > > - Camera reconfigurations without stopping CameraManager
> > > - Fd leaks across IPA IPC boundary.
> >
> > The test looks good!
> >
> > > Currently, it uses vimc, but can be easily changed to using another
> > > platform (for e.g. IPU3) by changing CAM_ID and IPA_PROXY_NAME defines.
> > >
> > > This test is geared towards reproducing [1], issue noticed on IPU3.
> > >
> > > Ensure that test is run in isolated mode.
> > > Running standalone:
> > > $ LIBCAMERA_IPA_FORCE_ISOLATION=1 ./build/test/camera/camera_reconfigure
> > >
> > > [1] https://bugs.libcamera.org/show_bug.cgi?id=63
> > >
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > > - still RFC because it's based on "Pass buffers to VIMC IPA" series
> > > - We probably need to set LIBCAMERA_IPA_FORCE_ISOLATION=1 from inside
> > > the test. Setting at run-time with setenv(), sets the env var but still
> > > the test is unable to run in isolated mode. For now, I only have it
> >
> > This doesn't work because the IPAs are created very early on, usually at
> > match() time.
>
> Which happens when CameraManager::start() is called, so it should be
> possible to set the environment variable before that.
>
> > > working, by running it standalone as mentioned in the commit message.
> > > OR any other ideas enforcing isolated mode on the test?
> >
> > Maybe you could extend the CameraTest class to add an additional extra-early
> > overridden init() (earlyInit() ?) before cm_ = new CameraManager?
>
> Or add a parameter to the constructor.
>
> > > - Some sprinkled prints are intentionally present, will be reworked in
> > > v1. Please ignore those for review.
> >
> > I'll withold my rb-tag then :D
> >
> > > (log snippet: https://paste.debian.net/1206765/)
> > > ---
> > > test/camera/camera_reconfigure.cpp | 250 +++++++++++++++++++++++++++++
> > > test/camera/meson.build | 1 +
> > > 2 files changed, 251 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..20a4a6a4
> > > --- /dev/null
> > > +++ b/test/camera/camera_reconfigure.cpp
> > > @@ -0,0 +1,250 @@
> > > +/* 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;
> > > +
> > > +#define CAM_ID "platform/vimc.0 Sensor B"
> > > +#define IPA_PROXY_NAME "vimc_ipa_proxy"
> > > +#define NUM_OF_RECONFIGURES 20
>
> 20 runs of one second each is long compared to the time taken to run the
> test suite today. Can this be sped up ?
>
> The C++ way would be
>
> static constexpr char *kCamId = "platform/vimc.0 Sensor B";
> static constexpr char *kIpaProxyName = "vimc_ipa_proxy";
> static constexpr unsigned int kNumOfReconfigures = 20;
>
> They can also be made members of the CameraReconfigure class.
>
> > > +
> > > +namespace {
> > > +
> > > +class CameraReconfigure : public CameraTest, public Test
> > > +{
> > > +public:
> > > + CameraReconfigure()
> > > + : CameraTest(CAM_ID)
> > > + {
> > > + }
> > > +
> > > +protected:
> > > + unsigned int configureRuns_;
> > > + unsigned int openFdsAtStart_;
> > > + string vimcProxyName_;
> > > + bool allocated_;
> > > +
> > > + vector<unique_ptr<Request>> requests_;
> > > +
> > > + unique_ptr<CameraConfiguration> config_;
> > > + FrameBufferAllocator *allocator_;
>
> Variables afer functions please. And these variables should be private.
>
> > > +
> > > + 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 ()
>
> Extra space.
>
> > > + {
> > > + StreamConfiguration &cfg = config_->at(0);
> > > +
> > > + if (camera_->acquire()) {
> > > + cout << "Failed to acquire the camera" << endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (camera_->configure(config_.get())) {
> > > + cout << "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) {
> > > + cout << "Failed to create request" << endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (request->addBuffer(stream, buffer.get())) {
> > > + cout << "Failed to associating buffer with request" << endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + requests_.push_back(move(request));
> > > + }
> > > +
> > > + camera_->requestCompleted.connect(this, &CameraReconfigure::requestComplete);
> > > +
> > > + if (camera_->start()) {
> > > + cout << "Failed to start camera" << endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + for (unique_ptr<Request> &request : requests_) {
> > > + if (camera_->queueRequest(request.get())) {
> > > + cout << "Failed to queue request" << endl;
> > > + return TestFail;
> > > + }
> > > + }
> > > +
> > > + EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
> > > +
> > > + Timer timer;
> > > + timer.start(1000);
> > > + while (timer.isRunning())
> > > + dispatcher->processEvents();
> > > +
> > > + if (camera_->stop()) {
> > > + cout << "Failed to stop camera" << endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + if (camera_->release()) {
> > > + cout << "Failed to release camera" << endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + camera_->requestCompleted.disconnect(this, &CameraReconfigure::requestComplete);
> > > +
> > > + requests_.clear();
> > > +
> > > + return 0;
> > > + }
> > > +
> > > + int fdsOpen(string pid)
> > > + {
> > > + string proxyFdPath = "/proc/" + pid + "/fd";
> > > + DIR *dir;
> > > + struct dirent *ptr;
> > > + unsigned int openFds = 0;
> > > +
> > > + dir = opendir(proxyFdPath.c_str());
>
> Error checking ?
>
> > > + while((ptr = readdir(dir)) != nullptr) {
>
> s/while/while /
>
> There are other missing spaces below.
>
> > > + if ((strcmp(ptr->d_name, ".") == 0) || (strcmp(ptr->d_name, "..") == 0))
> > > + continue;
> > > +
> > > + openFds++;
> > > + }
> > > + closedir(dir);
> > > +
> > > + return openFds;
> > > + }
> > > +
> > > + string findProxyPid()
>
> A pid is a number, I'd return a pid_t.
>
> > > + {
> > > + string proxyPid;
> > > + 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 == vimcProxyName_ ?
> > > + string(ptr->d_name) :
> > > + "";
> > > + }
> > > +
> > > + if (!proxyPid.empty())
> > > + break;
> > > + }
> > > + closedir(dir);
> > > +
> > > + return proxyPid;
> > > + }
> > > +
> > > + int init() override
> > > + {
> > > + if (status_ != TestPass)
> > > + return status_;
> > > +
> > > + config_ = camera_->generateConfiguration({ StreamRole::StillCapture });
> > > + if (!config_ || config_->size() != 1) {
> > > + cout << "Failed to generate default configuration" << endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + allocator_ = new FrameBufferAllocator(camera_);
> > > +
> > > + openFdsAtStart_ = 0;
> > > + configureRuns_ = NUM_OF_RECONFIGURES;
> > > + vimcProxyName_ = IPA_PROXY_NAME;
> > > + allocated_ = false;
> > > +
> > > + return TestPass;
> > > + }
> > > +
> > > + void cleanup() override
> > > + {
> > > + delete allocator_;
>
> You can store it in an std::unique_ptr<>.
>
> > > + }
> > > +
> > > + int run() override
> > > + {
> > > + unsigned int openFds = 0;
> > > + /* Find pid of vimc_ipa_proxy */
> > > + string proxyPid = findProxyPid();
> > > + if (proxyPid.empty()) {
> > > + cout << "Not running in isolated mode, exiting" << endl;
> > > + return TestFail;
> > > + }
> > > +
> > > + /* Find number of open fds by vimc_ipa_proxy */
> > > + openFdsAtStart_ = fdsOpen(proxyPid);
> > > + cout << IPA_PROXY_NAME << " PID:" << proxyPid
> > > + << " has open fds at start:" << openFdsAtStart_ << endl;
> > > +
> > > + for (unsigned int i = 0; i < configureRuns_; i++)
>
> You can drop the configureRuns_ member variable and use
> kNumOfReconfigures directly here. Same for the proxy name I think.
>
> > > + {
>
> This should be at the end of the previous line.
>
> > > + startAndStop();
> > > + openFds = fdsOpen(proxyPid);
> > > + cout << "No. of open fds after #" << i << " runs: " << openFds << endl;
> > > + }
> > > +
> > > + if (openFds == openFdsAtStart_)
>
> Shouldn't this be checked for every run of the loop ? I'd then print the
> message with the number of fds only on failure.
>
> > > + return TestPass;
> > > + else
> > > + return TestFail;
>
> I wonder if counting the number of open fds at exit time isn't something
> that the VIMC IPA should implement. That way, we would have the check in
> every test, not just this one.
>
> Paul, what do you think ?
Yeah, I think that would be useful.
Paul
>
> > > + }
> > > +};
> > > +
> > > +} /* 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
More information about the libcamera-devel
mailing list