[libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if DmaHeap is not valid
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jun 2 10:32:34 CEST 2023
Hi Harvey,
On Fri, Jun 02, 2023 at 02:39:33PM +0800, Cheng-Hao Yang wrote:
> Also, if we let users decide which heap implementations to be used
> directly, maybe there's no point having class HeapAllocator at all.
> We can just give users (pipeline handlers) access to each Heap's
> implementation directly.
>
> WDYT?
The first question to answer is what use case(s) you're trying to
address. Let's assume we would give users of the allocator class a
choice regarding what heap they allocate from (system vs. cma, those are
the only two options available in mainline). How would the user decide
which heap to use ? You can use the virtual pipeline handler as one use
case to answer that question (try to keep the big picture in mind, what,
for instance, if the virtual pipeline handler were to use a hardware
device - such as the GPU - to post-process images generated by the CPU,
or what if we wanted to achieve zero-copy when display images generated
by the virtual pipeline handler ?), and considering other use cases
would be helpful to design a good solution.
> On Tue, May 30, 2023 at 6:14 PM Cheng-Hao Yang wrote:
>
> > Hi Laurent, thanks for pointing it out. I'm a newbie to buffer allocation
> > in linux, so here are my confusions before updating the patch:
> >
> > On Tue, May 30, 2023 at 2:42 PM Laurent Pinchart wrote:
> >> On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote:
> >> > From: Cheng-Hao Yang <chenghaoyang at chromium.org>
> >> >
> >> > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.
> >>
> >> udmabuf and the DMA heaps are two completely different things, meant for
> >> different use cases. An allocator that would use "whatever is available"
> >> isn't a good generic helper.
> >
> > I assume you mean udmabuf and DMA buffers, unless you mean the
> > difference between buffers and heaps:
> > I saw [1], which states udmabuf as "User space mappable DMA buffer",
> > so I assume that it's one kind of DMA buffer, especially accessible for
> > userspace.
That's a different udmabuf than the one present in the mainline kernel
as far as I can tell. It seems to be an out-of-tree implementation, from
https://github.com/ikwzm/udmabuf.
The mainline udmabuf is a kernel API that allows wrapping a memfd into a
dmabuf object. Userspace gets a dmabuf FD, usable with all kernel APIs
that expect a dmabuf FD. memfd creates an anonymous file, backed by
anonymous memory, so you will essentially get discontiguous pages. If
I'm not mistaken, those pages are pinned to memory by calling
shmem_read_mapping_page() when the udmabuf is created, but don't quote
me on that.
DMA heaps, on the other hand, are a set of memory allocators aimed to
provide memory regions suitable for devices to use for DMA. The
allocators also expose the buffers as dmabuf FDs to userspace, and
that's the only thing they have in common with udmabuf. With DMA heaps,
userspace picks one of the allocators provided by the system, and asks
the kernel to allocate buffers using that allocator.
The "system" heap uses alloc_pages() and tries to split the allocation
in the smallest number of page sets, with each of the sets using the
highest possible page order. From a device DMA point of view, this is
similar to udmabuf, neither are suitable for usage with devices that
require DMA-contiguous memory and don't have an IOMMU.
The "cma" heap uses cma_alloc(), which guarantees physically-contiguous
memory. The memory is suitable for DMA with many (but not all) devices.
The downside is that the CMA area is limited in size, and physically
contiguous memory shouldn't be used when the device doesn't require it.
There is unfortunately no DMA heap that allocates DMA-capable buffers
for a specific device. Lots of work is still needed to provide Linux
systems with a unified device memory allocator.
Based on the above, the issue isn't so much about DMA heaps vs. udmabuf
as I initially implied, but more about system heap and udmabuf vs. CMA
heap. I think the system heap and udmabuf are likely to provide similar
types of allocations from the point of view of DMA. The page allocation
order, however, may be significantly different. I haven't investigated
that.
> > I agree that we shouldn't use "whatever is available" logic though. How
> > about an enum passed into HeapAllocator's c'tor that lets the user to
> > specify which heap(s) (with an order or not) to be used?
> >
> > [1]: https://www.silex.jp/products/license/z-1_for_sky/License/udmabuf/Readme.md
> >
> >> I expect writing the documentation will make you realize that there's a
> >> design issue here, as it's difficult to document clearly an API that
> >> doesn't have a clear design.
> >>
> >> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> >> > ---
> >> > src/libcamera/heap_allocator.cpp | 100 +++++++++++++++++++++++++++++++
> >> > 1 file changed, 100 insertions(+)
> >> >
> >> > diff --git a/src/libcamera/heap_allocator.cpp
> >> b/src/libcamera/heap_allocator.cpp
> >> > index 69f65062..8f99f590 100644
> >> > --- a/src/libcamera/heap_allocator.cpp
> >> > +++ b/src/libcamera/heap_allocator.cpp
> >> > @@ -11,10 +11,14 @@
> >> > #include <array>
> >> > #include <fcntl.h>
> >> > #include <sys/ioctl.h>
> >> > +#include <sys/mman.h>
> >> > +#include <sys/stat.h>
> >> > +#include <sys/types.h>
> >> > #include <unistd.h>
> >> >
> >> > #include <linux/dma-buf.h>
> >> > #include <linux/dma-heap.h>
> >> > +#include <linux/udmabuf.h>
> >> >
> >> > #include <libcamera/base/log.h>
> >> >
> >> > @@ -54,6 +58,14 @@ public:
> >> > UniqueFD alloc(const char *name, std::size_t size) override;
> >> > };
> >> >
> >> > +class UdmaHeap : public Heap
> >> > +{
> >> > +public:
> >> > + UdmaHeap();
> >> > + ~UdmaHeap();
> >> > + UniqueFD alloc(const char *name, std::size_t size) override;
> >> > +};
> >> > +
> >> > DmaHeap::DmaHeap()
> >> > {
> >> > for (const char *name : dmaHeapNames) {
> >> > @@ -65,6 +77,7 @@ DmaHeap::DmaHeap()
> >> > continue;
> >> > }
> >> >
> >> > + LOG(HeapAllocator, Info) << "Using DmaHeap allocator";
> >> > handle_ = UniqueFD(ret);
> >> > break;
> >> > }
> >> > @@ -105,9 +118,96 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> >> > return allocFd;
> >> > }
> >> >
> >> > +UdmaHeap::UdmaHeap()
> >> > +{
> >> > + int ret = ::open("/dev/udmabuf", O_RDWR, 0);
> >> > + if (ret < 0) {
> >> > + ret = errno;
> >> > + LOG(HeapAllocator, Error)
> >> > + << "UdmaHeap failed to open allocator: " << strerror(ret);
> >> > + } else {
> >> > + LOG(HeapAllocator, Info) << "Using UdmaHeap allocator";
> >> > + handle_ = UniqueFD(ret);
> >> > + }
> >> > +}
> >> > +
> >> > +UdmaHeap::~UdmaHeap() = default;
> >> > +
> >> > +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)
> >> > +{
> >> > + if (!isValid()) {
> >> > + LOG(HeapAllocator, Fatal) << "UdmaHeap: Allocation attempted without allocator" << name;
> >> > + return {};
> >> > + }
> >> > +
> >> > + int ret;
> >> > +
> >> > + ret = memfd_create(name, MFD_ALLOW_SEALING);
> >> > + if (ret < 0) {
> >> > + ret = errno;
> >> > + LOG(HeapAllocator, Error)
> >> > + << "UdmaHeap failed to allocate memfd storage: "
> >> > + << strerror(ret);
> >> > + return {};
> >> > + }
> >> > +
> >> > + UniqueFD memfd(ret);
> >> > +
> >> > + ret = ftruncate(memfd.get(), size);
> >> > + if (ret < 0) {
> >> > + ret = errno;
> >> > + LOG(HeapAllocator, Error)
> >> > + << "UdmaHeap failed to set memfd size: " << strerror(ret);
> >> > + return {};
> >> > + }
> >> > +
> >> > + /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */
> >> > + ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);
> >> > + if (ret < 0) {
> >> > + ret = errno;
> >> > + LOG(HeapAllocator, Error)
> >> > + << "UdmaHeap failed to seal the memfd: " << strerror(ret);
> >> > + return {};
> >> > + }
> >> > +
> >> > + struct udmabuf_create create;
> >> > +
> >> > + create.memfd = memfd.get();
> >> > + create.flags = UDMABUF_FLAGS_CLOEXEC;
> >> > + create.offset = 0;
> >> > + create.size = size;
> >> > +
> >> > + ret = ::ioctl(handle_.get(), UDMABUF_CREATE, &create);
> >> > + if (ret < 0) {
> >> > + ret = errno;
> >> > + LOG(HeapAllocator, Error)
> >> > + << "UdmaHeap failed to allocate " << size << " bytes: "
> >> > + << strerror(ret);
> >> > + return {};
> >> > + }
> >> > +
> >> > + /* The underlying memfd is kept as as a reference in the kernel */
> >> > + UniqueFD uDma(ret);
> >> > +
> >> > + if (create.size != size)
> >> > + LOG(HeapAllocator, Warning)
> >> > + << "UdmaHeap allocated " << create.size << " bytes instead of "
> >> > + << size << " bytes";
> >> > +
> >> > + /* Fail if not suitable, the allocation will be free'd by UniqueFD */
> >> > + if (create.size < size)
> >> > + return {};
> >> > +
> >> > + LOG(HeapAllocator, Debug) << "UdmaHeap allocated " << create.size << " bytes";
> >> > +
> >> > + return uDma;
> >> > +}
> >> > +
> >> > HeapAllocator::HeapAllocator()
> >> > {
> >> > heap_ = std::make_unique<DmaHeap>();
> >> > + if (!isValid())
> >> > + heap_ = std::make_unique<UdmaHeap>();
> >> > }
> >> >
> >> > HeapAllocator::~HeapAllocator() = default;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list