[libcamera-devel] [RFC] test: Move resource locking to test cases
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Jul 4 09:45:28 CEST 2019
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
> and lockf(). The call to lockf() blocks until it can acquire the
> resource and it keeps to lock until the test executable terminates
> closing the file descriptor to the lock file and freeing the lock. This
> design ensures the process never deadlocks. The reason a separate lock
> 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?
> +
> + 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
--
Kieran
More information about the libcamera-devel
mailing list