[libcamera-devel,v3,03/11] libcamera: fence: Introduce Fence

14917 diff mbox series

Introduce a Fence class which models a synchronization primitive that
allows to notify the availability of a resource.

The Fence is modeled as a wrapper of a UniqueFD instance where
read events are used to signal the Fence. The class can be later
extended to support additional signalling mechanisms.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/fence.h     |  31 ++++++++++
 include/libcamera/meson.build |   1 +
 src/libcamera/fence.cpp       | 108 ++++++++++++++++++++++++++++++++++
 src/libcamera/meson.build     |   1 +
 4 files changed, 141 insertions(+)
 create mode 100644 include/libcamera/fence.h
 create mode 100644 src/libcamera/fence.cpp

Hi Jacopo,

Thank you for the patch.

On Wed, Dec 01, 2021 at 12:36:26AM +0100, Jacopo Mondi wrote:
> Introduce a Fence class which models a synchronization primitive that
> allows to notify the availability of a resource.
> 
> The Fence is modeled as a wrapper of a UniqueFD instance where
> read events are used to signal the Fence. The class can be later
> extended to support additional signalling mechanisms.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/fence.h     |  31 ++++++++++
>  include/libcamera/meson.build |   1 +
>  src/libcamera/fence.cpp       | 108 ++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |   1 +
>  4 files changed, 141 insertions(+)
>  create mode 100644 include/libcamera/fence.h
>  create mode 100644 src/libcamera/fence.cpp
> 
> diff --git a/include/libcamera/fence.h b/include/libcamera/fence.h
> new file mode 100644
> index 000000000000..95f2dd07f10b
> --- /dev/null
> +++ b/include/libcamera/fence.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * internal/fence.h - Synchronization fence
> + */
> +
> +#pragma once
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/unique_fd.h>
> +
> +namespace libcamera {
> +
> +class Fence
> +{
> +public:
> +	Fence(UniqueFD fd);
> +
> +	bool isValid() const { return fd_.isValid(); }
> +	const UniqueFD *fd() const { return &fd_; }
> +
> +	UniqueFD reset() { return std::move(fd_); }

This should be called release(), and should return fd_.release().

> +
> +private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(Fence)
> +
> +	UniqueFD fd_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 5f42977c034b..8c2cae00d877 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -6,6 +6,7 @@ libcamera_public_headers = files([
>      'camera.h',
>      'camera_manager.h',
>      'controls.h',
> +    'fence.h',
>      'framebuffer.h',
>      'framebuffer_allocator.h',
>      'geometry.h',
> diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp
> new file mode 100644
> index 000000000000..2bab8d7de76a
> --- /dev/null
> +++ b/src/libcamera/fence.cpp
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * fence.cpp - Synchronization fence
> + */
> +
> +#include "libcamera/fence.h"
> +
> +namespace libcamera {
> +
> +/**
> + *
> + * \file libcamera/fence.h
> + * \brief Synchronization fence
> + */
> +
> +/**
> + * \class Fence
> + * \brief Synchronization fence class

Sweet, documentation :-)

I'm not sure I like the name "synchronization fence". The "fence" is a
synchronization primitive, so "synchronization fence" sounds a bit
redundant. Maybe

 * \brief Synchronization primitive to manage resources

or something similar ? Same through the file.

> + *
> + * The Fence class is a thin abstraction around a UniqueFD which simply allows
> + * to access it as a const pointer or to move its ownership to the caller.

I've noticed many times a tendency (not just from you, it's general) to
focus on how a class or function is implemented, instead of what it
represents, what it does, and how it should be used. Here, I would start
with

 * The Fence class models a synchronization primitive that can be used by
 * applications to explicitly synchronize resource usage, and can be shared by
 * multiple processes.

> + *
> + * Synchronization fences are most commonly used in association with frame
> + * buffers. A FrameBuffer can be associated with a Fence (\sa
> + * Request::addBuffer()) so that the library can wait for the Fence to be
> + * signalled before allowing the camera device to actually access the memory
> + * area described by the FrameBuffer.
> + *
> + * By using synchronization fences, application can then synchronize between
> + * frame buffer consumers and produces, as in example a video output device and

s/produces/producers/
s/in example/for example/
s/video output device/display device/

> + * camera, to gurantee that new data transfers only happen once the existing

s/camera/a camera/
s/gurantee/guarantee/

> + * frames have been displayed.
> + *

As the next paragraph is about internal usage of the Fence class, and
not about the application-facing API, I would move it to an \internal
block at the end. The rest of the documentation should be refactored a
bit to separate public and internal documentation.

> + * The Fence class only abstracts the underlying mechanism used by libcamera to
> + * implement synchronization primitives and allow to wait until an event is

s/allow/allows/

> + * signalled, but does not implement event notification itself which is instead
> + * implemented by the core library.
> + *
> + * \sa Request::prepare()
> + *
> + * A Fence can be realized by different event notification primitives, the most
> + * common of which is represented by waiting for read events to happen on a
> + * file descriptor. This is currently the only mechanism supported by libcamera,
> + * but others can be implemented by extending or subclassing this class and
> + * implementing opportune handling in the core library.

I think we should be a bit more precise here. It's not any file
descriptor, it's a handle to a kernel sync file (see sync_file.rst and
dma-buf.rst in Documentation/driver-api/).

> + *
> + * The usage of the Fence class allows to abstract the underlying
> + * synchronization mechanism in use and implement an interface towards other
> + * library components that will not change when new synchronization primitives
> + * will be added as fences.
> + *
> + * A Fence is contructed with a UniqueFD whose ownship is moved in the Fence. A
> + * FrameBuffer can be associated with a Fence by passing it to the
> + * Request::addBuffer() function, which will move the Fence into the FrameBuffer
> + * itself. Once a Request is queued to the Camera, a preparation phase gurantees
> + * that before actually applying the Request to the hardware, all the valid
> + * fences of the frame buffers in a Request are correctly signalled. Once a
> + * Fence has completed, the library will reset the FrameBuffer fence so that
> + * application won't be allowed to access it.
> + *
> + * An optional timeout can be started while waiting for a fence to complete. If
> + * waiting on a Fence fails for whatever reason, the FrameBuffer's fence is not
> + * reset and is made available to application for them to handle it, by
> + * resetting the Fence to correctly close the underlying UniqueFD.
> + *
> + * A failure in waiting for a Fence to complete will result in the Request to
> + * complete in failed state.
> + */
> +
> +/**
> + * \brief Create a synchronization fence
> + * \param[in] fd The synchronization fence unique file descriptor

s/unique //

The fact that we manage the fd through a UniqueFD is more of an
implementation detail, I would just write "file descriptor" through the
documentation here and below.

> + *
> + * The unique file descriptor ownership is moved to the Fence.
> + */
> +Fence::Fence(UniqueFD fd)
> +	: fd_(std::move(fd))
> +{
> +}
> +
> +/**
> + * \fn Fence::isValid()
> + * \brief Retrieve if a Fence is valid

"Check if a Fence is valid" maybe ?

> + *
> + * A Fence is valid if the UniqueFD it wraps is valid

s/$/./

> + *
> + * \return True if the Fence is valid, false otherwise
> + */
> +
> +/**
> + * \fn Fence::fd()
> + * \brief Retrieve a constant pointer to the UniqueFD
> + * \return A const pointer to the UniqueFD

Could this be a reference instead of a pointer ? Looking at the
implementation, it can never be null.

> + */
> +
> +/**
> + * \fn Fence::reset()
> + * \brief Reset the Fence by moving the UniqueFD ownership to the caller
> + *
> + * Reset the Fence by releasing the wrapped UniqueFD. Ownership of the UniqueFD
> + * is moved to the caller.

Calling this release(), it should read

 * \brief Release the ownership of the file descriptor
 *
 * Release the ownership of the wrapped UniqueFD and return it to the caller.

> + *
> + * \return The UniqueFD
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index b763110e74a6..b4882a2577f5 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',
>      'formats.cpp',
>      'framebuffer.cpp',
>      'framebuffer_allocator.cpp',

Hi Laurent

On Wed, Dec 01, 2021 at 03:33:37AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Dec 01, 2021 at 12:36:26AM +0100, Jacopo Mondi wrote:
> > Introduce a Fence class which models a synchronization primitive that
> > allows to notify the availability of a resource.
> >
> > The Fence is modeled as a wrapper of a UniqueFD instance where
> > read events are used to signal the Fence. The class can be later
> > extended to support additional signalling mechanisms.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/fence.h     |  31 ++++++++++
> >  include/libcamera/meson.build |   1 +
> >  src/libcamera/fence.cpp       | 108 ++++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build     |   1 +
> >  4 files changed, 141 insertions(+)
> >  create mode 100644 include/libcamera/fence.h
> >  create mode 100644 src/libcamera/fence.cpp
> >
> > diff --git a/include/libcamera/fence.h b/include/libcamera/fence.h
> > new file mode 100644
> > index 000000000000..95f2dd07f10b
> > --- /dev/null
> > +++ b/include/libcamera/fence.h
> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * internal/fence.h - Synchronization fence
> > + */
> > +
> > +#pragma once
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/base/unique_fd.h>
> > +
> > +namespace libcamera {
> > +
> > +class Fence
> > +{
> > +public:
> > +	Fence(UniqueFD fd);
> > +
> > +	bool isValid() const { return fd_.isValid(); }
> > +	const UniqueFD *fd() const { return &fd_; }
> > +
> > +	UniqueFD reset() { return std::move(fd_); }
>
> This should be called release(), and should return fd_.release().
>

UniqueFD::release() returns the integer file descriptor value which
would percolate from the Fence interface. Do we want to have the
FileDescriptor surfacing from the API, if the fence class sole purpose
is to abstract it away ? (I understand the counter-argument that
returning a UniqueFD is not that different).

> > +
> > +private:
> > +	LIBCAMERA_DISABLE_COPY_AND_MOVE(Fence)
> > +
> > +	UniqueFD fd_;
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 5f42977c034b..8c2cae00d877 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -6,6 +6,7 @@ libcamera_public_headers = files([
> >      'camera.h',
> >      'camera_manager.h',
> >      'controls.h',
> > +    'fence.h',
> >      'framebuffer.h',
> >      'framebuffer_allocator.h',
> >      'geometry.h',
> > diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp
> > new file mode 100644
> > index 000000000000..2bab8d7de76a
> > --- /dev/null
> > +++ b/src/libcamera/fence.cpp
> > @@ -0,0 +1,108 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * fence.cpp - Synchronization fence
> > + */
> > +
> > +#include "libcamera/fence.h"
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + *
> > + * \file libcamera/fence.h
> > + * \brief Synchronization fence
> > + */
> > +
> > +/**
> > + * \class Fence
> > + * \brief Synchronization fence class
>
> Sweet, documentation :-)
>
> I'm not sure I like the name "synchronization fence". The "fence" is a
> synchronization primitive, so "synchronization fence" sounds a bit
> redundant. Maybe
>
>  * \brief Synchronization primitive to manage resources
>
> or something similar ? Same through the file.
>
> > + *
> > + * The Fence class is a thin abstraction around a UniqueFD which simply allows
> > + * to access it as a const pointer or to move its ownership to the caller.
>
> I've noticed many times a tendency (not just from you, it's general) to
> focus on how a class or function is implemented, instead of what it
> represents, what it does, and how it should be used. Here, I would start
> with
>
>  * The Fence class models a synchronization primitive that can be used by
>  * applications to explicitly synchronize resource usage, and can be shared by
>  * multiple processes.
>
> > + *
> > + * Synchronization fences are most commonly used in association with frame
> > + * buffers. A FrameBuffer can be associated with a Fence (\sa
> > + * Request::addBuffer()) so that the library can wait for the Fence to be
> > + * signalled before allowing the camera device to actually access the memory
> > + * area described by the FrameBuffer.
> > + *
> > + * By using synchronization fences, application can then synchronize between
> > + * frame buffer consumers and produces, as in example a video output device and
>
> s/produces/producers/
> s/in example/for example/
> s/video output device/display device/
>
> > + * camera, to gurantee that new data transfers only happen once the existing
>
> s/camera/a camera/
> s/gurantee/guarantee/
>
> > + * frames have been displayed.
> > + *
>
> As the next paragraph is about internal usage of the Fence class, and
> not about the application-facing API, I would move it to an \internal
> block at the end. The rest of the documentation should be refactored a
> bit to separate public and internal documentation.
>
> > + * The Fence class only abstracts the underlying mechanism used by libcamera to
> > + * implement synchronization primitives and allow to wait until an event is
>
> s/allow/allows/
>
> > + * signalled, but does not implement event notification itself which is instead
> > + * implemented by the core library.
> > + *
> > + * \sa Request::prepare()
> > + *
> > + * A Fence can be realized by different event notification primitives, the most
> > + * common of which is represented by waiting for read events to happen on a
> > + * file descriptor. This is currently the only mechanism supported by libcamera,
> > + * but others can be implemented by extending or subclassing this class and
> > + * implementing opportune handling in the core library.
>
> I think we should be a bit more precise here. It's not any file
> descriptor, it's a handle to a kernel sync file (see sync_file.rst and
> dma-buf.rst in Documentation/driver-api/).
>
> > + *
> > + * The usage of the Fence class allows to abstract the underlying
> > + * synchronization mechanism in use and implement an interface towards other
> > + * library components that will not change when new synchronization primitives
> > + * will be added as fences.
> > + *
> > + * A Fence is contructed with a UniqueFD whose ownship is moved in the Fence. A
> > + * FrameBuffer can be associated with a Fence by passing it to the
> > + * Request::addBuffer() function, which will move the Fence into the FrameBuffer
> > + * itself. Once a Request is queued to the Camera, a preparation phase gurantees
> > + * that before actually applying the Request to the hardware, all the valid
> > + * fences of the frame buffers in a Request are correctly signalled. Once a
> > + * Fence has completed, the library will reset the FrameBuffer fence so that
> > + * application won't be allowed to access it.
> > + *
> > + * An optional timeout can be started while waiting for a fence to complete. If
> > + * waiting on a Fence fails for whatever reason, the FrameBuffer's fence is not
> > + * reset and is made available to application for them to handle it, by
> > + * resetting the Fence to correctly close the underlying UniqueFD.
> > + *
> > + * A failure in waiting for a Fence to complete will result in the Request to
> > + * complete in failed state.
> > + */
> > +
> > +/**
> > + * \brief Create a synchronization fence
> > + * \param[in] fd The synchronization fence unique file descriptor
>
> s/unique //
>
> The fact that we manage the fd through a UniqueFD is more of an
> implementation detail, I would just write "file descriptor" through the
> documentation here and below.
>
> > + *
> > + * The unique file descriptor ownership is moved to the Fence.
> > + */
> > +Fence::Fence(UniqueFD fd)
> > +	: fd_(std::move(fd))
> > +{
> > +}
> > +
> > +/**
> > + * \fn Fence::isValid()
> > + * \brief Retrieve if a Fence is valid
>
> "Check if a Fence is valid" maybe ?
>
> > + *
> > + * A Fence is valid if the UniqueFD it wraps is valid
>
> s/$/./
>
> > + *
> > + * \return True if the Fence is valid, false otherwise
> > + */
> > +
> > +/**
> > + * \fn Fence::fd()
> > + * \brief Retrieve a constant pointer to the UniqueFD
> > + * \return A const pointer to the UniqueFD
>
> Could this be a reference instead of a pointer ? Looking at the
> implementation, it can never be null.
>
> > + */
> > +
> > +/**
> > + * \fn Fence::reset()
> > + * \brief Reset the Fence by moving the UniqueFD ownership to the caller
> > + *
> > + * Reset the Fence by releasing the wrapped UniqueFD. Ownership of the UniqueFD
> > + * is moved to the caller.
>
> Calling this release(), it should read
>
>  * \brief Release the ownership of the file descriptor
>  *
>  * Release the ownership of the wrapped UniqueFD and return it to the caller.
>
> > + *
> > + * \return The UniqueFD
> > + */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index b763110e74a6..b4882a2577f5 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',
> >      'formats.cpp',
> >      'framebuffer.cpp',
> >      'framebuffer_allocator.cpp',
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Wed, Dec 01, 2021 at 11:29:29AM +0100, Jacopo Mondi wrote:
> On Wed, Dec 01, 2021 at 03:33:37AM +0200, Laurent Pinchart wrote:
> > On Wed, Dec 01, 2021 at 12:36:26AM +0100, Jacopo Mondi wrote:
> > > Introduce a Fence class which models a synchronization primitive that
> > > allows to notify the availability of a resource.
> > >
> > > The Fence is modeled as a wrapper of a UniqueFD instance where
> > > read events are used to signal the Fence. The class can be later
> > > extended to support additional signalling mechanisms.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  include/libcamera/fence.h     |  31 ++++++++++
> > >  include/libcamera/meson.build |   1 +
> > >  src/libcamera/fence.cpp       | 108 ++++++++++++++++++++++++++++++++++
> > >  src/libcamera/meson.build     |   1 +
> > >  4 files changed, 141 insertions(+)
> > >  create mode 100644 include/libcamera/fence.h
> > >  create mode 100644 src/libcamera/fence.cpp
> > >
> > > diff --git a/include/libcamera/fence.h b/include/libcamera/fence.h
> > > new file mode 100644
> > > index 000000000000..95f2dd07f10b
> > > --- /dev/null
> > > +++ b/include/libcamera/fence.h
> > > @@ -0,0 +1,31 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * internal/fence.h - Synchronization fence
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <libcamera/base/class.h>
> > > +#include <libcamera/base/unique_fd.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class Fence
> > > +{
> > > +public:
> > > +	Fence(UniqueFD fd);
> > > +
> > > +	bool isValid() const { return fd_.isValid(); }
> > > +	const UniqueFD *fd() const { return &fd_; }
> > > +
> > > +	UniqueFD reset() { return std::move(fd_); }
> >
> > This should be called release(), and should return fd_.release().
> 
> UniqueFD::release() returns the integer file descriptor value which
> would percolate from the Fence interface. Do we want to have the
> FileDescriptor surfacing from the API, if the fence class sole purpose
> is to abstract it away ? (I understand the counter-argument that
> returning a UniqueFD is not that different).

My bad, returning std::move(fd_) is the right thing to do. The function
should just be renamed.

> > > +
> > > +private:
> > > +	LIBCAMERA_DISABLE_COPY_AND_MOVE(Fence)
> > > +
> > > +	UniqueFD fd_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > index 5f42977c034b..8c2cae00d877 100644
> > > --- a/include/libcamera/meson.build
> > > +++ b/include/libcamera/meson.build
> > > @@ -6,6 +6,7 @@ libcamera_public_headers = files([
> > >      'camera.h',
> > >      'camera_manager.h',
> > >      'controls.h',
> > > +    'fence.h',
> > >      'framebuffer.h',
> > >      'framebuffer_allocator.h',
> > >      'geometry.h',
> > > diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp
> > > new file mode 100644
> > > index 000000000000..2bab8d7de76a
> > > --- /dev/null
> > > +++ b/src/libcamera/fence.cpp
> > > @@ -0,0 +1,108 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * fence.cpp - Synchronization fence
> > > + */
> > > +
> > > +#include "libcamera/fence.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + *
> > > + * \file libcamera/fence.h
> > > + * \brief Synchronization fence
> > > + */
> > > +
> > > +/**
> > > + * \class Fence
> > > + * \brief Synchronization fence class
> >
> > Sweet, documentation :-)
> >
> > I'm not sure I like the name "synchronization fence". The "fence" is a
> > synchronization primitive, so "synchronization fence" sounds a bit
> > redundant. Maybe
> >
> >  * \brief Synchronization primitive to manage resources
> >
> > or something similar ? Same through the file.
> >
> > > + *
> > > + * The Fence class is a thin abstraction around a UniqueFD which simply allows
> > > + * to access it as a const pointer or to move its ownership to the caller.
> >
> > I've noticed many times a tendency (not just from you, it's general) to
> > focus on how a class or function is implemented, instead of what it
> > represents, what it does, and how it should be used. Here, I would start
> > with
> >
> >  * The Fence class models a synchronization primitive that can be used by
> >  * applications to explicitly synchronize resource usage, and can be shared by
> >  * multiple processes.
> >
> > > + *
> > > + * Synchronization fences are most commonly used in association with frame
> > > + * buffers. A FrameBuffer can be associated with a Fence (\sa
> > > + * Request::addBuffer()) so that the library can wait for the Fence to be
> > > + * signalled before allowing the camera device to actually access the memory
> > > + * area described by the FrameBuffer.
> > > + *
> > > + * By using synchronization fences, application can then synchronize between
> > > + * frame buffer consumers and produces, as in example a video output device and
> >
> > s/produces/producers/
> > s/in example/for example/
> > s/video output device/display device/
> >
> > > + * camera, to gurantee that new data transfers only happen once the existing
> >
> > s/camera/a camera/
> > s/gurantee/guarantee/
> >
> > > + * frames have been displayed.
> > > + *
> >
> > As the next paragraph is about internal usage of the Fence class, and
> > not about the application-facing API, I would move it to an \internal
> > block at the end. The rest of the documentation should be refactored a
> > bit to separate public and internal documentation.
> >
> > > + * The Fence class only abstracts the underlying mechanism used by libcamera to
> > > + * implement synchronization primitives and allow to wait until an event is
> >
> > s/allow/allows/
> >
> > > + * signalled, but does not implement event notification itself which is instead
> > > + * implemented by the core library.
> > > + *
> > > + * \sa Request::prepare()
> > > + *
> > > + * A Fence can be realized by different event notification primitives, the most
> > > + * common of which is represented by waiting for read events to happen on a
> > > + * file descriptor. This is currently the only mechanism supported by libcamera,
> > > + * but others can be implemented by extending or subclassing this class and
> > > + * implementing opportune handling in the core library.
> >
> > I think we should be a bit more precise here. It's not any file
> > descriptor, it's a handle to a kernel sync file (see sync_file.rst and
> > dma-buf.rst in Documentation/driver-api/).
> >
> > > + *
> > > + * The usage of the Fence class allows to abstract the underlying
> > > + * synchronization mechanism in use and implement an interface towards other
> > > + * library components that will not change when new synchronization primitives
> > > + * will be added as fences.
> > > + *
> > > + * A Fence is contructed with a UniqueFD whose ownship is moved in the Fence. A
> > > + * FrameBuffer can be associated with a Fence by passing it to the
> > > + * Request::addBuffer() function, which will move the Fence into the FrameBuffer
> > > + * itself. Once a Request is queued to the Camera, a preparation phase gurantees
> > > + * that before actually applying the Request to the hardware, all the valid
> > > + * fences of the frame buffers in a Request are correctly signalled. Once a
> > > + * Fence has completed, the library will reset the FrameBuffer fence so that
> > > + * application won't be allowed to access it.
> > > + *
> > > + * An optional timeout can be started while waiting for a fence to complete. If
> > > + * waiting on a Fence fails for whatever reason, the FrameBuffer's fence is not
> > > + * reset and is made available to application for them to handle it, by
> > > + * resetting the Fence to correctly close the underlying UniqueFD.
> > > + *
> > > + * A failure in waiting for a Fence to complete will result in the Request to
> > > + * complete in failed state.
> > > + */
> > > +
> > > +/**
> > > + * \brief Create a synchronization fence
> > > + * \param[in] fd The synchronization fence unique file descriptor
> >
> > s/unique //
> >
> > The fact that we manage the fd through a UniqueFD is more of an
> > implementation detail, I would just write "file descriptor" through the
> > documentation here and below.
> >
> > > + *
> > > + * The unique file descriptor ownership is moved to the Fence.
> > > + */
> > > +Fence::Fence(UniqueFD fd)
> > > +	: fd_(std::move(fd))
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \fn Fence::isValid()
> > > + * \brief Retrieve if a Fence is valid
> >
> > "Check if a Fence is valid" maybe ?
> >
> > > + *
> > > + * A Fence is valid if the UniqueFD it wraps is valid
> >
> > s/$/./
> >
> > > + *
> > > + * \return True if the Fence is valid, false otherwise
> > > + */
> > > +
> > > +/**
> > > + * \fn Fence::fd()
> > > + * \brief Retrieve a constant pointer to the UniqueFD
> > > + * \return A const pointer to the UniqueFD
> >
> > Could this be a reference instead of a pointer ? Looking at the
> > implementation, it can never be null.
> >
> > > + */
> > > +
> > > +/**
> > > + * \fn Fence::reset()
> > > + * \brief Reset the Fence by moving the UniqueFD ownership to the caller
> > > + *
> > > + * Reset the Fence by releasing the wrapped UniqueFD. Ownership of the UniqueFD
> > > + * is moved to the caller.
> >
> > Calling this release(), it should read
> >
> >  * \brief Release the ownership of the file descriptor
> >  *
> >  * Release the ownership of the wrapped UniqueFD and return it to the caller.
> >
> > > + *
> > > + * \return The UniqueFD
> > > + */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index b763110e74a6..b4882a2577f5 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',
> > >      'formats.cpp',
> > >      'framebuffer.cpp',
> > >      'framebuffer_allocator.cpp',