[libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class

[Umang Jain]

Hi Jacopo,

On 10/28/21 4:45 PM, Jacopo Mondi wrote:
> Fences are a synchronization mechanism that allows to receive event
> notifications of a file descriptor with an optional expiration timeout.
>
> Introduce the Fence class to model such a mechanism in libcamera.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>   include/libcamera/internal/fence.h     |  64 +++++++++
>   include/libcamera/internal/meson.build |   1 +
>   src/libcamera/fence.cpp                | 185 +++++++++++++++++++++++++
>   src/libcamera/meson.build              |   1 +
>   4 files changed, 251 insertions(+)
>   create mode 100644 include/libcamera/internal/fence.h
>   create mode 100644 src/libcamera/fence.cpp
>
> diff --git a/include/libcamera/internal/fence.h b/include/libcamera/internal/fence.h
> new file mode 100644
> index 000000000000..5a78e0f864c7
> --- /dev/null
> +++ b/include/libcamera/internal/fence.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * internal/fence.h - Synchronization fence
> + */
> +#ifndef __LIBCAMERA_INTERNAL_FENCE_H__
> +#define __LIBCAMERA_INTERNAL_FENCE_H__
> +
> +#include <mutex>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/event_notifier.h>
> +#include <libcamera/base/timer.h>
> +#include <libcamera/file_descriptor.h>
> +
> +namespace libcamera {
> +
> +class Fence
> +{
> +public:
> +	explicit Fence(int fenceFd, unsigned int timeoutMs = kFenceTimeout);
> +	Fence(Fence &&other);
> +
> +	Fence &operator=(Fence &&other);
> +
> +	int fd() const { return fd_.fd(); }
> +
> +	unsigned int timeout() const { return timeout_; }
> +	bool completed() const { return completed_; }
> +	bool expired() const { return expired_; }
> +
> +	void enable();
> +	void disable();
> +
> +	Signal<> complete;
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY(Fence)
> +
> +	/* 300 milliseconds default timeout. */
> +	static constexpr unsigned int kFenceTimeout = 300;
> +
> +	void moveFence(Fence &other);
> +
> +	void activated();
> +	void timedout();
> +
> +	FileDescriptor fd_;
> +	EventNotifier notifier_;
> +
> +	Timer timer_;
> +	unsigned int timeout_;
> +
> +	bool completed_ = false;
> +	bool expired_ = false;
> +
> +	std::mutex mutex_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */
> +
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index cae65b0604ff..32d5c3387de3 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -22,6 +22,7 @@ libcamera_internal_headers = files([
>       'device_enumerator.h',
>       'device_enumerator_sysfs.h',
>       'device_enumerator_udev.h',
> +    'fence.h',
>       'formats.h',
>       'framebuffer.h',
>       'ipa_manager.h',
> diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp
> new file mode 100644
> index 000000000000..8fadb2c21f03
> --- /dev/null
> +++ b/src/libcamera/fence.cpp
> @@ -0,0 +1,185 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * fence.cpp - Synchronization fence
> + */
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/thread.h>
> +#include <libcamera/internal/fence.h>
> +
> +namespace libcamera {
> +
> +/**
> + * \file internal/fence.h
> + * \brief Synchronization fence
> + */
> +
> +/**
> + * \class Fence
> + * \brief Synchronization fence class
> + *
> + * A Fence is a synchronization mechanism that allows to wait for a read event
> + * to happen on a file descriptor before an optional timeout expires.
> + *
> + * A Fence is created with a default timeout of 300 milliseconds, which can
> + * be overridden through the 'timeout' constructor parameter. Passing a 0
> + * timeout to the constructor disables timeouts completely.
> + *
> + * A Fence wraps a FileDescriptor and implements a move-only semantic, as Fence
> + * instances cannot be copied but only moved, causing the moved Fence to be
> + * reset by invalidating the wrapped FileDescriptor by disabling its
> + * notification and timeout mechanisms.
> + *
> + * When a read event is notified, the Fence is said to be 'completed',
> + * alternatively if the timeout expires before any event is notified the Fence
> + * is said to be 'expired'.
> + *
> + * In both cases, when an event notification happens or a timeout expires, the
> + * class emits the 'complete' signal, to which users of the class can connect
> + * to and check if the Fence has completed or has expired by calling the
> + * completed() and expired() functions.


If this is the case, I would expect a value emitted as a signal 
parameter, to know whether it's completed or expired

> + *
> + * Fence instances are non-active by default (ie no events or timeout are
> + * generated) and need to be enabled by calling the enable() function. Likewise


Its inactive both when "constructed" and "moved" right ? It seems so, as 
per my reading below

> + * a Fence can be disabled by calling the disable() function.
> + *
> + * After a Fence has expired no events are generated and the Fence need to be
> + * manually re-enabled. Likewise, if the Fence is signalled the expiration
> + * timeout is cancelled.
> + */
> +
> +/**
> + * \brief Create a synchronization fence
> + * \param[in] fenceFd The synchronization fence file descriptor
> + * \param[in] timeoutMs The optional fence timeout. Set to 0 to disable timeout
> + */
> +Fence::Fence(int fenceFd, unsigned int timeoutMs)
> +	: fd_(std::move(fenceFd)),
> +	  notifier_(fd_.fd(), EventNotifier::Read, nullptr, false),
> +	  timeout_(timeoutMs)
> +{
> +	notifier_.activated.connect(this, &Fence::activated);
> +
> +	timer_.timeout.connect(this, &Fence::timedout);
> +}
> +
> +void Fence::moveFence(Fence &other)
> +{
> +	fd_ = std::move(other.fd_);
> +
> +	other.disable();
> +	if (other.timer_.isRunning())
> +		other.timer_.stop();
> +	other.timeout_ = 0;
> +}
> +
> +/**
> + * \brief Move-construct a Fence
> + * \param[in] other The other fence to move
> + */
> +Fence::Fence(Fence &&other)
> +	: notifier_(other.fd(), EventNotifier::Read, nullptr, false)
> +{
> +	moveFence(other);
> +
> +	notifier_.activated.connect(this, &Fence::activated);
> +	timer_.timeout.connect(this, &Fence::timedout);


I think Kieran has rightly pointed out in his replies to disconnect 
these from the 'other' Fence too.

> +}
> +
> +/**
> + * \brief Move-assign the value of the \a other fence to this
> + * \param[in] other The other fence to move
> + * \return This fence
> + */
> +Fence &Fence::operator=(Fence &&other)
> +{
> +	moveFence(other);
> +
> +	notifier_ = EventNotifier(fd_.fd(), EventNotifier::Read, nullptr, false);
> +
> +	return *this;
> +}


One fundamental question I am pondering is why a Fence needs to be moved 
after constructed. Hmm. I think I have a broad idea, that it happens 
while the iteration between FrameBuffer and Request class in subsequent 
patches, I'll try to keep on reading for exact answers.

> +
> +/**
> + * \fn Fence::fd() const
> + * \brief Return the synchronization fence file descriptor
> + * \return The synchronization fence file descriptor
> + */
> +
> +/**
> + * \fn Fence::timeout()
> + * \brief Retrieve the fence timeout
> + * \return The fence timeout in milliseconds
> + */
> +
> +/**
> + * \fn Fence::completed()
> + * \brief Check if the fence has completed before timing out
> + * \return True if the fence has completed
> + */
> +
> +/**
> + * \fn Fence::expired()
> + * \brief Check if the fence has expired before completing
> + * \return True if the fence has expired
> + */
> +
> +/**
> + * \brief Enable a fence by enabling events notifications
> + */
> +void Fence::enable()
> +{
> +	MutexLocker lock(mutex_);
> +
> +	expired_ = false;
> +	completed_ = false;
> +
> +	notifier_.setEnabled(true);
> +	if (timeout_)
> +		timer_.start(timeout_);
> +}
> +
> +/**
> + * \brief Disable a fence by disabling events notifications
> + */
> +void Fence::disable()
> +{
> +	notifier_.setEnabled(false);
> +}
> +
> +/**
> + * \var Fence::complete
> + * \brief The fence completion signal
> + *
> + * A Fence is completed if signalled or timeout.
> + */

Yep, I think it can emit with a value denoting whether it's a timeout or 
activated

> +
> +void Fence::activated()
> +{
> +	MutexLocker lock(mutex_);
> +
> +	if (timer_.isRunning())
> +		timer_.stop();
> +
> +	completed_ = true;
> +	expired_ = false;
> +
> +	complete.emit();
> +}
> +
> +void Fence::timedout()
> +{
> +	MutexLocker lock(mutex_);
> +
> +	expired_ = true;
> +	completed_ = false;
> +
> +	/* Disable events notification after timeout. */
> +	disable();
> +
> +	complete.emit();
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 6727a777d804..6fb0d5f49b63 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -14,6 +14,7 @@ libcamera_sources = files([
>       'delayed_controls.cpp',
>       'device_enumerator.cpp',
>       'device_enumerator_sysfs.cpp',
> +    'fence.cpp',
>       'file_descriptor.cpp',
>       'formats.cpp',
>       'framebuffer.cpp',