[libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug hot-unplug unit test

[Umang Jain]

Hi Laurent,

On 6/8/20 4:24 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Mon, Jun 08, 2020 at 01:24:37AM +0300, Laurent Pinchart wrote:
>> On Thu, May 21, 2020 at 01:54:27PM +0000, Umang Jain wrote:
>>> This test checks the code-paths for camera's hotplugged and
>>> unplugged support. It is based off bind/unbind of a UVC device
>> s/off/on/
>>
>>> from sysfs. Hence, this tests required root permissions to run
>> s/tests required/test requires/
>>
>>> and should have atleast one already bound UVC device present
>> s/atleast/at least/
>>
>>> on the system.
>> s/on/in/
>>
>>> Signed-off-by: Umang Jain <email at uajain.com>
>>> ---
>>>   test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++
>>>   test/meson.build         |   1 +
>>>   2 files changed, 154 insertions(+)
>>>   create mode 100644 test/hotplug-cameras.cpp
>>>
>>> diff --git a/test/hotplug-cameras.cpp b/test/hotplug-cameras.cpp
>>> new file mode 100644
>>> index 0000000..72c370f
>>> --- /dev/null
>>> +++ b/test/hotplug-cameras.cpp
>>> @@ -0,0 +1,153 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Copyright (C) 2020, Umang Jain <email at uajain.com>
>>> + *
>>> + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager
>> Maybe s/Emulate/Test/ ?
>>
>>> + */
>>> +
>>> +#include <dirent.h>
>>> +#include <fstream>
>>> +#include <iostream>
>>> +#include <string.h>
>>> +#include <unistd.h>
>>> +
>> No need for a blank line here, and you can then merge the two groups of
>> headers in alphabetical order.
>>
>>> +#include <fcntl.h>
>>> +#include <sys/stat.h>
>>> +#include <sys/types.h>
>>> +
>>> +#include <libcamera/camera.h>
>>> +#include <libcamera/camera_manager.h>
>>> +#include <libcamera/event_dispatcher.h>
>>> +#include <libcamera/timer.h>
>>> +
>>> +#include "libcamera/internal/file.h"
>>> +#include "libcamera/internal/thread.h"
>>> +
>>> +#include "test.h"
>>> +
>>> +using namespace std;
>> As you use the std namespace explicitly below, you can drop this line.
>>
>>> +using namespace libcamera;
>>> +
>>> +class HotplugTest : public Test
>>> +{
>>> +protected:
>>> +	void cameraAddedHandler(std::shared_ptr<Camera> cam)
>>> +	{
>>> +		cameraAddedPass_ = true;
>>> +	}
>>> +
>>> +	void cameraRemovedHandler(std::shared_ptr<Camera> cam)
>>> +	{
>>> +		cameraRemovedPass_ = true;
>>> +	}
>>> +
>>> +	int init()
>>> +	{
>>> +		if (!File::exists("/sys/module/uvcvideo")) {
>>> +			std::cout << "uvcvideo driver is not loaded, skipping" << std::endl;
>>> +			return TestSkip;
>>> +		}
>>> +
>>> +		if (geteuid() != 0) {
>>> +			std::cout << "This test requires root permissions, skipping" << std::endl;
>>> +			return TestSkip;
>>> +		}
>>> +
>>> +		cm_ = new CameraManager();
>>> +		if (cm_->start()) {
>>> +			std::cout << "Failed to start camera manager" << std::endl;
>>> +			return TestFail;
>>> +		}
>>> +
>>> +		cameraAddedPass_ = false;
>>> +		cameraRemovedPass_ = false;
>>> +
>>> +		cm_->newCameraAdded.connect(this, &HotplugTest::cameraAddedHandler);
>>> +		cm_->cameraRemoved.connect(this, &HotplugTest::cameraRemovedHandler);
>>> +
>>> +		uvc_toplevel_ = "/sys/module/uvcvideo/drivers/usb:uvcvideo/";
>> Maybe /sys/bus/usb/drivers/uvcvideo/ to avoid traversing symlinks ? It
>> makes little difference in practice I suppose.
>>
>> s/uvc_toplevel_/uvcTopLevel_/
>>
>> And I would even call this uvcDriverDir to make it more explicit. Now
>> that I think about it, as uvc_toplevel_ is constant, maybe we should
>> have
>>
>> 	static const std::string uvcDriverDir_;
>>
>> as a static class member.
>>
>>> +
>>> +		return 0;
>>> +	}
>>> +
>>> +	int run()
>>> +	{
>>> +		DIR *dir;
>>> +		struct dirent *dirent, *dirent2;
>>> +		std::string uvc_driver_dir;
>>> +		bool uvc_driver_found = false;
>> camelCase please :-)
>>
>>> +
>>> +		dir = opendir(uvc_toplevel_.c_str());
>>> +		/* Find a UVC device driver symlink, which we can bind/unbind */
>> s/unbind/unbind./ (same for the other comments below)
>>
>>> +		while ((dirent = readdir(dir)) != nullptr) {
>>> +			if (dirent->d_type != DT_LNK)
>>> +				continue;
>>> +
>>> +			std::string child_dir = uvc_toplevel_ + dirent->d_name;
>>> +			DIR *device_driver = opendir(child_dir.c_str());
>>> +			while ((dirent2 = readdir(device_driver)) != nullptr) {
>>> +				if (strncmp(dirent2->d_name, "video4linux", 11) == 0) {
>>> +					uvc_driver_dir = dirent->d_name;
>>> +					uvc_driver_found = true;
>>> +					break;
>>> +				}
>>> +			}
>>> +			closedir(device_driver);
>>> +
>>> +			if (uvc_driver_found)
>>> +				break;
>> Can't this be simplified with
>>
>> 			std::string child_dir = uvc_toplevel_ + dirent->d_name;
>> 			if (!File::exist(uvc_toplevel_ + dirent->d_name + "/video4linux"))
>> 				continue;
>>
>> 			uvc_driver_dir = dirent->d_name;
>> 			break;
>>
>>> +		}
>>> +		closedir(dir);
>>> +
>>> +		/* If no UVC driver found, skip */
>>> +		if (!uvc_driver_found)
>> And here,
>>
>> 		if (uvc_driver_dir.empty())
>>
>> to remove uvc_driver_found.
>>
>>> +			return TestSkip;
>>> +
>>> +		/* Unbind a camera, process events */
>>> +		int fd1 = open("/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind", O_WRONLY);
>> Maybe replacing the hardcoded string with uvc_toplevel_ + "unbind" ?
>> This would become
>>
>> 		int fd1 = open((uvcDriverDir_ + "unbind").c_str(), O_WRONLY);
>>
>>> +		write(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size());
>>> +		close(fd1);
>> Blank line here ?
>>
>> I wonder if using the C++ file API would be simpler here.
>>
>> 		std::ofstream(uvcDriverDir_ + "unbind", std::ios::binary)
>> 			<< uvc_driver_dir;;
>>
>> and that's it.
>>
>>> +		Timer timer;
>>> +		timer.start(1000);
>>> +		while (timer.isRunning())
>> 		while (timer.isRunning() && !cameraRemovedPass_)
>>
>> to shorten the wait time. Same below.
>>
>>> +			Thread::current()->eventDispatcher()->processEvents();
>>> +
>> How about already testing for cameraRemovedPass_ here ?
>>
>> 		if (!cameraRemovedPass_)
>> 			return TestFail;
>>
>>> +		/* \todo: Fix this workaround of stopping and starting the camera-manager.
>>> +		 *  We need to do this, so that cm_ release all references to the uvc media symlinks.
>>> +		 */
>> We can merge the series without fixing this, but I think it needs to be
>> handled sooner than latter.
> I've had a quick look, and this seems to be caused by the pipeline
> handler never getting deleted on hot-unplug. This is due to a reference
> to the pipeline handler being stored in the camera manager and never
> dropped.
>
> I'm not sure we actually need to store those references. Can I let you
> investigate ?

I see those references in CameraManager::pipes_ not getting dropped 
anywhere.

Also, I want to understand/handle this situation, in 
CameraManager::Private::init() (on master branch):

                 std::shared_ptr<PipelineHandler> pipe = 
factory->create(cm_);

pipe has use_count() of 2 (I checked); and a bit below:

                 pipes_.push_back(std::move(pipe));

pipes_ vector gets a created PipelineHander at refcount 2. Now, I can 
drop "a" reference

to this pipe that got added in pipes_ in maybe say, 
CameraManager::Private::removeCamera() on hot-unplug.

Well, even then it will retain the refcount 1, so still the 
PipelineHandler is not destroyed on hot-unplug,

which might be holding onto the media-device directory I pointed out as 
\todo in the test code.

Do you have any pointers on how can I make the refcount drop to 0, so 
the PipelineHandler in pipes_ can be destroyed?

>
>>> +		cm_->stop();
>>> +		if (cm_->start()) {
>>> +			std::cout << "Failed to restart camera-manager" << std::endl;
>>> +			return TestFail;
>>> +		}
>>> +
>>> +		/* Bind the camera again, process events */
>>> +		int fd2 = open("/sys/module/uvcvideo/drivers/usb:uvcvideo/bind", O_WRONLY);
>> You don't need fd2, you can reuse fd1 (which should be renamed fd).
>>
>>> +		write(fd2, uvc_driver_dir.c_str(), uvc_driver_dir.size());
>>> +		close(fd2);
>>> +
>>> +		timer.start(1000);
>>> +		while (timer.isRunning())
>>> +			Thread::current()->eventDispatcher()->processEvents();
>>> +
>>> +		if (cameraAddedPass_ && cameraRemovedPass_)
>>> +			return TestPass;
>>> +		else
>>> +			return TestFail;
>> This would become
>>
>> 		if (!cameraAddedPass_)
>> 			return TestFail;
>>
>> 		return TestPass;
>>
>>> +	}
>>> +
>>> +	void cleanup()
>>> +	{
>>> +		cm_->stop();
>>> +		delete cm_;
>>> +	}
>>> +
>>> +private:
>>> +	CameraManager *cm_;
>>> +	std::string uvc_toplevel_;
>>> +	bool cameraRemovedPass_;
>> Maybe s/cameraRemovedPass_/cameraRemoved_/ ?
>>
>>> +	bool cameraAddedPass_;
>> Same here.
>>
>>> +};
>>> +
>>> +TEST_REGISTER(HotplugTest)
>>> +
>>> diff --git a/test/meson.build b/test/meson.build
>>> index bd7da14..f7e27d7 100644
>>> --- a/test/meson.build
>>> +++ b/test/meson.build
>>> @@ -31,6 +31,7 @@ internal_tests = [
>>>       ['file',                            'file.cpp'],
>>>       ['file-descriptor',                 'file-descriptor.cpp'],
>>>       ['message',                         'message.cpp'],
>>> +    ['hotplug-cameras',                 'hotplug-cameras.cpp'],
>> Alphabetical order ?
>>
>>>       ['object',                          'object.cpp'],
>>>       ['object-invoke',                   'object-invoke.cpp'],
>>>       ['signal-threads',                  'signal-threads.cpp'],