[libcamera-devel] [PATCH v2 02/12] libcamera: fence: Introduce Fence

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sat, Nov 20, 2021 at 12:13:03PM +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 FileDescriptor 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              |  36 +++++++++
>  include/libcamera/internal/fence.h     |  37 +++++++++
>  include/libcamera/internal/meson.build |   1 +
>  include/libcamera/meson.build          |   1 +
>  src/libcamera/fence.cpp                | 104 +++++++++++++++++++++++++
>  src/libcamera/meson.build              |   1 +
>  6 files changed, 180 insertions(+)
>  create mode 100644 include/libcamera/fence.h
>  create mode 100644 include/libcamera/internal/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..5ae0ff6208d7
> --- /dev/null
> +++ b/include/libcamera/fence.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * internal/fence.h - Synchronization fence
> + */
> +#ifndef __LIBCAMERA_FENCE_H__
> +#define __LIBCAMERA_FENCE_H__
> +
> +#include <memory>
> +
> +#include <libcamera/base/class.h>
> +
> +#include <libcamera/file_descriptor.h>

You could use a forward declaration for FileDescriptor.

> +
> +namespace libcamera {
> +
> +class Fence : public Extensible
> +{
> +	LIBCAMERA_DECLARE_PRIVATE()
> +
> +public:
> +	Fence(const FileDescriptor &fd);
> +
> +	bool isValid() const;
> +	const FileDescriptor &fd();
> +
> +	void close();

This function is only used in test/fence.cpp.

> +
> +private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(Fence)
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_FENCE_H__ */
> diff --git a/include/libcamera/internal/fence.h b/include/libcamera/internal/fence.h
> new file mode 100644
> index 000000000000..5f03c0804d44
> --- /dev/null
> +++ b/include/libcamera/internal/fence.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * internal/fence.h - Internal synchronization fence
> + */
> +#ifndef __LIBCAMERA_INTERNAL_FENCE_H__
> +#define __LIBCAMERA_INTERNAL_FENCE_H__
> +
> +#include <memory>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/event_notifier.h>
> +
> +#include <libcamera/fence.h>
> +#include <libcamera/file_descriptor.h>
> +
> +namespace libcamera {
> +
> +class Fence::Private : public Extensible::Private
> +{
> +	LIBCAMERA_DECLARE_PUBLIC(Fence)
> +
> +public:
> +	Private(const FileDescriptor &fd);
> +
> +	bool isValid() const { return fd_.isValid(); }
> +	const FileDescriptor &fd() { return fd_; }

These two functions are only used by the Fence class. You can drop them
and access fd_ directly there. As a general rule, given that the
Extensible public class and the Private class are friends of each other,
we only need a public API in the Private class when used by other
classes internally in libcamera.

> +	void close();
> +
> +private:
> +	FileDescriptor fd_;
> +};
> +
> +} /* 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/include/libcamera/meson.build b/include/libcamera/meson.build
> index 7155ff203f6e..3721feb1ec92 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -7,6 +7,7 @@ libcamera_public_headers = files([
>      'camera_manager.h',
>      'compiler.h',
>      'controls.h',
> +    'fence.h',
>      'file_descriptor.h',
>      'framebuffer.h',
>      'framebuffer_allocator.h',
> diff --git a/src/libcamera/fence.cpp b/src/libcamera/fence.cpp
> new file mode 100644
> index 000000000000..9ad86b3fa115
> --- /dev/null
> +++ b/src/libcamera/fence.cpp
> @@ -0,0 +1,104 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * fence.cpp - Synchronization fence
> + */
> +
> +#include "libcamera/internal/fence.h"
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/thread.h>
> +
> +namespace libcamera {
> +
> +/**
> + *
> + * \file libcamera/fence.h
> + * \brief Synchronization fence

We need more documentation here, to explain what fences are about, how
they're used, ...

> + *
> + * \file internal/fence.h
> + * \brief Internal synchronization fence representation
> + */
> +
> +/**
> + * \class Fence::Private
> + * \brief Private synchronization Fence implementation
> + */
> +
> +/**
> + * \brief Construct a Fence::Private
> + * \param[in] fd The filedescriptor owned by the Fence
> + */
> +Fence::Private::Private(const FileDescriptor &fd)
> +	: fd_(std::move(fd))
> +{
> +}
> +
> +/**
> + * \fn Fence::Private::isValid()
> + * \brief Retrieve if a Fence is valid
> + *
> + * A Fence is valid if the FileDescriptor it wraps is valid
> + *
> + * \return True if the Fence is valid, false otherwise
> + */
> +
> +/**
> + * \fn Fence::Private::fd()
> + * \brief Retrieve a refernce to the FileDescriptor wrapped by this Fence
> + *
> + * \todo Document how to extract the FileDescriptor in case the Fence has failed
> + *
> + * \return A reference to the FileDescriptor this Fence wraps
> + */
> +
> +/**
> + * \brief Close the Fence by releasing the wrapped FileDescriptor
> + */
> +void Fence::Private::close()
> +{
> +	fd_ = FileDescriptor();
> +}
> +
> +/**
> + * \class Fence
> + * \brief Synchronization fence class
> + *
> + * \todo Documentation

Indeed :-) Same for the functions below.

> + */
> +
> +/**
> + * \brief Create a synchronization fence
> + * \param[in] fd The synchronization fence file descriptor
> + */
> +Fence::Fence(const FileDescriptor &fd)
> +	: Extensible(std::make_unique<Private>(fd))
> +{
> +}
> +
> +/**
> + * \copydoc Fence::Private::isValid()

As we'll likely drop the Private accessors we won't need copydoc, but if
we didn't, I'd document the public API in full, and copy the
documentation to the Private class.

> + */
> +bool Fence::isValid() const
> +{
> +	return _d()->isValid();
> +}
> +
> +/**
> + * \copydoc Fence::Private::fd()
> + */
> +const FileDescriptor &Fence::fd()
> +{
> +	return _d()->fd();
> +}
> +
> +/**
> + * \copydoc Fence::Private::close()
> + */
> +void Fence::close()
> +{
> +	_d()->close();
> +}
> +
> +} /* 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',

-- 
Regards,

Laurent Pinchart