[{"id":20752,"web_url":"https://patchwork.libcamera.org/comment/20752/","msgid":"<163646284310.1606134.5330663146541602267@Monstersaurus>","date":"2021-11-09T13:00:43","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class","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:13)\n> Fences are a synchronization mechanism that allows to receive event\n\ns/allows to/allow receiving/\n or\ns/allows to/allows us to/\n\n> notifications of a file descriptor with an optional expiration timeout.\n> \n> Introduce the Fence class to model such a mechanism in libcamera.\n\nI like seeing this wrapped in a well defined class. I'm afraid I have\nsome deeper comments that prevent me adding a tag directly...\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/fence.h | 64 +++++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/fence.cpp | 185 +++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 251 insertions(+)\n> create mode 100644 include/libcamera/internal/fence.h\n> create mode 100644 src/libcamera/fence.cpp\n> \n> diff --git a/include/libcamera/internal/fence.h b/include/libcamera/internal/fence.h\n> new file mode 100644\n> index 000000000000..5a78e0f864c7\n> --- /dev/null\n> +++ b/include/libcamera/internal/fence.h\n> @@ -0,0 +1,64 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * internal/fence.h - Synchronization fence\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_FENCE_H__\n> +#define __LIBCAMERA_INTERNAL_FENCE_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n\nI'm surprised checkpatch didn't suggest a separate grouping here.\n\nHrm - it really doesn't - I've just applied the patches and run it. But\nit should... I wonder if our checkstyle isn't working correctly on whole\nfile additions.\n\nAnyway, running clang-format directly gives the following change\nsuggestions:\n\n$ clang-format-diff-file ./include/libcamera/internal/fence.h \n--- ./include/libcamera/internal/fence.h\n+++ ./include/libcamera/internal/fence.h.clang\n@@ -12,6 +12,7 @@\n #include \n #include \n #include \n+\n #include \n \n namespace libcamera {\n@@ -61,4 +62,3 @@\n } /* namespace libcamera */\n \n #endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */\n-\n\n\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Fence\n> +{\n> +public:\n> + explicit Fence(int fenceFd, unsigned int timeoutMs = kFenceTimeout);\n\nShould the timeout use std::chrono in some way to make sure the units\nare coded?\n\n> + Fence(Fence &&other);\n> +\n> + Fence &operator=(Fence &&other);\n\nPersonally I'd group the move operator and constructor together...\n\nhttps://en.cppreference.com/w/cpp/language/rule_of_three\nstates you should have a destructor defined too.\n\nNot sure what it needs to do yet though...\n\n> + int fd() const { return fd_.fd(); }\n> +\n> + unsigned int timeout() const { return timeout_; }\n> + bool completed() const { return completed_; }\n> + bool expired() const { return expired_; }\n> +\n> + void enable();\n> + void disable();\n> +\n> + Signal<> complete;\n> +\n\nOtherwise, I like that interface so far.\n\n\n> +private:\n> + LIBCAMERA_DISABLE_COPY(Fence)\n> +\n> + /* 300 milliseconds default timeout. */\n> + static constexpr unsigned int kFenceTimeout = 300;\n\nstd::chrono::duration possibility...\n\n> +\n> + void moveFence(Fence &other);\n> +\n> + void activated();\n> + void timedout();\n> +\n> + FileDescriptor fd_;\n> + EventNotifier notifier_;\n> +\n> + Timer timer_;\n> + unsigned int timeout_;\n> +\n> + bool completed_ = false;\n> + bool expired_ = false;\n> +\n> + std::mutex mutex_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */\n> +\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index cae65b0604ff..32d5c3387de3 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -22,6 +22,7 @@ libcamera_internal_headers = files([\n> 'device_enumerator.h',\n> 'device_enumerator_sysfs.h',\n> 'device_enumerator_udev.h',\n> + 'fence.h',\n> 'formats.h',\n> 'framebuffer.h',\n> 'ipa_manager.h',\n> diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp\n> new file mode 100644\n> index 000000000000..8fadb2c21f03\n> --- /dev/null\n> +++ b/src/libcamera/fence.cpp\n> @@ -0,0 +1,185 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * fence.cpp - Synchronization fence\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n\n$ clang-format-diff-file src/libcamera/fence.cpp\n--- src/libcamera/fence.cpp\n+++ src/libcamera/fence.cpp.clang\n@@ -7,6 +7,7 @@\n \n #include \n #include \n+\n #include \n \n namespace libcamera {\n\n\n> +namespace libcamera {\n> +\n> +/**\n> + * \\file internal/fence.h\n> + * \\brief Synchronization fence\n> + */\n> +\n> +/**\n> + * \\class Fence\n> + * \\brief Synchronization fence class\n> + *\n> + * A Fence is a synchronization mechanism that allows to wait for a read event\n\n\"allows waiting for a \"\n\n> + * to happen on a file descriptor before an optional timeout expires.\n> + *\n> + * A Fence is created with a default timeout of 300 milliseconds, which can\n> + * be overridden through the 'timeout' constructor parameter. Passing a 0\n> + * timeout to the constructor disables timeouts completely.\n> + *\n> + * A Fence wraps a FileDescriptor and implements a move-only semantic, as Fence\n> + * instances cannot be copied but only moved, causing the moved Fence to be\n> + * reset by invalidating the wrapped FileDescriptor by disabling its\n> + * notification and timeout mechanisms.\n\n\"\"\"\n\nA Fence wraps a FileDescriptor and implements a move-only semantic, as\nFence instances cannot be copied.\n\nMoving a Fence will disable any existing notification and timeout\nmechanisms, but will allow the underlying notification FileDescriptor to\nbe re-used on the 'new' Fence. Existing listeners of the Fence signals\nwill no longer receive events, even if it is re-enabled.\n\n\"\"\"\n\n> + *\n> + * When a read event is notified, the Fence is said to be 'completed',\n> + * alternatively if the timeout expires before any event is notified the Fence\n> + * is said to be 'expired'.\n> + *\n> + * In both cases, when an event notification happens or a timeout expires, the\n> + * class emits the 'complete' signal, to which users of the class can connect\n> + * to and check if the Fence has completed or has expired by calling the\n> + * completed() and expired() functions.\n\nI wonder if the completion status could be passed as part of the signal?\nWould that be easier than the recipient having to call back and check?\n\n(I think having the status methods is still good though)\n\n> + *\n> + * Fence instances are non-active by default (ie no events or timeout are\n> + * generated) and need to be enabled by calling the enable() function. Likewise\n> + * a Fence can be disabled by calling the disable() function.\n> + *\n> + * After a Fence has expired no events are generated and the Fence need to be\n> + * manually re-enabled. Likewise, if the Fence is signalled the expiration\n> + * timeout is cancelled.\n> + */\n> +\n> +/**\n> + * \\brief Create a synchronization fence\n> + * \\param[in] fenceFd The synchronization fence file descriptor\n> + * \\param[in] timeoutMs The optional fence timeout. Set to 0 to disable timeout\n> + */\n> +Fence::Fence(int fenceFd, unsigned int timeoutMs)\n> + : fd_(std::move(fenceFd)),\n> + notifier_(fd_.fd(), EventNotifier::Read, nullptr, false),\n> + timeout_(timeoutMs)\n> +{\n> + notifier_.activated.connect(this, &Fence::activated);\n> +\n> + timer_.timeout.connect(this, &Fence::timedout);\n> +}\n> +\n> +void Fence::moveFence(Fence &other)\n\nI see now that the text above really did mean moving a fence invalidates\nit, but I'm not sure how that use case comes about.\n\nCould this funtion perhaps document /why/ moving a fence necessitates\ninvalidating it, and why it moves the internal fd to preserve it's\nlifetime - but - that the fence can no longer be used... (or something\nlike that if I've inferred correctly).\n\n\n> +{\n> + fd_ = std::move(other.fd_);\n> +\n\nThere might be code connected to the complete signal of the 'other'\nfence. Should they be moved over to this? or should we explictly state\nthat listeners to that completion event are 'disconnected' (somewhat\nimplicitly).\n\nPerhaps we should explictily either disconnect or move them in the\ncode...?\n\n> + other.disable();\n> + if (other.timer_.isRunning())\n> + other.timer_.stop();\n\nShould you move the timeout_ value over to this one too?\n\n> + other.timeout_ = 0;\n> +}\n> +\n> +/**\n> + * \\brief Move-construct a Fence\n> + * \\param[in] other The other fence to move\n\nOk things are becoming clearer. Perhaps:\n\n *\n * The other fence is disabled, and invalidated while the underlying\n * FileDescriptor is moved to this Fence. The newly moved fence can be\n * re-used when required.\n\n> + */\n> +Fence::Fence(Fence &&other)\n> + : notifier_(other.fd(), EventNotifier::Read, nullptr, false)\n> +{\n> + moveFence(other);\n> +\n> + notifier_.activated.connect(this, &Fence::activated);\n> + timer_.timeout.connect(this, &Fence::timedout);\n> +}\n> +\n> +/**\n> + * \\brief Move-assign the value of the \\a other fence to this\n> + * \\param[in] other The other fence to move\n> + * \\return This fence\n\nSomething similar to the above addition maybe...\n\n\n> + */\n> +Fence &Fence::operator=(Fence &&other)\n> +{\n> + moveFence(other);\n> +\n> + notifier_ = EventNotifier(fd_.fd(), EventNotifier::Read, nullptr, false);\n> +\n> + return *this;\n> +}\n> +\n> +/**\n> + * \\fn Fence::fd() const\n> + * \\brief Return the synchronization fence file descriptor\n> + * \\return The synchronization fence file descriptor\n> + */\n> +\n> +/**\n> + * \\fn Fence::timeout()\n> + * \\brief Retrieve the fence timeout\n> + * \\return The fence timeout in milliseconds\n> + */\n> +\n> +/**\n> + * \\fn Fence::completed()\n> + * \\brief Check if the fence has completed before timing out\n> + * \\return True if the fence has completed\n> + */\n> +\n> +/**\n> + * \\fn Fence::expired()\n> + * \\brief Check if the fence has expired before completing\n> + * \\return True if the fence has expired\n> + */\n> +\n> +/**\n> + * \\brief Enable a fence by enabling events notifications\n> + */\n> +void Fence::enable()\n> +{\n> + MutexLocker lock(mutex_);\n> +\n> + expired_ = false;\n> + completed_ = false;\n> +\n> + notifier_.setEnabled(true);\n> + if (timeout_)\n> + timer_.start(timeout_);\n> +}\n> +\n> +/**\n> + * \\brief Disable a fence by disabling events notifications\n> + */\n> +void Fence::disable()\n> +{\n> + notifier_.setEnabled(false);\n> +}\n> +\n> +/**\n> + * \\var Fence::complete\n> + * \\brief The fence completion signal\n> + *\n> + * A Fence is completed if signalled or timeout.\n\nA Fence is completed when signalled or if a timeout occurs.\n\n> + */\n> +\n> +void Fence::activated()\n> +{\n> + MutexLocker lock(mutex_);\n> +\n> + if (timer_.isRunning())\n> + timer_.stop();\n> +\n> + completed_ = true;\n> + expired_ = false;\n> +\n> + complete.emit();\n\nPassing the expiration state as a boolean here might help the receiver\nknow why it completed.\n\nBut maybe that's optional - or better handled as you have it anyway.\n\n> +}\n> +\n> +void Fence::timedout()\n> +{\n> + MutexLocker lock(mutex_);\n> +\n> + expired_ = true;\n> + completed_ = false;\n> +\n> + /* Disable events notification after timeout. */\n> + disable();\n> +\n> + complete.emit();\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 6727a777d804..6fb0d5f49b63 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -14,6 +14,7 @@ libcamera_sources = files([\n> 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> + 'fence.cpp',\n> 'file_descriptor.cpp',\n> 'formats.cpp',\n> 'framebuffer.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 06286BF415\n\tfor ;\n\tTue, 9 Nov 2021 13:00:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 23DE36035D;\n\tTue, 9 Nov 2021 14:00:47 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3208D600BF\n\tfor ;\n\tTue, 9 Nov 2021 14:00:46 +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 BBF424A6;\n\tTue, 9 Nov 2021 14:00:45 +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=\"u3J9HMBO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636462845;\n\tbh=RTus+gFZObOEw0Cyx6OSPn7A/e4lO+EovC+7oQXADb0=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=u3J9HMBOx/FrqfserpFNP2sUwGZFoMDeiABy6gK0N2UVNDWwx6PASS7NYfKxS9Yoi\n\tvLLR6bvunxMjZV1PWYmSe2Etnnsfyl72z5cicmnu+eqZjIJ3tz55uDUsPlTRHqNSZW\n\twgPLCsTi2hA2ND95vIu4zmAx7rj9r1sRcwE3u2/Q=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211028111520.256612-4-jacopo@jmondi.org>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-4-jacopo@jmondi.org>","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Nov 2021 13:00:43 +0000","Message-ID":"<163646284310.1606134.5330663146541602267@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20756,"web_url":"https://patchwork.libcamera.org/comment/20756/","msgid":"<163646519896.1606134.1056409825924975876@Monstersaurus>","date":"2021-11-09T13:39:58","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2021-11-09 13:00:43)\n> Quoting Jacopo Mondi (2021-10-28 12:15:13)\n> > Fences are a synchronization mechanism that allows to receive event\n> \n> s/allows to/allow receiving/\n> or\n> s/allows to/allows us to/\n> \n> > notifications of a file descriptor with an optional expiration timeout.\n> > \n> > Introduce the Fence class to model such a mechanism in libcamera.\n> \n> I like seeing this wrapped in a well defined class. I'm afraid I have\n> some deeper comments that prevent me adding a tag directly...\n> \n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/fence.h | 64 +++++++++\n> > include/libcamera/internal/meson.build | 1 +\n> > src/libcamera/fence.cpp | 185 +++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 251 insertions(+)\n> > create mode 100644 include/libcamera/internal/fence.h\n> > create mode 100644 src/libcamera/fence.cpp\n> > \n> > diff --git a/include/libcamera/internal/fence.h b/include/libcamera/internal/fence.h\n> > new file mode 100644\n> > index 000000000000..5a78e0f864c7\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/fence.h\n> > @@ -0,0 +1,64 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * internal/fence.h - Synchronization fence\n> > + */\n> > +#ifndef __LIBCAMERA_INTERNAL_FENCE_H__\n> > +#define __LIBCAMERA_INTERNAL_FENCE_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> \n> I'm surprised checkpatch didn't suggest a separate grouping here.\n> \n> Hrm - it really doesn't - I've just applied the patches and run it. But\n> it should... I wonder if our checkstyle isn't working correctly on whole\n> file additions.\n> \n> Anyway, running clang-format directly gives the following change\n> suggestions:\n> \n> $ clang-format-diff-file ./include/libcamera/internal/fence.h \n> --- ./include/libcamera/internal/fence.h\n> +++ ./include/libcamera/internal/fence.h.clang\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> #include \n> +\n> #include \n> \n> namespace libcamera {\n> @@ -61,4 +62,3 @@\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */\n> -\n> \n> \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class Fence\n> > +{\n> > +public:\n> > + explicit Fence(int fenceFd, unsigned int timeoutMs = kFenceTimeout);\n> \n> Should the timeout use std::chrono in some way to make sure the units\n> are coded?\n> \n> > + Fence(Fence &&other);\n> > +\n> > + Fence &operator=(Fence &&other);\n> \n> Personally I'd group the move operator and constructor together...\n> \n> https://en.cppreference.com/w/cpp/language/rule_of_three\n> states you should have a destructor defined too.\n> \n> Not sure what it needs to do yet though...\n> \n> > + int fd() const { return fd_.fd(); }\n> > +\n> > + unsigned int timeout() const { return timeout_; }\n> > + bool completed() const { return completed_; }\n> > + bool expired() const { return expired_; }\n> > +\n> > + void enable();\n> > + void disable();\n> > +\n> > + Signal<> complete;\n> > +\n> \n> Otherwise, I like that interface so far.\n> \n> \n> > +private:\n> > + LIBCAMERA_DISABLE_COPY(Fence)\n> > +\n> > + /* 300 milliseconds default timeout. */\n> > + static constexpr unsigned int kFenceTimeout = 300;\n> \n> std::chrono::duration possibility...\n> \n> > +\n> > + void moveFence(Fence &other);\n> > +\n> > + void activated();\n> > + void timedout();\n> > +\n> > + FileDescriptor fd_;\n> > + EventNotifier notifier_;\n> > +\n> > + Timer timer_;\n> > + unsigned int timeout_;\n> > +\n> > + bool completed_ = false;\n> > + bool expired_ = false;\n> > +\n> > + std::mutex mutex_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */\n> > +\n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index cae65b0604ff..32d5c3387de3 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -22,6 +22,7 @@ libcamera_internal_headers = files([\n> > 'device_enumerator.h',\n> > 'device_enumerator_sysfs.h',\n> > 'device_enumerator_udev.h',\n> > + 'fence.h',\n> > 'formats.h',\n> > 'framebuffer.h',\n> > 'ipa_manager.h',\n> > diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp\n> > new file mode 100644\n> > index 000000000000..8fadb2c21f03\n> > --- /dev/null\n> > +++ b/src/libcamera/fence.cpp\n> > @@ -0,0 +1,185 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * fence.cpp - Synchronization fence\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> \n> $ clang-format-diff-file src/libcamera/fence.cpp\n> --- src/libcamera/fence.cpp\n> +++ src/libcamera/fence.cpp.clang\n> @@ -7,6 +7,7 @@\n> \n> #include \n> #include \n> +\n> #include \n> \n> namespace libcamera {\n> \n> \n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\file internal/fence.h\n> > + * \\brief Synchronization fence\n> > + */\n> > +\n> > +/**\n> > + * \\class Fence\n> > + * \\brief Synchronization fence class\n> > + *\n> > + * A Fence is a synchronization mechanism that allows to wait for a read event\n> \n> \"allows waiting for a \"\n> \n> > + * to happen on a file descriptor before an optional timeout expires.\n> > + *\n> > + * A Fence is created with a default timeout of 300 milliseconds, which can\n> > + * be overridden through the 'timeout' constructor parameter. Passing a 0\n> > + * timeout to the constructor disables timeouts completely.\n> > + *\n> > + * A Fence wraps a FileDescriptor and implements a move-only semantic, as Fence\n> > + * instances cannot be copied but only moved, causing the moved Fence to be\n> > + * reset by invalidating the wrapped FileDescriptor by disabling its\n> > + * notification and timeout mechanisms.\n> \n> \"\"\"\n> \n> A Fence wraps a FileDescriptor and implements a move-only semantic, as\n> Fence instances cannot be copied.\n> \n> Moving a Fence will disable any existing notification and timeout\n> mechanisms, but will allow the underlying notification FileDescriptor to\n> be re-used on the 'new' Fence. Existing listeners of the Fence signals\n> will no longer receive events, even if it is re-enabled.\n> \n> \"\"\"\n> \n> > + *\n> > + * When a read event is notified, the Fence is said to be 'completed',\n> > + * alternatively if the timeout expires before any event is notified the Fence\n> > + * is said to be 'expired'.\n> > + *\n> > + * In both cases, when an event notification happens or a timeout expires, the\n> > + * class emits the 'complete' signal, to which users of the class can connect\n> > + * to and check if the Fence has completed or has expired by calling the\n> > + * completed() and expired() functions.\n> \n> I wonder if the completion status could be passed as part of the signal?\n> Would that be easier than the recipient having to call back and check?\n> \n> (I think having the status methods is still good though)\n> \n> > + *\n> > + * Fence instances are non-active by default (ie no events or timeout are\n> > + * generated) and need to be enabled by calling the enable() function. Likewise\n> > + * a Fence can be disabled by calling the disable() function.\n> > + *\n> > + * After a Fence has expired no events are generated and the Fence need to be\n> > + * manually re-enabled. Likewise, if the Fence is signalled the expiration\n> > + * timeout is cancelled.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Create a synchronization fence\n> > + * \\param[in] fenceFd The synchronization fence file descriptor\n> > + * \\param[in] timeoutMs The optional fence timeout. Set to 0 to disable timeout\n> > + */\n> > +Fence::Fence(int fenceFd, unsigned int timeoutMs)\n> > + : fd_(std::move(fenceFd)),\n> > + notifier_(fd_.fd(), EventNotifier::Read, nullptr, false),\n> > + timeout_(timeoutMs)\n> > +{\n> > + notifier_.activated.connect(this, &Fence::activated);\n> > +\n> > + timer_.timeout.connect(this, &Fence::timedout);\n> > +}\n> > +\n> > +void Fence::moveFence(Fence &other)\n> \n> I see now that the text above really did mean moving a fence invalidates\n> it, but I'm not sure how that use case comes about.\n> \n> Could this funtion perhaps document /why/ moving a fence necessitates\n> invalidating it, and why it moves the internal fd to preserve it's\n> lifetime - but - that the fence can no longer be used... (or something\n> like that if I've inferred correctly).\n> \n> \n> > +{\n> > + fd_ = std::move(other.fd_);\n> > +\n> \n> There might be code connected to the complete signal of the 'other'\n> fence. Should they be moved over to this? or should we explictly state\n> that listeners to that completion event are 'disconnected' (somewhat\n> implicitly).\n> \n> Perhaps we should explictily either disconnect or move them in the\n> code...?\n> \n> > + other.disable();\n> > + if (other.timer_.isRunning())\n> > + other.timer_.stop();\n\nalso - in this case here, the other timer is still running, so there is\nperhaps expected to be someone listing waiting for either a completed\nfence notification or a timeout. Does calling .stop() - activate the\nsignal to signal a timeout? If not - perhaps we should explicitly /\nmanually do so, and generate a timeout on it?\n\n> \n> Should you move the timeout_ value over to this one too?\n> \n> > + other.timeout_ = 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Move-construct a Fence\n> > + * \\param[in] other The other fence to move\n> \n> Ok things are becoming clearer. Perhaps:\n> \n> *\n> * The other fence is disabled, and invalidated while the underlying\n> * FileDescriptor is moved to this Fence. The newly moved fence can be\n> * re-used when required.\n> \n> > + */\n> > +Fence::Fence(Fence &&other)\n> > + : notifier_(other.fd(), EventNotifier::Read, nullptr, false)\n> > +{\n> > + moveFence(other);\n> > +\n> > + notifier_.activated.connect(this, &Fence::activated);\n> > + timer_.timeout.connect(this, &Fence::timedout);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Move-assign the value of the \\a other fence to this\n> > + * \\param[in] other The other fence to move\n> > + * \\return This fence\n> \n> Something similar to the above addition maybe...\n> \n> \n> > + */\n> > +Fence &Fence::operator=(Fence &&other)\n> > +{\n> > + moveFence(other);\n> > +\n> > + notifier_ = EventNotifier(fd_.fd(), EventNotifier::Read, nullptr, false);\n> > +\n> > + return *this;\n> > +}\n> > +\n> > +/**\n> > + * \\fn Fence::fd() const\n> > + * \\brief Return the synchronization fence file descriptor\n> > + * \\return The synchronization fence file descriptor\n> > + */\n> > +\n> > +/**\n> > + * \\fn Fence::timeout()\n> > + * \\brief Retrieve the fence timeout\n> > + * \\return The fence timeout in milliseconds\n> > + */\n> > +\n> > +/**\n> > + * \\fn Fence::completed()\n> > + * \\brief Check if the fence has completed before timing out\n> > + * \\return True if the fence has completed\n> > + */\n> > +\n> > +/**\n> > + * \\fn Fence::expired()\n> > + * \\brief Check if the fence has expired before completing\n> > + * \\return True if the fence has expired\n> > + */\n> > +\n> > +/**\n> > + * \\brief Enable a fence by enabling events notifications\n> > + */\n> > +void Fence::enable()\n> > +{\n> > + MutexLocker lock(mutex_);\n> > +\n> > + expired_ = false;\n> > + completed_ = false;\n> > +\n> > + notifier_.setEnabled(true);\n> > + if (timeout_)\n> > + timer_.start(timeout_);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Disable a fence by disabling events notifications\n> > + */\n> > +void Fence::disable()\n> > +{\n> > + notifier_.setEnabled(false);\n> > +}\n> > +\n> > +/**\n> > + * \\var Fence::complete\n> > + * \\brief The fence completion signal\n> > + *\n> > + * A Fence is completed if signalled or timeout.\n> \n> A Fence is completed when signalled or if a timeout occurs.\n> \n> > + */\n> > +\n> > +void Fence::activated()\n> > +{\n> > + MutexLocker lock(mutex_);\n> > +\n> > + if (timer_.isRunning())\n> > + timer_.stop();\n> > +\n> > + completed_ = true;\n> > + expired_ = false;\n> > +\n> > + complete.emit();\n> \n> Passing the expiration state as a boolean here might help the receiver\n> know why it completed.\n> \n> But maybe that's optional - or better handled as you have it anyway.\n> \n> > +}\n> > +\n> > +void Fence::timedout()\n> > +{\n> > + MutexLocker lock(mutex_);\n> > +\n> > + expired_ = true;\n> > + completed_ = false;\n> > +\n> > + /* Disable events notification after timeout. */\n> > + disable();\n> > +\n> > + complete.emit();\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 6727a777d804..6fb0d5f49b63 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -14,6 +14,7 @@ libcamera_sources = files([\n> > 'delayed_controls.cpp',\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > + 'fence.cpp',\n> > 'file_descriptor.cpp',\n> > 'formats.cpp',\n> > 'framebuffer.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 2C61FBF415\n\tfor ;\n\tTue, 9 Nov 2021 13:40:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 644F56034E;\n\tTue, 9 Nov 2021 14:40:02 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B638600BF\n\tfor ;\n\tTue, 9 Nov 2021 14:40:01 +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 11FCA5D;\n\tTue, 9 Nov 2021 14:40:01 +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=\"cJFWDZvx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636465201;\n\tbh=MNuhLn4c2yNTG6/0h8CkIp6C236D7AbaVxriN4w5aGg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=cJFWDZvxgyAglwZu67ua1IClrCP1pgs39szPjMrQdnkifIuCq766eGS4RhvVxleL0\n\tCRUfdc+2QqFvGkB8uoB+DNxyxkfp9AEnjJgb6foQAZYUp640bMaYE/tAeLhNsppvwh\n\tOTXnTG2IuCy+YZEYMk3T0O9TK8+kZFjBRgMJOcVE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<163646284310.1606134.5330663146541602267@Monstersaurus>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-4-jacopo@jmondi.org>\n\t<163646284310.1606134.5330663146541602267@Monstersaurus>","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Nov 2021 13:39:58 +0000","Message-ID":"<163646519896.1606134.1056409825924975876@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20765,"web_url":"https://patchwork.libcamera.org/comment/20765/","msgid":"<20211109172442.kwyt77t4sw3adph4@uno.localdomain>","date":"2021-11-09T17:24:42","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class","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:39:58PM +0000, Kieran Bingham wrote:\n> Quoting Kieran Bingham (2021-11-09 13:00:43)\n> > Quoting Jacopo Mondi (2021-10-28 12:15:13)\n\n[snip]\n\n> > > +void Fence::moveFence(Fence &other)\n> >\n> > I see now that the text above really did mean moving a fence invalidates\n> > it, but I'm not sure how that use case comes about.\n> >\n> > Could this funtion perhaps document /why/ moving a fence necessitates\n> > invalidating it, and why it moves the internal fd to preserve it's\n> > lifetime - but - that the fence can no longer be used... (or something\n> > like that if I've inferred correctly).\n> >\n> >\n> > > +{\n> > > + fd_ = std::move(other.fd_);\n> > > +\n> >\n> > There might be code connected to the complete signal of the 'other'\n> > fence. Should they be moved over to this? or should we explictly state\n> > that listeners to that completion event are 'disconnected' (somewhat\n> > implicitly).\n> >\n> > Perhaps we should explictily either disconnect or move them in the\n> > code...?\n> >\n> > > + other.disable();\n> > > + if (other.timer_.isRunning())\n> > > + other.timer_.stop();\n>\n> also - in this case here, the other timer is still running, so there is\n> perhaps expected to be someone listing waiting for either a completed\n> fence notification or a timeout. Does calling .stop() - activate the\n> signal to signal a timeout? If not - perhaps we should explicitly /\n> manually do so, and generate a timeout on it?\n>\n\nafaict Timer::stop() does not generate a timeout and I'm not sure\ngenerating an event on the moved Fence is the right thing to do. How\nis it the slot supposed to know if a real timeout has occurred ?\n\n> >\n> > Should you move the timeout_ value over to this one too?\n> >\n> > > + other.timeout_ = 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Move-construct a Fence\n> > > + * \\param[in] other The other fence to move\n> >\n> > Ok things are becoming clearer. Perhaps:\n> >\n> > *\n> > * The other fence is disabled, and invalidated while the underlying\n> > * FileDescriptor is moved to this Fence. The newly moved fence can be\n> > * re-used when required.\n> >\n> > > + */\n> > > +Fence::Fence(Fence &&other)\n> > > + : notifier_(other.fd(), EventNotifier::Read, nullptr, false)\n> > > +{\n> > > + moveFence(other);\n> > > +\n> > > + notifier_.activated.connect(this, &Fence::activated);\n> > > + timer_.timeout.connect(this, &Fence::timedout);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Move-assign the value of the \\a other fence to this\n> > > + * \\param[in] other The other fence to move\n> > > + * \\return This fence\n> >\n> > Something similar to the above addition maybe...\n> >\n> >\n> > > + */\n> > > +Fence &Fence::operator=(Fence &&other)\n> > > +{\n> > > + moveFence(other);\n> > > +\n> > > + notifier_ = EventNotifier(fd_.fd(), EventNotifier::Read, nullptr, false);\n> > > +\n> > > + return *this;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn Fence::fd() const\n> > > + * \\brief Return the synchronization fence file descriptor\n> > > + * \\return The synchronization fence file descriptor\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Fence::timeout()\n> > > + * \\brief Retrieve the fence timeout\n> > > + * \\return The fence timeout in milliseconds\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Fence::completed()\n> > > + * \\brief Check if the fence has completed before timing out\n> > > + * \\return True if the fence has completed\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Fence::expired()\n> > > + * \\brief Check if the fence has expired before completing\n> > > + * \\return True if the fence has expired\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Enable a fence by enabling events notifications\n> > > + */\n> > > +void Fence::enable()\n> > > +{\n> > > + MutexLocker lock(mutex_);\n> > > +\n> > > + expired_ = false;\n> > > + completed_ = false;\n> > > +\n> > > + notifier_.setEnabled(true);\n> > > + if (timeout_)\n> > > + timer_.start(timeout_);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Disable a fence by disabling events notifications\n> > > + */\n> > > +void Fence::disable()\n> > > +{\n> > > + notifier_.setEnabled(false);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\var Fence::complete\n> > > + * \\brief The fence completion signal\n> > > + *\n> > > + * A Fence is completed if signalled or timeout.\n> >\n> > A Fence is completed when signalled or if a timeout occurs.\n> >\n> > > + */\n> > > +\n> > > +void Fence::activated()\n> > > +{\n> > > + MutexLocker lock(mutex_);\n> > > +\n> > > + if (timer_.isRunning())\n> > > + timer_.stop();\n> > > +\n> > > + completed_ = true;\n> > > + expired_ = false;\n> > > +\n> > > + complete.emit();\n> >\n> > Passing the expiration state as a boolean here might help the receiver\n> > know why it completed.\n> >\n> > But maybe that's optional - or better handled as you have it anyway.\n> >\n> > > +}\n> > > +\n> > > +void Fence::timedout()\n> > > +{\n> > > + MutexLocker lock(mutex_);\n> > > +\n> > > + expired_ = true;\n> > > + completed_ = false;\n> > > +\n> > > + /* Disable events notification after timeout. */\n> > > + disable();\n> > > +\n> > > + complete.emit();\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 6727a777d804..6fb0d5f49b63 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -14,6 +14,7 @@ libcamera_sources = files([\n> > > 'delayed_controls.cpp',\n> > > 'device_enumerator.cpp',\n> > > 'device_enumerator_sysfs.cpp',\n> > > + 'fence.cpp',\n> > > 'file_descriptor.cpp',\n> > > 'formats.cpp',\n> > > 'framebuffer.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 98E4FBDB1C\n\tfor ;\n\tTue, 9 Nov 2021 17:23:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E7A706034E;\n\tTue, 9 Nov 2021 18:23:50 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC1F3600BF\n\tfor ;\n\tTue, 9 Nov 2021 18:23:49 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 2FCC7240004;\n\tTue, 9 Nov 2021 17:23:48 +0000 (UTC)"],"Date":"Tue, 9 Nov 2021 18:24:42 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20211109172442.kwyt77t4sw3adph4@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-4-jacopo@jmondi.org>\n\t<163646284310.1606134.5330663146541602267@Monstersaurus>\n\t<163646519896.1606134.1056409825924975876@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163646519896.1606134.1056409825924975876@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce 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\" "}},{"id":20774,"web_url":"https://patchwork.libcamera.org/comment/20774/","msgid":"<163648834243.1743947.1675499367845725493@Monstersaurus>","date":"2021-11-09T20:05:42","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-11-09 17:22:31)\n> Hi Kieran\n> \n> On Tue, Nov 09, 2021 at 01:00:43PM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2021-10-28 12:15:13)\n> > > Fences are a synchronization mechanism that allows to receive event\n> >\n> > s/allows to/allow receiving/\n> > or\n> > s/allows to/allows us to/\n> >\n> > > notifications of a file descriptor with an optional expiration timeout.\n> > >\n> > > Introduce the Fence class to model such a mechanism in libcamera.\n> >\n> > I like seeing this wrapped in a well defined class. I'm afraid I have\n> > some deeper comments that prevent me adding a tag directly...\n> >\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/fence.h | 64 +++++++++\n> > > include/libcamera/internal/meson.build | 1 +\n> > > src/libcamera/fence.cpp | 185 +++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 4 files changed, 251 insertions(+)\n> > > create mode 100644 include/libcamera/internal/fence.h\n> > > create mode 100644 src/libcamera/fence.cpp\n> > >\n> > > diff --git a/include/libcamera/internal/fence.h b/include/libcamera/internal/fence.h\n> > > new file mode 100644\n> > > index 000000000000..5a78e0f864c7\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/fence.h\n> > > @@ -0,0 +1,64 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * internal/fence.h - Synchronization fence\n> > > + */\n> > > +#ifndef __LIBCAMERA_INTERNAL_FENCE_H__\n> > > +#define __LIBCAMERA_INTERNAL_FENCE_H__\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> >\n> > I'm surprised checkpatch didn't suggest a separate grouping here.\n> >\n> > Hrm - it really doesn't - I've just applied the patches and run it. But\n> > it should... I wonder if our checkstyle isn't working correctly on whole\n> > file additions.\n> >\n> > Anyway, running clang-format directly gives the following change\n> > suggestions:\n> >\n> > $ clang-format-diff-file ./include/libcamera/internal/fence.h\n> > --- ./include/libcamera/internal/fence.h\n> > +++ ./include/libcamera/internal/fence.h.clang\n> > @@ -12,6 +12,7 @@\n> > #include \n> > #include \n> > #include \n> > +\n> > #include \n> >\n> > namespace libcamera {\n> > @@ -61,4 +62,3 @@\n> > } /* namespace libcamera */\n> >\n> > #endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */\n> > -\n> >\n> >\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class Fence\n> > > +{\n> > > +public:\n> > > + explicit Fence(int fenceFd, unsigned int timeoutMs = kFenceTimeout);\n> >\n> > Should the timeout use std::chrono in some way to make sure the units\n> > are coded?\n> >\n> \n> Good idea, I'll try it\n> \n> > > + Fence(Fence &&other);\n> > > +\n> > > + Fence &operator=(Fence &&other);\n> >\n> > Personally I'd group the move operator and constructor together...\n> >\n> > https://en.cppreference.com/w/cpp/language/rule_of_three\n> > states you should have a destructor defined too.\n> >\n> > Not sure what it needs to do yet though...\n> >\n> > > + int fd() const { return fd_.fd(); }\n> > > +\n> > > + unsigned int timeout() const { return timeout_; }\n> > > + bool completed() const { return completed_; }\n> > > + bool expired() const { return expired_; }\n> > > +\n> > > + void enable();\n> > > + void disable();\n> > > +\n> > > + Signal<> complete;\n> > > +\n> >\n> > Otherwise, I like that interface so far.\n> >\n> >\n> > > +private:\n> > > + LIBCAMERA_DISABLE_COPY(Fence)\n> > > +\n> > > + /* 300 milliseconds default timeout. */\n> > > + static constexpr unsigned int kFenceTimeout = 300;\n> >\n> > std::chrono::duration possibility...\n> >\n> > > +\n> > > + void moveFence(Fence &other);\n> > > +\n> > > + void activated();\n> > > + void timedout();\n> > > +\n> > > + FileDescriptor fd_;\n> > > + EventNotifier notifier_;\n> > > +\n> > > + Timer timer_;\n> > > + unsigned int timeout_;\n> > > +\n> > > + bool completed_ = false;\n> > > + bool expired_ = false;\n> > > +\n> > > + std::mutex mutex_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */\n> > > +\n> > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > > index cae65b0604ff..32d5c3387de3 100644\n> > > --- a/include/libcamera/internal/meson.build\n> > > +++ b/include/libcamera/internal/meson.build\n> > > @@ -22,6 +22,7 @@ libcamera_internal_headers = files([\n> > > 'device_enumerator.h',\n> > > 'device_enumerator_sysfs.h',\n> > > 'device_enumerator_udev.h',\n> > > + 'fence.h',\n> > > 'formats.h',\n> > > 'framebuffer.h',\n> > > 'ipa_manager.h',\n> > > diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp\n> > > new file mode 100644\n> > > index 000000000000..8fadb2c21f03\n> > > --- /dev/null\n> > > +++ b/src/libcamera/fence.cpp\n> > > @@ -0,0 +1,185 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * fence.cpp - Synchronization fence\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> >\n> > $ clang-format-diff-file src/libcamera/fence.cpp\n> > --- src/libcamera/fence.cpp\n> > +++ src/libcamera/fence.cpp.clang\n> > @@ -7,6 +7,7 @@\n> >\n> > #include \n> > #include \n> > +\n> > #include \n> >\n> > namespace libcamera {\n> >\n> >\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\file internal/fence.h\n> > > + * \\brief Synchronization fence\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\class Fence\n> > > + * \\brief Synchronization fence class\n> > > + *\n> > > + * A Fence is a synchronization mechanism that allows to wait for a read event\n> >\n> > \"allows waiting for a \"\n> >\n> > > + * to happen on a file descriptor before an optional timeout expires.\n> > > + *\n> > > + * A Fence is created with a default timeout of 300 milliseconds, which can\n> > > + * be overridden through the 'timeout' constructor parameter. Passing a 0\n> > > + * timeout to the constructor disables timeouts completely.\n> > > + *\n> > > + * A Fence wraps a FileDescriptor and implements a move-only semantic, as Fence\n> > > + * instances cannot be copied but only moved, causing the moved Fence to be\n> > > + * reset by invalidating the wrapped FileDescriptor by disabling its\n> > > + * notification and timeout mechanisms.\n> >\n> > \"\"\"\n> >\n> > A Fence wraps a FileDescriptor and implements a move-only semantic, as\n> > Fence instances cannot be copied.\n> >\n> > Moving a Fence will disable any existing notification and timeout\n> > mechanisms, but will allow the underlying notification FileDescriptor to\n> > be re-used on the 'new' Fence. Existing listeners of the Fence signals\n> > will no longer receive events, even if it is re-enabled.\n> >\n> > \"\"\"\n> >\n> > > + *\n> > > + * When a read event is notified, the Fence is said to be 'completed',\n> > > + * alternatively if the timeout expires before any event is notified the Fence\n> > > + * is said to be 'expired'.\n> > > + *\n> > > + * In both cases, when an event notification happens or a timeout expires, the\n> > > + * class emits the 'complete' signal, to which users of the class can connect\n> > > + * to and check if the Fence has completed or has expired by calling the\n> > > + * completed() and expired() functions.\n> >\n> > I wonder if the completion status could be passed as part of the signal?\n> > Would that be easier than the recipient having to call back and check?\n> >\n> > (I think having the status methods is still good though)\n> >\n> \n> That's also a good suggestion!\n> \n> > > + *\n> > > + * Fence instances are non-active by default (ie no events or timeout are\n> > > + * generated) and need to be enabled by calling the enable() function. Likewise\n> > > + * a Fence can be disabled by calling the disable() function.\n> > > + *\n> > > + * After a Fence has expired no events are generated and the Fence need to be\n> > > + * manually re-enabled. Likewise, if the Fence is signalled the expiration\n> > > + * timeout is cancelled.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Create a synchronization fence\n> > > + * \\param[in] fenceFd The synchronization fence file descriptor\n> > > + * \\param[in] timeoutMs The optional fence timeout. Set to 0 to disable timeout\n> > > + */\n> > > +Fence::Fence(int fenceFd, unsigned int timeoutMs)\n> > > + : fd_(std::move(fenceFd)),\n> > > + notifier_(fd_.fd(), EventNotifier::Read, nullptr, false),\n> > > + timeout_(timeoutMs)\n> > > +{\n> > > + notifier_.activated.connect(this, &Fence::activated);\n> > > +\n> > > + timer_.timeout.connect(this, &Fence::timedout);\n> > > +}\n> > > +\n> > > +void Fence::moveFence(Fence &other)\n> >\n> > I see now that the text above really did mean moving a fence invalidates\n> > it, but I'm not sure how that use case comes about.\n> >\n> > Could this funtion perhaps document /why/ moving a fence necessitates\n> > invalidating it, and why it moves the internal fd to preserve it's\n> > lifetime - but - that the fence can no longer be used... (or something\n> > like that if I've inferred correctly).\n> >\n> \n> I'm not sure this adds anything to the definition of the C++ move\n> semantic. A moved object is left in \"valid but undefined state\"\n> \n> https://en.cppreference.com/w/cpp/utility/move\n> Unless otherwise specified, all standard library objects that have\n> been moved from are placed in a \"valid but unspecified state\", meaning\n> the object's class invariants hold (so functions without\n> preconditions, such as the assignment operator, can be safely used on\n> the object after it was moved from):\n\nI think we've crossed syntaxes a little.\n\nYes, C++ move syntax means that the 'other' object (piece of memory) is\ninvalidated, but with the Fence here - I'm weary that when we do a move\nwe're invalidating the Fence itself... (to some degree), and the\nconnections that are made against the fence.\n\nIts defining what happens to the existing 'users' (Anything that has a\nconnection to a fence which is about to be moved) that I think we should\nexplicitly document here.\n\n\nUltimately - from what I understand that is now that \n\n\"Moving a Fence will disconnect any existing listners to events on that\nFence and stop any timer that may be actived on the Fence. No completion\nevents will be sent on the Fence after it has been moved until it is\nre-enabled.\"\n\n\nOtherwise, a move operation should 'move' the entire thing across,\nconnections and all.\n\n\n> > > +{\n> > > + fd_ = std::move(other.fd_);\n> > > +\n> >\n> > There might be code connected to the complete signal of the 'other'\n> > fence. Should they be moved over to this? or should we explictly state\n> > that listeners to that completion event are 'disconnected' (somewhat\n> > implicitly).\n> >\n> > Perhaps we should explictily either disconnect or move them in the\n> > code...?\n> \n> I should indeed disconnect the existing slots.\n> \n> >\n> > > + other.disable();\n> > > + if (other.timer_.isRunning())\n> > > + other.timer_.stop();\n> >\n> > Should you move the timeout_ value over to this one too?\n> >\n> \n> Yes indeed!\n> \n> > > + other.timeout_ = 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Move-construct a Fence\n> > > + * \\param[in] other The other fence to move\n> >\n> > Ok things are becoming clearer. Perhaps:\n> >\n> > *\n> > * The other fence is disabled, and invalidated while the underlying\n> > * FileDescriptor is moved to this Fence. The newly moved fence can be\n> > * re-used when required.\n> >\n> > > + */\n> > > +Fence::Fence(Fence &&other)\n> > > + : notifier_(other.fd(), EventNotifier::Read, nullptr, false)\n> > > +{\n> > > + moveFence(other);\n> > > +\n> > > + notifier_.activated.connect(this, &Fence::activated);\n> > > + timer_.timeout.connect(this, &Fence::timedout);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Move-assign the value of the \\a other fence to this\n> > > + * \\param[in] other The other fence to move\n> > > + * \\return This fence\n> >\n> > Something similar to the above addition maybe...\n> >\n> >\n> > > + */\n> > > +Fence &Fence::operator=(Fence &&other)\n> > > +{\n> > > + moveFence(other);\n> > > +\n> > > + notifier_ = EventNotifier(fd_.fd(), EventNotifier::Read, nullptr, false);\n> > > +\n> > > + return *this;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn Fence::fd() const\n> > > + * \\brief Return the synchronization fence file descriptor\n> > > + * \\return The synchronization fence file descriptor\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Fence::timeout()\n> > > + * \\brief Retrieve the fence timeout\n> > > + * \\return The fence timeout in milliseconds\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Fence::completed()\n> > > + * \\brief Check if the fence has completed before timing out\n> > > + * \\return True if the fence has completed\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Fence::expired()\n> > > + * \\brief Check if the fence has expired before completing\n> > > + * \\return True if the fence has expired\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Enable a fence by enabling events notifications\n> > > + */\n> > > +void Fence::enable()\n> > > +{\n> > > + MutexLocker lock(mutex_);\n> > > +\n> > > + expired_ = false;\n> > > + completed_ = false;\n> > > +\n> > > + notifier_.setEnabled(true);\n> > > + if (timeout_)\n> > > + timer_.start(timeout_);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Disable a fence by disabling events notifications\n> > > + */\n> > > +void Fence::disable()\n> > > +{\n> > > + notifier_.setEnabled(false);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\var Fence::complete\n> > > + * \\brief The fence completion signal\n> > > + *\n> > > + * A Fence is completed if signalled or timeout.\n> >\n> > A Fence is completed when signalled or if a timeout occurs.\n> >\n> > > + */\n> > > +\n> > > +void Fence::activated()\n> > > +{\n> > > + MutexLocker lock(mutex_);\n> > > +\n> > > + if (timer_.isRunning())\n> > > + timer_.stop();\n> > > +\n> > > + completed_ = true;\n> > > + expired_ = false;\n> > > +\n> > > + complete.emit();\n> >\n> > Passing the expiration state as a boolean here might help the receiver\n> > know why it completed.\n> >\n> > But maybe that's optional - or better handled as you have it anyway.\n> >\n> > > +}\n> > > +\n> > > +void Fence::timedout()\n> > > +{\n> > > + MutexLocker lock(mutex_);\n> > > +\n> > > + expired_ = true;\n> > > + completed_ = false;\n> > > +\n> > > + /* Disable events notification after timeout. */\n> > > + disable();\n> > > +\n> > > + complete.emit();\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 6727a777d804..6fb0d5f49b63 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -14,6 +14,7 @@ libcamera_sources = files([\n> > > 'delayed_controls.cpp',\n> > > 'device_enumerator.cpp',\n> > > 'device_enumerator_sysfs.cpp',\n> > > + 'fence.cpp',\n> > > 'file_descriptor.cpp',\n> > > 'formats.cpp',\n> > > 'framebuffer.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 06263BDB1C\n\tfor ;\n\tTue, 9 Nov 2021 20:05:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 364946034E;\n\tTue, 9 Nov 2021 21:05:47 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB545600BF\n\tfor ;\n\tTue, 9 Nov 2021 21:05:45 +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 650B6893;\n\tTue, 9 Nov 2021 21:05:45 +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=\"vTIGNg72\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636488345;\n\tbh=DCsIWTzWCBWv0To43YTCilhNF491+TQQsrDwAXU8Ezg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=vTIGNg72f3Ob8Fgu/wRxuf0NdY1A/MfMC9oyYpRvZFCmSbVPzgJ550KdIN0ItAfv+\n\tNtIqt10kE7IHqETVGnWrC+QN4i0HP5fFCP7yDZwV5GPEd5Y0U5fQ5C1ILCfG5Jczai\n\tVNcpwkjbbg5LoQ/Rv0BfwWRQd0J/lsIZukW3ILeo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211109172231.b5ukhm3yszvmgmf3@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-4-jacopo@jmondi.org>\n\t<163646284310.1606134.5330663146541602267@Monstersaurus>\n\t<20211109172231.b5ukhm3yszvmgmf3@uno.localdomain>","From":"Kieran Bingham ","To":"Jacopo Mondi ","Date":"Tue, 09 Nov 2021 20:05:42 +0000","Message-ID":"<163648834243.1743947.1675499367845725493@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce 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\" "}},{"id":20781,"web_url":"https://patchwork.libcamera.org/comment/20781/","msgid":"","date":"2021-11-10T09:01:33","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 10/28/21 4:45 PM, Jacopo Mondi wrote:\n> Fences are a synchronization mechanism that allows to receive event\n> notifications of a file descriptor with an optional expiration timeout.\n>\n> Introduce the Fence class to model such a mechanism in libcamera.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/fence.h | 64 +++++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/fence.cpp | 185 +++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 251 insertions(+)\n> create mode 100644 include/libcamera/internal/fence.h\n> create mode 100644 src/libcamera/fence.cpp\n>\n> diff --git a/include/libcamera/internal/fence.h b/include/libcamera/internal/fence.h\n> new file mode 100644\n> index 000000000000..5a78e0f864c7\n> --- /dev/null\n> +++ b/include/libcamera/internal/fence.h\n> @@ -0,0 +1,64 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * internal/fence.h - Synchronization fence\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_FENCE_H__\n> +#define __LIBCAMERA_INTERNAL_FENCE_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Fence\n> +{\n> +public:\n> +\texplicit Fence(int fenceFd, unsigned int timeoutMs = kFenceTimeout);\n> +\tFence(Fence &&other);\n> +\n> +\tFence &operator=(Fence &&other);\n> +\n> +\tint fd() const { return fd_.fd(); }\n> +\n> +\tunsigned int timeout() const { return timeout_; }\n> +\tbool completed() const { return completed_; }\n> +\tbool expired() const { return expired_; }\n> +\n> +\tvoid enable();\n> +\tvoid disable();\n> +\n> +\tSignal<> complete;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY(Fence)\n> +\n> +\t/* 300 milliseconds default timeout. */\n> +\tstatic constexpr unsigned int kFenceTimeout = 300;\n> +\n> +\tvoid moveFence(Fence &other);\n> +\n> +\tvoid activated();\n> +\tvoid timedout();\n> +\n> +\tFileDescriptor fd_;\n> +\tEventNotifier notifier_;\n> +\n> +\tTimer timer_;\n> +\tunsigned int timeout_;\n> +\n> +\tbool completed_ = false;\n> +\tbool expired_ = false;\n> +\n> +\tstd::mutex mutex_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */\n> +\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index cae65b0604ff..32d5c3387de3 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -22,6 +22,7 @@ libcamera_internal_headers = files([\n> 'device_enumerator.h',\n> 'device_enumerator_sysfs.h',\n> 'device_enumerator_udev.h',\n> + 'fence.h',\n> 'formats.h',\n> 'framebuffer.h',\n> 'ipa_manager.h',\n> diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp\n> new file mode 100644\n> index 000000000000..8fadb2c21f03\n> --- /dev/null\n> +++ b/src/libcamera/fence.cpp\n> @@ -0,0 +1,185 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * fence.cpp - Synchronization fence\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\file internal/fence.h\n> + * \\brief Synchronization fence\n> + */\n> +\n> +/**\n> + * \\class Fence\n> + * \\brief Synchronization fence class\n> + *\n> + * A Fence is a synchronization mechanism that allows to wait for a read event\n> + * to happen on a file descriptor before an optional timeout expires.\n> + *\n> + * A Fence is created with a default timeout of 300 milliseconds, which can\n> + * be overridden through the 'timeout' constructor parameter. Passing a 0\n> + * timeout to the constructor disables timeouts completely.\n> + *\n> + * A Fence wraps a FileDescriptor and implements a move-only semantic, as Fence\n> + * instances cannot be copied but only moved, causing the moved Fence to be\n> + * reset by invalidating the wrapped FileDescriptor by disabling its\n> + * notification and timeout mechanisms.\n> + *\n> + * When a read event is notified, the Fence is said to be 'completed',\n> + * alternatively if the timeout expires before any event is notified the Fence\n> + * is said to be 'expired'.\n> + *\n> + * In both cases, when an event notification happens or a timeout expires, the\n> + * class emits the 'complete' signal, to which users of the class can connect\n> + * to and check if the Fence has completed or has expired by calling the\n> + * completed() and expired() functions.\n\n\nIf this is the case, I would expect a value emitted as a signal \nparameter, to know whether it's completed or expired\n\n> + *\n> + * Fence instances are non-active by default (ie no events or timeout are\n> + * generated) and need to be enabled by calling the enable() function. Likewise\n\n\nIts inactive both when \"constructed\" and \"moved\" right ? It seems so, as \nper my reading below\n\n> + * a Fence can be disabled by calling the disable() function.\n> + *\n> + * After a Fence has expired no events are generated and the Fence need to be\n> + * manually re-enabled. Likewise, if the Fence is signalled the expiration\n> + * timeout is cancelled.\n> + */\n> +\n> +/**\n> + * \\brief Create a synchronization fence\n> + * \\param[in] fenceFd The synchronization fence file descriptor\n> + * \\param[in] timeoutMs The optional fence timeout. Set to 0 to disable timeout\n> + */\n> +Fence::Fence(int fenceFd, unsigned int timeoutMs)\n> +\t: fd_(std::move(fenceFd)),\n> +\t notifier_(fd_.fd(), EventNotifier::Read, nullptr, false),\n> +\t timeout_(timeoutMs)\n> +{\n> +\tnotifier_.activated.connect(this, &Fence::activated);\n> +\n> +\ttimer_.timeout.connect(this, &Fence::timedout);\n> +}\n> +\n> +void Fence::moveFence(Fence &other)\n> +{\n> +\tfd_ = std::move(other.fd_);\n> +\n> +\tother.disable();\n> +\tif (other.timer_.isRunning())\n> +\t\tother.timer_.stop();\n> +\tother.timeout_ = 0;\n> +}\n> +\n> +/**\n> + * \\brief Move-construct a Fence\n> + * \\param[in] other The other fence to move\n> + */\n> +Fence::Fence(Fence &&other)\n> +\t: notifier_(other.fd(), EventNotifier::Read, nullptr, false)\n> +{\n> +\tmoveFence(other);\n> +\n> +\tnotifier_.activated.connect(this, &Fence::activated);\n> +\ttimer_.timeout.connect(this, &Fence::timedout);\n\n\nI think Kieran has rightly pointed out in his replies to disconnect \nthese from the 'other' Fence too.\n\n> +}\n> +\n> +/**\n> + * \\brief Move-assign the value of the \\a other fence to this\n> + * \\param[in] other The other fence to move\n> + * \\return This fence\n> + */\n> +Fence &Fence::operator=(Fence &&other)\n> +{\n> +\tmoveFence(other);\n> +\n> +\tnotifier_ = EventNotifier(fd_.fd(), EventNotifier::Read, nullptr, false);\n> +\n> +\treturn *this;\n> +}\n\n\nOne fundamental question I am pondering is why a Fence needs to be moved \nafter constructed. Hmm. I think I have a broad idea, that it happens \nwhile the iteration between FrameBuffer and Request class in subsequent \npatches, I'll try to keep on reading for exact answers.\n\n> +\n> +/**\n> + * \\fn Fence::fd() const\n> + * \\brief Return the synchronization fence file descriptor\n> + * \\return The synchronization fence file descriptor\n> + */\n> +\n> +/**\n> + * \\fn Fence::timeout()\n> + * \\brief Retrieve the fence timeout\n> + * \\return The fence timeout in milliseconds\n> + */\n> +\n> +/**\n> + * \\fn Fence::completed()\n> + * \\brief Check if the fence has completed before timing out\n> + * \\return True if the fence has completed\n> + */\n> +\n> +/**\n> + * \\fn Fence::expired()\n> + * \\brief Check if the fence has expired before completing\n> + * \\return True if the fence has expired\n> + */\n> +\n> +/**\n> + * \\brief Enable a fence by enabling events notifications\n> + */\n> +void Fence::enable()\n> +{\n> +\tMutexLocker lock(mutex_);\n> +\n> +\texpired_ = false;\n> +\tcompleted_ = false;\n> +\n> +\tnotifier_.setEnabled(true);\n> +\tif (timeout_)\n> +\t\ttimer_.start(timeout_);\n> +}\n> +\n> +/**\n> + * \\brief Disable a fence by disabling events notifications\n> + */\n> +void Fence::disable()\n> +{\n> +\tnotifier_.setEnabled(false);\n> +}\n> +\n> +/**\n> + * \\var Fence::complete\n> + * \\brief The fence completion signal\n> + *\n> + * A Fence is completed if signalled or timeout.\n> + */\n\nYep, I think it can emit with a value denoting whether it's a timeout or \nactivated\n\n> +\n> +void Fence::activated()\n> +{\n> +\tMutexLocker lock(mutex_);\n> +\n> +\tif (timer_.isRunning())\n> +\t\ttimer_.stop();\n> +\n> +\tcompleted_ = true;\n> +\texpired_ = false;\n> +\n> +\tcomplete.emit();\n> +}\n> +\n> +void Fence::timedout()\n> +{\n> +\tMutexLocker lock(mutex_);\n> +\n> +\texpired_ = true;\n> +\tcompleted_ = false;\n> +\n> +\t/* Disable events notification after timeout. */\n> +\tdisable();\n> +\n> +\tcomplete.emit();\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 6727a777d804..6fb0d5f49b63 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -14,6 +14,7 @@ libcamera_sources = files([\n> 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> + 'fence.cpp',\n> 'file_descriptor.cpp',\n> 'formats.cpp',\n> 'framebuffer.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 2E5A5BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 09:01:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E36D6034A;\n\tWed, 10 Nov 2021 10:01:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28FC860128\n\tfor ;\n\tWed, 10 Nov 2021 10:01:38 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 56F0ED8B;\n\tWed, 10 Nov 2021 10:01: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=\"BQp7gUTv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636534897;\n\tbh=2sqcq7bVHtJGh7x+PPby5O7SFLYxBSYNhaLe11gv84M=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=BQp7gUTvrp2oZuBGnjuO4WiNMgZ0Llv13n9syBYoMooqTUz66hJ3q75Gz0X7j/0Sy\n\tyBsA7MmW6A1VWhefRYMajFP4aLYliGbnNnCBgYEtDZnrDGJEwAxkkCKSDSbP00sBTi\n\t0HPWP3bdv+pBsOy/kfIiXoApOtk1NRcWxX+cT69g=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-4-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 10 Nov 2021 14:31:33 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211028111520.256612-4-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20794,"web_url":"https://patchwork.libcamera.org/comment/20794/","msgid":"<163654107637.1896795.10918452126784754005@Monstersaurus>","date":"2021-11-10T10:44:36","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-11-09 17:24:42)\n> Hi Kieran\n> \n> On Tue, Nov 09, 2021 at 01:39:58PM +0000, Kieran Bingham wrote:\n> > Quoting Kieran Bingham (2021-11-09 13:00:43)\n> > > Quoting Jacopo Mondi (2021-10-28 12:15:13)\n> \n> [snip]\n> \n> > > > +void Fence::moveFence(Fence &other)\n> > >\n> > > I see now that the text above really did mean moving a fence invalidates\n> > > it, but I'm not sure how that use case comes about.\n> > >\n> > > Could this funtion perhaps document /why/ moving a fence necessitates\n> > > invalidating it, and why it moves the internal fd to preserve it's\n> > > lifetime - but - that the fence can no longer be used... (or something\n> > > like that if I've inferred correctly).\n> > >\n> > >\n> > > > +{\n> > > > + fd_ = std::move(other.fd_);\n> > > > +\n> > >\n> > > There might be code connected to the complete signal of the 'other'\n> > > fence. Should they be moved over to this? or should we explictly state\n> > > that listeners to that completion event are 'disconnected' (somewhat\n> > > implicitly).\n> > >\n> > > Perhaps we should explictily either disconnect or move them in the\n> > > code...?\n> > >\n> > > > + other.disable();\n> > > > + if (other.timer_.isRunning())\n> > > > + other.timer_.stop();\n> >\n> > also - in this case here, the other timer is still running, so there is\n> > perhaps expected to be someone listing waiting for either a completed\n> > fence notification or a timeout. Does calling .stop() - activate the\n> > signal to signal a timeout? If not - perhaps we should explicitly /\n> > manually do so, and generate a timeout on it?\n> >\n> \n> afaict Timer::stop() does not generate a timeout and I'm not sure\n> generating an event on the moved Fence is the right thing to do. How\n> is it the slot supposed to know if a real timeout has occurred ?\n\nI'm weary that I may be into hypotheticals of situations that won't\noccur, but I'm trying to look at it from how the class is designed and\ntherefore /could/ be used.\n\nMy concern is that you are indicating in the code that we are moving a\nFence which has a /running/ timer set on it.\n\nWe are 'sweeping' the rug out from under the listener, whomever that may\nbe. They are expecting either a fence completion event, or a timeout -\nbut the move operation is cancelling both of those (and we're about to\ndisconnect the signal entirely too I expect).\n\nSo I think the listener should be told there is a timeout at least.\n\nIf this is unexpected behaviour, and moving a Fence which has an active\nrunning timer shouldn't happen - then we either need to ASSERT() or\nLOG(Error) on it here in Fence::moveFence() and make sure that expected\nbehaviour is documented.\n\n\n> > > Should you move the timeout_ value over to this one too?\n> > >\n> > > > + other.timeout_ = 0;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Move-construct a Fence\n> > > > + * \\param[in] other The other fence to move\n> > >\n> > > Ok things are becoming clearer. Perhaps:\n> > >\n> > > *\n> > > * The other fence is disabled, and invalidated while the underlying\n> > > * FileDescriptor is moved to this Fence. The newly moved fence can be\n> > > * re-used when required.\n> > >\n> > > > + */\n> > > > +Fence::Fence(Fence &&other)\n> > > > + : notifier_(other.fd(), EventNotifier::Read, nullptr, false)\n> > > > +{\n> > > > + moveFence(other);\n> > > > +\n> > > > + notifier_.activated.connect(this, &Fence::activated);\n> > > > + timer_.timeout.connect(this, &Fence::timedout);\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Move-assign the value of the \\a other fence to this\n> > > > + * \\param[in] other The other fence to move\n> > > > + * \\return This fence\n> > >\n> > > Something similar to the above addition maybe...\n> > >\n> > >\n> > > > + */\n> > > > +Fence &Fence::operator=(Fence &&other)\n> > > > +{\n> > > > + moveFence(other);\n> > > > +\n> > > > + notifier_ = EventNotifier(fd_.fd(), EventNotifier::Read, nullptr, false);\n> > > > +\n> > > > + return *this;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\fn Fence::fd() const\n> > > > + * \\brief Return the synchronization fence file descriptor\n> > > > + * \\return The synchronization fence file descriptor\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Fence::timeout()\n> > > > + * \\brief Retrieve the fence timeout\n> > > > + * \\return The fence timeout in milliseconds\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Fence::completed()\n> > > > + * \\brief Check if the fence has completed before timing out\n> > > > + * \\return True if the fence has completed\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Fence::expired()\n> > > > + * \\brief Check if the fence has expired before completing\n> > > > + * \\return True if the fence has expired\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Enable a fence by enabling events notifications\n> > > > + */\n> > > > +void Fence::enable()\n> > > > +{\n> > > > + MutexLocker lock(mutex_);\n> > > > +\n> > > > + expired_ = false;\n> > > > + completed_ = false;\n> > > > +\n> > > > + notifier_.setEnabled(true);\n> > > > + if (timeout_)\n> > > > + timer_.start(timeout_);\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Disable a fence by disabling events notifications\n> > > > + */\n> > > > +void Fence::disable()\n> > > > +{\n> > > > + notifier_.setEnabled(false);\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\var Fence::complete\n> > > > + * \\brief The fence completion signal\n> > > > + *\n> > > > + * A Fence is completed if signalled or timeout.\n> > >\n> > > A Fence is completed when signalled or if a timeout occurs.\n> > >\n> > > > + */\n> > > > +\n> > > > +void Fence::activated()\n> > > > +{\n> > > > + MutexLocker lock(mutex_);\n> > > > +\n> > > > + if (timer_.isRunning())\n> > > > + timer_.stop();\n> > > > +\n> > > > + completed_ = true;\n> > > > + expired_ = false;\n> > > > +\n> > > > + complete.emit();\n> > >\n> > > Passing the expiration state as a boolean here might help the receiver\n> > > know why it completed.\n> > >\n> > > But maybe that's optional - or better handled as you have it anyway.\n> > >\n> > > > +}\n> > > > +\n> > > > +void Fence::timedout()\n> > > > +{\n> > > > + MutexLocker lock(mutex_);\n> > > > +\n> > > > + expired_ = true;\n> > > > + completed_ = false;\n> > > > +\n> > > > + /* Disable events notification after timeout. */\n> > > > + disable();\n> > > > +\n> > > > + complete.emit();\n> > > > +}\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > index 6727a777d804..6fb0d5f49b63 100644\n> > > > --- a/src/libcamera/meson.build\n> > > > +++ b/src/libcamera/meson.build\n> > > > @@ -14,6 +14,7 @@ libcamera_sources = files([\n> > > > 'delayed_controls.cpp',\n> > > > 'device_enumerator.cpp',\n> > > > 'device_enumerator_sysfs.cpp',\n> > > > + 'fence.cpp',\n> > > > 'file_descriptor.cpp',\n> > > > 'formats.cpp',\n> > > > 'framebuffer.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 8A445BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 10:44:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B8F460128;\n\tWed, 10 Nov 2021 11:44:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2199760128\n\tfor ;\n\tWed, 10 Nov 2021 11:44:39 +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 B832BD8B;\n\tWed, 10 Nov 2021 11:44:38 +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=\"D5R+lxiv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636541078;\n\tbh=xram9NSHNcehnwBZy/MJqnhjxgtKvYbJeSFF39cYcr8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=D5R+lxivl1srGTvTB+VTylGoKY3rG9smu5zqhcCiB4VY/Hfm7AuiNIl67JhQpMY/Y\n\t54Z/CJOw0je+eBwvmuwh82NbFv98vG1oSavDCVnHHwN0lsFyMGQvPJ09l+hs3NAKCx\n\tSUQY515lhUMOL3XlMYYYhTQ+UaRwHCshSgentbP4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211109172442.kwyt77t4sw3adph4@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-4-jacopo@jmondi.org>\n\t<163646284310.1606134.5330663146541602267@Monstersaurus>\n\t<163646519896.1606134.1056409825924975876@Monstersaurus>\n\t<20211109172442.kwyt77t4sw3adph4@uno.localdomain>","From":"Kieran Bingham ","To":"Jacopo Mondi ","Date":"Wed, 10 Nov 2021 10:44:36 +0000","Message-ID":"<163654107637.1896795.10918452126784754005@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce 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\" "}},{"id":20915,"web_url":"https://patchwork.libcamera.org/comment/20915/","msgid":"","date":"2021-11-12T13:22:16","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Nov 10, 2021 at 10:44:36AM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2021-11-09 17:24:42)\n> > Hi Kieran\n> > \n> > On Tue, Nov 09, 2021 at 01:39:58PM +0000, Kieran Bingham wrote:\n> > > Quoting Kieran Bingham (2021-11-09 13:00:43)\n> > > > Quoting Jacopo Mondi (2021-10-28 12:15:13)\n> > \n> > [snip]\n> > \n> > > > > +void Fence::moveFence(Fence &other)\n> > > >\n> > > > I see now that the text above really did mean moving a fence invalidates\n> > > > it, but I'm not sure how that use case comes about.\n> > > >\n> > > > Could this funtion perhaps document /why/ moving a fence necessitates\n> > > > invalidating it, and why it moves the internal fd to preserve it's\n> > > > lifetime - but - that the fence can no longer be used... (or something\n> > > > like that if I've inferred correctly).\n> > > >\n> > > >\n> > > > > +{\n> > > > > + fd_ = std::move(other.fd_);\n> > > > > +\n> > > >\n> > > > There might be code connected to the complete signal of the 'other'\n> > > > fence. Should they be moved over to this? or should we explictly state\n> > > > that listeners to that completion event are 'disconnected' (somewhat\n> > > > implicitly).\n> > > >\n> > > > Perhaps we should explictily either disconnect or move them in the\n> > > > code...?\n> > > >\n> > > > > + other.disable();\n> > > > > + if (other.timer_.isRunning())\n> > > > > + other.timer_.stop();\n> > >\n> > > also - in this case here, the other timer is still running, so there is\n> > > perhaps expected to be someone listing waiting for either a completed\n> > > fence notification or a timeout. Does calling .stop() - activate the\n> > > signal to signal a timeout? If not - perhaps we should explicitly /\n> > > manually do so, and generate a timeout on it?\n> > \n> > afaict Timer::stop() does not generate a timeout and I'm not sure\n> > generating an event on the moved Fence is the right thing to do. How\n> > is it the slot supposed to know if a real timeout has occurred ?\n> \n> I'm weary that I may be into hypotheticals of situations that won't\n> occur, but I'm trying to look at it from how the class is designed and\n> therefore /could/ be used.\n> \n> My concern is that you are indicating in the code that we are moving a\n> Fence which has a /running/ timer set on it.\n> \n> We are 'sweeping' the rug out from under the listener, whomever that may\n> be. They are expecting either a fence completion event, or a timeout -\n> but the move operation is cancelling both of those (and we're about to\n> disconnect the signal entirely too I expect).\n> \n> So I think the listener should be told there is a timeout at least.\n> \n> If this is unexpected behaviour, and moving a Fence which has an active\n> running timer shouldn't happen - then we either need to ASSERT() or\n> LOG(Error) on it here in Fence::moveFence() and make sure that expected\n> behaviour is documented.\n\nSetting the cat among the pigeons, I'd like to propose making fences\nnon-movable, and passing them by (unique where appropriate) pointer.\n\n> > > > Should you move the timeout_ value over to this one too?\n> > > >\n> > > > > + other.timeout_ = 0;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Move-construct a Fence\n> > > > > + * \\param[in] other The other fence to move\n> > > >\n> > > > Ok things are becoming clearer. Perhaps:\n> > > >\n> > > > *\n> > > > * The other fence is disabled, and invalidated while the underlying\n> > > > * FileDescriptor is moved to this Fence. The newly moved fence can be\n> > > > * re-used when required.\n> > > >\n> > > > > + */\n> > > > > +Fence::Fence(Fence &&other)\n> > > > > + : notifier_(other.fd(), EventNotifier::Read, nullptr, false)\n> > > > > +{\n> > > > > + moveFence(other);\n> > > > > +\n> > > > > + notifier_.activated.connect(this, &Fence::activated);\n> > > > > + timer_.timeout.connect(this, &Fence::timedout);\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Move-assign the value of the \\a other fence to this\n> > > > > + * \\param[in] other The other fence to move\n> > > > > + * \\return This fence\n> > > >\n> > > > Something similar to the above addition maybe...\n> > > >\n> > > > > + */\n> > > > > +Fence &Fence::operator=(Fence &&other)\n> > > > > +{\n> > > > > + moveFence(other);\n> > > > > +\n> > > > > + notifier_ = EventNotifier(fd_.fd(), EventNotifier::Read, nullptr, false);\n> > > > > +\n> > > > > + return *this;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn Fence::fd() const\n> > > > > + * \\brief Return the synchronization fence file descriptor\n> > > > > + * \\return The synchronization fence file descriptor\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn Fence::timeout()\n> > > > > + * \\brief Retrieve the fence timeout\n> > > > > + * \\return The fence timeout in milliseconds\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn Fence::completed()\n> > > > > + * \\brief Check if the fence has completed before timing out\n> > > > > + * \\return True if the fence has completed\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn Fence::expired()\n> > > > > + * \\brief Check if the fence has expired before completing\n> > > > > + * \\return True if the fence has expired\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Enable a fence by enabling events notifications\n> > > > > + */\n> > > > > +void Fence::enable()\n> > > > > +{\n> > > > > + MutexLocker lock(mutex_);\n> > > > > +\n> > > > > + expired_ = false;\n> > > > > + completed_ = false;\n> > > > > +\n> > > > > + notifier_.setEnabled(true);\n> > > > > + if (timeout_)\n> > > > > + timer_.start(timeout_);\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Disable a fence by disabling events notifications\n> > > > > + */\n> > > > > +void Fence::disable()\n> > > > > +{\n> > > > > + notifier_.setEnabled(false);\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\var Fence::complete\n> > > > > + * \\brief The fence completion signal\n> > > > > + *\n> > > > > + * A Fence is completed if signalled or timeout.\n> > > >\n> > > > A Fence is completed when signalled or if a timeout occurs.\n> > > >\n> > > > > + */\n> > > > > +\n> > > > > +void Fence::activated()\n> > > > > +{\n> > > > > + MutexLocker lock(mutex_);\n> > > > > +\n> > > > > + if (timer_.isRunning())\n> > > > > + timer_.stop();\n> > > > > +\n> > > > > + completed_ = true;\n> > > > > + expired_ = false;\n> > > > > +\n> > > > > + complete.emit();\n> > > >\n> > > > Passing the expiration state as a boolean here might help the receiver\n> > > > know why it completed.\n> > > >\n> > > > But maybe that's optional - or better handled as you have it anyway.\n> > > >\n> > > > > +}\n> > > > > +\n> > > > > +void Fence::timedout()\n> > > > > +{\n> > > > > + MutexLocker lock(mutex_);\n> > > > > +\n> > > > > + expired_ = true;\n> > > > > + completed_ = false;\n> > > > > +\n> > > > > + /* Disable events notification after timeout. */\n> > > > > + disable();\n> > > > > +\n> > > > > + complete.emit();\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > > index 6727a777d804..6fb0d5f49b63 100644\n> > > > > --- a/src/libcamera/meson.build\n> > > > > +++ b/src/libcamera/meson.build\n> > > > > @@ -14,6 +14,7 @@ libcamera_sources = files([\n> > > > > 'delayed_controls.cpp',\n> > > > > 'device_enumerator.cpp',\n> > > > > 'device_enumerator_sysfs.cpp',\n> > > > > + 'fence.cpp',\n> > > > > 'file_descriptor.cpp',\n> > > > > 'formats.cpp',\n> > > > > 'framebuffer.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 D99E7BF415\n\tfor ;\n\tFri, 12 Nov 2021 13:22:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7699C60369;\n\tFri, 12 Nov 2021 14:22:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A4696032C\n\tfor ;\n\tFri, 12 Nov 2021 14:22:38 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B41C74C;\n\tFri, 12 Nov 2021 14:22:38 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"U7j/3n/C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636723358;\n\tbh=F0lmF4G6hKlr15cuHOBmW1ltXkezLUUbG+0svxhSMKI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U7j/3n/CovjmBbmW+m622MLzZ4ACy8Jv1ORu2r9Nyl7iFdPukYxDj8YlATYNyrDgd\n\tDExH2spNi6Z2pvfEOnYsGfyoEOCQZh5wQ1IeCWag2bl5+5quj8Fu3iQ4y0kv/xFqlq\n\tln4yghgGdOtA5f8s3NWCo0iARl8EPti7r/y9PfhY=","Date":"Fri, 12 Nov 2021 15:22:16 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-4-jacopo@jmondi.org>\n\t<163646284310.1606134.5330663146541602267@Monstersaurus>\n\t<163646519896.1606134.1056409825924975876@Monstersaurus>\n\t<20211109172442.kwyt77t4sw3adph4@uno.localdomain>\n\t<163654107637.1896795.10918452126784754005@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163654107637.1896795.10918452126784754005@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce 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\" "}},{"id":20916,"web_url":"https://patchwork.libcamera.org/comment/20916/","msgid":"","date":"2021-11-12T15:02:21","subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce 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 Thu, Oct 28, 2021 at 01:15:13PM +0200, Jacopo Mondi wrote:\n> Fences are a synchronization mechanism that allows to receive event\n> notifications of a file descriptor with an optional expiration timeout.\n> \n> Introduce the Fence class to model such a mechanism in libcamera.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/fence.h | 64 +++++++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/fence.cpp | 185 +++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 251 insertions(+)\n> create mode 100644 include/libcamera/internal/fence.h\n> create mode 100644 src/libcamera/fence.cpp\n> \n> diff --git a/include/libcamera/internal/fence.h b/include/libcamera/internal/fence.h\n> new file mode 100644\n> index 000000000000..5a78e0f864c7\n> --- /dev/null\n> +++ b/include/libcamera/internal/fence.h\n> @@ -0,0 +1,64 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * internal/fence.h - Synchronization fence\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_FENCE_H__\n> +#define __LIBCAMERA_INTERNAL_FENCE_H__\n\nThe series hides the Fence class, and exposes an fd in the public API\nwhen constructing a FrameBuffer. I'm wondering if it wouldn't be better\nto expose a Fence class in the public API. This would decouple the\nplatform fence implementation from the FrameBuffer class. Among the\nadvantages I can see, the FrameBuffer constructor won't have to deal\nwith int fds, making the ownership semantics clearer. It could also help\nsupporting different types of fences in the future by inheriting from\nthe Fence class, without changing the public API of the FrameBuffer\nclass.\n\nWe would need to make the Fence class Extensible, as most of the\nimplementation shouldn't be visible to applications.\n\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Fence\n> +{\n> +public:\n> +\texplicit Fence(int fenceFd, unsigned int timeoutMs = kFenceTimeout);\n\nCan we pass a FileDescriptor for the fd, to avoid having to define\nfd-handling semantics (borrow vs. acquire vs. dup) in the Fence API ?\n\nI don't think the timeout belongs here. The Fence class should model a\nfence (also called sync object in other APIs, such as OpenGL (ES)). It\nshould likely offer functions to help waiting on the fence (probably as\npart of the non-public API implemented in Fence::Private to start with,\nbut I wouldn't be surprised if there was a need to implement out fences\nin the future too), but the wait should be decoupled from fence\nconstruction. The timeout should be passed to wait-related functions (if\nany).\n\nRegarding timeouts, as Kieran mentioned, let's use std::chrono types.\n\n> +\tFence(Fence &&other);\n> +\n> +\tFence &operator=(Fence &&other);\n> +\n> +\tint fd() const { return fd_.fd(); }\n> +\n> +\tunsigned int timeout() const { return timeout_; }\n> +\tbool completed() const { return completed_; }\n> +\tbool expired() const { return expired_; }\n> +\n> +\tvoid enable();\n> +\tvoid disable();\n> +\n> +\tSignal<> complete;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY(Fence)\n> +\n> +\t/* 300 milliseconds default timeout. */\n> +\tstatic constexpr unsigned int kFenceTimeout = 300;\n> +\n> +\tvoid moveFence(Fence &other);\n> +\n> +\tvoid activated();\n> +\tvoid timedout();\n> +\n> +\tFileDescriptor fd_;\n> +\tEventNotifier notifier_;\n> +\n> +\tTimer timer_;\n> +\tunsigned int timeout_;\n> +\n> +\tbool completed_ = false;\n> +\tbool expired_ = false;\n> +\n> +\tstd::mutex mutex_;\n\nA comment to tell what the lock protects (until thread-safety\nannotations land in) would be useful.\n\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */\n> +\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index cae65b0604ff..32d5c3387de3 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -22,6 +22,7 @@ libcamera_internal_headers = files([\n> 'device_enumerator.h',\n> 'device_enumerator_sysfs.h',\n> 'device_enumerator_udev.h',\n> + 'fence.h',\n> 'formats.h',\n> 'framebuffer.h',\n> 'ipa_manager.h',\n> diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp\n> new file mode 100644\n> index 000000000000..8fadb2c21f03\n> --- /dev/null\n> +++ b/src/libcamera/fence.cpp\n> @@ -0,0 +1,185 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * fence.cpp - Synchronization fence\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\file internal/fence.h\n> + * \\brief Synchronization fence\n> + */\n> +\n> +/**\n> + * \\class Fence\n> + * \\brief Synchronization fence class\n> + *\n> + * A Fence is a synchronization mechanism that allows to wait for a read event\n> + * to happen on a file descriptor before an optional timeout expires.\n\nYou may want to read\nhttps://www.khronos.org/registry/EGL/extensions/KHR/EGL_KHR_fence_sync.txt\nfor a description of how fences can be defined. I don't think we should\nmention read events anywhere, that's an implementation detail of the\ncorresponding kernel API. I'd reuse the \"fences are signaled\" and\n\"completion\" terminology.\n\n> + *\n> + * A Fence is created with a default timeout of 300 milliseconds, which can\n> + * be overridden through the 'timeout' constructor parameter. Passing a 0\n> + * timeout to the constructor disables timeouts completely.\n> + *\n> + * A Fence wraps a FileDescriptor and implements a move-only semantic, as Fence\n> + * instances cannot be copied but only moved, causing the moved Fence to be\n> + * reset by invalidating the wrapped FileDescriptor by disabling its\n> + * notification and timeout mechanisms.\n\nI wonder if it wouldn't be better to disable move operations too, and\npass fences as (unique where appropriate) pointers. That would simplify\nthe semantics of the class. I haven't checked in details if there would\nbe any drawback in doing so.\n\n> + *\n> + * When a read event is notified, the Fence is said to be 'completed',\n> + * alternatively if the timeout expires before any event is notified the Fence\n> + * is said to be 'expired'.\n> + *\n> + * In both cases, when an event notification happens or a timeout expires, the\n> + * class emits the 'complete' signal, to which users of the class can connect\n> + * to and check if the Fence has completed or has expired by calling the\n> + * completed() and expired() functions.\n> + *\n> + * Fence instances are non-active by default (ie no events or timeout are\n> + * generated) and need to be enabled by calling the enable() function. Likewise\n> + * a Fence can be disabled by calling the disable() function.\n\nI'm tempted to remove timeout handling from the Fence class. In the\nfirst use case we have to implement, the timeout applies to a group of\nfences, as we want to queue the request to the pipeline handler only\nafter all fences have been signalled. There's thus no need to have a\ntimeout per fence, only a global timeout at the request level. Storing\nthe Timer in the PipelineHandler base class would be the best option if\npossible.\n\nIt could be argued that the notifier could also be moved out of the\nFence class, but as it's needed in our most common use case for all\nfences, it makes sense to have it as a helper in Fence::Private.\n\n> + *\n> + * After a Fence has expired no events are generated and the Fence need to be\n> + * manually re-enabled. Likewise, if the Fence is signalled the expiration\n> + * timeout is cancelled.\n> + */\n> +\n> +/**\n> + * \\brief Create a synchronization fence\n> + * \\param[in] fenceFd The synchronization fence file descriptor\n> + * \\param[in] timeoutMs The optional fence timeout. Set to 0 to disable timeout\n> + */\n> +Fence::Fence(int fenceFd, unsigned int timeoutMs)\n> +\t: fd_(std::move(fenceFd)),\n> +\t notifier_(fd_.fd(), EventNotifier::Read, nullptr, false),\n> +\t timeout_(timeoutMs)\n> +{\n> +\tnotifier_.activated.connect(this, &Fence::activated);\n> +\n> +\ttimer_.timeout.connect(this, &Fence::timedout);\n> +}\n> +\n> +void Fence::moveFence(Fence &other)\n> +{\n> +\tfd_ = std::move(other.fd_);\n> +\n> +\tother.disable();\n> +\tif (other.timer_.isRunning())\n> +\t\tother.timer_.stop();\n> +\tother.timeout_ = 0;\n> +}\n> +\n> +/**\n> + * \\brief Move-construct a Fence\n> + * \\param[in] other The other fence to move\n> + */\n> +Fence::Fence(Fence &&other)\n> +\t: notifier_(other.fd(), EventNotifier::Read, nullptr, false)\n> +{\n> +\tmoveFence(other);\n> +\n> +\tnotifier_.activated.connect(this, &Fence::activated);\n> +\ttimer_.timeout.connect(this, &Fence::timedout);\n> +}\n> +\n> +/**\n> + * \\brief Move-assign the value of the \\a other fence to this\n> + * \\param[in] other The other fence to move\n> + * \\return This fence\n> + */\n> +Fence &Fence::operator=(Fence &&other)\n> +{\n> +\tmoveFence(other);\n> +\n> +\tnotifier_ = EventNotifier(fd_.fd(), EventNotifier::Read, nullptr, false);\n> +\n> +\treturn *this;\n> +}\n> +\n> +/**\n> + * \\fn Fence::fd() const\n> + * \\brief Return the synchronization fence file descriptor\n> + * \\return The synchronization fence file descriptor\n> + */\n\nBeside unit tests, this function is only used by the FrameBuffer class\nto implement the fence() function, which in turn is only used in\nPipelineHandler::queueRequest() to test if the framebuffer has a fence\nattached to it. I'd drop Fence::fd() (until we need it in pipeline\nhandlers, when V4L2 will support fences natively), add a\nFence::isValid(), and use isValid() in PipelineHandler::queueRequest().\n\n> +\n> +/**\n> + * \\fn Fence::timeout()\n> + * \\brief Retrieve the fence timeout\n> + * \\return The fence timeout in milliseconds\n> + */\n> +\n> +/**\n> + * \\fn Fence::completed()\n> + * \\brief Check if the fence has completed before timing out\n> + * \\return True if the fence has completed\n> + */\n> +\n> +/**\n> + * \\fn Fence::expired()\n> + * \\brief Check if the fence has expired before completing\n> + * \\return True if the fence has expired\n> + */\n> +\n> +/**\n> + * \\brief Enable a fence by enabling events notifications\n> + */\n> +void Fence::enable()\n\nIt's not the fence that is enabled or disabled, but the notification.\nI'd name the function enableNotification() (better names probably\nexist).\n\n> +{\n> +\tMutexLocker lock(mutex_);\n> +\n> +\texpired_ = false;\n> +\tcompleted_ = false;\n> +\n> +\tnotifier_.setEnabled(true);\n> +\tif (timeout_)\n> +\t\ttimer_.start(timeout_);\n> +}\n> +\n> +/**\n> + * \\brief Disable a fence by disabling events notifications\n> + */\n> +void Fence::disable()\n> +{\n> +\tnotifier_.setEnabled(false);\n> +}\n> +\n> +/**\n> + * \\var Fence::complete\n> + * \\brief The fence completion signal\n> + *\n> + * A Fence is completed if signalled or timeout.\n> + */\n> +\n> +void Fence::activated()\n> +{\n> +\tMutexLocker lock(mutex_);\n> +\n> +\tif (timer_.isRunning())\n> +\t\ttimer_.stop();\n> +\n> +\tcompleted_ = true;\n> +\texpired_ = false;\n> +\n> +\tcomplete.emit();\n> +}\n> +\n> +void Fence::timedout()\n> +{\n> +\tMutexLocker lock(mutex_);\n> +\n> +\texpired_ = true;\n> +\tcompleted_ = false;\n> +\n> +\t/* Disable events notification after timeout. */\n> +\tdisable();\n> +\n> +\tcomplete.emit();\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 6727a777d804..6fb0d5f49b63 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -14,6 +14,7 @@ libcamera_sources = files([\n> 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> + 'fence.cpp',\n> 'file_descriptor.cpp',\n> 'formats.cpp',\n> 'framebuffer.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 5AA41BDB1C\n\tfor ;\n\tFri, 12 Nov 2021 15:02:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D92A6036E;\n\tFri, 12 Nov 2021 16:02:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 284356032C\n\tfor ;\n\tFri, 12 Nov 2021 16:02:44 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A543C74C;\n\tFri, 12 Nov 2021 16:02:43 +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=\"kZe5tpf4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636729363;\n\tbh=VtnyfIGdFPiABeVd/DqVxs0d0/yuaFEn6otwr9qSA40=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kZe5tpf4GHsiYKPJ9XCPY4vIZxVDjAmIKqsydWKBsC6uElxQD1bi4/LApbXdbEQVa\n\tBgdq+SIVA1khYrjxFO1xwpIdpuPt4FW/CciPXL/ogJoYc9fPl/MW5o7qwl8uXTYROb\n\tiR8Sd1uy5Tz3/TJjXDXDwA9pyw1GUsPWO7L5HKsc=","Date":"Fri, 12 Nov 2021 17:02:21 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211028111520.256612-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 03/10] libcamera: Introduce 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\" "}}]