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

18637 diff mbox series

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 | 106 +++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

Quoting Harvey Yang via libcamera-devel (2023-05-16 09:03:18)
> 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 | 106 +++++++++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
> 
> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp
> index e9476de5..682a7e01 100644
> --- a/src/libcamera/heap_allocator.cpp
> +++ b/src/libcamera/heap_allocator.cpp
> @@ -10,10 +10,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>
>  
> @@ -52,6 +56,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) {
> @@ -103,9 +115,103 @@ 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);
> +
> +               if (ret == EACCES) {
> +                       LOG(HeapAllocator, Info)
> +                               << "UdmaHeap: Consider making /dev/udmabuf accessible by the video group";
> +                       LOG(HeapAllocator, Info)
> +                               << "UdmaHeap: Alternatively, add your user to the kvm group.";

I think I wrote this code, but I'm torn here for upstream. These are
probably very 'distro' specific comments which we probably shouldn't
include here.

I like that they help inform the user what they need to do - but I fear
maybe we should drop these comments for now.

> +               }
> +
> +       } else {
> +               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())

I think if we've failed to create a DmaHeap - that's probably important
to report to the user.

I'm weary of this error path being taken for instance on an RPi or real
device that wouldn't necessarily work with UdmaHeap ? (Maybe it will, I
don't know) - but the fact we're using a different heap should probably
be reported in that instance.

Perhaps at least just a
		LOG(HeapAllocator, Info) << "Using UdmaHeap allocator";

Which would give us an indicator if we have issues reported on hardware
pipelines that can't use the buffers.

Otherwise, the allocation code all looks about how I remember it being
so 


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> +               heap_ = std::make_unique<UdmaHeap>();
>  }
>  
>  HeapAllocator::~HeapAllocator() = default;
> -- 
> 2.40.1.606.ga4b1b128d6-goog
>

Thanks Kieran!

On Tue, May 16, 2023 at 7:21 PM Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Harvey Yang via libcamera-devel (2023-05-16 09:03:18)
> > 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 | 106 +++++++++++++++++++++++++++++++
> >  1 file changed, 106 insertions(+)
> >
> > diff --git a/src/libcamera/heap_allocator.cpp
> b/src/libcamera/heap_allocator.cpp
> > index e9476de5..682a7e01 100644
> > --- a/src/libcamera/heap_allocator.cpp
> > +++ b/src/libcamera/heap_allocator.cpp
> > @@ -10,10 +10,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>
> >
> > @@ -52,6 +56,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) {
> > @@ -103,9 +115,103 @@ 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);
> > +
> > +               if (ret == EACCES) {
> > +                       LOG(HeapAllocator, Info)
> > +                               << "UdmaHeap: Consider making
> /dev/udmabuf accessible by the video group";
> > +                       LOG(HeapAllocator, Info)
> > +                               << "UdmaHeap: Alternatively, add your
> user to the kvm group.";
>
> I think I wrote this code, but I'm torn here for upstream. These are
> probably very 'distro' specific comments which we probably shouldn't
> include here.
>
> I like that they help inform the user what they need to do - but I fear
> maybe we should drop these comments for now.
>
>
I see. Removed.


> > +               }
> > +
> > +       } else {
> > +               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())
>
> I think if we've failed to create a DmaHeap - that's probably important
> to report to the user.
>
> I'm weary of this error path being taken for instance on an RPi or real
> device that wouldn't necessarily work with UdmaHeap ? (Maybe it will, I
> don't know) - but the fact we're using a different heap should probably
> be reported in that instance.
>
> Perhaps at least just a
>                 LOG(HeapAllocator, Info) << "Using UdmaHeap allocator";
>
> Which would give us an indicator if we have issues reported on hardware
> pipelines that can't use the buffers.
>
> Otherwise, the allocation code all looks about how I remember it being
> so
>
>
Yes, your worry totally makes sense. I've considered letting users decide
which heap to be used actually. For instance, we can create an enum list
that contains all the options (Dma & Udma) for now, and let users choose.
WDYT?

For the log though, actually we'll get informed by "Dmaheap could not
open any dmaHeap device" in DmaHeap's c'tor, when it fails to initialize.
I've added corresponiding Info log in DmaHeap's and UdmaHeap's c'tors
though, to make it clearer.


>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
> > +               heap_ = std::make_unique<UdmaHeap>();
> >  }
> >
> >  HeapAllocator::~HeapAllocator() = default;
> > --
> > 2.40.1.606.ga4b1b128d6-goog
> >
>