[libcamera-devel] [PATCH v2 2/2] test: camera: Camera reconfiguration and fd-leak test

[Kieran Bingham]

On 18/08/2021 10:02, Umang Jain wrote:

I think we can introduce this patch at the beginning of the commit message:

"""
Development and usage of isolated IPA's has identified bugs in the
transmission of the file descriptors between the pipeline handler and
the IPA through.

Add tests to identify these bugs and prevent regressions.
"""


> 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=1 environment variable.


That last statement could be rephrased as LIBCAMERA_IPA_FORCE_ISOLATION
isn't visibly being set in this code path:

"""
The test validates IPA IPC code paths which are used when the IPA is run
in isolated mode. The CameraTest is constructed with the isolate flag
set to enforce this.
"""


> 
> [1] https://bugs.libcamera.org/show_bug.cgi?id=63
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  test/camera/camera_reconfigure.cpp | 255 +++++++++++++++++++++++++++++
>  test/camera/meson.build            |   1 +
>  2 files changed, 256 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..90f798fe
> --- /dev/null
> +++ b/test/camera/camera_reconfigure.cpp
> @@ -0,0 +1,255 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * Test:
> + * - Camera's multiple reconfigurations without stopping CameraManager

"Multiple reconfigurations of the Camera without stopping the CameraManager"

> + * - leaking fds across IPA IPC boundary

"Validate there are no file descriptor leaks when using IPC"

> + */
> +
> +#include <dirent.h>
> +#include <fstream>
> +#include <iostream>
> +
> +#include <libcamera/base/event_dispatcher.h>
> +#include <libcamera/base/file.h>
> +#include <libcamera/base/thread.h>
> +#include <libcamera/base/timer.h>
> +
> +#include <libcamera/framebuffer_allocator.h>
> +
> +#include "camera_test.h"
> +#include "test.h"
> +
> +using namespace std;
> +
> +namespace {
> +
> +class CameraReconfigure : public CameraTest, public Test
> +{
> +public:
> +	CameraReconfigure()
> +		: CameraTest(kCamId_, true)

If we're not going to make the bool::isolate an enum, perhaps we need to
add a comment above the constructor to say "Initialise the CameraTest
with an Isolated IPA"

  "CameraTest(kCamId_, true)"

doesn't really convey that without having to go look up the CameraTest
constructor.


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

Is this really creating a new request?

It's more
"Reuse the request, and re-queue it with the same buffers."

I think that can also be done by setting the ReuseBuffers flag on
request->reuse() which would be clearer.

> +		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();
> +

I would add a comment here to say,

		/*
		 * The configuration is consistent so we can re-use the
		 * same buffer allocation for each run.
		 */

> +		if (!allocated_) {
> +			int ret = allocator_->allocate(stream);
> +			if (ret < 0)

Should we print a failure reason here?

> +				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;

s/associating/associate/

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

I think the condition below would be enough to catch the errors, but I'm
not opposed to the check above as well.

With the minors resolved as you see fit:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>



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