[libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if DmaHeap is not valid
Cheng-Hao Yang
chenghaoyang at chromium.org
Mon Jun 19 07:55:10 CEST 2023
Hi Laurent,
As discussed in the libcamera weekly meeting before, have you brought out
the discussion in irc.oftc.net 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.
Thanks!
On Fri, Jun 2, 2023 at 4:32 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230619/d8dca12a/attachment.htm>
More information about the libcamera-devel
mailing list