[libcamera-devel] [PATCH 05/30] libcamera: buffer: Add Dmabuf to describe a dma buffer

[Jacopo Mondi]

Hi Niklas,

On Wed, Nov 27, 2019 at 12:35:55AM +0100, Niklas Söderlund wrote:
> A FrameBuffer object that holds a frame captured from a sensor consists
> of one or more plane(s). The memory of each plane can be accessed by
> using a dma buffer. Add a class that describes a dmabuf to make it easy
> for applications and IPAs to interact with memory.
>

I'm not sure I like this patch. I mean, abstracting access to the
planes memory to applications with an helper class is indeed a good
idea, but I'm not sure I'll make it a "Dmabuf"..

The whole class and documentation is pretty dmabuf-centric, and while
this is the method used to export memory information through a file
descriptor, the class also provide a CPU accessible address which maps
to the memory location that contains image data...

To me, this is just a plane, which an application can access
through a dmabuf exported file descriptor or through a memory address.

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/buffer.h |  19 +++++++
>  src/libcamera/buffer.cpp   | 113 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 132 insertions(+)
>
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 33793b4ccf881eda..3c430afbfe8e9a05 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -58,6 +58,25 @@ private:
>  	int fd_;
>  };
>
> +class Dmabuf final
> +{
> +public:
> +	Dmabuf(int fd, unsigned int length);
> +	~Dmabuf();
> +
> +	int fd() const { return fd_.fd(); }
> +	unsigned int length() const { return length_; }
> +	void *mem();
> +
> +private:
> +	int mmap();
> +	int munmap();
> +
> +	FileDescriptor fd_;
> +	unsigned int length_;
> +	void *mem_;

This really duplicates information we have in a Plane, with just an
additional memory mapped address, which is fine, but again to me this
an abstraction of the memory area a Plan refers to.

> +};
> +
>  class Plane final
>  {
>  public:
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 0676586ae3be2a61..5516055b2ea885c2 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -177,6 +177,119 @@ FileDescriptor::~FileDescriptor()
>   * an image may or may not be contiguous.
>   */
>
> +/**
> + * \class Dmabuf
> + * \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 Dmabuf 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.

I'm not sure I get what "access to memory both for the application and
for DMA" means. I think you're referring to dmabuf here, but "DMA
access" is confusing to me (yes, the numerical id is used by drivers
to retrieve the memory location where to point their DMA engines to,
but the users of this class are applications which really should care
only about the fact that they have a mean to have a dmabuf-consumable
representation of the memory area they intend to share between
different subsystems).

I would also drop from here any refernce to the image format and how
they map to one or multiple planes.

> + *
> + * To support DMA access, planes are associated with dmabuf objects represented

Here as well, what do you mean with "DMA access" ?

> + * 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.

I'm not following here either. We'll have one Dmabuf instance per
plane, right ? Each Dmabuf instance should have it's own
dmabuf-exported representation in form of a file handle and a CPU
accessible memory pointer where data for that plane are mapped to.
Why what the file handles refers to is relevant, and why the DMA engine
capabilities are of any interest for applications ?

> + *
> + * 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.

I don't think it's relevant either.

> + */
> +
> +/**
> + * \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.
> + */
> +Dmabuf::Dmabuf(int fd, unsigned int length)
> +	: fd_(fd), length_(length), mem_(nullptr)
> +{
> +}
> +
> +Dmabuf::~Dmabuf()
> +{
> +	munmap();
> +}
> +
> +/**
> + * \fn Dmabuf::fd()
> + * \brief Get the dmabuf file handle backing the buffer
> + */
> +
> +/**
> + * \fn Dmabuf::length()
> + * \brief Retrieve the length of the memory region
> + * \return The length of the memory region
> + */
> +
> +/**
> + * \fn Dmabuf::mem()
> + * \brief Retrieve the CPU accessible memory address of the Dmabuf
> + * \return The CPU accessible memory address on success or nullptr otherwise.
> + */
> +void *Dmabuf::mem()
> +{
> +	if (!mem_)
> +		mmap();
> +
> +	return mem_;
> +}
> +
> +/**
> + * \brief Map the plane memory data to a CPU accessible address
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int Dmabuf::mmap()
> +{
> +	void *map;
> +
> +	if (mem_)
> +		return 0;
> +
> +	map = ::mmap(NULL, length_, PROT_READ | PROT_WRITE, MAP_SHARED, fd_.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 Dmabuf::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;
> +}
> +

Also, looking at how it is used, the dmabufs_ vector of a FrameBuffer
really only uses informations copied from planes_.

I would be tempted to ask why application cannot access Plane
instances in a frame buffer and get their dmabuf and memory
representation from there, which would make this class not required,
but I might be missing something...

Thanks
   j

>  Plane::Plane()
>  	: fd_(-1), length_(0), mem_(0)
>  {
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191127/183e1080/attachment.sig>