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

[Laurent Pinchart]

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 at chromium.org>
> > >
> > > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.
> > >
> > > 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);
> >
> > 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;

-- 
Regards,

Laurent Pinchart