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

14398 diff mbox series

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

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

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

Hi Kieran

On Tue, Nov 09, 2021 at 01:39:58PM +0000, Kieran Bingham wrote:
> Quoting Kieran Bingham (2021-11-09 13:00:43)
> > Quoting Jacopo Mondi (2021-10-28 12:15:13)

[snip]

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

afaict Timer::stop() does not generate a timeout and I'm not sure
generating an event on the moved Fence is the right thing to do. How
is it the slot supposed to know if a real timeout has occurred ?

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

Quoting Jacopo Mondi (2021-11-09 17:22:31)
> 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@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):

I think we've crossed syntaxes a little.

Yes, C++ move syntax means that the 'other' object (piece of memory) is
invalidated, but with the Fence here - I'm weary that when we do a move
we're invalidating the Fence itself... (to some degree), and the
connections that are made against the fence.

Its defining what happens to the existing 'users' (Anything that has a
connection to a fence which is about to be moved) that I think we should
explicitly document here.


Ultimately - from what I understand that is now that 

"Moving a Fence will disconnect any existing listners to events on that
Fence and stop any timer that may be actived on the Fence. No completion
events will be sent on the Fence after it has been moved until it is
re-enabled."


Otherwise, a move operation should 'move' the entire thing across,
connections and all.


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

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@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',

Quoting Jacopo Mondi (2021-11-09 17:24:42)
> Hi Kieran
> 
> On Tue, Nov 09, 2021 at 01:39:58PM +0000, Kieran Bingham wrote:
> > Quoting Kieran Bingham (2021-11-09 13:00:43)
> > > Quoting Jacopo Mondi (2021-10-28 12:15:13)
> 
> [snip]
> 
> > > > +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?
> >
> 
> afaict Timer::stop() does not generate a timeout and I'm not sure
> generating an event on the moved Fence is the right thing to do. How
> is it the slot supposed to know if a real timeout has occurred ?

I'm weary that I may be into hypotheticals of situations that won't
occur, but I'm trying to look at it from how the class is designed and
therefore /could/ be used.

My concern is that you are indicating in the code that we are moving a
Fence which has a /running/ timer set on it.

We are 'sweeping' the rug out from under the listener, whomever that may
be. They are expecting either a fence completion event, or a timeout -
but the move operation is cancelling both of those (and we're about to
disconnect the signal entirely too I expect).

So I think the listener should be told there is a timeout at least.

If this is unexpected behaviour, and moving a Fence which has an active
running timer shouldn't happen - then we either need to ASSERT() or
LOG(Error) on it here in Fence::moveFence() and make sure that expected
behaviour is documented.


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

On Wed, Nov 10, 2021 at 10:44:36AM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2021-11-09 17:24:42)
> > Hi Kieran
> > 
> > On Tue, Nov 09, 2021 at 01:39:58PM +0000, Kieran Bingham wrote:
> > > Quoting Kieran Bingham (2021-11-09 13:00:43)
> > > > Quoting Jacopo Mondi (2021-10-28 12:15:13)
> > 
> > [snip]
> > 
> > > > > +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?
> > 
> > afaict Timer::stop() does not generate a timeout and I'm not sure
> > generating an event on the moved Fence is the right thing to do. How
> > is it the slot supposed to know if a real timeout has occurred ?
> 
> I'm weary that I may be into hypotheticals of situations that won't
> occur, but I'm trying to look at it from how the class is designed and
> therefore /could/ be used.
> 
> My concern is that you are indicating in the code that we are moving a
> Fence which has a /running/ timer set on it.
> 
> We are 'sweeping' the rug out from under the listener, whomever that may
> be. They are expecting either a fence completion event, or a timeout -
> but the move operation is cancelling both of those (and we're about to
> disconnect the signal entirely too I expect).
> 
> So I think the listener should be told there is a timeout at least.
> 
> If this is unexpected behaviour, and moving a Fence which has an active
> running timer shouldn't happen - then we either need to ASSERT() or
> LOG(Error) on it here in Fence::moveFence() and make sure that expected
> behaviour is documented.

Setting the cat among the pigeons, I'd like to propose making fences
non-movable, and passing them by (unique where appropriate) pointer.

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

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@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',