| Message ID | 20200521135416.13685-6-email@uajain.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. 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@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@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. > + 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'],
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@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@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. > > > + 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) > > + I forgot to mention, there's an extra blank line here. > > 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'],
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@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@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'],
Hi Laurent, Thanks for the review. 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@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@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 ? Sure. I am starting to address these review comments locally, so I will sort this issue out before submitting another round. The investigation has been pending for a while now from my side, thanks for looking into it and finding out the problem. > >>> + 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'],
Hi Umang, On Mon, Jun 08, 2020 at 02:20:07PM +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@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@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 ? > > Sure. > > I am starting to address these review comments locally, so I will sort > this issue out before submitting another round. > > The investigation has been pending for a while now from my side, thanks > for looking into it and finding out the problem. You're welcome. Thank you for working on it in the first place :-) Once you have a fix, I recommend running this test under valgrind, it should help catching use-after-free issues. I've sent a patch yesterday that avoids a valgrind crash with libcamera, I hope to get it merged soon, but you may want to apply it locally in the meantime. > >>> + 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'],
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@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@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'],
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@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@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'],
Hi Laurent, On 6/11/20 1:57 PM, Laurent Pinchart wrote: > 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@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@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. I see. Yes, I initially thought pipes_ vector can be removed completely as I couldn't see how it is useful in the manager.But felt a bit against the intuition that a CameraManager has no placeholdersfor managing created pipelines. As for the camera deletion, we do (in this series) pass a reference to the camera when we emit a camera[Added | Removed]. So my understanding, in context of this patch series, since applications will get another reference on cameraRemoved, they still can access the camera data safely if they want (possibly in their signal handler), before the camera gets destroyed and subsequently the pipeline handler. For now, I am considering your suggestion of removing the pipes_ vector from CameraManager to finally put an end to this rabbithole. Thanks for your pointer. > >> 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'],
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@uajain.com> + * + * hotplug-cameras.cpp - Emulate cameraAdded/cameraRemoved signals in CameraManager + */ + +#include <dirent.h> +#include <fstream> +#include <iostream> +#include <string.h> +#include <unistd.h> + +#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; +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/"; + + return 0; + } + + int run() + { + DIR *dir; + struct dirent *dirent, *dirent2; + std::string uvc_driver_dir; + bool uvc_driver_found = false; + + dir = opendir(uvc_toplevel_.c_str()); + /* Find a UVC device driver symlink, which we can bind/unbind */ + 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; + } + closedir(dir); + + /* If no UVC driver found, skip */ + if (!uvc_driver_found) + return TestSkip; + + /* Unbind a camera, process events */ + int fd1 = open("/sys/module/uvcvideo/drivers/usb:uvcvideo/unbind", O_WRONLY); + write(fd1, uvc_driver_dir.c_str(), uvc_driver_dir.size()); + close(fd1); + Timer timer; + timer.start(1000); + while (timer.isRunning()) + Thread::current()->eventDispatcher()->processEvents(); + + /* \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. + */ + 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); + 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; + } + + void cleanup() + { + cm_->stop(); + delete cm_; + } + +private: + CameraManager *cm_; + std::string uvc_toplevel_; + bool cameraRemovedPass_; + bool cameraAddedPass_; +}; + +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'], ['object', 'object.cpp'], ['object-invoke', 'object-invoke.cpp'], ['signal-threads', 'signal-threads.cpp'],
This test checks the code-paths for camera's hotplugged and unplugged support. It is based off bind/unbind of a UVC device from sysfs. Hence, this tests required root permissions to run and should have atleast one already bound UVC device present on the system. Signed-off-by: Umang Jain <email@uajain.com> --- test/hotplug-cameras.cpp | 153 +++++++++++++++++++++++++++++++++++++++ test/meson.build | 1 + 2 files changed, 154 insertions(+) create mode 100644 test/hotplug-cameras.cpp