[{"id":21075,"web_url":"https://patchwork.libcamera.org/comment/21075/","msgid":"","date":"2021-11-21T18:33:24","subject":"Re: [libcamera-devel] [PATCH v2 05/12] test: fence: Add test for\n\tthe Fence class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Sat, Nov 20, 2021 at 12:13:06PM +0100, Jacopo Mondi wrote:\n> Add a test for the Fence class by testing a Fence failure case, and\n> by testing a successfully signalled fence capture cycle.\n\nThis makes me think that it would be useful to inject fence timeouts in\norder to test applications. Beside unit tests (to test individual\nobjects) and lc-compliance (to test higher-level use cases and pipeline\nhandlers), a mode where libcamera would periodically return various\ntypes of errors would help hardening applications.\n\nThis is certainly not a candidate for this patch series, but I'd like to\nhear feedback on the idea.\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> test/fence.cpp | 339 +++++++++++++++++++++++++++++++++++++++++++++++\n> test/meson.build | 1 +\n> 2 files changed, 340 insertions(+)\n> create mode 100644 test/fence.cpp\n> \n> diff --git a/test/fence.cpp b/test/fence.cpp\n> new file mode 100644\n> index 000000000000..7434542a89f8\n> --- /dev/null\n> +++ b/test/fence.cpp\n> @@ -0,0 +1,339 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * fence.cpp - Fence test\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"camera_test.h\"\n> +#include \"test.h\"\n> +\n> +using namespace std::chrono_literals;\n> +using namespace libcamera;\n> +using namespace std;\n> +\n> +class FenceTest : public CameraTest, public Test\n> +{\n> +public:\n> +\tFenceTest();\n> +\n> +protected:\n> +\tint init();\n> +\tint run();\n> +\tvoid cleanup();\n> +\n> +private:\n> +\tint validateExpiredRequest(Request *request);\n> +\tvoid requestComplete(Request *request);\n> +\tvoid signalFence();\n> +\n> +\tstd::unique_ptr fence_;\n> +\tEventDispatcher *dispatcher_;\n> +\tFileDescriptor eventFd_;\n> +\tTimer fenceTimer_;\n> +\n> +\tstd::vector> requests_;\n> +\tstd::unique_ptr config_;\n> +\tFrameBufferAllocator *allocator_;\n> +\n> +\tStream *stream_;\n> +\n> +\tbool expectedCompletionResult_ = true;\n> +\tbool expectFenceFailure_ = true;\n> +\n> +\tunsigned int completedRequest_;\n> +\n> +\tunsigned int signalledRequestId_;\n> +\tunsigned int expiredRequestId_;\n> +\tunsigned int nbuffers_;\n> +\n> +\tint efd2Copy_;\n> +\tint efdCopy_;\n\nI'd drop the \"Copy\" suffix, those are not really copies. Maybe\nefdFenceTimeout and efdFenceSignalled would be better names.\n\n> +};\n> +\n> +FenceTest::FenceTest()\n> +\t: CameraTest(\"platform/vimc.0 Sensor B\")\n> +{\n> +}\n> +\n> +int FenceTest::init()\n> +{\n> +\tdispatcher_ = Thread::current()->eventDispatcher();\n> +\n> +\tint efd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n> +\tif (efd < 0) {\n> +\t\tcerr << \"Unable to create eventfd\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n\nIt likely makes no difference for now, but if we wanted to test the real\nfence API, we could create a fence object using the\nSW_SYNC_IOC_CREATE_FENCE ioctl. It needs to be called on the\nsync/sw_sync debugfs file. The downside is that we could depend on\ndebugfs.\n\n> +\n> +\t/*\n> +\t * Keep a copy of the original fd value to validate that an expired\n> +\t * fence has the same fd value.\n> +\t */\n> +\tefdCopy_ = efd;\n> +\n> +\tFileDescriptor eventFd(std::move(efd));\n> +\tfence_ = std::make_unique(eventFd);\n> +\tif (!fence_->isValid()) {\n> +\t\tcerr << \"Fence should be valid when created with a valid FileDescriptor\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tif (fence_->fd().fd() != efdCopy_) {\n> +\t\tcerr << \"Fence creation should not duplicate file descriptor\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n\nThese two tests should go to run().\n\n> +\n> +\tconfig_ = camera_->generateConfiguration({ StreamRole::Viewfinder });\n> +\tif (!config_ || config_->size() != 1) {\n> +\t\tcerr << \"Failed to generate default configuration\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tif (camera_->acquire()) {\n> +\t\tcerr << \"Failed to acquire the camera\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tif (camera_->configure(config_.get())) {\n> +\t\tcerr << \"Failed to set default configuration\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tStreamConfiguration &cfg = config_->at(0);\n> +\tstream_ = cfg.stream();\n> +\n> +\tallocator_ = new FrameBufferAllocator(camera_);\n> +\tif (allocator_->allocate(stream_) < 0)\n> +\t\treturn TestFail;\n> +\n> +\tnbuffers_ = allocator_->buffers(stream_).size();\n\nYou may want to check that nbuffers_ is high enough.\n\n> +\tsignalledRequestId_ = nbuffers_ - 2;\n> +\texpiredRequestId_ = nbuffers_ - 1;\n> +\n> +\treturn TestPass;\n> +}\n> +\n> +int FenceTest::validateExpiredRequest(Request *request)\n> +{\n> +\tFrameBuffer *buffer = request->buffers().begin()->second;\n> +\n> +\tFence *fence = buffer->fence();\n> +\tif (!fence) {\n> +\t\tcerr << \"The expired fence should be present\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tif (!fence->isValid()) {\n> +\t\tcerr << \"The expired fence should be valid\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tif (fence->fd().fd() != efdCopy_) {\n> +\t\tcerr << \"The expired fence file descriptor should not change\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tfence->close();\n> +\n> +\texpectFenceFailure_ = false;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/* Callback to signal a fence waiting on the eventfd file descriptor. */\n> +void FenceTest::signalFence()\n> +{\n> +\tuint64_t value = 1;\n> +\twrite(efd2Copy_, &value, sizeof(value));\n> +\tdispatcher_->processEvents();\n> +}\n> +\n> +void FenceTest::requestComplete(Request *request)\n> +{\n> +\tuint64_t cookie = request->cookie();\n> +\tcompletedRequest_ = cookie;\n> +\n> +\t/* We expect the last request to fail, if it doesn't happen, test fails. */\n> +\tif (expectFenceFailure_ && cookie == expiredRequestId_ &&\n> +\t request->status() != Request::RequestCancelled) {\n> +\t\tcerr << \"The last request should have failed: \" << cookie << endl;\n> +\n> +\t\texpectedCompletionResult_ = false;\n> +\t\tdispatcher_->interrupt();\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* All other requests but the last one should be completed. */\n> +\tif (expectFenceFailure_ && cookie < expiredRequestId_ &&\n> +\t request->status() != Request::RequestComplete) {\n> +\t\tcerr << \"All requests but last should complete: \" << cookie << endl;\n> +\n> +\t\texpectedCompletionResult_ = false;\n> +\t\tdispatcher_->interrupt();\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t * If the last request has failed already, all the ones queued after\n> +\t * that shall complete.\n> +\t */\n> +\tif (!expectFenceFailure_ &&\n> +\t request->status() != Request::RequestComplete) {\n> +\t\tcerr << \"Unexpected request failure: \" << cookie << endl;\n> +\n> +\t\texpectedCompletionResult_ = false;\n> +\t\tdispatcher_->interrupt();\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t * The last request has failed.\n> +\t * Validate its fence status and do not re-queue it.\n> +\t */\n> +\tif (cookie == expiredRequestId_) {\n> +\t\tint ret = validateExpiredRequest(request);\n> +\t\tif (ret)\n> +\t\t\texpectedCompletionResult_ = false;\n> +\n> +\t\tdispatcher_->interrupt();\n> +\t\treturn;\n> +\t}\n> +\n> +\tconst Request::BufferMap &buffers = request->buffers();\n> +\tconst Stream *stream = buffers.begin()->first;\n> +\tFrameBuffer *buffer = buffers.begin()->second;\n> +\n> +\t/* A succesfully completed request should have the Fence closed. */\n> +\tif (buffer->fence()) {\n> +\t\tcerr << \"Unexpected valid fence in completed request\" << endl;\n> +\n> +\t\texpectedCompletionResult_ = false;\n> +\t\tdispatcher_->interrupt();\n> +\t\treturn;\n> +\t}\n> +\n> +\trequest->reuse();\n> +\n> +\tif (cookie == signalledRequestId_) {\n> +\t\t/*\n> +\t\t * The second time this request is about to be queued add\n> +\t\t * a fence to it.\n> +\t\t *\n> +\t\t * The main loop should signal it by using a timer to write to\n> +\t\t * the eventfd file descriptor before the fence expires.\n> +\t\t */\n> +\t\tint efd2 = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n> +\t\tif (efd2 < 0) {\n> +\t\t\tcerr << \"Unable to create eventfd\" << endl;\n> +\t\t\texpectedCompletionResult_ = false;\n> +\t\t\treturn;\n> +\t\t}\n> +\t\tefd2Copy_ = efd2;\n\nI wonder if this could be simplified by queuing a request with the fence\nto be signalled in run().\n\n> +\n> +\t\tFileDescriptor eventFd(std::move(efd2));\n> +\t\tstd::unique_ptr fence = std::make_unique(eventFd);\n> +\t\trequest->addBuffer(stream, buffer, std::move(fence));\n> +\t} else {\n> +\t\t/* All the other requests continue to operate without fences. */\n> +\t\trequest->addBuffer(stream, buffer);\n> +\t}\n> +\n> +\tcamera_->queueRequest(request);\n> +\n> +\t/*\n> +\t * Interrupt the dispatcher to return control to the main loop and\n> +\t * activate the fenceTimer\n> +\t . */\n> +\tdispatcher_->interrupt();\n> +}\n> +\n> +int FenceTest::run()\n> +{\n> +\tfor (const auto &[i, buffer] : utils::enumerate(allocator_->buffers(stream_))) {\n> +\t\tstd::unique_ptr request = camera_->createRequest(i);\n> +\t\tif (!request) {\n> +\t\t\tcerr << \"Failed to create request\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tint ret;\n> +\t\tif (i == expiredRequestId_)\n> +\t\t\t/* This request will have a fence, and it will expire. */\n> +\t\t\tret = request->addBuffer(stream_, buffer.get(), std::move(fence_));\n> +\t\telse\n> +\t\t\tret = request->addBuffer(stream_, buffer.get());\n> +\n> +\t\tif (ret) {\n> +\t\t\tcerr << \"Failed to associate buffer with request\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\trequests_.push_back(std::move(request));\n> +\t}\n> +\n> +\tcamera_->requestCompleted.connect(this, &FenceTest::requestComplete);\n> +\n> +\tif (camera_->start()) {\n> +\t\tcerr << \"Failed to start camera\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\tfor (std::unique_ptr &request : requests_) {\n> +\t\tif (camera_->queueRequest(request.get())) {\n> +\t\t\tcerr << \"Failed to queue request\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\t}\n> +\n> +\texpectedCompletionResult_ = true;\n> +\n> +\t/* This timer serves to signal fences associated with \"signalledRequestId_\" */\n> +\tTimer fenceTimer;\n> +\tfenceTimer.timeout.connect(this, &FenceTest::signalFence);\n> +\n> +\t/* Loop for one second. */\n> +\tTimer timer;\n> +\ttimer.start(1000);\n> +\twhile (timer.isRunning() && expectedCompletionResult_) {\n> +\t\tif (completedRequest_ == signalledRequestId_)\n> +\t\t\t/*\n> +\t\t\t * signalledRequestId_ has just completed and it has\n> +\t\t\t * been re-queued with a fence. Start the timer to\n> +\t\t\t * signal the fence in 10 msec.\n> +\t\t\t */\n> +\t\t\tfenceTimer.start(10);\n> +\n> +\t\tdispatcher_->processEvents();\n> +\t}\n> +\n> +\tcamera_->requestCompleted.disconnect();\n> +\n> +\tif (camera_->stop()) {\n> +\t\tcerr << \"Failed to stop camera\" << endl;\n> +\t\treturn TestFail;\n> +\t}\n> +\n> +\treturn expectedCompletionResult_ ? TestPass : TestFail;\n> +}\n> +\n> +void FenceTest::cleanup()\n> +{\n> +\tdelete allocator_;\n> +}\n> +\n> +TEST_REGISTER(FenceTest)\n> diff --git a/test/meson.build b/test/meson.build\n> index d0466f17d7b6..377e392628bf 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -40,6 +40,7 @@ internal_tests = [\n> ['event-dispatcher', 'event-dispatcher.cpp'],\n> ['event-thread', 'event-thread.cpp'],\n> ['file', 'file.cpp'],\n> + ['fence', 'fence.cpp'],\n> ['file-descriptor', 'file-descriptor.cpp'],\n> ['flags', 'flags.cpp'],\n> ['hotplug-cameras', 'hotplug-cameras.cpp'],","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 909D5BF415\n\tfor ;\n\tSun, 21 Nov 2021 18:33:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0AEFC6033C;\n\tSun, 21 Nov 2021 19:33:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA3AF60233\n\tfor ;\n\tSun, 21 Nov 2021 19:33:47 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2FB8E9DD;\n\tSun, 21 Nov 2021 19:33:47 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Xpc+LkiX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637519627;\n\tbh=zeQRkOiG7zo+HMuuCC4riSJcxtLGItOZq+iKA8kX7Xk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Xpc+LkiXCdvaaJfAAn4HdcOZKD4/FfHTILYxfJxTqbbLOXu97D1OOozUDkbxwTpvF\n\tDgXAuF071tM/MzvDAeAOAxlkHMTQ46Ke+9yx9WRCDl2L5npo5wCvUHK2db5dhzFnqp\n\tvtveD5E7bCDO36+hhthfxx+/BSFfJWg4arhU53K0=","Date":"Sun, 21 Nov 2021 20:33:24 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211120111313.106621-1-jacopo@jmondi.org>\n\t<20211120111313.106621-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211120111313.106621-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 05/12] test: fence: Add test for\n\tthe Fence class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]