[libcamera-devel] [RFC] test: Move resource locking to test cases
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 4 14:49:53 CEST 2019
Hello,
> Hi Niklas,
>
> On 04/07/2019 02:30, Niklas Söderlund wrote:
> > Instead of blocking any test which require access to a kernel resource
> > (vimc or vivid) from running in parallel move the resource locking into
> > test library and allow meson to execute them in parallel.
> >
> > Resource locking is achieve using a lockfile for each resource in /tmp
s/achieve/achieved/
> > and lockf(). The call to lockf() blocks until it can acquire the
> > resource and it keeps to lock until the test executable terminates
s/to lock/the lock/
s/terminates/terminates,/
> > closing the file descriptor to the lock file and freeing the lock. This
s/ / /
> > design ensures the process never deadlocks. The reason a separate lock
s/process never deadlocks/test processes never deadlock/
This is however not entirely true, tests that need to lock multiple
devices (for instance to test buffer sharing) may deadlock if they don't
take the locks in the same order.
Edit: I've now read the rest of the series and I see you're not locking
individual devices, but you lock at the vimc or vivid level. There
should thus be no risk of deadlock indeed.
> > file is needed instead of locking the media device associated with the
> > resource is that such a locking scheme is already used by libcamera
> > itself.
>
> Can't we instead request the lock from the libcamera locking then ?
>
> > With this change I cut down the runtime of tests by 50%. The largest
> > increase is from allowing self-contained tests in parallel with tests
> > which require a kernel resource. This is because most of the tests who
> > needs a kernel resource needs vimc so there is heavy contention for that
> > single resource and little gain from meson to execute them in parallel.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > test/camera/camera_test.h | 2 +-
> > test/camera/meson.build | 2 +-
> > test/controls/control_list.cpp | 4 ++
> > test/controls/meson.build | 2 +-
> > test/libtest/test.cpp | 37 ++++++++++++++++++-
> > test/libtest/test.h | 15 +++++++-
> > test/media_device/media_device_test.h | 2 +-
> > test/media_device/meson.build | 2 +-
> > test/v4l2_subdevice/meson.build | 2 +-
> > test/v4l2_subdevice/v4l2_subdevice_test.h | 2 +-
> > test/v4l2_videodevice/buffer_sharing.cpp | 2 +-
> > test/v4l2_videodevice/capture_async.cpp | 2 +-
> > test/v4l2_videodevice/double_open.cpp | 2 +-
> > test/v4l2_videodevice/formats.cpp | 2 +-
> > test/v4l2_videodevice/meson.build | 2 +-
> > test/v4l2_videodevice/request_buffers.cpp | 2 +-
> > test/v4l2_videodevice/stream_on_off.cpp | 2 +-
> > .../v4l2_videodevice_test.cpp | 16 +++++++-
> > test/v4l2_videodevice/v4l2_videodevice_test.h | 4 +-
> > 19 files changed, 85 insertions(+), 19 deletions(-)
> >
> > diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h
> > index ffc8a485bfaff836..0f3c15cc3dd34f3c 100644
> > --- a/test/camera/camera_test.h
> > +++ b/test/camera/camera_test.h
> > @@ -17,7 +17,7 @@ class CameraTest : public Test
> > {
> > public:
> > CameraTest()
> > - : cm_(nullptr) {}
> > + : Test(VIMC), cm_(nullptr) {}
> >
> > protected:
> > int init();
> > diff --git a/test/camera/meson.build b/test/camera/meson.build
> > index 35e97ce5de1a72ca..b10b1e76aaec0233 100644
> > --- a/test/camera/meson.build
> > +++ b/test/camera/meson.build
> > @@ -12,5 +12,5 @@ foreach t : camera_tests
> > dependencies : libcamera_dep,
> > link_with : test_libraries,
> > include_directories : test_includes_internal)
> > - test(t[0], exe, suite : 'camera', is_parallel : false)
> > + test(t[0], exe, suite : 'camera')
> > endforeach
> > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp
> > index c834edc352f5d7d4..0fc3ded2545df2e1 100644
> > --- a/test/controls/control_list.cpp
> > +++ b/test/controls/control_list.cpp
> > @@ -18,6 +18,10 @@ using namespace libcamera;
> >
> > class ControlListTest : public Test
> > {
> > +public:
> > + ControlListTest()
> > + : Test(VIMC) {}
> > +
> > protected:
> > int init()
> > {
> > diff --git a/test/controls/meson.build b/test/controls/meson.build
> > index f4fc7b947dd9e2a6..724871e3136f369b 100644
> > --- a/test/controls/meson.build
> > +++ b/test/controls/meson.build
> > @@ -9,5 +9,5 @@ foreach t : control_tests
> > dependencies : libcamera_dep,
> > link_with : test_libraries,
> > include_directories : test_includes_internal)
> > - test(t[0], exe, suite : 'controls', is_parallel : false)
> > + test(t[0], exe, suite : 'controls')
> > endforeach
> > diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp
> > index b119cf19273900d2..cf13fa42dd9fdc5a 100644
> > --- a/test/libtest/test.cpp
> > +++ b/test/libtest/test.cpp
> > @@ -5,11 +5,15 @@
> > * test.cpp - libcamera test base class
> > */
> >
> > +#include <fcntl.h>
> > +#include <iostream>
> > #include <stdlib.h>
> > +#include <sys/stat.h>
> >
> > #include "test.h"
> >
> > -Test::Test()
> > +Test::Test(TestResource resource)
> > + : resource_(resource)
> > {
> > }
> >
> > @@ -25,6 +29,9 @@ int Test::execute()
> > if (ret)
> > return errno;
> >
> > + if (!resourceLock())
> > + return TestFail;
> > +
> > ret = init();
> > if (ret)
> > return ret;
> > @@ -35,3 +42,31 @@ int Test::execute()
> >
> > return ret;
> > }
> > +
> > +bool Test::resourceLock()
> > +{
> > + const char *path;
> > +
> > + switch (resource_) {
> > + case None:
> > + return true;
> > + case VIMC:
> > + path = "/tmp/libcamera-test-vimc";
> > + break;
> > + case VIVID:
> > + path = "/tmp/libcamera-test-vivid";
> > + break;
> > + }
> > +
> > + int fd = ::open(path, O_CREAT | O_RDWR, 0666);
> > + if (fd < 0) {
> > + std::cerr << "Failed to open resource lockfile" << std::endl;
> > + perror("FOO");
> > + return false;
> > + }
> > +
> > + if (lockf(fd, F_LOCK, 0))
> > + return false;
>
> Is there a way that we can do this at the library level, perhaps within
> our media controller layer?
I wonder if we should rework this by making resources more explicitly
managed. I especially foresee the need to lock multiple resources, so
handling this at the media device level could indeed be a good idea.
Would it make sense to add a lock function that takes a list of media
devices and tries to lock them all ? We could avoid AB-BA deadlocks by
sorting the resources in the lock function. I think I would prefer for
the lock function to be called explicitly in the init() or run()
handler, instead of modifying the base Test constructor. We could add a
MediaResourceLocker (the name can be discussed) class that could serve
as a base of all tests that require media devices and add the required
features there.
> > +
> > + return true;
> > +}
> > diff --git a/test/libtest/test.h b/test/libtest/test.h
> > index ec08bf97c03d563e..259fb589eb621489 100644
> > --- a/test/libtest/test.h
> > +++ b/test/libtest/test.h
> > @@ -15,10 +15,16 @@ enum TestStatus {
> > TestSkip = 77,
> > };
> >
> > +enum TestResource {
> > + None,
> > + VIMC,
> > + VIVID,
> > +};
> > +
> > class Test
> > {
> > public:
> > - Test();
> > + Test(TestResource resource = None);
> > virtual ~Test();
> >
> > int execute();
> > @@ -27,6 +33,13 @@ protected:
> > virtual int init() { return 0; }
> > virtual int run() = 0;
> > virtual void cleanup() { }
> > +
> > + TestResource resource() { return resource_; }
> > +
> > +private:
> > + bool resourceLock();
> > +
> > + TestResource resource_;
> > };
> >
> > #define TEST_REGISTER(klass) \
> > diff --git a/test/media_device/media_device_test.h b/test/media_device/media_device_test.h
> > index cdbd14841d5ccca2..9e34ba86c397a99c 100644
> > --- a/test/media_device/media_device_test.h
> > +++ b/test/media_device/media_device_test.h
> > @@ -20,7 +20,7 @@ class MediaDeviceTest : public Test
> > {
> > public:
> > MediaDeviceTest()
> > - : media_(nullptr), enumerator_(nullptr) {}
> > + : Test(VIMC), media_(nullptr), enumerator_(nullptr) {}
> >
> > protected:
> > int init();
> > diff --git a/test/media_device/meson.build b/test/media_device/meson.build
> > index 6a0e468434b58efe..6a5b24d27aa0f618 100644
> > --- a/test/media_device/meson.build
> > +++ b/test/media_device/meson.build
> > @@ -18,5 +18,5 @@ foreach t : media_device_tests
> > link_with : [test_libraries, lib_mdev_test],
> > include_directories : test_includes_internal)
> >
> > - test(t[0], exe, suite : 'media_device', is_parallel : false)
> > + test(t[0], exe, suite : 'media_device')
> > endforeach
> > diff --git a/test/v4l2_subdevice/meson.build b/test/v4l2_subdevice/meson.build
> > index 0521984b2a787ab9..47676adbe5a33f7e 100644
> > --- a/test/v4l2_subdevice/meson.build
> > +++ b/test/v4l2_subdevice/meson.build
> > @@ -8,5 +8,5 @@ foreach t : v4l2_subdevice_tests
> > dependencies : libcamera_dep,
> > link_with : test_libraries,
> > include_directories : test_includes_internal)
> > - test(t[0], exe, suite : 'v4l2_subdevice', is_parallel : false)
> > + test(t[0], exe, suite : 'v4l2_subdevice')
> > endforeach
> > diff --git a/test/v4l2_subdevice/v4l2_subdevice_test.h b/test/v4l2_subdevice/v4l2_subdevice_test.h
> > index 96646a155536aadf..a9002834bf8959b2 100644
> > --- a/test/v4l2_subdevice/v4l2_subdevice_test.h
> > +++ b/test/v4l2_subdevice/v4l2_subdevice_test.h
> > @@ -21,7 +21,7 @@ class V4L2SubdeviceTest : public Test
> > {
> > public:
> > V4L2SubdeviceTest()
> > - : scaler_(nullptr){};
> > + : Test(VIMC), scaler_(nullptr){};
> >
> > protected:
> > int init() override;
> > diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp
> > index 1bc478fe8f8e1163..d619713257055fd5 100644
> > --- a/test/v4l2_videodevice/buffer_sharing.cpp
> > +++ b/test/v4l2_videodevice/buffer_sharing.cpp
> > @@ -23,7 +23,7 @@ class BufferSharingTest : public V4L2VideoDeviceTest
> > {
> > public:
> > BufferSharingTest()
> > - : V4L2VideoDeviceTest("vivid", "vivid-000-vid-cap"),
> > + : V4L2VideoDeviceTest(VIVID, "vivid-000-vid-cap"),
> > output_(nullptr), framesCaptured_(0), framesOutput_(0) {}
> >
> > protected:
> > diff --git a/test/v4l2_videodevice/capture_async.cpp b/test/v4l2_videodevice/capture_async.cpp
> > index cea4fffbf7a5603d..9ae8043b31a68f0e 100644
> > --- a/test/v4l2_videodevice/capture_async.cpp
> > +++ b/test/v4l2_videodevice/capture_async.cpp
> > @@ -18,7 +18,7 @@ class CaptureAsyncTest : public V4L2VideoDeviceTest
> > {
> > public:
> > CaptureAsyncTest()
> > - : V4L2VideoDeviceTest("vimc", "Raw Capture 0"), frames(0) {}
> > + : V4L2VideoDeviceTest(VIMC, "Raw Capture 0"), frames(0) {}
> >
> > void receiveBuffer(Buffer *buffer)
> > {
> > diff --git a/test/v4l2_videodevice/double_open.cpp b/test/v4l2_videodevice/double_open.cpp
> > index 5768d4043d0ba03f..ff1c7b136e803929 100644
> > --- a/test/v4l2_videodevice/double_open.cpp
> > +++ b/test/v4l2_videodevice/double_open.cpp
> > @@ -15,7 +15,7 @@ class DoubleOpen : public V4L2VideoDeviceTest
> > {
> > public:
> > DoubleOpen()
> > - : V4L2VideoDeviceTest("vimc", "Raw Capture 0") {}
> > + : V4L2VideoDeviceTest(VIMC, "Raw Capture 0") {}
> > protected:
> > int run()
> > {
> > diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp
> > index ee7d357de0752cae..25cd147f804ba0d5 100644
> > --- a/test/v4l2_videodevice/formats.cpp
> > +++ b/test/v4l2_videodevice/formats.cpp
> > @@ -19,7 +19,7 @@ class Format : public V4L2VideoDeviceTest
> > {
> > public:
> > Format()
> > - : V4L2VideoDeviceTest("vimc", "Raw Capture 0") {}
> > + : V4L2VideoDeviceTest(VIMC, "Raw Capture 0") {}
> > protected:
> > int run()
> > {
> > diff --git a/test/v4l2_videodevice/meson.build b/test/v4l2_videodevice/meson.build
> > index 76be5e142bb63aa6..dc8204872efdc038 100644
> > --- a/test/v4l2_videodevice/meson.build
> > +++ b/test/v4l2_videodevice/meson.build
> > @@ -14,5 +14,5 @@ foreach t : v4l2_videodevice_tests
> > dependencies : libcamera_dep,
> > link_with : test_libraries,
> > include_directories : test_includes_internal)
> > - test(t[0], exe, suite : 'v4l2_videodevice', is_parallel : false)
> > + test(t[0], exe, suite : 'v4l2_videodevice')
> > endforeach
> > diff --git a/test/v4l2_videodevice/request_buffers.cpp b/test/v4l2_videodevice/request_buffers.cpp
> > index c4aedf7b3cd61e80..bbc4073c363c128a 100644
> > --- a/test/v4l2_videodevice/request_buffers.cpp
> > +++ b/test/v4l2_videodevice/request_buffers.cpp
> > @@ -11,7 +11,7 @@ class RequestBuffersTest : public V4L2VideoDeviceTest
> > {
> > public:
> > RequestBuffersTest()
> > - : V4L2VideoDeviceTest("vimc", "Raw Capture 0") {}
> > + : V4L2VideoDeviceTest(VIMC, "Raw Capture 0") {}
> >
> > protected:
> > int run()
> > diff --git a/test/v4l2_videodevice/stream_on_off.cpp b/test/v4l2_videodevice/stream_on_off.cpp
> > index 7664adc4c1f07046..fe021667c44559fa 100644
> > --- a/test/v4l2_videodevice/stream_on_off.cpp
> > +++ b/test/v4l2_videodevice/stream_on_off.cpp
> > @@ -11,7 +11,7 @@ class StreamOnStreamOffTest : public V4L2VideoDeviceTest
> > {
> > public:
> > StreamOnStreamOffTest()
> > - : V4L2VideoDeviceTest("vimc", "Raw Capture 0") {}
> > + : V4L2VideoDeviceTest(VIMC, "Raw Capture 0") {}
> > protected:
> > int run()
> > {
> > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> > index b26d06ad45197c8f..aa39e455360c868c 100644
> > --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> > @@ -28,6 +28,20 @@ bool exists(const std::string &path)
> >
> > int V4L2VideoDeviceTest::init()
> > {
> > + const char *driver;
> > +
> > + switch (resource()) {
> > + case VIMC:
> > + driver = "vimc";
> > + break;
> > + case VIVID:
> > + driver = "vivid";
> > + break;
> > + default:
> > + std::cerr << "Unkown driver for resource" << std::endl;
>
> s/Unkown/Unknown/
>
> > + return TestFail;
> > + }
> > +
> > enumerator_ = DeviceEnumerator::create();
> > if (!enumerator_) {
> > cerr << "Failed to create device enumerator" << endl;
> > @@ -39,7 +53,7 @@ int V4L2VideoDeviceTest::init()
> > return TestFail;
> > }
> >
> > - DeviceMatch dm(driver_);
> > + DeviceMatch dm(driver);
> > dm.add(entity_);
> >
> > media_ = enumerator_->search(dm);
> > diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h
> > index 3321b5a4f98fdb1d..b90a5fca6ce48fdd 100644
> > --- a/test/v4l2_videodevice/v4l2_videodevice_test.h
> > +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h
> > @@ -22,8 +22,8 @@ using namespace libcamera;
> > class V4L2VideoDeviceTest : public Test
> > {
> > public:
> > - V4L2VideoDeviceTest(const char *driver, const char *entity)
> > - : driver_(driver), entity_(entity), capture_(nullptr)
> > + V4L2VideoDeviceTest(TestResource resource, const char *entity)
> > + : Test(resource), entity_(entity), capture_(nullptr)
> > {
> > }
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list