[libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if DmaHeap is not valid
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 2 22:32:01 CEST 2023
Hi Harvey,
(CC'ing Javier)
On Mon, Jun 19, 2023 at 01:55:10PM +0800, Cheng-Hao Yang wrote:
> 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.
According to the discussions I've had with Javier in the #libcamera IRC
channel, Fedora ships both udmabuf and DMA heaps. The device node for
the former is owned by root:kvm, while the latter are owned by
root:root.
I believe Debian doesn't ship udmabuf or DMA heaps, according to the
kernel config ([1], I have checked amd64 and arm64). I'm a bit surprised
by the lack of udmabuf support, as it gets used by qemu, so I may be
missing something. I haven't contacted the Debian kernel team about
this, but found a corresponding bug that went unanswered ([2].
The ownership for /dev/udmabuf is controlled by the systemd-udev default
rules, see [3]. This was authored four years ago ([4]), to fix an issue
([5]). How to make the device accessible to libcamera is an open
question. One option would be to create a new dmabuf group, and add
local users to it automatically based on system policies. The user under
which the pipewire daemon gets run would be added to that group, should
it be different that the account for logged users (as in pipewire
running as a system deamon instead of a user daemon). Someone should
engage with the relevant communities to discuss this. Would you
volunteer ? :-)
While at it, we should also discuss permissions on DMA heaps, I think
that would be a better path forward.
[1] https://packages.debian.org/bookworm/linux-config-6.1
[2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=994449
[3] https://github.com/systemd/systemd/blob/main/rules.d/50-udev-default.rules.in#L116
[4] https://github.com/systemd/systemd/commit/0fde5a3591d7bc7f689937aa86bd8b30e40d9bf8
[5] https://github.com/systemd/systemd/issues/12283
> On Fri, Jun 2, 2023 at 4:32 PM Laurent Pinchart wrote:
> > 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