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

9230 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

v4:
 - Use reference when unmapping, rather than a copy of the planes.
 - Move MappedPlane to MappedBuffer::Plane
 - Remove valid_
 - Make constructor protected
 - Rename move constructor parameters from rhs to other
 - Rework move constructor documentation.
---
 include/libcamera/internal/buffer.h |  47 +++++++++
 src/libcamera/buffer.cpp            | 152 +++++++++++++++++++++++++++-
 2 files changed, 198 insertions(+), 1 deletion(-)
 create mode 100644 include/libcamera/internal/buffer.h

Hi Kieran,

Thank you for the patch.

On Wed, Aug 05, 2020 at 04:14:57PM +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
> 
> v4:
>  - Use reference when unmapping, rather than a copy of the planes.
>  - Move MappedPlane to MappedBuffer::Plane
>  - Remove valid_
>  - Make constructor protected
>  - Rename move constructor parameters from rhs to other
>  - Rework move constructor documentation.
> ---
>  include/libcamera/internal/buffer.h |  47 +++++++++
>  src/libcamera/buffer.cpp            | 152 +++++++++++++++++++++++++++-
>  2 files changed, 198 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..b7b0173f93f5
> --- /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 {
> +
> +class MappedBuffer
> +{
> +public:
> +	using Plane = Span<uint8_t>;
> +
> +	~MappedBuffer();
> +
> +	MappedBuffer(MappedBuffer &&other);
> +	MappedBuffer &operator=(MappedBuffer &&other);
> +
> +	bool isValid() const { return error_ == 0; }
> +	int error() const { return error_; }
> +	const std::vector<Plane> &maps() const { return maps_; }
> +
> +protected:
> +	MappedBuffer();
> +
> +	int error_;
> +	std::vector<Plane> 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..8666f1992a77 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

I wonder, do you need a \brief after each \file entry ?

> + * \file libcamera/internal/buffer.h
>   * \brief Buffer handling
>   */
>  
> @@ -290,4 +292,152 @@ 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 the CPU.
> + *
> + * This class 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.
> + */
> +
> +/**
> + * \typedef MappedBuffer::Plane
> + * \brief A mapped region of memory accessible to the CPU
> + *
> + * The MappedBuffer::Plane uses the Span interface to describe the mapped memory
> + * region.
> + */
> +
> +/**
> + * \brief Construct an empty MappedBuffer
> + *
> + * Base class initialisation for MappedBuffer.

You can drop this sentence, still no need to explain C++ :-)

> + */
> +MappedBuffer::MappedBuffer()
> +	: error_(0)
> +{
> +}
> +
> +/**
> + * \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.
> + */
> +MappedBuffer::MappedBuffer(MappedBuffer &&other)
> +{
> +	*this = std::move(other);
> +}
> +
> +/**
> + * \brief Move assignment operator, replace the mappings with those of \a other
> + * mappings

Should it be s/other mappings/other/ ?

> + * \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.
> + */
> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
> +{
> +	error_ = other.error_;
> +	maps_ = std::move(other.maps_);
> +	other.error_ = -ENOENT;
> +
> +	return *this;
> +}
> +
> +MappedBuffer::~MappedBuffer()
> +{
> +	for (Plane &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

I think this function always succeeds :-) Maybe "\return The map error
code" ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + */
> +
> +/**
> + * \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::error_
> + * \brief Stores the error value if present
> + *
> + * MappedBuffer derived classes 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 derived classes 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 Map a FrameBuffer using the MappedBuffer interface
> + */
> +
> +/**
> + * \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);
> +	}
> +}
> +
>  } /* namespace libcamera */

Hi Laurent,

On 06/08/2020 13:38, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wed, Aug 05, 2020 at 04:14:57PM +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
>>
>> v4:
>>  - Use reference when unmapping, rather than a copy of the planes.
>>  - Move MappedPlane to MappedBuffer::Plane
>>  - Remove valid_
>>  - Make constructor protected
>>  - Rename move constructor parameters from rhs to other
>>  - Rework move constructor documentation.
>> ---
>>  include/libcamera/internal/buffer.h |  47 +++++++++
>>  src/libcamera/buffer.cpp            | 152 +++++++++++++++++++++++++++-
>>  2 files changed, 198 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..b7b0173f93f5
>> --- /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 {
>> +
>> +class MappedBuffer
>> +{
>> +public:
>> +	using Plane = Span<uint8_t>;
>> +
>> +	~MappedBuffer();
>> +
>> +	MappedBuffer(MappedBuffer &&other);
>> +	MappedBuffer &operator=(MappedBuffer &&other);
>> +
>> +	bool isValid() const { return error_ == 0; }
>> +	int error() const { return error_; }
>> +	const std::vector<Plane> &maps() const { return maps_; }
>> +
>> +protected:
>> +	MappedBuffer();
>> +
>> +	int error_;
>> +	std::vector<Plane> 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..8666f1992a77 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
> 
> I wonder, do you need a \brief after each \file entry ?

Actually, I can see that they should.

With this, only internal/buffer.h is described in the api-html.

> 
>> + * \file libcamera/internal/buffer.h
>>   * \brief Buffer handling

updating with

/**
 * \file libcamera/buffer.h
 * \brief Buffer handling
 *
 * \file libcamera/internal/buffer.h
 * \brief Internal buffer handling support
 */

Which now correctly renders in api-html/files.html


>>   */
>>  
>> @@ -290,4 +292,152 @@ 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 the CPU.
>> + *
>> + * This class 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.
>> + */
>> +
>> +/**
>> + * \typedef MappedBuffer::Plane
>> + * \brief A mapped region of memory accessible to the CPU
>> + *
>> + * The MappedBuffer::Plane uses the Span interface to describe the mapped memory
>> + * region.
>> + */
>> +
>> +/**
>> + * \brief Construct an empty MappedBuffer
>> + *
>> + * Base class initialisation for MappedBuffer.
> 
> You can drop this sentence, still no need to explain C++ :-)
> 

Dropped.


>> + */
>> +MappedBuffer::MappedBuffer()
>> +	: error_(0)
>> +{
>> +}
>> +
>> +/**
>> + * \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.
>> + */
>> +MappedBuffer::MappedBuffer(MappedBuffer &&other)
>> +{
>> +	*this = std::move(other);
>> +}
>> +
>> +/**
>> + * \brief Move assignment operator, replace the mappings with those of \a other
>> + * mappings
> 
> Should it be s/other mappings/other/ ?
> 

Indeed.


>> + * \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.
>> + */
>> +MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
>> +{
>> +	error_ = other.error_;
>> +	maps_ = std::move(other.maps_);
>> +	other.error_ = -ENOENT;
>> +
>> +	return *this;
>> +}
>> +
>> +MappedBuffer::~MappedBuffer()
>> +{
>> +	for (Plane &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
> 
> I think this function always succeeds :-) Maybe "\return The map error
> code" ?

Ooops, indeed it does.

Corrected,

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Collected,

And will push after a final re-test cycle.

Thanks.

--
Kieran


> 
>> + */
>> +
>> +/**
>> + * \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::error_
>> + * \brief Stores the error value if present
>> + *
>> + * MappedBuffer derived classes 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 derived classes 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 Map a FrameBuffer using the MappedBuffer interface
>> + */
>> +
>> +/**
>> + * \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);
>> +	}
>> +}
>> +
>>  } /* namespace libcamera */
>