[libcamera-devel] [PATCH v6 2/3] libcamera: Add UdmaHeap if DmaHeap is not valid

[Cheng-Hao Yang]

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 at 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 at 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 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.
>
> 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/20230602/8d6b7b93/attachment.htm>