[libcamera-devel,v3,03/13] libcamera: buffer: Create a MappedBuffer

9196 diff mbox series

Provide a MappedFrameBuffer helper class which will map
all of the Planes within a FrameBuffer and provide CPU addressable
pointers for those planes.

The MappedFrameBuffer implements the interface of the MappedBuffer
allowing other buffer types to be constructed of the same form, with a
common interface and cleanup.

This allows MappedBuffer instances to be created from Camera3Buffer types.

Mappings are removed upon destruction.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
v3
 - Documentation fixups
---
 include/libcamera/internal/buffer.h |  47 ++++++++
 src/libcamera/buffer.cpp            | 162 +++++++++++++++++++++++++++-
 2 files changed, 208 insertions(+), 1 deletion(-)
 create mode 100644 include/libcamera/internal/buffer.h

Hi Kieran,

On 04/08/2020 22:47, Kieran Bingham wrote:
> Provide a MappedFrameBuffer helper class which will map
> all of the Planes within a FrameBuffer and provide CPU addressable
> pointers for those planes.
> 
> The MappedFrameBuffer implements the interface of the MappedBuffer
> allowing other buffer types to be constructed of the same form, with a
> common interface and cleanup.
> 
> This allows MappedBuffer instances to be created from Camera3Buffer types.
> 
> Mappings are removed upon destruction.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v3
>  - Documentation fixups
> ---
>  include/libcamera/internal/buffer.h |  47 ++++++++
>  src/libcamera/buffer.cpp            | 162 +++++++++++++++++++++++++++-
>  2 files changed, 208 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/internal/buffer.h
> 
> diff --git a/include/libcamera/internal/buffer.h b/include/libcamera/internal/buffer.h
> new file mode 100644
> index 000000000000..e86a003cd8ba
> --- /dev/null
> +++ b/include/libcamera/internal/buffer.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * buffer.h - Internal buffer handling
> + */
> +#ifndef __LIBCAMERA_INTERNAL_BUFFER_H__
> +#define __LIBCAMERA_INTERNAL_BUFFER_H__
> +
> +#include <sys/mman.h>
> +#include <vector>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/span.h>
> +
> +namespace libcamera {
> +
> +using MappedPlane = Span<uint8_t>;
> +
> +class MappedBuffer
> +{
> +public:
> +	MappedBuffer();
> +	~MappedBuffer();
> +
> +	MappedBuffer(MappedBuffer &&rhs);
> +	MappedBuffer &operator=(MappedBuffer &&rhs);
> +
> +	bool isValid() const { return valid_; }
> +	int error() const { return error_; }
> +	const std::vector<MappedPlane> &maps() const { return maps_; }
> +
> +protected:
> +	int error_;
> +	bool valid_;
> +	std::vector<MappedPlane> maps_;
> +};
> +
> +class MappedFrameBuffer : public MappedBuffer
> +{
> +public:
> +	MappedFrameBuffer(const FrameBuffer *buffer, int flags);
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_BUFFER_H__ */
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 8278f8a92af4..5f7dff60a48b 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <libcamera/buffer.h>
> +#include "libcamera/internal/buffer.h"
>  
>  #include <errno.h>
>  #include <string.h>
> @@ -15,7 +16,8 @@
>  #include "libcamera/internal/log.h"
>  
>  /**
> - * \file buffer.h
> + * \file libcamera/buffer.h
> + * \file libcamera/internal/buffer.h
>   * \brief Buffer handling
>   */
>  
> @@ -290,4 +292,162 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)
>  	return 0;
>  }
>  
> +/**
> + * \class MappedBuffer
> + * \brief Provide an interface to support managing memory mapped buffers
> + *
> + * The MappedBuffer interface provides access to a set of MappedPlanes which
> + * are available for access by a CPU.
> + *
> + * The MappedBuffer interface does not implement any valid constructor but
> + * defines the move operators and destructors for the derived implementations,
> + * which are able to construct according to their derived types and given
> + * flags.
> + *
> + * This allows treating CPU accessible memory through a generic interface
> + * regardless of whether it originates from a libcamera FrameBuffer or other
> + * source.
> + */
> +
> +/**
> + * \brief Construct an empty MappedBuffer
> + *
> + * A default constructor is required to allow subclassing the MappedBuffer
> + * class. Construct an initialised, but invalid MappedBuffer.
> + */
> +MappedBuffer::MappedBuffer()
> +	: error_(0), valid_(false)
> +{
> +}
> +
> +/**
> + * \brief Construct a MappedBuffer by taking the \a rhs mappings
> + * \param[in] rhs The other MappedBuffer
> + *
> + * Moving a MappedBuffer moves the mappings contained in the \a rhs to the new
> + * MappedBuffer and invalidates the \a rhs. No mappings are unmapped or
> + * destroyed in this process.
> + */
> +MappedBuffer::MappedBuffer(MappedBuffer &&rhs)
> +{
> +	*this = std::move(rhs);
> +}
> +
> +/**
> + * \brief Move assingment operator, move a MappedBuffer by taking the \a rhs mappings
> + * \param[in] rhs The other MappedBuffer
> + *
> + * Moving a MappedBuffer moves the mappings contained in the \a rhs to the new
> + * MappedBuffer and invalidates the \a rhs. No mappings are unmapped or
> + * destroyed in this process.
> + */
> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&rhs)
> +{
> +	error_ = rhs.error_;
> +	valid_ = rhs.valid_;
> +	maps_ = std::move(rhs.maps_);
> +	rhs.valid_ = false;
> +	rhs.error_ = 0;
> +
> +	return *this;
> +}
> +
> +MappedBuffer::~MappedBuffer()
> +{
> +	for (MappedPlane map : maps_)

This should be:
	for (MappedPlane &map : maps_)

As has already been highlighted in an earlier review.

Now updated locally.



> +		munmap(map.data(), map.size());
> +}
> +
> +/**
> + * \fn MappedBuffer::isValid()
> + * \brief Check if the MappedBuffer instance is valid
> + * \return True if the MappedBuffer has valid mappings, false otherwise
> + */
> +
> +/**
> + * \fn MappedBuffer::error()
> + * \brief Retrieve the map error status
> + *
> + * This function retrieves the error status from the MappedBuffer.
> + * The error status is a negative number as defined by errno.h. If
> + * no error occurred, this function returns 0.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \fn MappedBuffer::maps()
> + * \brief Retrieve the mapped planes
> + *
> + * This function retrieves the successfully mapped planes stored as a vector
> + * of Span<uint8_t> to provide access to the mapped memory.
> + *
> + * \return A vector of the mapped planes.
> + */
> +
> +/**
> + * \var MappedBuffer::valid_
> + * \brief Stores the status of the mapping
> + *
> + * MappedBuffer implementations shall set this to represent if the mapping
> + * was successfully completed without errors.
> + */
> +
> +/**
> + * \var MappedBuffer::error_
> + * \brief Stores the error value if present
> + *
> + * MappedBuffer implementations shall set this to a negative value as defined
> + * by errno.h if an error occured during the mapping process
> + */
> +
> +/**
> + * \var MappedBuffer::maps_
> + * \brief Stores the internal
> + *
> + * MappedBuffer implementations shall store the mappings they create in this
> + * vector which is parsed during destruct to unmap any memory mappings which
> + * completed successfully.
> + */
> +
> +/**
> + * \class MappedFrameBuffer
> + * \brief Maps a FrameBuffer using the MappedBuffer interface
> + *
> + * The MappedFrameBuffer interface maps a FrameBuffer instance
> + *
> + * The MappedBuffer interface does not implement any constructor but defines
> + * the move operators and destructors for the derived implementations, which
> + * are able to construct according to their derived types and given flags.
> + */
> +
> +/**
> + * \brief Map all planes of a FrameBuffer
> + * \param[in] buffer FrameBuffer to be mapped
> + * \param[in] flags Protection flags to apply to map
> + *
> + * Construct an object to map a frame buffer for CPU access.
> + * The flags are passed directly to mmap and should be either PROT_READ,
> + * PROT_WRITE, or a bitwise-or combination of both.
> + */
> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
> +{
> +	maps_.reserve(buffer->planes().size());
> +
> +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> +		void *address = mmap(nullptr, plane.length, flags,
> +				     MAP_SHARED, plane.fd.fd(), 0);
> +
> +		if (address == MAP_FAILED) {
> +			error_ = -errno;
> +			LOG(Buffer, Error) << "Failed to mmap plane";
> +			break;
> +		}
> +
> +		maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> +	}
> +
> +	valid_ = buffer->planes().size() == maps_.size();
> +}
> +
>  } /* namespace libcamera */
>

Hi Kieran,

Thank you for the patch.

On Tue, Aug 04, 2020 at 10:47:01PM +0100, Kieran Bingham wrote:
> Provide a MappedFrameBuffer helper class which will map
> all of the Planes within a FrameBuffer and provide CPU addressable
> pointers for those planes.
> 
> The MappedFrameBuffer implements the interface of the MappedBuffer
> allowing other buffer types to be constructed of the same form, with a
> common interface and cleanup.
> 
> This allows MappedBuffer instances to be created from Camera3Buffer types.
> 
> Mappings are removed upon destruction.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v3
>  - Documentation fixups
> ---
>  include/libcamera/internal/buffer.h |  47 ++++++++
>  src/libcamera/buffer.cpp            | 162 +++++++++++++++++++++++++++-
>  2 files changed, 208 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/internal/buffer.h
> 
> diff --git a/include/libcamera/internal/buffer.h b/include/libcamera/internal/buffer.h
> new file mode 100644
> index 000000000000..e86a003cd8ba
> --- /dev/null
> +++ b/include/libcamera/internal/buffer.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * buffer.h - Internal buffer handling
> + */
> +#ifndef __LIBCAMERA_INTERNAL_BUFFER_H__
> +#define __LIBCAMERA_INTERNAL_BUFFER_H__
> +
> +#include <sys/mman.h>
> +#include <vector>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/span.h>
> +
> +namespace libcamera {
> +
> +using MappedPlane = Span<uint8_t>;
> +
> +class MappedBuffer
> +{
> +public:

How about moving MappedPlane here, and naming it Plane ?

	using Plane = Span<uint8_t>;

This makes it clear that Plane is meant to be used with MappedBuffer.

> +	MappedBuffer();

Do you want to allow construction of the base class, or only of derived
classes ? In the latter case, you should move this constructor to the
protected section below.

> +	~MappedBuffer();
> +
> +	MappedBuffer(MappedBuffer &&rhs);
> +	MappedBuffer &operator=(MappedBuffer &&rhs);

The parameter is customarily called other for the copy and move
constructors and assignment operators. rhs is usually used in
conjunction with lhs for non-member operators.

> +
> +	bool isValid() const { return valid_; }
> +	int error() const { return error_; }
> +	const std::vector<MappedPlane> &maps() const { return maps_; }
> +
> +protected:
> +	int error_;
> +	bool valid_;

Do you need both ? Could isValid() return error_ == 0 ?

> +	std::vector<MappedPlane> maps_;
> +};
> +
> +class MappedFrameBuffer : public MappedBuffer
> +{
> +public:
> +	MappedFrameBuffer(const FrameBuffer *buffer, int flags);
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_BUFFER_H__ */
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 8278f8a92af4..5f7dff60a48b 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <libcamera/buffer.h>
> +#include "libcamera/internal/buffer.h"
>  
>  #include <errno.h>
>  #include <string.h>
> @@ -15,7 +16,8 @@
>  #include "libcamera/internal/log.h"
>  
>  /**
> - * \file buffer.h
> + * \file libcamera/buffer.h
> + * \file libcamera/internal/buffer.h
>   * \brief Buffer handling
>   */
>  
> @@ -290,4 +292,162 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)
>  	return 0;
>  }
>  
> +/**
> + * \class MappedBuffer
> + * \brief Provide an interface to support managing memory mapped buffers
> + *
> + * The MappedBuffer interface provides access to a set of MappedPlanes which
> + * are available for access by a CPU.

s/a CPU/the CPU/ ?

You're clearly thinking as a kernel developer :-)

> + *
> + * The MappedBuffer interface does not implement any valid constructor but
> + * defines the move operators and destructors for the derived implementations,
> + * which are able to construct according to their derived types and given
> + * flags.

That's a bit of implementation details. Maybe "This class is not meant
to be constructed directly, and thus has no public default constructor.
Derived classes shall be used instead." ?

> + *
> + * This allows treating CPU accessible memory through a generic interface
> + * regardless of whether it originates from a libcamera FrameBuffer or other
> + * source.
> + */
> +
> +/**
> + * \brief Construct an empty MappedBuffer
> + *
> + * A default constructor is required to allow subclassing the MappedBuffer
> + * class. Construct an initialised, but invalid MappedBuffer.

You can drop the first sentence, you're explaining C++ :-)

> + */
> +MappedBuffer::MappedBuffer()
> +	: error_(0), valid_(false)
> +{
> +}
> +
> +/**
> + * \brief Construct a MappedBuffer by taking the \a rhs mappings

Construct the MappedBuffer with the contents of \a other using move
semantics

(This is the usual way to document move constructors in
https://en.cppreference.com/, which we're mimicking in libcamera.)

> + * \param[in] rhs The other MappedBuffer
> + *
> + * Moving a MappedBuffer moves the mappings contained in the \a rhs to the new
> + * MappedBuffer and invalidates the \a rhs. No mappings are unmapped or
> + * destroyed in this process.
> + */
> +MappedBuffer::MappedBuffer(MappedBuffer &&rhs)
> +{
> +	*this = std::move(rhs);
> +}
> +
> +/**
> + * \brief Move assingment operator, move a MappedBuffer by taking the \a rhs mappings

s/assingment/assignement/

But for the same reason as above,

Replace the contents with those of \a other using move semantics

> + * \param[in] rhs The other MappedBuffer
> + *
> + * Moving a MappedBuffer moves the mappings contained in the \a rhs to the new

s/the \a rhs/\a other/
s/ new// (this is not a newly constructed instance)

> + * MappedBuffer and invalidates the \a rhs. No mappings are unmapped or

s/the \a rhs/\a other/

> + * destroyed in this process.
> + */
> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&rhs)
> +{
> +	error_ = rhs.error_;
> +	valid_ = rhs.valid_;
> +	maps_ = std::move(rhs.maps_);
> +	rhs.valid_ = false;
> +	rhs.error_ = 0;
> +
> +	return *this;
> +}
> +
> +MappedBuffer::~MappedBuffer()
> +{
> +	for (MappedPlane map : maps_)
> +		munmap(map.data(), map.size());
> +}
> +
> +/**
> + * \fn MappedBuffer::isValid()
> + * \brief Check if the MappedBuffer instance is valid
> + * \return True if the MappedBuffer has valid mappings, false otherwise
> + */
> +
> +/**
> + * \fn MappedBuffer::error()
> + * \brief Retrieve the map error status
> + *
> + * This function retrieves the error status from the MappedBuffer.
> + * The error status is a negative number as defined by errno.h. If
> + * no error occurred, this function returns 0.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \fn MappedBuffer::maps()
> + * \brief Retrieve the mapped planes
> + *
> + * This function retrieves the successfully mapped planes stored as a vector
> + * of Span<uint8_t> to provide access to the mapped memory.
> + *
> + * \return A vector of the mapped planes.

s/.$//

> + */
> +
> +/**
> + * \var MappedBuffer::valid_
> + * \brief Stores the status of the mapping
> + *
> + * MappedBuffer implementations shall set this to represent if the mapping

s/implementations/derived classes/ ? Same below.

> + * was successfully completed without errors.
> + */
> +
> +/**
> + * \var MappedBuffer::error_
> + * \brief Stores the error value if present
> + *
> + * MappedBuffer implementations shall set this to a negative value as defined
> + * by errno.h if an error occured during the mapping process
> + */
> +
> +/**
> + * \var MappedBuffer::maps_
> + * \brief Stores the internal
> + *
> + * MappedBuffer implementations shall store the mappings they create in this
> + * vector which is parsed during destruct to unmap any memory mappings which
> + * completed successfully.
> + */
> +
> +/**
> + * \class MappedFrameBuffer
> + * \brief Maps a FrameBuffer using the MappedBuffer interface

s/Maps/Map/

> + *
> + * The MappedFrameBuffer interface maps a FrameBuffer instance

s/$/./

Or maybe drop the sentence as it duplicates the brief.

> + *
> + * The MappedBuffer interface does not implement any constructor but defines
> + * the move operators and destructors for the derived implementations, which
> + * are able to construct according to their derived types and given flags.

Is this a leftover from a copy & paste ? I think ou can drop this
paragraph.

> + */
> +
> +/**
> + * \brief Map all planes of a FrameBuffer
> + * \param[in] buffer FrameBuffer to be mapped
> + * \param[in] flags Protection flags to apply to map
> + *
> + * Construct an object to map a frame buffer for CPU access.
> + * The flags are passed directly to mmap and should be either PROT_READ,
> + * PROT_WRITE, or a bitwise-or combination of both.
> + */
> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
> +{
> +	maps_.reserve(buffer->planes().size());
> +
> +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> +		void *address = mmap(nullptr, plane.length, flags,
> +				     MAP_SHARED, plane.fd.fd(), 0);
> +

You can drop the blank line.

> +		if (address == MAP_FAILED) {
> +			error_ = -errno;
> +			LOG(Buffer, Error) << "Failed to mmap plane";
> +			break;
> +		}
> +
> +		maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> +	}
> +
> +	valid_ = buffer->planes().size() == maps_.size();
> +}
> +
>  } /* namespace libcamera */

Hi Laurent,

On 05/08/2020 01:29, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tue, Aug 04, 2020 at 10:47:01PM +0100, Kieran Bingham wrote:
>> Provide a MappedFrameBuffer helper class which will map
>> all of the Planes within a FrameBuffer and provide CPU addressable
>> pointers for those planes.
>>
>> The MappedFrameBuffer implements the interface of the MappedBuffer
>> allowing other buffer types to be constructed of the same form, with a
>> common interface and cleanup.
>>
>> This allows MappedBuffer instances to be created from Camera3Buffer types.
>>
>> Mappings are removed upon destruction.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> ---
>> v3
>>  - Documentation fixups
>> ---
>>  include/libcamera/internal/buffer.h |  47 ++++++++
>>  src/libcamera/buffer.cpp            | 162 +++++++++++++++++++++++++++-
>>  2 files changed, 208 insertions(+), 1 deletion(-)
>>  create mode 100644 include/libcamera/internal/buffer.h
>>
>> diff --git a/include/libcamera/internal/buffer.h b/include/libcamera/internal/buffer.h
>> new file mode 100644
>> index 000000000000..e86a003cd8ba
>> --- /dev/null
>> +++ b/include/libcamera/internal/buffer.h
>> @@ -0,0 +1,47 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * buffer.h - Internal buffer handling
>> + */
>> +#ifndef __LIBCAMERA_INTERNAL_BUFFER_H__
>> +#define __LIBCAMERA_INTERNAL_BUFFER_H__
>> +
>> +#include <sys/mman.h>
>> +#include <vector>
>> +
>> +#include <libcamera/buffer.h>
>> +#include <libcamera/span.h>
>> +
>> +namespace libcamera {
>> +
>> +using MappedPlane = Span<uint8_t>;
>> +
>> +class MappedBuffer
>> +{
>> +public:
> 
> How about moving MappedPlane here, and naming it Plane ?
> 
> 	using Plane = Span<uint8_t>;
> 
> This makes it clear that Plane is meant to be used with MappedBuffer.

Defining it in the class namespace certainly makes sense. I was a bit
weary of using the name Plane, as it could conflict with our other Plane
types, but as it's scoped inside the MappedBuffer, I think that's OK,
and after all - it is a Plane.


>> +	MappedBuffer();
> 
> Do you want to allow construction of the base class, or only of derived
> classes ? In the latter case, you should move this constructor to the
> protected section below.

Hrm, I thought I needed this public to support STL construction of
vectors or emplace or something.

Looks like it's working in protected, so I'll move it there.


>> +	~MappedBuffer();
>> +
>> +	MappedBuffer(MappedBuffer &&rhs);
>> +	MappedBuffer &operator=(MappedBuffer &&rhs);
> 
> The parameter is customarily called other for the copy and move
> constructors and assignment operators. rhs is usually used in
> conjunction with lhs for non-member operators.

Renamed.

> 
>> +
>> +	bool isValid() const { return valid_; }
>> +	int error() const { return error_; }
>> +	const std::vector<MappedPlane> &maps() const { return maps_; }
>> +
>> +protected:
>> +	int error_;
>> +	bool valid_;
> 
> Do you need both ? Could isValid() return error_ == 0 ?


I can do that, I will initialise error_ to -ENOENT in
MappedBuffer::MappedBuffer().



>> +	std::vector<MappedPlane> maps_;
>> +};
>> +
>> +class MappedFrameBuffer : public MappedBuffer
>> +{
>> +public:
>> +	MappedFrameBuffer(const FrameBuffer *buffer, int flags);
>> +};
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_INTERNAL_BUFFER_H__ */
>> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
>> index 8278f8a92af4..5f7dff60a48b 100644
>> --- a/src/libcamera/buffer.cpp
>> +++ b/src/libcamera/buffer.cpp
>> @@ -6,6 +6,7 @@
>>   */
>>  
>>  #include <libcamera/buffer.h>
>> +#include "libcamera/internal/buffer.h"
>>  
>>  #include <errno.h>
>>  #include <string.h>
>> @@ -15,7 +16,8 @@
>>  #include "libcamera/internal/log.h"
>>  
>>  /**
>> - * \file buffer.h
>> + * \file libcamera/buffer.h
>> + * \file libcamera/internal/buffer.h
>>   * \brief Buffer handling
>>   */
>>  
>> @@ -290,4 +292,162 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * \class MappedBuffer
>> + * \brief Provide an interface to support managing memory mapped buffers
>> + *
>> + * The MappedBuffer interface provides access to a set of MappedPlanes which
>> + * are available for access by a CPU.
> 
> s/a CPU/the CPU/ ?
> 
> You're clearly thinking as a kernel developer :-)

Well, sometimes ;-) I mean 'the' cpu is 'a' cpu ;-)

Also, I guess it depends on how you name the multiple cores/threads from
userspace.

/sys/bus/cpu/devices/cpuN implies that each available core is a 'CPU',
and it would be accessible from each of those CPU's...


Of course, Mapping on 'my' CPU wouldn't be available for access on
'your' CPU which is also 'a' CPU... so I think 'the CPU' is good.


>> + *
>> + * The MappedBuffer interface does not implement any valid constructor but
>> + * defines the move operators and destructors for the derived implementations,
>> + * which are able to construct according to their derived types and given
>> + * flags.
> 
> That's a bit of implementation details. Maybe "This class is not meant
> to be constructed directly, and thus has no public default constructor.
> Derived classes shall be used instead." ?
> 


Replaced with:


 * The MappedBuffer interface provides access to a set of MappedPlanes which
 * are available for access by the CPU.
 *
 * The MappedBuffer is not meant to be constructed directly, but instead
derived
 * classes should be used to implement the correct mapping of a source
buffer.
 *
 * This allows treating CPU accessible memory through a generic interface
 * regardless of whether it originates from a libcamera FrameBuffer or other
 * source.


>> + *
>> + * This allows treating CPU accessible memory through a generic interface
>> + * regardless of whether it originates from a libcamera FrameBuffer or other
>> + * source.
>> + */
>> +
>> +/**
>> + * \brief Construct an empty MappedBuffer
>> + *
>> + * A default constructor is required to allow subclassing the MappedBuffer
>> + * class. Construct an initialised, but invalid MappedBuffer.
> 
> You can drop the first sentence, you're explaining C++ :-)

Well someone's got to explain it to me ... it may as well be me ;-)

(dropped).

>> + */
>> +MappedBuffer::MappedBuffer()
>> +	: error_(0), valid_(false)


Actually, now that this is protected, all this really does is initialise
the base class, and that's now only : error_(0).

I've considered making this error_(-ENOENT), so it starts off invalid,
but the pattern used in both existing constructors for this now set
error_ = 0; and set the error if an error occurs...

So I think this is better left as error_(0), and update the function doc
to just:

/**
 * \brief Construct an empty MappedBuffer
 *
 * Base class initialisation for MappedBuffer.
 */


>> +{
>> +}
>> +
>> +/**
>> + * \brief Construct a MappedBuffer by taking the \a rhs mappings
> 
> Construct the MappedBuffer with the contents of \a other using move
> semantics
> 
> (This is the usual way to document move constructors in
> https://en.cppreference.com/, which we're mimicking in libcamera.)

It's new to me that we are 'mimicking' en.cppreference.com.
Maybe that needs to be documented somehow.

I think I based my original text templates on the file_descriptor
constructors you authored, which differ slightly:

* \brief Move constructor, create a FileDescriptor by taking over \a other

...



>> + * \param[in] rhs The other MappedBuffer
>> + *
>> + * Moving a MappedBuffer moves the mappings contained in the \a rhs to the new
>> + * MappedBuffer and invalidates the \a rhs. No mappings are unmapped or
>> + * destroyed in this process.
>> + */
>> +MappedBuffer::MappedBuffer(MappedBuffer &&rhs)
>> +{
>> +	*this = std::move(rhs);
>> +}
>> +
>> +/**
>> + * \brief Move assingment operator, move a MappedBuffer by taking the \a rhs mappings
> 
> s/assingment/assignement/
> 
> But for the same reason as above,
> 
> Replace the contents with those of \a other using move semantics

Again, this was based upon text from file_descriptor:

 * \brief Move assignment operator, replace the wrapped file descriptor
 * by taking over \a other

I think I like keeping the 'name of the constructor' to make it clear.

It's not quickly clear to me looking at
MappedBuffer::MappedBuffer(MappedBuffer &&rhs) 'which' type of
constructor it is, so I think that's important.


>> + * \param[in] rhs The other MappedBuffer
>> + *
>> + * Moving a MappedBuffer moves the mappings contained in the \a rhs to the new
> 
> s/the \a rhs/\a other/
> s/ new// (this is not a newly constructed instance)

likewise here, the wording was based on text you already wrote.

You wrote:

 * Moving a FileDescriptor moves the reference to the wrapped descriptor
 * owned by \a other to the new FileDescriptor.


This is exhausting. Trying to emulate your text so you don't rewrite it,
but then you rewrite it anyway ;-)



>> + * MappedBuffer and invalidates the \a rhs. No mappings are unmapped or
> 
> s/the \a rhs/\a other/



Ok, so the new text for both of those is:



/**
 * \brief Move constructor, construct the MappedBuffer with the contents
of \a
 * other using move semantics
 * \param[in] other The other MappedBuffer
 *
 * Moving a MappedBuffer moves the mappings contained in the \a other to
the new
 * MappedBuffer and invalidates the \a other.
 *
 * No mappings are unmapped or destroyed in this process.
 */


/**
 * \brief Move assignment operator, replace the mappings with those of
\a other
 * mappings
 * \param[in] other The other MappedBuffer
 *
 * Moving a MappedBuffer moves the mappings contained in the \a other to
the new
 * MappedBuffer and invalidates the \a other.
 *
 * No mappings are unmapped or destroyed in this process.
 */



>> + * destroyed in this process.
>> + */
>> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&rhs)
>> +{
>> +	error_ = rhs.error_;
>> +	valid_ = rhs.valid_;
>> +	maps_ = std::move(rhs.maps_);
>> +	rhs.valid_ = false;
>> +	rhs.error_ = 0;
>> +
>> +	return *this;
>> +}
>> +
>> +MappedBuffer::~MappedBuffer()
>> +{
>> +	for (MappedPlane map : maps_)
>> +		munmap(map.data(), map.size());
>> +}
>> +
>> +/**
>> + * \fn MappedBuffer::isValid()
>> + * \brief Check if the MappedBuffer instance is valid
>> + * \return True if the MappedBuffer has valid mappings, false otherwise
>> + */
>> +
>> +/**
>> + * \fn MappedBuffer::error()
>> + * \brief Retrieve the map error status
>> + *
>> + * This function retrieves the error status from the MappedBuffer.
>> + * The error status is a negative number as defined by errno.h. If
>> + * no error occurred, this function returns 0.
>> + *
>> + * \return 0 on success or a negative error code otherwise
>> + */
>> +
>> +/**
>> + * \fn MappedBuffer::maps()
>> + * \brief Retrieve the mapped planes
>> + *
>> + * This function retrieves the successfully mapped planes stored as a vector
>> + * of Span<uint8_t> to provide access to the mapped memory.
>> + *
>> + * \return A vector of the mapped planes.
> 
> s/.$//
> 

Removed.

>> + */
>> +
>> +/**
>> + * \var MappedBuffer::valid_
>> + * \brief Stores the status of the mapping
>> + *
>> + * MappedBuffer implementations shall set this to represent if the mapping
> 
> s/implementations/derived classes/ ? Same below.
> 

valid_ removed.


>> + * was successfully completed without errors.
>> + */
>> +
>> +/**
>> + * \var MappedBuffer::error_
>> + * \brief Stores the error value if present
>> + *
>> + * MappedBuffer implementations shall set this to a negative value as defined
>> + * by errno.h if an error occured during the mapping process
>> + */
>> +
>> +/**
>> + * \var MappedBuffer::maps_
>> + * \brief Stores the internal
>> + *
>> + * MappedBuffer implementations shall store the mappings they create in this
>> + * vector which is parsed during destruct to unmap any memory mappings which
>> + * completed successfully.
>> + */
>> +
>> +/**
>> + * \class MappedFrameBuffer
>> + * \brief Maps a FrameBuffer using the MappedBuffer interface
> 
> s/Maps/Map/
> 
>> + *
>> + * The MappedFrameBuffer interface maps a FrameBuffer instance
> 
> s/$/./
> 
> Or maybe drop the sentence as it duplicates the brief.
> 
>> + *
>> + * The MappedBuffer interface does not implement any constructor but defines
>> + * the move operators and destructors for the derived implementations, which
>> + * are able to construct according to their derived types and given flags.
> 
> Is this a leftover from a copy & paste ? I think ou can drop this
> paragraph.


Dropping it leaves just the brief. Maybe that's fine. But it seems a bit
brief ;-)

I thought the point of this section is to expand on the purpose of the
class as an overview, so I thought that paragraph was relevant as a summary.


>> + */
>> +
>> +/**
>> + * \brief Map all planes of a FrameBuffer
>> + * \param[in] buffer FrameBuffer to be mapped
>> + * \param[in] flags Protection flags to apply to map
>> + *
>> + * Construct an object to map a frame buffer for CPU access.
>> + * The flags are passed directly to mmap and should be either PROT_READ,
>> + * PROT_WRITE, or a bitwise-or combination of both.
>> + */
>> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
>> +{
>> +	maps_.reserve(buffer->planes().size());
>> +
>> +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
>> +		void *address = mmap(nullptr, plane.length, flags,
>> +				     MAP_SHARED, plane.fd.fd(), 0);
>> +
> 
> You can drop the blank line.

I liked that one, but dropped.


> 
>> +		if (address == MAP_FAILED) {
>> +			error_ = -errno;
>> +			LOG(Buffer, Error) << "Failed to mmap plane";
>> +			break;
>> +		}
>> +
>> +		maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
>> +	}
>> +
>> +	valid_ = buffer->planes().size() == maps_.size();
>> +}
>> +
>>  } /* namespace libcamera */
>

Hi Laurent,

On 05/08/2020 12:27, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 05/08/2020 01:29, Laurent Pinchart wrote:
>> Hi Kieran,
>>
>> Thank you for the patch.
>>
>> On Tue, Aug 04, 2020 at 10:47:01PM +0100, Kieran Bingham wrote:
>>> Provide a MappedFrameBuffer helper class which will map
>>> all of the Planes within a FrameBuffer and provide CPU addressable
>>> pointers for those planes.
>>>
>>> The MappedFrameBuffer implements the interface of the MappedBuffer
>>> allowing other buffer types to be constructed of the same form, with a
>>> common interface and cleanup.
>>>
>>> This allows MappedBuffer instances to be created from Camera3Buffer types.
>>>
>>> Mappings are removed upon destruction.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>
>>> ---
>>> v3
>>>  - Documentation fixups
>>> ---
>>>  include/libcamera/internal/buffer.h |  47 ++++++++
>>>  src/libcamera/buffer.cpp            | 162 +++++++++++++++++++++++++++-
>>>  2 files changed, 208 insertions(+), 1 deletion(-)
>>>  create mode 100644 include/libcamera/internal/buffer.h
>>>
>>> diff --git a/include/libcamera/internal/buffer.h b/include/libcamera/internal/buffer.h
>>> new file mode 100644
>>> index 000000000000..e86a003cd8ba
>>> --- /dev/null
>>> +++ b/include/libcamera/internal/buffer.h
>>> @@ -0,0 +1,47 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2020, Google Inc.
>>> + *
>>> + * buffer.h - Internal buffer handling
>>> + */
>>> +#ifndef __LIBCAMERA_INTERNAL_BUFFER_H__
>>> +#define __LIBCAMERA_INTERNAL_BUFFER_H__
>>> +
>>> +#include <sys/mman.h>
>>> +#include <vector>
>>> +
>>> +#include <libcamera/buffer.h>
>>> +#include <libcamera/span.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +using MappedPlane = Span<uint8_t>;
>>> +
>>> +class MappedBuffer
>>> +{
>>> +public:
>>
>> How about moving MappedPlane here, and naming it Plane ?
>>
>> 	using Plane = Span<uint8_t>;
>>
>> This makes it clear that Plane is meant to be used with MappedBuffer.
> 
> Defining it in the class namespace certainly makes sense. I was a bit
> weary of using the name Plane, as it could conflict with our other Plane
> types, but as it's scoped inside the MappedBuffer, I think that's OK,
> and after all - it is a Plane.
> 
> 
>>> +	MappedBuffer();
>>
>> Do you want to allow construction of the base class, or only of derived
>> classes ? In the latter case, you should move this constructor to the
>> protected section below.
> 
> Hrm, I thought I needed this public to support STL construction of
> vectors or emplace or something.
> 
> Looks like it's working in protected, so I'll move it there.
> 
> 
>>> +	~MappedBuffer();
>>> +
>>> +	MappedBuffer(MappedBuffer &&rhs);
>>> +	MappedBuffer &operator=(MappedBuffer &&rhs);
>>
>> The parameter is customarily called other for the copy and move
>> constructors and assignment operators. rhs is usually used in
>> conjunction with lhs for non-member operators.
> 
> Renamed.
> 
>>
>>> +
>>> +	bool isValid() const { return valid_; }
>>> +	int error() const { return error_; }
>>> +	const std::vector<MappedPlane> &maps() const { return maps_; }
>>> +
>>> +protected:
>>> +	int error_;
>>> +	bool valid_;
>>
>> Do you need both ? Could isValid() return error_ == 0 ?
> 
> 
> I can do that, I will initialise error_ to -ENOENT in
> MappedBuffer::MappedBuffer().
> 
> 
> 
>>> +	std::vector<MappedPlane> maps_;
>>> +};
>>> +
>>> +class MappedFrameBuffer : public MappedBuffer
>>> +{
>>> +public:
>>> +	MappedFrameBuffer(const FrameBuffer *buffer, int flags);
>>> +};
>>> +
>>> +} /* namespace libcamera */
>>> +
>>> +#endif /* __LIBCAMERA_INTERNAL_BUFFER_H__ */
>>> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
>>> index 8278f8a92af4..5f7dff60a48b 100644
>>> --- a/src/libcamera/buffer.cpp
>>> +++ b/src/libcamera/buffer.cpp
>>> @@ -6,6 +6,7 @@
>>>   */
>>>  
>>>  #include <libcamera/buffer.h>
>>> +#include "libcamera/internal/buffer.h"
>>>  
>>>  #include <errno.h>
>>>  #include <string.h>
>>> @@ -15,7 +16,8 @@
>>>  #include "libcamera/internal/log.h"
>>>  
>>>  /**
>>> - * \file buffer.h
>>> + * \file libcamera/buffer.h
>>> + * \file libcamera/internal/buffer.h
>>>   * \brief Buffer handling
>>>   */
>>>  
>>> @@ -290,4 +292,162 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)
>>>  	return 0;
>>>  }
>>>  
>>> +/**
>>> + * \class MappedBuffer
>>> + * \brief Provide an interface to support managing memory mapped buffers
>>> + *
>>> + * The MappedBuffer interface provides access to a set of MappedPlanes which
>>> + * are available for access by a CPU.
>>
>> s/a CPU/the CPU/ ?
>>
>> You're clearly thinking as a kernel developer :-)
> 
> Well, sometimes ;-) I mean 'the' cpu is 'a' cpu ;-)
> 
> Also, I guess it depends on how you name the multiple cores/threads from
> userspace.
> 
> /sys/bus/cpu/devices/cpuN implies that each available core is a 'CPU',
> and it would be accessible from each of those CPU's...
> 
> 
> Of course, Mapping on 'my' CPU wouldn't be available for access on
> 'your' CPU which is also 'a' CPU... so I think 'the CPU' is good.
> 
> 
>>> + *
>>> + * The MappedBuffer interface does not implement any valid constructor but
>>> + * defines the move operators and destructors for the derived implementations,
>>> + * which are able to construct according to their derived types and given
>>> + * flags.
>>
>> That's a bit of implementation details. Maybe "This class is not meant
>> to be constructed directly, and thus has no public default constructor.
>> Derived classes shall be used instead." ?
>>
> 
> 
> Replaced with:
> 
> 
>  * The MappedBuffer interface provides access to a set of MappedPlanes which
>  * are available for access by the CPU.
>  *
>  * The MappedBuffer is not meant to be constructed directly, but instead
> derived
>  * classes should be used to implement the correct mapping of a source
> buffer.
>  *
>  * This allows treating CPU accessible memory through a generic interface
>  * regardless of whether it originates from a libcamera FrameBuffer or other
>  * source.
> 
> 
>>> + *
>>> + * This allows treating CPU accessible memory through a generic interface
>>> + * regardless of whether it originates from a libcamera FrameBuffer or other
>>> + * source.
>>> + */
>>> +
>>> +/**
>>> + * \brief Construct an empty MappedBuffer
>>> + *
>>> + * A default constructor is required to allow subclassing the MappedBuffer
>>> + * class. Construct an initialised, but invalid MappedBuffer.
>>
>> You can drop the first sentence, you're explaining C++ :-)
> 
> Well someone's got to explain it to me ... it may as well be me ;-)
> 
> (dropped).
> 
>>> + */
>>> +MappedBuffer::MappedBuffer()
>>> +	: error_(0), valid_(false)
> 
> 
> Actually, now that this is protected, all this really does is initialise
> the base class, and that's now only : error_(0).
> 
> I've considered making this error_(-ENOENT), so it starts off invalid,
> but the pattern used in both existing constructors for this now set
> error_ = 0; and set the error if an error occurs...
> 
> So I think this is better left as error_(0), and update the function doc
> to just:
> 
> /**
>  * \brief Construct an empty MappedBuffer
>  *
>  * Base class initialisation for MappedBuffer.
>  */
> 
> 
>>> +{
>>> +}
>>> +
>>> +/**
>>> + * \brief Construct a MappedBuffer by taking the \a rhs mappings
>>
>> Construct the MappedBuffer with the contents of \a other using move
>> semantics
>>
>> (This is the usual way to document move constructors in
>> https://en.cppreference.com/, which we're mimicking in libcamera.)
> 
> It's new to me that we are 'mimicking' en.cppreference.com.
> Maybe that needs to be documented somehow.
> 
> I think I based my original text templates on the file_descriptor
> constructors you authored, which differ slightly:
> 
> * \brief Move constructor, create a FileDescriptor by taking over \a other
> 
> ...
> 
> 
> 
>>> + * \param[in] rhs The other MappedBuffer
>>> + *
>>> + * Moving a MappedBuffer moves the mappings contained in the \a rhs to the new
>>> + * MappedBuffer and invalidates the \a rhs. No mappings are unmapped or
>>> + * destroyed in this process.
>>> + */
>>> +MappedBuffer::MappedBuffer(MappedBuffer &&rhs)
>>> +{
>>> +	*this = std::move(rhs);
>>> +}
>>> +
>>> +/**
>>> + * \brief Move assingment operator, move a MappedBuffer by taking the \a rhs mappings
>>
>> s/assingment/assignement/
>>
>> But for the same reason as above,
>>
>> Replace the contents with those of \a other using move semantics
> 
> Again, this was based upon text from file_descriptor:
> 
>  * \brief Move assignment operator, replace the wrapped file descriptor
>  * by taking over \a other
> 
> I think I like keeping the 'name of the constructor' to make it clear.
> 
> It's not quickly clear to me looking at
> MappedBuffer::MappedBuffer(MappedBuffer &&rhs) 'which' type of
> constructor it is, so I think that's important.
> 
> 
>>> + * \param[in] rhs The other MappedBuffer
>>> + *
>>> + * Moving a MappedBuffer moves the mappings contained in the \a rhs to the new
>>
>> s/the \a rhs/\a other/
>> s/ new// (this is not a newly constructed instance)
> 
> likewise here, the wording was based on text you already wrote.
> 
> You wrote:
> 
>  * Moving a FileDescriptor moves the reference to the wrapped descriptor
>  * owned by \a other to the new FileDescriptor.
> 
> 
> This is exhausting. Trying to emulate your text so you don't rewrite it,
> but then you rewrite it anyway ;-)
> 
> 
> 
>>> + * MappedBuffer and invalidates the \a rhs. No mappings are unmapped or
>>
>> s/the \a rhs/\a other/
> 
> 
> 
> Ok, so the new text for both of those is:
> 
> 
> 
> /**
>  * \brief Move constructor, construct the MappedBuffer with the contents
> of \a
>  * other using move semantics
>  * \param[in] other The other MappedBuffer
>  *
>  * Moving a MappedBuffer moves the mappings contained in the \a other to
> the new
>  * MappedBuffer and invalidates the \a other.
>  *
>  * No mappings are unmapped or destroyed in this process.
>  */
> 
> 
> /**
>  * \brief Move assignment operator, replace the mappings with those of
> \a other
>  * mappings
>  * \param[in] other The other MappedBuffer
>  *
>  * Moving a MappedBuffer moves the mappings contained in the \a other to
> the new
>  * MappedBuffer and invalidates the \a other.
>  *
>  * No mappings are unmapped or destroyed in this process.
>  */
> 
> 
> 
>>> + * destroyed in this process.
>>> + */
>>> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&rhs)
>>> +{
>>> +	error_ = rhs.error_;
>>> +	valid_ = rhs.valid_;
>>> +	maps_ = std::move(rhs.maps_);
>>> +	rhs.valid_ = false;
>>> +	rhs.error_ = 0;
>>> +
>>> +	return *this;
>>> +}
>>> +
>>> +MappedBuffer::~MappedBuffer()
>>> +{
>>> +	for (MappedPlane map : maps_)
>>> +		munmap(map.data(), map.size());
>>> +}
>>> +
>>> +/**
>>> + * \fn MappedBuffer::isValid()
>>> + * \brief Check if the MappedBuffer instance is valid
>>> + * \return True if the MappedBuffer has valid mappings, false otherwise
>>> + */
>>> +
>>> +/**
>>> + * \fn MappedBuffer::error()
>>> + * \brief Retrieve the map error status
>>> + *
>>> + * This function retrieves the error status from the MappedBuffer.
>>> + * The error status is a negative number as defined by errno.h. If
>>> + * no error occurred, this function returns 0.
>>> + *
>>> + * \return 0 on success or a negative error code otherwise
>>> + */
>>> +
>>> +/**
>>> + * \fn MappedBuffer::maps()
>>> + * \brief Retrieve the mapped planes
>>> + *
>>> + * This function retrieves the successfully mapped planes stored as a vector
>>> + * of Span<uint8_t> to provide access to the mapped memory.
>>> + *
>>> + * \return A vector of the mapped planes.
>>
>> s/.$//
>>
> 
> Removed.
> 
>>> + */
>>> +
>>> +/**
>>> + * \var MappedBuffer::valid_
>>> + * \brief Stores the status of the mapping
>>> + *
>>> + * MappedBuffer implementations shall set this to represent if the mapping
>>
>> s/implementations/derived classes/ ? Same below.
>>
> 
> valid_ removed.
> 
> 
>>> + * was successfully completed without errors.
>>> + */
>>> +
>>> +/**
>>> + * \var MappedBuffer::error_
>>> + * \brief Stores the error value if present
>>> + *
>>> + * MappedBuffer implementations shall set this to a negative value as defined
>>> + * by errno.h if an error occured during the mapping process
>>> + */
>>> +
>>> +/**
>>> + * \var MappedBuffer::maps_
>>> + * \brief Stores the internal
>>> + *
>>> + * MappedBuffer implementations shall store the mappings they create in this
>>> + * vector which is parsed during destruct to unmap any memory mappings which
>>> + * completed successfully.
>>> + */
>>> +
>>> +/**
>>> + * \class MappedFrameBuffer
>>> + * \brief Maps a FrameBuffer using the MappedBuffer interface
>>
>> s/Maps/Map/
>>
>>> + *
>>> + * The MappedFrameBuffer interface maps a FrameBuffer instance
>>
>> s/$/./
>>
>> Or maybe drop the sentence as it duplicates the brief.
>>
>>> + *
>>> + * The MappedBuffer interface does not implement any constructor but defines
>>> + * the move operators and destructors for the derived implementations, which
>>> + * are able to construct according to their derived types and given flags.
>>
>> Is this a leftover from a copy & paste ? I think ou can drop this
>> paragraph.
> 
> 
> Dropping it leaves just the brief. Maybe that's fine. But it seems a bit
> brief ;-)
> 
> I thought the point of this section is to expand on the purpose of the
> class as an overview, so I thought that paragraph was relevant as a summary.

Aha, I just realised this is \class MappedFrameBuffer not \class
MappedBuffer ;-)

So given the lightweight intention of that class, indeed a brief is
probably sufficient.


> 
>>> + */
>>> +
>>> +/**
>>> + * \brief Map all planes of a FrameBuffer
>>> + * \param[in] buffer FrameBuffer to be mapped
>>> + * \param[in] flags Protection flags to apply to map
>>> + *
>>> + * Construct an object to map a frame buffer for CPU access.
>>> + * The flags are passed directly to mmap and should be either PROT_READ,
>>> + * PROT_WRITE, or a bitwise-or combination of both.
>>> + */
>>> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
>>> +{
>>> +	maps_.reserve(buffer->planes().size());
>>> +
>>> +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
>>> +		void *address = mmap(nullptr, plane.length, flags,
>>> +				     MAP_SHARED, plane.fd.fd(), 0);
>>> +
>>
>> You can drop the blank line.
> 
> I liked that one, but dropped.
> 
> 
>>
>>> +		if (address == MAP_FAILED) {
>>> +			error_ = -errno;
>>> +			LOG(Buffer, Error) << "Failed to mmap plane";
>>> +			break;
>>> +		}
>>> +
>>> +		maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
>>> +	}
>>> +
>>> +	valid_ = buffer->planes().size() == maps_.size();
>>> +}
>>> +
>>>  } /* namespace libcamera */
>>
>