[libcamera-devel] [RFC PATCH 1/8] libcamera: buffer: Create a MappedFrameBuffer

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Mon, Jul 20, 2020 at 11:42:25PM +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.
> 
> Mappings are removed upon destruction.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> 
> This introduces an automatically mapping/unmapping MappedFrameBuffer
> object, with a move constructor (essential to prevent un-desirable
> unmapping).
> 
> 
> 
>  include/libcamera/buffer.h | 23 ++++++++++++++++++++

Should this be an internal helper ? The main foreseen flow is for
applications to create frame buffers from dmabuf fds they recently
outside of libcamera, so I would expect them to handle memory mapping.
I'd thus rather start with an internal helper, which we could graduate
to a public API later if needed/desired.

>  src/libcamera/buffer.cpp   | 43 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 6bb2e4f8558f..881d736da2db 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -71,6 +71,29 @@ private:
>  	unsigned int cookie_;
>  };
>  
> +class MappedFrameBuffer {
> +public:
> +	MappedFrameBuffer(const FrameBuffer *buffer, int flags);
> +	~MappedFrameBuffer();
> +
> +	/* Move constructor only, copying is not permitted. */

We usually don't put comments in the header files. This should go to the
\class documentation block below.

> +	MappedFrameBuffer(MappedFrameBuffer &&rhs);

Do you actually need this ? If it's only for the purpose of preventing
copy-construction, you can simply write

	MappedFrameBuffer(const MappedFrameBuffer &other) = delete;
	MappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;

The C++ rules governing implicitly-defined and defaulted copy and move
constructors and assignment operators are not the easiest to follow.
It's usually best to be explicit. If you need move semantics,

	MappedFrameBuffer(const MappedFrameBuffer &other) = delete;
	MappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;

	MappedFrameBuffer(MappedFrameBuffer &&other);
	MappedFrameBuffer operator=(MappedFrameBuffer &&other);

(either neither or both of the move constructor and the move assignment
operator should be defined) otherwise you can leave the latter two out.

> +
> +	struct MappedPlane {
> +		void *address;
> +		size_t length;
> +	};

Would it make sense to use Span<uint8_t> to replace MappedPlane ?

> +
> +	bool isValid() const { return valid_; }
> +	int error() const { return error_; }
> +	const std::vector<MappedPlane> &maps() const { return maps_; }
> +
> +private:
> +	int error_;
> +	bool valid_;
> +	std::vector<MappedPlane> maps_;
> +};
> +
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_BUFFER_H__ */
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 1a1d4bac7aed..28b43d937f57 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -290,4 +290,47 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)
>  	return 0;
>  }
>  

Missing

/**
 * \class MappedFrameBuffer
 * ...
 */

> +/**
> + * \brief Map all planes of a FrameBuffer

As this is a constructor, I think it should be documented as "Construct
..." to follow the usual pattern.

> + * \param[in] src Buffer to be mapped

s/src/buffer/

> + * \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)
> +	: error_(0)
> +{
> +	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";
> +			continue;

Shouldn't you break ?

> +		}
> +
> +		maps_.push_back({address, plane.length});
> +	}
> +
> +	valid_ = buffer->planes().size() == maps_.size();
> +}
> +
> +MappedFrameBuffer::~MappedFrameBuffer()
> +{
> +	for (MappedPlane map : maps_)

	for (MappedPlane &map : maps_)

> +		munmap(map.address, map.length);
> +}
> +
> +MappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs)
> +{
> +	error_ = rhs.error_;
> +	valid_ = rhs.valid_;
> +	maps_ = std::move(rhs.maps_);

This is the default move constructor, so you could simply write

	MappedFrameBuffer(MappedFrameBuffer &&other) = default;

in the class definition. However, I think you should keep this, and add

	rhs.error_ = 0;
	rhs.valid_ = false;

Note that you can implement the move constructor in terms of the move
assignment operator:

	*this = std::move(other);

> +}
> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart