[libcamera-devel] [PATCH 03/10] libcamera: Introduce Fence class
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Nov 9 14:39:58 CET 2021
Quoting Kieran Bingham (2021-11-09 13:00:43)
> 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();
also - in this case here, the other timer is still running, so there is
perhaps expected to be someone listing waiting for either a completed
fence notification or a timeout. Does calling .stop() - activate the
signal to signal a timeout? If not - perhaps we should explicitly /
manually do so, and generate a timeout on it?
>
> 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
> >
More information about the libcamera-devel
mailing list