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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart