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

[paul.elder at ideasonboard.com]

Hi Umang,

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.

>   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?

> - Some sprinkled prints are intentionally present, will be reworked in
>   v1. Please ignore those for review.

I'll withold my rb-tag then :D


Thanks,

Paul

>   (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
> +
> +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_;
> +
> +	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()) {
> +			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());
> +		while((ptr = readdir(dir)) != nullptr) {
> +			if ((strcmp(ptr->d_name, ".") == 0) || (strcmp(ptr->d_name, "..") == 0))
> +				continue;
> +
> +			openFds++;
> +		}
> +		closedir(dir);
> +
> +		return openFds;
> +	}
> +
> +	string findProxyPid()
> +	{
> +		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_;
> +	}
> +
> +	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++)
> +		{
> +			startAndStop();
> +			openFds = fdsOpen(proxyPid);
> +			cout << "No. of open fds after #" << i << " runs: " << openFds << endl;
> +		}
> +
> +		if (openFds == openFdsAtStart_)
> +			return TestPass;
> +		else
> +			return TestFail;
> +	}
> +};
> +
> +} /* 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
> -- 
> 2.31.1
>