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

[Kieran Bingham]

Quoting Jacopo Mondi (2021-10-28 12:15:13)
> Fences are a synchronization mechanism that allows to receive event

s/allows to/allow receiving/
   or
s/allows to/allows us to/

> notifications of a file descriptor with an optional expiration timeout.
> 
> Introduce the Fence class to model such a mechanism in libcamera.

I like seeing this wrapped in a well defined class. I'm afraid I have
some deeper comments that prevent me adding a tag directly...

> 
> 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>

I'm surprised checkpatch didn't suggest a separate grouping here.

Hrm - it really doesn't - I've just applied the patches and run it. But
it should... I wonder if our checkstyle isn't working correctly on whole
file additions.

Anyway, running clang-format directly gives the following change
suggestions:

$ clang-format-diff-file ./include/libcamera/internal/fence.h 
--- ./include/libcamera/internal/fence.h
+++ ./include/libcamera/internal/fence.h.clang
@@ -12,6 +12,7 @@
 #include <libcamera/base/class.h>
 #include <libcamera/base/event_notifier.h>
 #include <libcamera/base/timer.h>
+
 #include <libcamera/file_descriptor.h>
 
 namespace libcamera {
@@ -61,4 +62,3 @@
 } /* namespace libcamera */
 
 #endif /* __LIBCAMERA_INTERNAL_FENCE_H__ */
-


> +#include <libcamera/file_descriptor.h>
> +
> +namespace libcamera {
> +
> +class Fence
> +{
> +public:
> +       explicit Fence(int fenceFd, unsigned int timeoutMs = kFenceTimeout);

Should the timeout use std::chrono in some way to make sure the units
are coded?

> +       Fence(Fence &&other);
> +
> +       Fence &operator=(Fence &&other);

Personally I'd group the move operator and constructor together...

https://en.cppreference.com/w/cpp/language/rule_of_three
states you should have a destructor defined too.

Not sure what it needs to do yet though...

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

Otherwise, I like that interface so far.


> +private:
> +       LIBCAMERA_DISABLE_COPY(Fence)
> +
> +       /* 300 milliseconds default timeout. */
> +       static constexpr unsigned int kFenceTimeout = 300;

std::chrono::duration possibility...

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

$ clang-format-diff-file src/libcamera/fence.cpp
--- src/libcamera/fence.cpp
+++ src/libcamera/fence.cpp.clang
@@ -7,6 +7,7 @@
 
 #include <libcamera/base/log.h>
 #include <libcamera/base/thread.h>
+
 #include <libcamera/internal/fence.h>
 
 namespace libcamera {


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

"allows waiting for a "

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

"""

A Fence wraps a FileDescriptor and implements a move-only semantic, as
Fence instances cannot be copied.

Moving a Fence will disable any existing notification and timeout
mechanisms, but will allow the underlying notification FileDescriptor to
be re-used on the 'new' Fence. Existing listeners of the Fence signals
will no longer receive events, even if it is re-enabled.

"""

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

I wonder if the completion status could be passed as part of the signal?
Would that be easier than the recipient having to call back and check?

(I think having the status methods is still good though)

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

I see now that the text above really did mean moving a fence invalidates
it, but I'm not sure how that use case comes about.

Could this funtion perhaps document /why/ moving a fence necessitates
invalidating it, and why it moves the internal fd to preserve it's
lifetime - but - that the fence can no longer be used... (or something
like that if I've inferred correctly).


> +{
> +       fd_ = std::move(other.fd_);
> +

There might be code connected to the complete signal of the 'other'
fence. Should they be moved over to this? or should we explictly state
that listeners to that completion event are 'disconnected' (somewhat
implicitly).

Perhaps we should explictily either disconnect or move them in the
code...?

> +       other.disable();
> +       if (other.timer_.isRunning())
> +               other.timer_.stop();

Should you move the timeout_ value over to this one too?

> +       other.timeout_ = 0;
> +}
> +
> +/**
> + * \brief Move-construct a Fence
> + * \param[in] other The other fence to move

Ok things are becoming clearer. Perhaps:

 *
 * The other fence is disabled, and invalidated while the underlying
 * FileDescriptor is moved to this Fence. The newly moved fence can be
 * re-used when required.

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

Something similar to the above addition maybe...


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

A Fence is completed when signalled or if a timeout occurs.

> + */
> +
> +void Fence::activated()
> +{
> +       MutexLocker lock(mutex_);
> +
> +       if (timer_.isRunning())
> +               timer_.stop();
> +
> +       completed_ = true;
> +       expired_ = false;
> +
> +       complete.emit();

Passing the expiration state as a boolean here might help the receiver
know why it completed.

But maybe that's optional - or better handled as you have it anyway.

> +}
> +
> +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',
> -- 
> 2.33.1
>