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

18269 diff mbox series

If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/meson.build    |   1 +
 include/libcamera/udma_heap.h    |  20 ++++++
 src/libcamera/heap_allocator.cpp |   3 +
 src/libcamera/meson.build        |   1 +
 src/libcamera/udma_heap.cpp      | 117 +++++++++++++++++++++++++++++++
 5 files changed, 142 insertions(+)
 create mode 100644 include/libcamera/udma_heap.h
 create mode 100644 src/libcamera/udma_heap.cpp

Hi Harvey,

Thanks for looking at this to make it easier to get allocations
throughout libcamera.

Quoting Harvey Yang via libcamera-devel (2023-02-08 09:59:21)
> If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.
> 
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  include/libcamera/meson.build    |   1 +
>  include/libcamera/udma_heap.h    |  20 ++++++
>  src/libcamera/heap_allocator.cpp |   3 +
>  src/libcamera/meson.build        |   1 +
>  src/libcamera/udma_heap.cpp      | 117 +++++++++++++++++++++++++++++++
>  5 files changed, 142 insertions(+)
>  create mode 100644 include/libcamera/udma_heap.h
>  create mode 100644 src/libcamera/udma_heap.cpp
> 
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index f486630a..a1414a57 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -19,6 +19,7 @@ libcamera_public_headers = files([
>      'request.h',
>      'stream.h',
>      'transform.h',
> +    'udma_heap.h',
>  ])
>  
>  subdir('base')
> diff --git a/include/libcamera/udma_heap.h b/include/libcamera/udma_heap.h
> new file mode 100644
> index 00000000..b5b86922
> --- /dev/null
> +++ b/include/libcamera/udma_heap.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Google Inc.

I have the same comment as from Dave in regards to Copyright.

I expect this is highly derived from the work I commenced, at

https://github.com/kbingham/libcamera/commit/8d79af53eaf461bab246c2fdeb1d9fdcbb288743

So please keep some form of attribution to Ideas on Board Oy.


> + *
> + * udma_heap.h - Udma Heap implementation.
> + */
> +
> +#include <libcamera/heap.h>
> +
> +namespace libcamera {
> +
> +class UdmaHeap : public Heap
> +{
> +public:
> +       UdmaHeap();
> +       ~UdmaHeap();
> +       UniqueFD alloc(const char *name, std::size_t size) override;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp
> index 594b1d6a..179c2c21 100644
> --- a/src/libcamera/heap_allocator.cpp
> +++ b/src/libcamera/heap_allocator.cpp
> @@ -18,6 +18,7 @@
>  #include <libcamera/base/log.h>
>  
>  #include <libcamera/dma_heap.h>
> +#include <libcamera/udma_heap.h>
>  
>  namespace libcamera {
>  
> @@ -26,6 +27,8 @@ LOG_DEFINE_CATEGORY(HeapAllocator)
>  HeapAllocator::HeapAllocator()
>  {
>         heap_ = std::make_unique<DmaHeap>();
> +       if (!isValid())
> +               heap_ = std::make_unique<UdmaHeap>();
>  }
>  
>  HeapAllocator::~HeapAllocator() = default;
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index ee586c0d..bce26d4b 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -45,6 +45,7 @@ libcamera_sources = files([
>      'stream.cpp',
>      'sysfs.cpp',
>      'transform.cpp',
> +    'udma_heap.cpp',
>      'v4l2_device.cpp',
>      'v4l2_pixelformat.cpp',
>      'v4l2_subdevice.cpp',
> diff --git a/src/libcamera/udma_heap.cpp b/src/libcamera/udma_heap.cpp
> new file mode 100644
> index 00000000..2a470f6b
> --- /dev/null
> +++ b/src/libcamera/udma_heap.cpp
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Google Inc.

And here of course.


> + *
> + * dma_heap.h - Dma Heap implementation.
> + */
> +
> +#include <libcamera/udma_heap.h>
> +
> +#include <fcntl.h>
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <linux/udmabuf.h>
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(UdmaHeap)
> +
> +UdmaHeap::UdmaHeap()
> +{
> +       int ret = ::open("/dev/udmabuf", O_RDWR, 0);
> +       if (ret < 0) {
> +               ret = errno;
> +               LOG(UdmaHeap, Error)
> +                       << "Failed to open allocator: " << strerror(ret);
> +
> +               if (ret == EACCES) {
> +                       LOG(UdmaHeap, Info)
> +                               << "Consider making /dev/udmabuf accessible by the video group";
> +                       LOG(UdmaHeap, Info)
> +                               << "Alternatively, add your user to the kvm group.";
> +               }
> +
> +       } else {
> +               handle_ = UniqueFD(ret);
> +       }
> +}
> +
> +UdmaHeap::~UdmaHeap() = default;
> +
> +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)
> +{
> +       if (!isValid()) {
> +               LOG(UdmaHeap, Fatal) << "Allocation attempted without allocator" << name;
> +               return {};
> +       }
> +
> +       int ret;
> +
> +       ret = memfd_create(name, MFD_ALLOW_SEALING);
> +       if (ret < 0) {
> +               ret = errno;
> +               LOG(UdmaHeap, Error)
> +                       << "Failed to allocate memfd storage: "
> +                       << strerror(ret);
> +               return {};
> +       }
> +
> +       UniqueFD memfd(ret);
> +
> +       ret = ftruncate(memfd.get(), size);
> +       if (ret < 0) {
> +               ret = errno;
> +               LOG(UdmaHeap, Error)
> +                       << "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(UdmaHeap, Error)
> +                       << "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(UdmaHeap, Error)
> +                       << "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(UdmaHeap, Warning)
> +                       << "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(UdmaHeap, Debug) << "Allocated " << create.size << " bytes";
> +
> +       return uDma;
> +}
> +
> +} /* namespace libcamera */
> -- 
> 2.39.1.519.gcb327c4b5f-goog
>

Hi Kieran,

Thanks for pointing it out and so sorry about that. Updated in v2.

On Thu, Feb 9, 2023 at 10:54 PM Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Hi Harvey,
>
> Thanks for looking at this to make it easier to get allocations
> throughout libcamera.
>
> Quoting Harvey Yang via libcamera-devel (2023-02-08 09:59:21)
> > If DmaHeap is not valid, fall back to UdmaHeap to allocate buffers.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  include/libcamera/meson.build    |   1 +
> >  include/libcamera/udma_heap.h    |  20 ++++++
> >  src/libcamera/heap_allocator.cpp |   3 +
> >  src/libcamera/meson.build        |   1 +
> >  src/libcamera/udma_heap.cpp      | 117 +++++++++++++++++++++++++++++++
> >  5 files changed, 142 insertions(+)
> >  create mode 100644 include/libcamera/udma_heap.h
> >  create mode 100644 src/libcamera/udma_heap.cpp
> >
> > diff --git a/include/libcamera/meson.build
> b/include/libcamera/meson.build
> > index f486630a..a1414a57 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -19,6 +19,7 @@ libcamera_public_headers = files([
> >      'request.h',
> >      'stream.h',
> >      'transform.h',
> > +    'udma_heap.h',
> >  ])
> >
> >  subdir('base')
> > diff --git a/include/libcamera/udma_heap.h
> b/include/libcamera/udma_heap.h
> > new file mode 100644
> > index 00000000..b5b86922
> > --- /dev/null
> > +++ b/include/libcamera/udma_heap.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Google Inc.
>
> I have the same comment as from Dave in regards to Copyright.
>
> I expect this is highly derived from the work I commenced, at
>
>
> https://github.com/kbingham/libcamera/commit/8d79af53eaf461bab246c2fdeb1d9fdcbb288743
>
> So please keep some form of attribution to Ideas on Board Oy.
>
>
> > + *
> > + * udma_heap.h - Udma Heap implementation.
> > + */
> > +
> > +#include <libcamera/heap.h>
> > +
> > +namespace libcamera {
> > +
> > +class UdmaHeap : public Heap
> > +{
> > +public:
> > +       UdmaHeap();
> > +       ~UdmaHeap();
> > +       UniqueFD alloc(const char *name, std::size_t size) override;
> > +};
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/heap_allocator.cpp
> b/src/libcamera/heap_allocator.cpp
> > index 594b1d6a..179c2c21 100644
> > --- a/src/libcamera/heap_allocator.cpp
> > +++ b/src/libcamera/heap_allocator.cpp
> > @@ -18,6 +18,7 @@
> >  #include <libcamera/base/log.h>
> >
> >  #include <libcamera/dma_heap.h>
> > +#include <libcamera/udma_heap.h>
> >
> >  namespace libcamera {
> >
> > @@ -26,6 +27,8 @@ LOG_DEFINE_CATEGORY(HeapAllocator)
> >  HeapAllocator::HeapAllocator()
> >  {
> >         heap_ = std::make_unique<DmaHeap>();
> > +       if (!isValid())
> > +               heap_ = std::make_unique<UdmaHeap>();
> >  }
> >
> >  HeapAllocator::~HeapAllocator() = default;
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index ee586c0d..bce26d4b 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -45,6 +45,7 @@ libcamera_sources = files([
> >      'stream.cpp',
> >      'sysfs.cpp',
> >      'transform.cpp',
> > +    'udma_heap.cpp',
> >      'v4l2_device.cpp',
> >      'v4l2_pixelformat.cpp',
> >      'v4l2_subdevice.cpp',
> > diff --git a/src/libcamera/udma_heap.cpp b/src/libcamera/udma_heap.cpp
> > new file mode 100644
> > index 00000000..2a470f6b
> > --- /dev/null
> > +++ b/src/libcamera/udma_heap.cpp
> > @@ -0,0 +1,117 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Google Inc.
>
> And here of course.
>
>
> > + *
> > + * dma_heap.h - Dma Heap implementation.
> > + */
> > +
> > +#include <libcamera/udma_heap.h>
> > +
> > +#include <fcntl.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/mman.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +
> > +#include <linux/udmabuf.h>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(UdmaHeap)
> > +
> > +UdmaHeap::UdmaHeap()
> > +{
> > +       int ret = ::open("/dev/udmabuf", O_RDWR, 0);
> > +       if (ret < 0) {
> > +               ret = errno;
> > +               LOG(UdmaHeap, Error)
> > +                       << "Failed to open allocator: " << strerror(ret);
> > +
> > +               if (ret == EACCES) {
> > +                       LOG(UdmaHeap, Info)
> > +                               << "Consider making /dev/udmabuf
> accessible by the video group";
> > +                       LOG(UdmaHeap, Info)
> > +                               << "Alternatively, add your user to the
> kvm group.";
> > +               }
> > +
> > +       } else {
> > +               handle_ = UniqueFD(ret);
> > +       }
> > +}
> > +
> > +UdmaHeap::~UdmaHeap() = default;
> > +
> > +UniqueFD UdmaHeap::alloc(const char *name, std::size_t size)
> > +{
> > +       if (!isValid()) {
> > +               LOG(UdmaHeap, Fatal) << "Allocation attempted without
> allocator" << name;
> > +               return {};
> > +       }
> > +
> > +       int ret;
> > +
> > +       ret = memfd_create(name, MFD_ALLOW_SEALING);
> > +       if (ret < 0) {
> > +               ret = errno;
> > +               LOG(UdmaHeap, Error)
> > +                       << "Failed to allocate memfd storage: "
> > +                       << strerror(ret);
> > +               return {};
> > +       }
> > +
> > +       UniqueFD memfd(ret);
> > +
> > +       ret = ftruncate(memfd.get(), size);
> > +       if (ret < 0) {
> > +               ret = errno;
> > +               LOG(UdmaHeap, Error)
> > +                       << "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(UdmaHeap, Error)
> > +                       << "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(UdmaHeap, Error)
> > +                       << "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(UdmaHeap, Warning)
> > +                       << "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(UdmaHeap, Debug) << "Allocated " << create.size << " bytes";
> > +
> > +       return uDma;
> > +}
> > +
> > +} /* namespace libcamera */
> > --
> > 2.39.1.519.gcb327c4b5f-goog
> >
>