Message ID | 20191230120510.938333-6-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Niklas, On Mon, Dec 30, 2019 at 01:04:50PM +0100, Niklas Söderlund wrote: > There are no users left of the Plane class, drop it. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/buffer.h | 21 ------ > src/libcamera/buffer.cpp | 149 ------------------------------------- The patch changelog I like most Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > 2 files changed, 170 deletions(-) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index 71633492e4752efb..2d8885e9f3a1234f 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -65,27 +65,6 @@ private: > unsigned int cookie_; > }; > > -class Plane final > -{ > -public: > - Plane(); > - ~Plane(); > - > - int dmabuf() const { return fd_; } > - int setDmabuf(int fd, unsigned int length); > - > - void *mem(); > - unsigned int length() const { return length_; } > - > -private: > - int mmap(); > - int munmap(); > - > - int fd_; > - unsigned int length_; > - void *mem_; > -}; > - > class BufferMemory final > { > public: > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > index fde0b33511c64c9e..3ea982306843b116 100644 > --- a/src/libcamera/buffer.cpp > +++ b/src/libcamera/buffer.cpp > @@ -85,155 +85,6 @@ LOG_DEFINE_CATEGORY(Buffer) > * \brief Array holding plane specific metadata > */ > > -/** > - * \class Plane > - * \brief A memory region to store a single plane of a frame > - * > - * Planar pixel formats use multiple memory regions to store planes > - * corresponding to the different colour components of a frame. The Plane class > - * tracks the specific details of a memory region used to store a single plane > - * for a given frame and provides the means to access the memory, both for the > - * application and for DMA. A Buffer then contains one or multiple planes > - * depending on its pixel format. > - * > - * To support DMA access, planes are associated with dmabuf objects represented > - * by file handles. Each plane carries a dmabuf file handle and an offset within > - * the buffer. Those file handles may refer to the same dmabuf object, depending > - * on whether the devices accessing the memory regions composing the image > - * support non-contiguous DMA to planes ore require DMA-contiguous memory. > - * > - * To support CPU access, planes carry the CPU address of their backing memory. > - * Similarly to the dmabuf file handles, the CPU addresses for planes composing > - * an image may or may not be contiguous. > - */ > - > -Plane::Plane() > - : fd_(-1), length_(0), mem_(0) > -{ > -} > - > -Plane::~Plane() > -{ > - munmap(); > - > - if (fd_ != -1) > - close(fd_); > -} > - > -/** > - * \fn Plane::dmabuf() > - * \brief Get the dmabuf file handle backing the buffer > - */ > - > -/** > - * \brief Set the dmabuf file handle backing the buffer > - * \param[in] fd The dmabuf file handle > - * \param[in] length The size of the memory region > - * > - * The \a fd dmabuf file handle is duplicated and stored. The caller may close > - * the original file handle. > - * > - * \return 0 on success or a negative error code otherwise > - */ > -int Plane::setDmabuf(int fd, unsigned int length) > -{ > - if (fd < 0) { > - LOG(Buffer, Error) << "Invalid dmabuf fd provided"; > - return -EINVAL; > - } > - > - if (fd_ != -1) { > - close(fd_); > - fd_ = -1; > - } > - > - fd_ = dup(fd); > - if (fd_ == -1) { > - int ret = -errno; > - LOG(Buffer, Error) > - << "Failed to duplicate dmabuf: " << strerror(-ret); > - return ret; > - } > - > - length_ = length; > - > - 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 code otherwise > - */ > -int Plane::mmap() > -{ > - void *map; > - > - if (mem_) > - return 0; > - > - map = ::mmap(NULL, length_, PROT_READ | PROT_WRITE, MAP_SHARED, fd_, 0); > - if (map == MAP_FAILED) { > - int ret = -errno; > - LOG(Buffer, Error) > - << "Failed to mmap plane: " << strerror(-ret); > - return ret; > - } > - > - mem_ = map; > - > - return 0; > -} > - > -/** > - * \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 code otherwise > - */ > -int Plane::munmap() > -{ > - int ret = 0; > - > - if (mem_) > - ret = ::munmap(mem_, length_); > - > - if (ret) { > - ret = -errno; > - LOG(Buffer, Warning) > - << "Failed to unmap plane: " << strerror(-ret); > - } else { > - mem_ = 0; > - } > - > - return ret; > -} > - > -/** > - * \fn Plane::mem() > - * \brief Retrieve the CPU accessible memory address of the Plane > - * \return The CPU accessible memory address on success or nullptr otherwise. > - */ > -void *Plane::mem() > -{ > - if (!mem_) > - mmap(); > - > - return mem_; > -} > - > -/** > - * \fn Plane::length() const > - * \brief Retrieve the length of the memory region > - * \return The length of the memory region > - */ > - > /** > * \class BufferMemory > * \brief A memory buffer to store an image > -- > 2.24.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Mon, Dec 30, 2019 at 01:04:50PM +0100, Niklas Söderlund wrote: > There are no users left of the Plane class, drop it. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/buffer.h | 21 ------ > src/libcamera/buffer.cpp | 149 ------------------------------------- > 2 files changed, 170 deletions(-) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index 71633492e4752efb..2d8885e9f3a1234f 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -65,27 +65,6 @@ private: > unsigned int cookie_; > }; > > -class Plane final > -{ > -public: > - Plane(); > - ~Plane(); > - > - int dmabuf() const { return fd_; } > - int setDmabuf(int fd, unsigned int length); > - > - void *mem(); > - unsigned int length() const { return length_; } > - > -private: > - int mmap(); > - int munmap(); > - > - int fd_; > - unsigned int length_; > - void *mem_; > -}; > - > class BufferMemory final > { > public: > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > index fde0b33511c64c9e..3ea982306843b116 100644 > --- a/src/libcamera/buffer.cpp > +++ b/src/libcamera/buffer.cpp > @@ -85,155 +85,6 @@ LOG_DEFINE_CATEGORY(Buffer) > * \brief Array holding plane specific metadata > */ > > -/** > - * \class Plane > - * \brief A memory region to store a single plane of a frame > - * > - * Planar pixel formats use multiple memory regions to store planes > - * corresponding to the different colour components of a frame. The Plane class > - * tracks the specific details of a memory region used to store a single plane > - * for a given frame and provides the means to access the memory, both for the > - * application and for DMA. A Buffer then contains one or multiple planes > - * depending on its pixel format. > - * > - * To support DMA access, planes are associated with dmabuf objects represented > - * by file handles. Each plane carries a dmabuf file handle and an offset within > - * the buffer. Those file handles may refer to the same dmabuf object, depending > - * on whether the devices accessing the memory regions composing the image > - * support non-contiguous DMA to planes ore require DMA-contiguous memory. This seems useful information that we should somehow keep in the new FrameBuffer::Plane class. I propose /** * \struct FrameBuffer::Plane * \brief A memory region to store a single plane of a frame * * Planar pixel formats use multiple memory regions to store the different * colour components of a frame. The Plane structure describes such a memory * region by a dmabuf file descriptor and a length. A FrameBuffer then * contains one or multiple planes, depending on the pixel format of the * frames it is meant to store. * * To support DMA access, planes are associated with dmabuf objects represented * by FileDescriptor handles. The Plane class doesn't handle mapping of the * memory to the CPU, but applications and IPAs may use the dmabuf file * descriptors to map the plane memory with mmap() and access its contents. * * \todo Once we have a Kernel API which can express offsets within a plane * this structure shall be extended to contain this information. See commit * 83148ce8be55e for initial documentation of this feature. */ For this patch itself, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > - * > - * To support CPU access, planes carry the CPU address of their backing memory. > - * Similarly to the dmabuf file handles, the CPU addresses for planes composing > - * an image may or may not be contiguous. > - */ > - > -Plane::Plane() > - : fd_(-1), length_(0), mem_(0) > -{ > -} > - > -Plane::~Plane() > -{ > - munmap(); > - > - if (fd_ != -1) > - close(fd_); > -} > - > -/** > - * \fn Plane::dmabuf() > - * \brief Get the dmabuf file handle backing the buffer > - */ > - > -/** > - * \brief Set the dmabuf file handle backing the buffer > - * \param[in] fd The dmabuf file handle > - * \param[in] length The size of the memory region > - * > - * The \a fd dmabuf file handle is duplicated and stored. The caller may close > - * the original file handle. > - * > - * \return 0 on success or a negative error code otherwise > - */ > -int Plane::setDmabuf(int fd, unsigned int length) > -{ > - if (fd < 0) { > - LOG(Buffer, Error) << "Invalid dmabuf fd provided"; > - return -EINVAL; > - } > - > - if (fd_ != -1) { > - close(fd_); > - fd_ = -1; > - } > - > - fd_ = dup(fd); > - if (fd_ == -1) { > - int ret = -errno; > - LOG(Buffer, Error) > - << "Failed to duplicate dmabuf: " << strerror(-ret); > - return ret; > - } > - > - length_ = length; > - > - 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 code otherwise > - */ > -int Plane::mmap() > -{ > - void *map; > - > - if (mem_) > - return 0; > - > - map = ::mmap(NULL, length_, PROT_READ | PROT_WRITE, MAP_SHARED, fd_, 0); > - if (map == MAP_FAILED) { > - int ret = -errno; > - LOG(Buffer, Error) > - << "Failed to mmap plane: " << strerror(-ret); > - return ret; > - } > - > - mem_ = map; > - > - return 0; > -} > - > -/** > - * \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 code otherwise > - */ > -int Plane::munmap() > -{ > - int ret = 0; > - > - if (mem_) > - ret = ::munmap(mem_, length_); > - > - if (ret) { > - ret = -errno; > - LOG(Buffer, Warning) > - << "Failed to unmap plane: " << strerror(-ret); > - } else { > - mem_ = 0; > - } > - > - return ret; > -} > - > -/** > - * \fn Plane::mem() > - * \brief Retrieve the CPU accessible memory address of the Plane > - * \return The CPU accessible memory address on success or nullptr otherwise. > - */ > -void *Plane::mem() > -{ > - if (!mem_) > - mmap(); > - > - return mem_; > -} > - > -/** > - * \fn Plane::length() const > - * \brief Retrieve the length of the memory region > - * \return The length of the memory region > - */ > - > /** > * \class BufferMemory > * \brief A memory buffer to store an image
diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h index 71633492e4752efb..2d8885e9f3a1234f 100644 --- a/include/libcamera/buffer.h +++ b/include/libcamera/buffer.h @@ -65,27 +65,6 @@ private: unsigned int cookie_; }; -class Plane final -{ -public: - Plane(); - ~Plane(); - - int dmabuf() const { return fd_; } - int setDmabuf(int fd, unsigned int length); - - void *mem(); - unsigned int length() const { return length_; } - -private: - int mmap(); - int munmap(); - - int fd_; - unsigned int length_; - void *mem_; -}; - class BufferMemory final { public: diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp index fde0b33511c64c9e..3ea982306843b116 100644 --- a/src/libcamera/buffer.cpp +++ b/src/libcamera/buffer.cpp @@ -85,155 +85,6 @@ LOG_DEFINE_CATEGORY(Buffer) * \brief Array holding plane specific metadata */ -/** - * \class Plane - * \brief A memory region to store a single plane of a frame - * - * Planar pixel formats use multiple memory regions to store planes - * corresponding to the different colour components of a frame. The Plane class - * tracks the specific details of a memory region used to store a single plane - * for a given frame and provides the means to access the memory, both for the - * application and for DMA. A Buffer then contains one or multiple planes - * depending on its pixel format. - * - * To support DMA access, planes are associated with dmabuf objects represented - * by file handles. Each plane carries a dmabuf file handle and an offset within - * the buffer. Those file handles may refer to the same dmabuf object, depending - * on whether the devices accessing the memory regions composing the image - * support non-contiguous DMA to planes ore require DMA-contiguous memory. - * - * To support CPU access, planes carry the CPU address of their backing memory. - * Similarly to the dmabuf file handles, the CPU addresses for planes composing - * an image may or may not be contiguous. - */ - -Plane::Plane() - : fd_(-1), length_(0), mem_(0) -{ -} - -Plane::~Plane() -{ - munmap(); - - if (fd_ != -1) - close(fd_); -} - -/** - * \fn Plane::dmabuf() - * \brief Get the dmabuf file handle backing the buffer - */ - -/** - * \brief Set the dmabuf file handle backing the buffer - * \param[in] fd The dmabuf file handle - * \param[in] length The size of the memory region - * - * The \a fd dmabuf file handle is duplicated and stored. The caller may close - * the original file handle. - * - * \return 0 on success or a negative error code otherwise - */ -int Plane::setDmabuf(int fd, unsigned int length) -{ - if (fd < 0) { - LOG(Buffer, Error) << "Invalid dmabuf fd provided"; - return -EINVAL; - } - - if (fd_ != -1) { - close(fd_); - fd_ = -1; - } - - fd_ = dup(fd); - if (fd_ == -1) { - int ret = -errno; - LOG(Buffer, Error) - << "Failed to duplicate dmabuf: " << strerror(-ret); - return ret; - } - - length_ = length; - - 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 code otherwise - */ -int Plane::mmap() -{ - void *map; - - if (mem_) - return 0; - - map = ::mmap(NULL, length_, PROT_READ | PROT_WRITE, MAP_SHARED, fd_, 0); - if (map == MAP_FAILED) { - int ret = -errno; - LOG(Buffer, Error) - << "Failed to mmap plane: " << strerror(-ret); - return ret; - } - - mem_ = map; - - return 0; -} - -/** - * \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 code otherwise - */ -int Plane::munmap() -{ - int ret = 0; - - if (mem_) - ret = ::munmap(mem_, length_); - - if (ret) { - ret = -errno; - LOG(Buffer, Warning) - << "Failed to unmap plane: " << strerror(-ret); - } else { - mem_ = 0; - } - - return ret; -} - -/** - * \fn Plane::mem() - * \brief Retrieve the CPU accessible memory address of the Plane - * \return The CPU accessible memory address on success or nullptr otherwise. - */ -void *Plane::mem() -{ - if (!mem_) - mmap(); - - return mem_; -} - -/** - * \fn Plane::length() const - * \brief Retrieve the length of the memory region - * \return The length of the memory region - */ - /** * \class BufferMemory * \brief A memory buffer to store an image