| Message ID | 20190129135357.32339-5-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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
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@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 >
Hi Kieran, On Thu, Jan 31, 2019 at 08:31:43PM +0100, Kieran Bingham wrote: > On 31/01/2019 09:16, Laurent Pinchart wrote: > > 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@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? That's better, but I still think we need to improve the documentation. > >> * > >> - * 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? The dup() is necessary for external allocation (dmabuf import) to ensure that the file descriptor won't be closed with a buffer still referencing it. For internal allocation, there's no requirement to duplicate the V4L2 device fd, but it should not be passed to the mmap() method. The mmap() method is meant to be called by applications to mmap the buffer, and applications are not aware of the underlying device. You should thus pass the V4L2Device when creating the buffers and use its fd for mapping in the Plane::mmap() call, transparently for applications. It's important that the V4L2Device doesn't appear in the public API. > >> + */ > >> +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
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 * - * 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 + * may be backed by it's own dmaBuf handle, and represents a single plane from a + * Buffer. In the case of multi-planar image formats, multiple Plane objects are + * attached to a Buffer to manage the whole picture. */ -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. + */ +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
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@ideasonboard.com> --- include/libcamera/buffer.h | 38 +++++++- src/libcamera/buffer.cpp | 189 +++++++++++++++++++++++++++++++++++-- 2 files changed, 215 insertions(+), 12 deletions(-)