[libcamera-devel] [PATCH v2 05/25] libcamera: buffers: Remove Plane class

[Laurent Pinchart]

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 at ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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 at 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

-- 
Regards,

Laurent Pinchart