[libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug hot-unplug unit test
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 11 10:27:23 CEST 2020
Hi Umang,
On Wed, Jun 10, 2020 at 02:42:56PM +0000, Umang Jain wrote:
> On 6/8/20 4:24 AM, Laurent Pinchart wrote:
> > 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:
2 ? I wonder where the second one comes from. Is it right after that
line, or after the pipe->match() call ? Camera object retain a reference
to the PipelineHandler, so that would explain it (one reference in the
local pipe variable here, one reference in Camera::Private::pipe_).
> 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.
I was wondering if we could actually remove the pipes_ vector
completely. It would require being careful in the unplug path, as the
pipeline handler will be deleted as soon as the last camera is deleted,
which can happen in PipelineHandler::disconnect(). The cameras._clear()
call at the end of the function may happen after the pipeline object is
deleted. Yes, objects can be deleted while their functions are running,
it's scary, but allowed if you ensure that they don't access data :-)
Running the test in valgrind will help catch problems.
> 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'],
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list