[libcamera-devel] [PATCH 4/4] libcamera: buffer: Provide Buffer Planes

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tue, Jan 29, 2019 at 01:53:57PM +0000, Kieran Bingham wrote:
> Extend the Buffer management to support multi-planar formats.
> 
> An image within the system may use one or more Plane objects to track each
> plane in the case of multi-planar image formats. The Buffer class manages all
> of the data required to render or interpret the raw image data.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/buffer.h |  38 +++++++-
>  src/libcamera/buffer.cpp   | 189 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 215 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index dda5075f2879..08a74fb70b88 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -11,20 +11,50 @@
>  
>  namespace libcamera {
>  
> -class Buffer
> +class Plane
>  {
>  public:
> -	Buffer();
> -	~Buffer();
> +	Plane();
> +	Plane(unsigned int length, unsigned int offset);
> +	~Plane();
>  
>  	int dmabuf() const { return fd_; };
>  	int setDmabuf(int fd);
>  
> +	int mmap(int fd);
>  	int mmap();
> -	void munmap();
> +	int munmap();
> +
> +	unsigned int length() const { return length_; };
> +	void *mem() const { return mem_; };
>  
>  private:
>  	int fd_;
> +	unsigned int length_;
> +	unsigned int offset_;
> +	void *mem_;
> +};
> +
> +class Buffer
> +{
> +public:
> +	Buffer();
> +	virtual ~Buffer();
> +
> +	int mmap(int fd);
> +	int munmap();
> +
> +	unsigned int index() const { return index_; };
> +	const std::vector<Plane *> &planes() { return planes_; };
> +
> +private:
> +	unsigned int index_;
> +
> +	unsigned int format_;
> +	unsigned int width_;
> +	unsigned int height_;
> +
> +	std::vector<Plane *> planes_;
>  };
>  
>  class BufferPool
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 4a870df77e92..7b84a97dcd2b 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -6,10 +6,14 @@
>   */
>  
>  #include <errno.h>
> +#include <string.h>
> +#include <sys/mman.h>
>  #include <unistd.h>
>  
>  #include <libcamera/buffer.h>
>  
> +#include "log.h"
> +
>  /**
>   * \file buffer.h
>   * \brief Buffer handling
> @@ -17,25 +21,46 @@
>  
>  namespace libcamera {
>  
> +LOG_DEFINE_CATEGORY(Buffer)
> +
>  /**
> - * \class Buffer
> - * \brief A memory buffer to store a frame
> + * \class Plane
> + * \brief A memory buffer to store a single plane of a frame

Defining Plane as a memory buffer, and Buffer as a set of Plane, is too
confusing. We need better than that.

>   *
> - * The Buffer class represents a memory buffer used to store a frame.
> + * The Plane class represents a memory region used to store a single plane. It

And here defining Plane as a plane won't really help understanding the
API. We need an overview of how the Buffer and Plane objects interact.

> + * may be backed by it's own dmaBuf handle, and represents a single plane from a

s/it's/its/

> + * Buffer. In the case of multi-planar image formats, multiple Plane objects are
> + * attached to a Buffer to manage the whole picture.

Try reading this paragraph with a newcommer's point of view and I think
you'll realize we need better :-)

>   */
> -Buffer::Buffer()
> -	: fd_(-1)
> +
> +Plane::Plane()
> +	: fd_(-1), length_(0), offset_(0), mem_(0)
>  {
>  }
>  
> -Buffer::~Buffer()
> +/**
> + * \brief Construct a Plane memory region for CPU mappings
> + * \param[in] length The size of the memory region which should be mapped
> + * \param[in] offset The offset into the file descriptor base at which the
> + * buffer commences
> + *
> + * \sa mmap(int fd)
> + */
> +Plane::Plane(unsigned int length, unsigned int offset)
> +	: fd_(-1), length_(length), offset_(offset), mem_(0)
> +{
> +}
> +
> +Plane::~Plane()
>  {
> +	munmap();
> +
>  	if (fd_ != -1)
>  		close(fd_);
>  }
>  
>  /**
> - * \fn Buffer::dmabuf()
> + * \fn Plane::dmabuf()
>   * \brief Get the dmabuf file handle backing the buffer
>   */
>  
> @@ -44,7 +69,7 @@ Buffer::~Buffer()
>   *
>   * The \a fd dmabuf file handle is duplicated and stored.
>   */
> -int Buffer::setDmabuf(int fd)
> +int Plane::setDmabuf(int fd)
>  {
>  	if (fd_ != -1) {
>  		close(fd_);
> @@ -61,6 +86,154 @@ int Buffer::setDmabuf(int fd)
>  	return 0;
>  }
>  
> +/**
> + * \brief Map the plane memory data to a CPU accessible address
> + *
> + * The \a fd specifies the file descriptor to map the memory from.
> + *
> + * \return 0 on success or a negative error value otherwise.
> + */
> +int Plane::mmap(int fd)
> +{
> +	void *map;
> +
> +	map = ::mmap(NULL, length_, PROT_READ | PROT_WRITE, MAP_SHARED, fd,
> +		     offset_);
> +	if (map == reinterpret_cast<void *>(-1)) {
> +		int ret = -errno;
> +		LOG(Buffer, Error)
> +			<< "Failed to mmap buffer: " << strerror(ret);
> +		return ret;
> +	}
> +
> +	mem_ = map;
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Map the plane memory data to a CPU accessible address
> + *
> + * The file descriptor to map the memory from must be set by a call to
> + * setDmaBuf before calling this function.
> + *
> + * \sa setDmaBuf
> + *
> + * \return 0 on success or a negative error value otherwise.

I don't understand why you need two version of mmap().

> + */
> +int Plane::mmap()
> +{
> +	return mmap(fd_);
> +}
> +
> +/**
> + * \brief Unmap any existing CPU accessible mapping
> + *
> + * Unmap the memory mapped by an earlier call to mmap.
> + *
> + * \return 0 on success or a negative error value otherwise.
> + */
> +int Plane::munmap()
> +{
> +	int ret = 0;
> +
> +	if (mem_)
> +		ret = ::munmap(mem_, length_);
> +
> +	if (ret) {
> +		ret = -errno;
> +		LOG(Buffer, Warning)
> +			<< "Failed to unmap buffer: " << strerror(ret);
> +	} else {
> +		mem_ = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * \fn Plane::length()
> + * \brief Get the length of the memory buffer
> + */
> +
> +/**
> + * \fn Plane::mem()
> + * \brief Get the CPU accessible memory address if mapped
> + */
> +
> +/**
> + * \class Buffer
> + * \brief A memory buffer to store an image
> + *
> + * The Buffer class represents the memory buffers used to store a
> + * full frame image, which may contain multiple separate memory Plane
> + * objects if the image format is multi-planar.
> + */
> +
> +Buffer::Buffer()
> +	: index_(-1), format_(0), width_(0), height_(0)
> +{
> +}
> +
> +Buffer::~Buffer()
> +{
> +	for (Plane *plane : planes_)
> +		delete plane;
> +
> +	planes_.clear();
> +}
> +
> +/**
> + * \brief Map each buffer plane to a CPU accessible address
> + *
> + * Utilise the \a fd file descriptor to map each of the Buffer's planes to a CPU
> + * accessible address.
> + *
> + * \return 0 on success or a negative error value otherwise.
> + */
> +int Buffer::mmap(int fd)
> +{
> +	int ret;
> +
> +	for (Plane *plane : planes_) {
> +		ret = plane->mmap(fd);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Unmap any existing CPU accessible mapping for each plane
> + *
> + * Unmap the memory mapped by an earlier call to mmap.
> + *
> + * \return 0 on success or a negative error value otherwise.
> + */
> +int Buffer::munmap()
> +{
> +	int ret;
> +
> +	for (Plane *plane : planes_) {
> +		ret = plane->munmap();
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn Buffer::index()
> + * \brief Get the Buffer index value
> + */
> +
> +/**
> + * \fn Buffer::planes()
> + * \brief Return a reference to the vector holding all Planes within the buffer
> + */
> +
>  /**
>   * \class BufferPool
>   * \brief A pool of buffers

-- 
Regards,

Laurent Pinchart