[libcamera-devel] [PATCH 4/4] libcamera: buffer: Provide Buffer Planes
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Jan 31 20:31:43 CET 2019
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
More information about the libcamera-devel
mailing list