[libcamera-devel] [PATCH v3 5/5] tests: Introduce hotplug hot-unplug unit test
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 8 00:54:40 CEST 2020
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 ?
> > + 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