| Message ID | 20191126233620.1695316-6-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Wed, Nov 27, 2019 at 12:55:34PM +0100, Jacopo Mondi wrote: > 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 s/dma buffer/dmabuf/ (this is the standard term, dma buffer may refer to many different things) > > 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. I share Jacopo's concern, I think that the Dmabuf and FrameBuffer::Plane classes overlap too much to make me happy. The boundary between them is quite fuzzy, which leads me to believe that more work is needed in this area to either merge the two classes or clarify their purpose. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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(); } How about returning a const FileDescriptor & instead ? This would give callers an easy way to duplicate the FileDescriptor if needed. > > + 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. The FrameBuffer::Plane class is also missing an offset field, to describe at which offset in a dmabuf data is stored for a plane. This may be applicable to the Dmabuf class too, although it seems that this proposed abstraction of the Dmabuf class aims at describing a memory region, regardless of whether it stores a single plane or multiple planes. Data offset isn't supported by V4L2 at the moment, but I think it should be part of our multi-plane API. This is how DRM/KMS describes a framebuffer, and extending V4L2 to use a similar mechanism has been proposed (the most recent proposal being presented by Boris during the ELCE V4L2 workshop this year). Stride information may also belong to FrameBuffer::Plane. Both data offset and stride could be added on top of this patch series, but the design of the FrameBuffer::Plane and Dmabuf classes should already take this into account in order to define the boundaries of the two classes clearly. > > +}; > > + > > 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); This doesn't need to be implemented now, but please already think about the fact that we will likely need to support read-only mappings in the future. We may also need to support different caching attributes for mappings (although I'm not sure what kernel API would be used for this). Could you try to think how it could affect the Dmabuf class API, and possibly already take this into account in the design (without a full implementation yet of course) ? > > + 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... As far as I understand, the Dmabuf class is only used by IPAs and by applications, in order to map buffers to the CPU. The IPA use case is certainly something we need to support, but towards applications, I wonder if it's really the duty of the FrameBuffer class to provide CPU mappings. When importing dmabufs (which is expected to be the main use case going forward) applications should already have a CPU mapping, and when exporting dmabufs we could expose the CPU mappings in the BufferAllocator class. > > Plane::Plane() > > : fd_(-1), length_(0), mem_(0) > > {
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_; +}; + 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. + * + * 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. + */ + +/** + * \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; +} + Plane::Plane() : fd_(-1), length_(0), mem_(0) {
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. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/buffer.h | 19 +++++++ src/libcamera/buffer.cpp | 113 +++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+)