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

[Kieran Bingham]

Hi Laurent,

On 31/01/2019 09:16, Laurent Pinchart wrote:
> 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.

Memory Region as below?

> 
>>   *
>> - * 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().

One allows me to map the internal buffers with the FD from the
v4l2device open call directly without necessitating a dup() for every
buffer.

Would you prefer that every operation goes through the dmaBuf ?

Won't that then enforce a lot of file description duplication where it
might not always be necessary?




>> + */
>> +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
--
Kieran