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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Oct 28, 2021 at 01:15:13PM +0200, 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__

The series hides the Fence class, and exposes an fd in the public API
when constructing a FrameBuffer. I'm wondering if it wouldn't be better
to expose a Fence class in the public API. This would decouple the
platform fence implementation from the FrameBuffer class. Among the
advantages I can see, the FrameBuffer constructor won't have to deal
with int fds, making the ownership semantics clearer. It could also help
supporting different types of fences in the future by inheriting from
the Fence class, without changing the public API of the FrameBuffer
class.

We would need to make the Fence class Extensible, as most of the
implementation shouldn't be visible to applications.

> +
> +#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);

Can we pass a FileDescriptor for the fd, to avoid having to define
fd-handling semantics (borrow vs. acquire vs. dup) in the Fence API ?

I don't think the timeout belongs here. The Fence class should model a
fence (also called sync object in other APIs, such as OpenGL (ES)). It
should likely offer functions to help waiting on the fence (probably as
part of the non-public API implemented in Fence::Private to start with,
but I wouldn't be surprised if there was a need to implement out fences
in the future too), but the wait should be decoupled from fence
construction. The timeout should be passed to wait-related functions (if
any).

Regarding timeouts, as Kieran mentioned, let's use std::chrono types.

> +	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_;

A comment to tell what the lock protects (until thread-safety
annotations land in) would be useful.

> +};
> +
> +} /* 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.

You may want to read
https://www.khronos.org/registry/EGL/extensions/KHR/EGL_KHR_fence_sync.txt
for a description of how fences can be defined. I don't think we should
mention read events anywhere, that's an implementation detail of the
corresponding kernel API. I'd reuse the "fences are signaled" and
"completion" terminology.

> + *
> + * 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.

I wonder if it wouldn't be better to disable move operations too, and
pass fences as (unique where appropriate) pointers. That would simplify
the semantics of the class. I haven't checked in details if there would
be any drawback in doing so.

> + *
> + * 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.
> + *
> + * 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
> + * a Fence can be disabled by calling the disable() function.

I'm tempted to remove timeout handling from the Fence class. In the
first use case we have to implement, the timeout applies to a group of
fences, as we want to queue the request to the pipeline handler only
after all fences have been signalled. There's thus no need to have a
timeout per fence, only a global timeout at the request level. Storing
the Timer in the PipelineHandler base class would be the best option if
possible.

It could be argued that the notifier could also be moved out of the
Fence class, but as it's needed in our most common use case for all
fences, it makes sense to have it as a helper in Fence::Private.

> + *
> + * 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);
> +}
> +
> +/**
> + * \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;
> +}
> +
> +/**
> + * \fn Fence::fd() const
> + * \brief Return the synchronization fence file descriptor
> + * \return The synchronization fence file descriptor
> + */

Beside unit tests, this function is only used by the FrameBuffer class
to implement the fence() function, which in turn is only used in
PipelineHandler::queueRequest() to test if the framebuffer has a fence
attached to it. I'd drop Fence::fd() (until we need it in pipeline
handlers, when V4L2 will support fences natively), add a
Fence::isValid(), and use isValid() in PipelineHandler::queueRequest().

> +
> +/**
> + * \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()

It's not the fence that is enabled or disabled, but the notification.
I'd name the function enableNotification() (better names probably
exist).

> +{
> +	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.
> + */
> +
> +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',

-- 
Regards,

Laurent Pinchart