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

[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 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().
> 
> 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',

-- 
Regards,

Laurent Pinchart