[libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class
Jacopo Mondi
jacopo at jmondi.org
Tue Nov 9 18:22:31 CET 2021
Hi Kieran
On Tue, Nov 09, 2021 at 01:00:43PM +0000, Kieran Bingham wrote:
> 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?
>
Good idea, I'll try it
> > + 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)
>
That's also a good suggestion!
> > + *
> > + * 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).
>
I'm not sure this adds anything to the definition of the C++ move
semantic. A moved object is left in "valid but undefined state"
https://en.cppreference.com/w/cpp/utility/move
Unless otherwise specified, all standard library objects that have
been moved from are placed in a "valid but unspecified state", meaning
the object's class invariants hold (so functions without
preconditions, such as the assignment operator, can be safely used on
the object after it was moved from):
>
> > +{
> > + 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...?
I should indeed disconnect the existing slots.
>
> > + other.disable();
> > + if (other.timer_.isRunning())
> > + other.timer_.stop();
>
> Should you move the timeout_ value over to this one too?
>
Yes indeed!
> > + 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
> >
More information about the libcamera-devel
mailing list