[{"id":20753,"web_url":"https://patchwork.libcamera.org/comment/20753/","msgid":"<163646355424.1606134.2177352972555566329@Monstersaurus>","date":"2021-11-09T13:12:34","subject":"Re: [libcamera-devel] [PATCH 04/10] test: Add test for the Fence\n\tclass","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-10-28 12:15:14)\n> Add a test for the Fence class by testing completion and expiration of\n> a Fence and testing that move semantic is implemented correctly.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> test/fence.cpp | 148 +++++++++++++++++++++++++++++++++++++++++++++++\n> test/meson.build | 1 +\n> 2 files changed, 149 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..9fb92b1c250b\n> --- /dev/null\n> +++ b/test/fence.cpp\n> @@ -0,0 +1,148 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n\n2021 perhaps.\n\n> + *\n> + * fence.cpp - Fence test\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n\nDefinitely something wrong with checkstyle missing these:\n\n$ clang-format-diff-file test/fence.cpp\n--- test/fence.cpp\n+++ test/fence.cpp.clang\n@@ -12,6 +12,7 @@\n #include \n #include \n #include \n+\n #include \n\n #include \"test.h\"\n\n> +\n> +#include \"test.h\"\n> +\n> +using namespace libcamera;\n> +using namespace std;\n> +\n> +class FenceTest : public Test\n> +{\n> +protected:\n> + int init();\n> + int run();\n> + void cleanup();\n> +\n> +private:\n> + void timeout();\n> + void completed();\n> +\n> + int eventfd_;\n> + Timer timer_;\n> + EventDispatcher *dispatcher;\n> + Fence *fence_;\n> +};\n> +\n> +int FenceTest::init()\n> +{\n> + timer_.timeout.connect(this, &FenceTest::timeout);\n> + dispatcher = Thread::current()->eventDispatcher();\n> +\n> + eventfd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n> + if (eventfd_ < 0) {\n> + cerr << \"Unable to create eventfd\" << endl;\n> + return TestFail;\n> + }\n> +\n> + fence_ = new Fence(eventfd_);\n> + if (!eventfd_) {\n> + cerr << \"Fence creation should not reset the file descriptor\" << endl;\n> + return TestFail;\n> + }\n> +\n> + if (fence_->fd() != eventfd_) {\n> + cerr << \"Fence file descriptor should not be duplicated\" << endl;\n\nWhy not? What happens if someone closes the underlying eventfd? Can we\nhave a copy so that ours stays with us until we're done with it? Or does\nthat break some semantics somewhere...?\n\n> + return TestFail;\n> + }\n> +\n> + fence_->complete.connect(this, &FenceTest::completed);\n> +\n> + return 0;\n> +}\n> +\n\naha, the timeout() below is a helper to 'activate' the fence. Can that\nbe highlighted with a banner comment for the function? It was really\neasy to miss - as it sounded like a timeout event handler - not an event\n'generator'.\n\n\n> +void FenceTest::timeout()\n> +{\n> + uint64_t value = 1;\n> + ssize_t ret = write(eventfd_, &value, sizeof(value));\n> + if (ret != sizeof(value))\n> + cerr << \"Failed to write to event fd\" << endl;\n> +\n> + /* Let the fence complete on the eventfd read event. */\n> + dispatcher->processEvents();\n> +}\n> +\n\nSame here...\n\n> +void FenceTest::completed()\n> +{\n> + if (fence_->expired())\n> + return;\n> +\n> + uint64_t value;\n> + ssize_t ret = read(eventfd_, &value, sizeof(value));\n> + if (ret != sizeof(value))\n> + cerr << \"Failed to read from event fd\" << endl;\n> +}\n> +\n> +int FenceTest::run()\n> +{\n> + /* Activate the fence and schedule a wake-up before it expires. */\n> + fence_->enable();\n> + timer_.start(50);\n\nDoes this activate the underlying eventfd somehow?\n(Edit, yes, I see it now - but it wasn't obvious to start with).\n\n> +\n> + dispatcher->processEvents();\n> +\n> + if (fence_->expired()) {\n> + cerr << \"Fence should not have expired\" << endl;\n> + return TestFail;\n> + }\n> +\n> + if (!fence_->completed()) {\n> + cerr << \"Fence should have completed\" << endl;\n> + return TestFail;\n> + }\n> +\n> + /* Now let the fence expire. */\n> + fence_->enable();\n> + dispatcher->processEvents();\n> +\n> + if (fence_->completed()) {\n> + cerr << \"Fence should not have completed\" << endl;\n> + return TestFail;\n> + }\n> +\n> + if (!fence_->expired()) {\n> + cerr << \"Fence should have expired\" << endl;\n> + return TestFail;\n> + }\n> +\n> + /*\n> + * Test fence move semantic.\n> + *\n> + * Create two temporary fences and verify we can move them.\n> + */\n> + Fence fence1(eventfd_, 500);\n> + Fence fence2(0, 500);\n> + fence2 = std::move(fence1);\n> +\n> + if (fence1.fd() != -1) {\n> + cerr << \"A moved fence should have an invalid fd\" << endl;\n> + return TestFail;\n> + }\n> +\n> + if (fence2.fd() != eventfd_ || fence2.timeout() != 500) {\n> + cerr << \"Faile to move fence\" << endl;\n> + return TestFail;\n> + }\n\nWhat happens to the signals that are connected to fence1 and fence2. Is\nthere anything we can do to test those, and their expected interactions\nafter a move?\n\n\n> +\n> + return 0;\n> +}\n> +\n> +void FenceTest::cleanup()\n> +{\n> + close(eventfd_);\n> + delete fence_;\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'],\n> -- \n> 2.33.1\n>","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 A38F7BDB1C\n\tfor ;\n\tTue, 9 Nov 2021 13:12:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD5F96034E;\n\tTue, 9 Nov 2021 14:12:38 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97293600BF\n\tfor ;\n\tTue, 9 Nov 2021 14:12:37 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2CD8BDEE;\n\tTue, 9 Nov 2021 14:12:37 +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=\"ZiqPr9Rr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636463557;\n\tbh=CT6jqafD3itVbIQWLdPZ2sRibph5EPi5HN7ZWIgAIYQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=ZiqPr9RrnExU1r4rciFBR/TXDHu0u1Xw2UNgSdQdC4Z3FpUWPw7uNwbTPYbPM2G2G\n\ttAIxIHdJGE6KYzCUUyeHy8/gATflybm24jyg8l+Rr2wXfTvr/OfyePVteb8T4oRJoW\n\tkL1/uovMamqSDDKceErciwEpThHh5X934hBzyKwU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211028111520.256612-5-jacopo@jmondi.org>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-5-jacopo@jmondi.org>","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Nov 2021 13:12:34 +0000","Message-ID":"<163646355424.1606134.2177352972555566329@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 04/10] test: Add test for the Fence\n\tclass","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20766,"web_url":"https://patchwork.libcamera.org/comment/20766/","msgid":"<20211109172834.5zxlyyiheckjybji@uno.localdomain>","date":"2021-11-09T17:28:34","subject":"Re: [libcamera-devel] [PATCH 04/10] test: Add test for the Fence\n\tclass","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Tue, Nov 09, 2021 at 01:12:34PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2021-10-28 12:15:14)\n> > Add a test for the Fence class by testing completion and expiration of\n> > a Fence and testing that move semantic is implemented correctly.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > test/fence.cpp | 148 +++++++++++++++++++++++++++++++++++++++++++++++\n> > test/meson.build | 1 +\n> > 2 files changed, 149 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..9fb92b1c250b\n> > --- /dev/null\n> > +++ b/test/fence.cpp\n> > @@ -0,0 +1,148 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n>\n> 2021 perhaps.\n>\n\nUps\n\n> > + *\n> > + * fence.cpp - Fence test\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n>\n> Definitely something wrong with checkstyle missing these:\n>\n> $ clang-format-diff-file test/fence.cpp\n> --- test/fence.cpp\n> +++ test/fence.cpp.clang\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> #include \n> +\n> #include \n>\n> #include \"test.h\"\n>\n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace libcamera;\n> > +using namespace std;\n> > +\n> > +class FenceTest : public Test\n> > +{\n> > +protected:\n> > + int init();\n> > + int run();\n> > + void cleanup();\n> > +\n> > +private:\n> > + void timeout();\n> > + void completed();\n> > +\n> > + int eventfd_;\n> > + Timer timer_;\n> > + EventDispatcher *dispatcher;\n> > + Fence *fence_;\n> > +};\n> > +\n> > +int FenceTest::init()\n> > +{\n> > + timer_.timeout.connect(this, &FenceTest::timeout);\n> > + dispatcher = Thread::current()->eventDispatcher();\n> > +\n> > + eventfd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);\n> > + if (eventfd_ < 0) {\n> > + cerr << \"Unable to create eventfd\" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + fence_ = new Fence(eventfd_);\n> > + if (!eventfd_) {\n> > + cerr << \"Fence creation should not reset the file descriptor\" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (fence_->fd() != eventfd_) {\n> > + cerr << \"Fence file descriptor should not be duplicated\" << endl;\n>\n> Why not? What happens if someone closes the underlying eventfd? Can we\n> have a copy so that ours stays with us until we're done with it? Or does\n> that break some semantics somewhere...?\n>\n\nIt would break the move-only semantic of the Fence class. Making sure\nthe underlying FileDescriptor is handled by a single component is the\nkey to make it easier to guarantee the Fence is either closed or has\nto be handled by the application when returned from a completed\nRequest.\n\nWe could indeed make the FrameBuffer interface accept a &&FileDescriptor\nto make sure application do not try to close it while it is being\nhandled by the library.\n\n> > + return TestFail;\n> > + }\n> > +\n> > + fence_->complete.connect(this, &FenceTest::completed);\n> > +\n> > + return 0;\n> > +}\n> > +\n>\n> aha, the timeout() below is a helper to 'activate' the fence. Can that\n> be highlighted with a banner comment for the function? It was really\n> easy to miss - as it sounded like a timeout event handler - not an event\n> 'generator'.\n>\n>\n> > +void FenceTest::timeout()\n> > +{\n> > + uint64_t value = 1;\n> > + ssize_t ret = write(eventfd_, &value, sizeof(value));\n> > + if (ret != sizeof(value))\n> > + cerr << \"Failed to write to event fd\" << endl;\n> > +\n> > + /* Let the fence complete on the eventfd read event. */\n> > + dispatcher->processEvents();\n> > +}\n> > +\n>\n> Same here...\n>\n> > +void FenceTest::completed()\n> > +{\n> > + if (fence_->expired())\n> > + return;\n> > +\n> > + uint64_t value;\n> > + ssize_t ret = read(eventfd_, &value, sizeof(value));\n> > + if (ret != sizeof(value))\n> > + cerr << \"Failed to read from event fd\" << endl;\n> > +}\n> > +\n> > +int FenceTest::run()\n> > +{\n> > + /* Activate the fence and schedule a wake-up before it expires. */\n> > + fence_->enable();\n> > + timer_.start(50);\n>\n> Does this activate the underlying eventfd somehow?\n> (Edit, yes, I see it now - but it wasn't obvious to start with).\n>\n> > +\n> > + dispatcher->processEvents();\n> > +\n> > + if (fence_->expired()) {\n> > + cerr << \"Fence should not have expired\" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (!fence_->completed()) {\n> > + cerr << \"Fence should have completed\" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* Now let the fence expire. */\n> > + fence_->enable();\n> > + dispatcher->processEvents();\n> > +\n> > + if (fence_->completed()) {\n> > + cerr << \"Fence should not have completed\" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (!fence_->expired()) {\n> > + cerr << \"Fence should have expired\" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /*\n> > + * Test fence move semantic.\n> > + *\n> > + * Create two temporary fences and verify we can move them.\n> > + */\n> > + Fence fence1(eventfd_, 500);\n> > + Fence fence2(0, 500);\n> > + fence2 = std::move(fence1);\n> > +\n> > + if (fence1.fd() != -1) {\n> > + cerr << \"A moved fence should have an invalid fd\" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + if (fence2.fd() != eventfd_ || fence2.timeout() != 500) {\n> > + cerr << \"Faile to move fence\" << endl;\n> > + return TestFail;\n> > + }\n>\n> What happens to the signals that are connected to fence1 and fence2. Is\n> there anything we can do to test those, and their expected interactions\n> after a move?\n\nAs per the previous review comment, I should disconnect slots from a\nmoved fence.\n\nThanks\n j\n\n>\n>\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +void FenceTest::cleanup()\n> > +{\n> > + close(eventfd_);\n> > + delete fence_;\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'],\n> > --\n> > 2.33.1\n> >","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 5F2CDBF415\n\tfor ;\n\tTue, 9 Nov 2021 17:27:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B120560234;\n\tTue, 9 Nov 2021 18:27:43 +0100 (CET)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 56C96600BF\n\tfor ;\n\tTue, 9 Nov 2021 18:27:42 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 982186000C;\n\tTue, 9 Nov 2021 17:27:41 +0000 (UTC)"],"Date":"Tue, 9 Nov 2021 18:28:34 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20211109172834.5zxlyyiheckjybji@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-5-jacopo@jmondi.org>\n\t<163646355424.1606134.2177352972555566329@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163646355424.1606134.2177352972555566329@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 04/10] test: Add test for the Fence\n\tclass","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\" "}}]