[libcamera-devel] [RFC PATCH] test: camera: Camera reconfiguration and fd-leak test

[Laurent Pinchart]

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 ?

> > +	}
> > +};
> > +
> > +} /* 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

-- 
Regards,

Laurent Pinchart