[{"id":21072,"web_url":"https://patchwork.libcamera.org/comment/21072/","msgid":"","date":"2021-11-21T16:51:52","subject":"Re: [libcamera-devel] [PATCH v2 02/12] libcamera: fence: Introduce\n\tFence","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Sat, Nov 20, 2021 at 12:13:03PM +0100, Jacopo Mondi wrote:\n> Introduce a Fence class which models a synchronization primitive that\n> allows to notify the availability of a resource.\n> \n> The Fence is modeled as a wrapper of a FileDescriptor instance where\n> read events are used to signal the Fence. The class can be later\n> extended to support additional signalling mechanisms.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/fence.h | 36 +++++++++\n> include/libcamera/internal/fence.h | 37 +++++++++\n> include/libcamera/internal/meson.build | 1 +\n> include/libcamera/meson.build | 1 +\n> src/libcamera/fence.cpp | 104 +++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 6 files changed, 180 insertions(+)\n> create mode 100644 include/libcamera/fence.h\n> create mode 100644 include/libcamera/internal/fence.h\n> create mode 100644 src/libcamera/fence.cpp\n> \n> diff --git a/include/libcamera/fence.h b/include/libcamera/fence.h\n> new file mode 100644\n> index 000000000000..5ae0ff6208d7\n> --- /dev/null\n> +++ b/include/libcamera/fence.h\n> @@ -0,0 +1,36 @@\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_FENCE_H__\n> +#define __LIBCAMERA_FENCE_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \n\nYou could use a forward declaration for FileDescriptor.\n\n> +\n> +namespace libcamera {\n> +\n> +class Fence : public Extensible\n> +{\n> +\tLIBCAMERA_DECLARE_PRIVATE()\n> +\n> +public:\n> +\tFence(const FileDescriptor &fd);\n> +\n> +\tbool isValid() const;\n> +\tconst FileDescriptor &fd();\n> +\n> +\tvoid close();\n\nThis function is only used in test/fence.cpp.\n\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(Fence)\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_FENCE_H__ */\n> diff --git a/include/libcamera/internal/fence.h b/include/libcamera/internal/fence.h\n> new file mode 100644\n> index 000000000000..5f03c0804d44\n> --- /dev/null\n> +++ b/include/libcamera/internal/fence.h\n> @@ -0,0 +1,37 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * internal/fence.h - Internal synchronization fence\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_FENCE_H__\n> +#define __LIBCAMERA_INTERNAL_FENCE_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Fence::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(Fence)\n> +\n> +public:\n> +\tPrivate(const FileDescriptor &fd);\n> +\n> +\tbool isValid() const { return fd_.isValid(); }\n> +\tconst FileDescriptor &fd() { return fd_; }\n\nThese two functions are only used by the Fence class. You can drop them\nand access fd_ directly there. As a general rule, given that the\nExtensible public class and the Private class are friends of each other,\nwe only need a public API in the Private class when used by other\nclasses internally in libcamera.\n\n> +\tvoid close();\n> +\n> +private:\n> +\tFileDescriptor fd_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */\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/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 7155ff203f6e..3721feb1ec92 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -7,6 +7,7 @@ libcamera_public_headers = files([\n> 'camera_manager.h',\n> 'compiler.h',\n> 'controls.h',\n> + 'fence.h',\n> 'file_descriptor.h',\n> 'framebuffer.h',\n> 'framebuffer_allocator.h',\n> diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp\n> new file mode 100644\n> index 000000000000..9ad86b3fa115\n> --- /dev/null\n> +++ b/src/libcamera/fence.cpp\n> @@ -0,0 +1,104 @@\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 \"libcamera/internal/fence.h\"\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +/**\n> + *\n> + * \\file libcamera/fence.h\n> + * \\brief Synchronization fence\n\nWe need more documentation here, to explain what fences are about, how\nthey're used, ...\n\n> + *\n> + * \\file internal/fence.h\n> + * \\brief Internal synchronization fence representation\n> + */\n> +\n> +/**\n> + * \\class Fence::Private\n> + * \\brief Private synchronization Fence implementation\n> + */\n> +\n> +/**\n> + * \\brief Construct a Fence::Private\n> + * \\param[in] fd The filedescriptor owned by the Fence\n> + */\n> +Fence::Private::Private(const FileDescriptor &fd)\n> +\t: fd_(std::move(fd))\n> +{\n> +}\n> +\n> +/**\n> + * \\fn Fence::Private::isValid()\n> + * \\brief Retrieve if a Fence is valid\n> + *\n> + * A Fence is valid if the FileDescriptor it wraps is valid\n> + *\n> + * \\return True if the Fence is valid, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn Fence::Private::fd()\n> + * \\brief Retrieve a refernce to the FileDescriptor wrapped by this Fence\n> + *\n> + * \\todo Document how to extract the FileDescriptor in case the Fence has failed\n> + *\n> + * \\return A reference to the FileDescriptor this Fence wraps\n> + */\n> +\n> +/**\n> + * \\brief Close the Fence by releasing the wrapped FileDescriptor\n> + */\n> +void Fence::Private::close()\n> +{\n> +\tfd_ = FileDescriptor();\n> +}\n> +\n> +/**\n> + * \\class Fence\n> + * \\brief Synchronization fence class\n> + *\n> + * \\todo Documentation\n\nIndeed :-) Same for the functions below.\n\n> + */\n> +\n> +/**\n> + * \\brief Create a synchronization fence\n> + * \\param[in] fd The synchronization fence file descriptor\n> + */\n> +Fence::Fence(const FileDescriptor &fd)\n> +\t: Extensible(std::make_unique(fd))\n> +{\n> +}\n> +\n> +/**\n> + * \\copydoc Fence::Private::isValid()\n\nAs we'll likely drop the Private accessors we won't need copydoc, but if\nwe didn't, I'd document the public API in full, and copy the\ndocumentation to the Private class.\n\n> + */\n> +bool Fence::isValid() const\n> +{\n> +\treturn _d()->isValid();\n> +}\n> +\n> +/**\n> + * \\copydoc Fence::Private::fd()\n> + */\n> +const FileDescriptor &Fence::fd()\n> +{\n> +\treturn _d()->fd();\n> +}\n> +\n> +/**\n> + * \\copydoc Fence::Private::close()\n> + */\n> +void Fence::close()\n> +{\n> +\t_d()->close();\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 36C9BBDB13\n\tfor ;\n\tSun, 21 Nov 2021 16:52:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5867260371;\n\tSun, 21 Nov 2021 17:52:17 +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 EF6B660233\n\tfor ;\n\tSun, 21 Nov 2021 17:52:15 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 51DD39DD;\n\tSun, 21 Nov 2021 17:52:15 +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=\"v4H5V2ea\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637513535;\n\tbh=hs9D6KkhHhC0RkkwwCz1xhad9rrf1ufKS3nXOSUooNw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v4H5V2eaTBDxwHNVD5TPrIm7pdCosiV8r1RiCBdKgv6bqN2mtLtGqF+Sux/UoU1c7\n\tdV0EWTx41yysD5BTzQixxuI1zqdxcC0yHwjNXfNSFZxMKjrL7r+IEB6vxq8Ap8KTEG\n\txle+7i/EPZNDg/oYdYlMtUBhBkVikJj+6sP31+pc=","Date":"Sun, 21 Nov 2021 18:51:52 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211120111313.106621-1-jacopo@jmondi.org>\n\t<20211120111313.106621-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211120111313.106621-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 02/12] libcamera: fence: Introduce\n\tFence","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\" "}}]