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

[Cheng-Hao Yang]

Thanks Jacopo,

On Mon, May 29, 2023 at 3:58 PM Jacopo Mondi <jacopo.mondi at 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 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


> > +     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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230802/cb928947/attachment.htm>