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