| Message ID | 20230522083546.2465448-3-chenghaoyang@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote: > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers. > > Signed-off-by: Harvey Yang <chenghaoyang@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); Should you add O_CLOEXEC or does it generate issues ? Same question as in 1/3 on the third argument > + if (ret < 0) { > + ret = errno; > + LOG(HeapAllocator, Error) > + << "UdmaHeap failed to open allocator: " << strerror(ret); You could return here and save an... > + } else { ... indendation level here > + 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 {}; > + } UdmaHeap::alloc() is called by HeapAllocator::alloc() which already checks for validity with a Fatal error. You can drop this change > + > + int ret; > + > + ret = memfd_create(name, MFD_ALLOW_SEALING); You can declare and assign ret in one line > + 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"; Should we continue or is it an error ? Also I would move this check before the creation of uDma(ret); > + > + /* Fail if not suitable, the allocation will be free'd by UniqueFD */ > + if (create.size < size) > + return {}; Ah so larger size is ok, smaller it's not. So I would check for this before the previous one > + > + 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; > -- > 2.40.1.698.g37aff9b760-goog >
Hi Harvey, Thank you for the patch. On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote: > From: Cheng-Hao Yang <chenghaoyang@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 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@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;
Hi Harvey, On 5/22/23 2:05 PM, Harvey Yang via libcamera-devel wrote: > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers. Does udmaheap requires any addition to the udev rules? I noticed that the dma_heap requires: https://github.com/RPi-Distro/raspberrypi-sys-mods/blob/master/etc.armhf/udev/rules.d/99-com.rules#L8 such a rule, so the Allocator possibility can't be used out of the box (unless libcamera takes care of installing such a rule). > Signed-off-by: Harvey Yang <chenghaoyang@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;
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 < laurent.pinchart@ideasonboard.com> wrote: > Hi Harvey, > > Thank you for the patch. > > On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel > wrote: > > From: Cheng-Hao Yang <chenghaoyang@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. 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@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 >
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? On Tue, May 30, 2023 at 6:14 PM Cheng-Hao Yang <chenghaoyang@chromium.org> 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 < > laurent.pinchart@ideasonboard.com> wrote: > >> Hi Harvey, >> >> Thank you for the patch. >> >> On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel >> wrote: >> > From: Cheng-Hao Yang <chenghaoyang@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. > > 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@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 >> >
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@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@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;
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@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@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@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 >
Thanks Jacopo, On Mon, May 29, 2023 at 3:58 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Harvey > > On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel > wrote: > > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > > > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers. > > > > Signed-off-by: Harvey Yang <chenghaoyang@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); > > Should you add O_CLOEXEC or does it generate issues ? Same question as > in 1/3 on the third argument > > Removed the third argument for now. Kieran, I copied and pasted from your code [1]. Maybe you know better than I do? [1]: https://github.com/kbingham/libcamera/commit/c028d61f8bc3ea941ca1ef32c478385f5a00d05c#diff-fb4c595461a4d74b20a4684fa27b9ea77b2571102ba45381666d5e6e40f0a143R29 > > + if (ret < 0) { > > + ret = errno; > > + LOG(HeapAllocator, Error) > > + << "UdmaHeap failed to open allocator: " << > strerror(ret); > > You could return here and save an... > > > + } else { > > ... indendation level here > > Yeah thanks! > > + 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 {}; > > + } > > UdmaHeap::alloc() is called by HeapAllocator::alloc() which already > checks for validity with a Fatal error. You can drop this change > Right, removed. > > + > > + int ret; > > + > > + ret = memfd_create(name, MFD_ALLOW_SEALING); > > You can declare and assign ret in one line > > Done > > + 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"; > > Should we continue or is it an error ? Also I would move this check > before the creation of uDma(ret); > > Yeah creating |uDma| when we're returning should be better. > > + > > + /* Fail if not suitable, the allocation will be free'd by UniqueFD > */ > > + if (create.size < size) > > + return {}; > > Ah so larger size is ok, smaller it's not. > > So I would check for this before the previous one > > Hmm, do you mean we should generate different logs for the two cases? Updated the order and the logs. Please check. > > + > > + 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; > > -- > > 2.40.1.698.g37aff9b760-goog > > >
On Wed, Aug 02, 2023 at 03:07:16PM +0800, Cheng-Hao Yang via libcamera-devel wrote: > On Mon, May 29, 2023 at 3:58 PM Jacopo Mondi wrote: > > On Mon, May 22, 2023 at 08:35:07AM +0000, Harvey Yang via libcamera-devel wrote: > > > From: Cheng-Hao Yang <chenghaoyang@chromium.org> > > > > > > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers. > > > > > > Signed-off-by: Harvey Yang <chenghaoyang@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); > > > > Should you add O_CLOEXEC or does it generate issues ? Same question as > > in 1/3 on the third argument > > Removed the third argument for now. > Kieran, I copied and pasted from your code [1]. Maybe you know better than I > do? > > [1]: https://github.com/kbingham/libcamera/commit/c028d61f8bc3ea941ca1ef32c478385f5a00d05c#diff-fb4c595461a4d74b20a4684fa27b9ea77b2571102ba45381666d5e6e40f0a143R29 Kieran's code predates the commit that added O_CLOEXEC to src/libcamera/pipeline/raspberrypi/dma_heaps.cpp. It should be used here too. > > > + if (ret < 0) { > > > + ret = errno; > > > + LOG(HeapAllocator, Error) > > > + << "UdmaHeap failed to open allocator: " << strerror(ret); > > > > You could return here and save an... > > > > > + } else { > > > > ... indendation level here > > Yeah thanks! > > > > + 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 {}; > > > + } > > > > UdmaHeap::alloc() is called by HeapAllocator::alloc() which already > > checks for validity with a Fatal error. You can drop this change > > Right, removed. > > > > + > > > + int ret; > > > + > > > + ret = memfd_create(name, MFD_ALLOW_SEALING); > > > > You can declare and assign ret in one line > > Done > > > > + 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"; > > > > Should we continue or is it an error ? Also I would move this check > > before the creation of uDma(ret); > > Yeah creating |uDma| when we're returning should be better. > > > > + > > > + /* Fail if not suitable, the allocation will be free'd by UniqueFD */ > > > + if (create.size < size) > > > + return {}; > > > > Ah so larger size is ok, smaller it's not. > > > > So I would check for this before the previous one > > Hmm, do you mean we should generate different logs for the two cases? > Updated the order and the logs. Please check. > > > > + > > > + 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;
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@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@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;
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;