libcamera: dma_heaps: Add support for using udmabuf to alloc DMA heaps

20102 diff mbox series

Add support for using udmabuf to alloc DMA heaps, this is basically:
https://patchwork.libcamera.org/patch/18922/

ported from the never merged HeapAllocator class to the current DmaHeap
class.

Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/libcamera/internal/dma_heaps.h |   3 +
 src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
 2 files changed, 109 insertions(+), 21 deletions(-)

Hi Hans,

Thanks for refreshing this work!

Quoting Hans de Goede (2024-05-27 15:16:47)
> Add support for using udmabuf to alloc DMA heaps, this is basically:
> https://patchwork.libcamera.org/patch/18922/
> 
> ported from the never merged HeapAllocator class to the current DmaHeap
> class.

True statements, but not really specific to the content of the patch?

Perhaps:

"""
The DMA heap allocator currently allocates from CMA and system heaps.

These provide contiguous device memory but require dedicated and
reserved memory regions to be configured on the platform and may require
elevated privilidges to access.

Extend the dma_heap allocator to support falling back to the memfd
allocator backed by the udmabuf framework to prepare buffers that are
compatible with linux drivers and the libcamera API.

The buffers allocated through memfd/udmabuf will not be contiguous and
are likely not suitable for directly rendering to a display or encoder
but may facilitate more usecases for allocating memory in pipelines that
make use of the CPU for processing including virtual pipeline handlers
or the SoftISP.
"""

Feel free to adapt the above of course if I've made any errors.



> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  include/libcamera/internal/dma_heaps.h |   3 +
>  src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
>  2 files changed, 109 insertions(+), 21 deletions(-)
> 
> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> index f0a8aa5d..7e1271ed 100644
> --- a/include/libcamera/internal/dma_heaps.h
> +++ b/include/libcamera/internal/dma_heaps.h
> @@ -30,7 +30,10 @@ public:
>         UniqueFD alloc(const char *name, std::size_t size);
>  
>  private:
> +       UniqueFD allocFromHeap(const char *name, std::size_t size);
> +       UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
>         UniqueFD dmaHeapHandle_;
> +       bool useUDmaBuf_;
>  };
>  
>  LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)
> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
> index d4cb880b..bb707786 100644
> --- a/src/libcamera/dma_heaps.cpp
> +++ b/src/libcamera/dma_heaps.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>
>  
> @@ -36,13 +40,15 @@ namespace libcamera {
>  struct DmaHeapInfo {
>         DmaHeap::DmaHeapFlag type;
>         const char *deviceNodeName;
> +       bool useUDmaBuf;
>  };
>  #endif
>  
> -static constexpr std::array<DmaHeapInfo, 3> heapInfos = { {
> -       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma" },
> -       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved" },
> -       { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system" },
> +static constexpr std::array<DmaHeapInfo, 4> heapInfos = { {
> +       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma", false },
> +       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved", false },
> +       { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system", false },
> +       { DmaHeap::DmaHeapFlag::System, "/dev/udmabuf", true },

Should we distinguise UDMA with a different flag? or just simply keep it
as a fall back?

If we do use a different DmaHeapFlag then I think we don't need the
boolean...


>  } };
>  
>  LOG_DEFINE_CATEGORY(DmaHeap)
> @@ -105,6 +111,7 @@ DmaHeap::DmaHeap(DmaHeapFlags type)
>  
>                 LOG(DmaHeap, Debug) << "Using " << info.deviceNodeName;
>                 dmaHeapHandle_ = UniqueFD(ret);
> +               useUDmaBuf_ = info.useUDmaBuf;
>                 break;
>         }
>  
> @@ -123,25 +130,10 @@ DmaHeap::~DmaHeap() = default;
>   * \return True if the DmaHeap is valid, false otherwise
>   */
>  
> -/**
> - * \brief Allocate a dma-buf from the DmaHeap
> - * \param [in] name The name to set for the allocated buffer
> - * \param [in] size The size of the buffer to allocate
> - *
> - * Allocates a dma-buf with read/write access.
> - *
> - * If the allocation fails, return an invalid UniqueFD.
> - *
> - * \return The UniqueFD of the allocated buffer
> - */
> -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> +UniqueFD DmaHeap::allocFromHeap(const char *name, std::size_t size)
>  {
> -       int ret;
> -
> -       if (!name)
> -               return {};
> -
>         struct dma_heap_allocation_data alloc = {};
> +       int ret;
>  
>         alloc.len = size;
>         alloc.fd_flags = O_CLOEXEC | O_RDWR;
> @@ -162,4 +154,97 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>         return allocFd;
>  }
>  
> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)
> +{
> +       /* size must be a multiple of the page-size round it up */
> +       std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> +       size = (size + pageMask) & ~pageMask;
> +
> +       int ret = memfd_create(name, MFD_ALLOW_SEALING);
> +       if (ret < 0) {
> +               ret = errno;
> +               LOG(DmaHeap, Error)
> +                       << "UdmaHeap failed to allocate memfd storage: "
> +                       << strerror(ret);

Completely optional/just an idea - I wonder if we should use 'name' in
the log messages here and below.

Would probably apply to allocFromHeap() too so likely a patch on top -
not an update here.


> +               return {};
> +       }
> +
> +       UniqueFD memfd(ret);
> +
> +       ret = ftruncate(memfd.get(), size);
> +       if (ret < 0) {
> +               ret = errno;
> +               LOG(DmaHeap, 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(DmaHeap, 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(dmaHeapHandle_.get(), UDMABUF_CREATE, &create);
> +       if (ret < 0) {
> +               ret = errno;
> +               LOG(DmaHeap, Error)
> +                       << "UdmaHeap failed to allocate " << size << " bytes: "
> +                       << strerror(ret);
> +               return {};
> +       }
> +
> +       if (create.size < size) {
> +               LOG(DmaHeap, Error)
> +                       << "UdmaHeap allocated " << create.size << " bytes instead of "
> +                       << size << " bytes";

Hrm, I think this path doesn't handle the now successfully created
UDMABuf handle. Do we need to free it?

Maybe we should move the 'UniqueFD uDma(ret);' above this statement so
that the UniqueFD handles it correctly for both paths?


> +               return {};
> +       }
> +
> +       if (create.size != size)
> +               LOG(DmaHeap, Warning)
> +                       << "UdmaHeap allocated " << create.size << " bytes, "
> +                       << "which is greater than requested : " << size << " bytes";
> +
> +       /* Fail if not suitable, the allocation will be free'd by UniqueFD */
> +       LOG(DmaHeap, Debug) << "UdmaHeap allocated " << create.size << " bytes";
> +
> +       /* The underlying memfd is kept as as a reference in the kernel */
> +       UniqueFD uDma(ret);
> +
> +       return uDma;
> +}
> +
> +/**
> + * \brief Allocate a dma-buf from the DmaHeap

I think this would now change from 'the DmaHeap' to 'a DmaHeap' but
that's just a nit.


Otherwise / with the above considered I'd put my name to this ;-)
(err... I think I co-wrote it in the past so I'm biased ;D)


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


> + * \param [in] name The name to set for the allocated buffer
> + * \param [in] size The size of the buffer to allocate
> + *
> + * Allocates a dma-buf with read/write access.
> + *
> + * If the allocation fails, return an invalid UniqueFD.
> + *
> + * \return The UniqueFD of the allocated buffer
> + */
> +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> +{
> +       if (!name)
> +               return {};
> +
> +       if (useUDmaBuf_)
> +               return allocFromUDmaBuf(name, size);
> +       else
> +               return allocFromHeap(name, size);
> +}
> +
>  } /* namespace libcamera */
> -- 
> 2.45.1
>

Hi Kieran,

On Tue, May 28, 2024 at 12:32:10PM GMT, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-05-27 15:16:47)
> > Add support for using udmabuf to alloc DMA heaps, this is basically:
> > https://patchwork.libcamera.org/patch/18922/
> > 
> > ported from the never merged HeapAllocator class to the current DmaHeap
> > class.
> 
> True statements, but not really specific to the content of the patch?
> 
> Perhaps:
> 
> """
> The DMA heap allocator currently allocates from CMA and system heaps.
> 
> These provide contiguous device memory but require dedicated and
> reserved memory regions to be configured on the platform and may require
> elevated privilidges to access.

Only the CMA allocator (and heap) reserves memory. The system heap uses
the page allocator.

Maxime

Hello,

On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-05-27 15:16:47)
> > Add support for using udmabuf to alloc DMA heaps, this is basically:
> > https://patchwork.libcamera.org/patch/18922/
> > 
> > ported from the never merged HeapAllocator class to the current DmaHeap
> > class.
> 
> True statements, but not really specific to the content of the patch?
> 
> Perhaps:
> 
> """
> The DMA heap allocator currently allocates from CMA and system heaps.
> 
> These provide contiguous device memory but require dedicated and
> reserved memory regions to be configured on the platform and may require
> elevated privilidges to access.

The system heap doesn't require reserved memory regions, and also
doesn't provide contiguous device memory. The CMA heap also doesn't
required reserved memory regions actually.

> Extend the dma_heap allocator to support falling back to the memfd
> allocator backed by the udmabuf framework to prepare buffers that are
> compatible with linux drivers and the libcamera API.

When reading the patch, my very first thought is that udmabuf doesn't
make use of DMA heaps, so the allocator should belong to a different
class.

Thinking a bit more about it, I understand the appeal of hiding this
fallback from the DmaHeap user. I'm however a bit concerned that the
dmabuf instances allocated from the DMA system heap and through udmabuf
will not operate in a fully identical way. Returning one or the other
without the user being aware of what has been allocated may lead to
issues.

I may worry needlessly though. I haven't compared the system heap and
the udmabuf implementations, to see if the dmabuf operations differ in
ways that are significant. I think this should at least be checked
before we make a decision.

> The buffers allocated through memfd/udmabuf will not be contiguous and
> are likely not suitable for directly rendering to a display or encoder
> but may facilitate more usecases for allocating memory in pipelines that

Let's make a stronger statement : "are not suitable for zero-copy buffer
sharing with other devices".

> make use of the CPU for processing including virtual pipeline handlers
> or the SoftISP.
> """
> 
> Feel free to adapt the above of course if I've made any errors.
> 
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  include/libcamera/internal/dma_heaps.h |   3 +
> >  src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
> >  2 files changed, 109 insertions(+), 21 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> > index f0a8aa5d..7e1271ed 100644
> > --- a/include/libcamera/internal/dma_heaps.h
> > +++ b/include/libcamera/internal/dma_heaps.h
> > @@ -30,7 +30,10 @@ public:
> >         UniqueFD alloc(const char *name, std::size_t size);
> >  
> >  private:
> > +       UniqueFD allocFromHeap(const char *name, std::size_t size);
> > +       UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
> >         UniqueFD dmaHeapHandle_;
> > +       bool useUDmaBuf_;
> >  };
> >  
> >  LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)
> > diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
> > index d4cb880b..bb707786 100644
> > --- a/src/libcamera/dma_heaps.cpp
> > +++ b/src/libcamera/dma_heaps.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>
> >  
> > @@ -36,13 +40,15 @@ namespace libcamera {
> >  struct DmaHeapInfo {
> >         DmaHeap::DmaHeapFlag type;
> >         const char *deviceNodeName;
> > +       bool useUDmaBuf;
> >  };
> >  #endif
> >  
> > -static constexpr std::array<DmaHeapInfo, 3> heapInfos = { {
> > -       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma" },
> > -       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved" },
> > -       { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system" },
> > +static constexpr std::array<DmaHeapInfo, 4> heapInfos = { {
> > +       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma", false },
> > +       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved", false },
> > +       { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system", false },
> > +       { DmaHeap::DmaHeapFlag::System, "/dev/udmabuf", true },
> 
> Should we distinguise UDMA with a different flag? or just simply keep it
> as a fall back?
> 
> If we do use a different DmaHeapFlag then I think we don't need the
> boolean...

Exposing the difference to the user may alleviate some of my concerns.
Should we then rename the DmaHeap class to DmaBufAllocator ?

> >  } };
> >  
> >  LOG_DEFINE_CATEGORY(DmaHeap)
> > @@ -105,6 +111,7 @@ DmaHeap::DmaHeap(DmaHeapFlags type)
> >  
> >                 LOG(DmaHeap, Debug) << "Using " << info.deviceNodeName;
> >                 dmaHeapHandle_ = UniqueFD(ret);
> > +               useUDmaBuf_ = info.useUDmaBuf;
> >                 break;
> >         }
> >  
> > @@ -123,25 +130,10 @@ DmaHeap::~DmaHeap() = default;
> >   * \return True if the DmaHeap is valid, false otherwise
> >   */
> >  
> > -/**
> > - * \brief Allocate a dma-buf from the DmaHeap
> > - * \param [in] name The name to set for the allocated buffer
> > - * \param [in] size The size of the buffer to allocate
> > - *
> > - * Allocates a dma-buf with read/write access.
> > - *
> > - * If the allocation fails, return an invalid UniqueFD.
> > - *
> > - * \return The UniqueFD of the allocated buffer
> > - */
> > -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> > +UniqueFD DmaHeap::allocFromHeap(const char *name, std::size_t size)
> >  {
> > -       int ret;
> > -
> > -       if (!name)
> > -               return {};
> > -
> >         struct dma_heap_allocation_data alloc = {};
> > +       int ret;
> >  
> >         alloc.len = size;
> >         alloc.fd_flags = O_CLOEXEC | O_RDWR;
> > @@ -162,4 +154,97 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> >         return allocFd;
> >  }
> >  
> > +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)
> > +{
> > +       /* size must be a multiple of the page-size round it up */

s/size/Size/
s/ up/ up./

> > +       std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> > +       size = (size + pageMask) & ~pageMask;
> > +
> > +       int ret = memfd_create(name, MFD_ALLOW_SEALING);

Needs sys/mman.h.

> > +       if (ret < 0) {
> > +               ret = errno;
> > +               LOG(DmaHeap, Error)
> > +                       << "UdmaHeap failed to allocate memfd storage: "

UdmaHeap sounds like a frankenstein monster :-) Did you mean UDmaBuf
here ? I would just drop the prefix from this message and the ones below
actually.

> > +                       << strerror(ret);
> 
> Completely optional/just an idea - I wonder if we should use 'name' in
> the log messages here and below.
> 
> Would probably apply to allocFromHeap() too so likely a patch on top -
> not an update here.
> 
> > +               return {};
> > +       }
> > +
> > +       UniqueFD memfd(ret);
> > +
> > +       ret = ftruncate(memfd.get(), size);
> > +       if (ret < 0) {
> > +               ret = errno;
> > +               LOG(DmaHeap, Error)
> > +                       << "UdmaHeap failed to set memfd size: " << strerror(ret);
> > +               return {};
> > +       }
> > +
> > +       /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */

s/seal/seal./

> > +       ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);
> > +       if (ret < 0) {
> > +               ret = errno;
> > +               LOG(DmaHeap, 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(dmaHeapHandle_.get(), UDMABUF_CREATE, &create);
> > +       if (ret < 0) {
> > +               ret = errno;
> > +               LOG(DmaHeap, Error)
> > +                       << "UdmaHeap failed to allocate " << size << " bytes: "
> > +                       << strerror(ret);
> > +               return {};
> > +       }
> > +
> > +       if (create.size < size) {
> > +               LOG(DmaHeap, Error)
> > +                       << "UdmaHeap allocated " << create.size << " bytes instead of "
> > +                       << size << " bytes";
> 
> Hrm, I think this path doesn't handle the now successfully created
> UDMABuf handle. Do we need to free it?
> 
> Maybe we should move the 'UniqueFD uDma(ret);' above this statement so
> that the UniqueFD handles it correctly for both paths?

Sounds good.

> > +               return {};
> > +       }
> > +
> > +       if (create.size != size)
> > +               LOG(DmaHeap, Warning)
> > +                       << "UdmaHeap allocated " << create.size << " bytes, "
> > +                       << "which is greater than requested : " << size << " bytes";

Why/when does this happen ?

> > +
> > +       /* Fail if not suitable, the allocation will be free'd by UniqueFD */

s/UniqueFD/UniqueFD./

That comment doesn't seem correct though, there are no further checks.
Is it a leftover ?

> > +       LOG(DmaHeap, Debug) << "UdmaHeap allocated " << create.size << " bytes";
> > +
> > +       /* The underlying memfd is kept as as a reference in the kernel */
> > +       UniqueFD uDma(ret);
> > +
> > +       return uDma;
> > +}
> > +
> > +/**
> > + * \brief Allocate a dma-buf from the DmaHeap
> 
> I think this would now change from 'the DmaHeap' to 'a DmaHeap' but
> that's just a nit.

I don't think that's correct. The allocator still allocates the buffer
from one specific backend, selected when the allocator is constructed.

> Otherwise / with the above considered I'd put my name to this ;-)
> (err... I think I co-wrote it in the past so I'm biased ;D)
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > + * \param [in] name The name to set for the allocated buffer
> > + * \param [in] size The size of the buffer to allocate
> > + *
> > + * Allocates a dma-buf with read/write access.
> > + *
> > + * If the allocation fails, return an invalid UniqueFD.
> > + *
> > + * \return The UniqueFD of the allocated buffer
> > + */
> > +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
> > +{
> > +       if (!name)
> > +               return {};
> > +
> > +       if (useUDmaBuf_)
> > +               return allocFromUDmaBuf(name, size);
> > +       else
> > +               return allocFromHeap(name, size);
> > +}
> > +
> >  } /* namespace libcamera */

Quoting Hans de Goede (2024-05-27 15:16:47)
> Add support for using udmabuf to alloc DMA heaps, this is basically:
> https://patchwork.libcamera.org/patch/18922/
> 
> ported from the never merged HeapAllocator class to the current DmaHeap
> class.
> 
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  include/libcamera/internal/dma_heaps.h |   3 +
>  src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
>  2 files changed, 109 insertions(+), 21 deletions(-)
> 
> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> index f0a8aa5d..7e1271ed 100644
> --- a/include/libcamera/internal/dma_heaps.h
> +++ b/include/libcamera/internal/dma_heaps.h
> @@ -30,7 +30,10 @@ public:
>         UniqueFD alloc(const char *name, std::size_t size);
>  
>  private:
> +       UniqueFD allocFromHeap(const char *name, std::size_t size);
> +       UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
>         UniqueFD dmaHeapHandle_;
> +       bool useUDmaBuf_;
>  };
>  
>  LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)
> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
> index d4cb880b..bb707786 100644
> --- a/src/libcamera/dma_heaps.cpp
> +++ b/src/libcamera/dma_heaps.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>

Hrm. this fails on CI. Perhaps it's a new header, and we might have to
bring this into include/linux in libcamera.

 - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/59258127

--
Kieran

On Wed, May 29, 2024 at 12:07:08AM +0100, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-05-27 15:16:47)
> > Add support for using udmabuf to alloc DMA heaps, this is basically:
> > https://patchwork.libcamera.org/patch/18922/
> > 
> > ported from the never merged HeapAllocator class to the current DmaHeap
> > class.
> > 
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  include/libcamera/internal/dma_heaps.h |   3 +
> >  src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
> >  2 files changed, 109 insertions(+), 21 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
> > index f0a8aa5d..7e1271ed 100644
> > --- a/include/libcamera/internal/dma_heaps.h
> > +++ b/include/libcamera/internal/dma_heaps.h
> > @@ -30,7 +30,10 @@ public:
> >         UniqueFD alloc(const char *name, std::size_t size);
> >  
> >  private:
> > +       UniqueFD allocFromHeap(const char *name, std::size_t size);
> > +       UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
> >         UniqueFD dmaHeapHandle_;
> > +       bool useUDmaBuf_;
> >  };
> >  
> >  LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)
> > diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
> > index d4cb880b..bb707786 100644
> > --- a/src/libcamera/dma_heaps.cpp
> > +++ b/src/libcamera/dma_heaps.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>
> 
> Hrm. this fails on CI. Perhaps it's a new header, and we might have to
> bring this into include/linux in libcamera.
> 
>  - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/59258127

Please update and use utils/update-kernel-headers.sh to fix that.

On 27/05/2024 15:16, Hans de Goede wrote:
> @@ -105,6 +111,7 @@ DmaHeap::DmaHeap(DmaHeapFlags type)
>   
>   		LOG(DmaHeap, Debug) << "Using " << info.deviceNodeName;
>   		dmaHeapHandle_ = UniqueFD(ret);
> +		useUDmaBuf_ = info.useUDmaBuf;
>   		break;

The comment at the top of this function needs to be updated

87ee158ec

  * The DMA heap type is selected with the \a type parameter, which 
defaults to
  * the CMA heap. If no heap of the given type can be accessed, the 
constructed
  * DmaHeap instance is invalid as indicated by the isValid() function.
  *

---
bod

On 27/05/2024 15:16, Hans de Goede wrote:
> +static constexpr std::array<DmaHeapInfo, 4> heapInfos = { {
> +	{ DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma", false },
> +	{ DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved", false },
> +	{ DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system", false },
> +	{ DmaHeap::DmaHeapFlag::System, "/dev/udmabuf", true },
>   } };

I have

deckard@x13s-linux:~/Development/linux$ ls /dev/udmabuf
/dev/udmabuf
deckard@x13s-linux:~/Development/linux$ ls /dev/d
disk/     dma_heap/ dri/
deckard@x13s-linux:~/Development/linux$ ls /dev/dma_heap/
linux,cma  system

LIBCAMERA_LOG_LEVELS=*:DEBUG ./build.master.dbg/src/apps/qcam/qcam

Gives

[0:15:09.128108059] [6874] DEBUG DmaHeap dma_heaps.cpp:112 Using 
/dev/dma_heap/linux,cma

I thought we said udmabuf should be default over cma_heap ?

At a risk of displaying a lack of knowledge, I tried forcing the flag to 
useUDmaBuf = true;

I get

Zero-copy enabled
[0:18:13.834305484] [7388] ERROR DmaHeap dma_heaps.cpp:201 UdmaHeap 
failed to allocate 15073280 bytes: Invalid argument
[0:18:13.834365480] [7388] ERROR SoftwareIsp software_isp.cpp:246 failed 
to allocate a dma_buf

Do I need to do something more than CONFIG_UDAMBUF=y ?

---
bod

Hi,

On 5/28/24 11:37 PM, Laurent Pinchart wrote:
> Hello,
> 
> On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:
>> Quoting Hans de Goede (2024-05-27 15:16:47)
>>> Add support for using udmabuf to alloc DMA heaps, this is basically:
>>> https://patchwork.libcamera.org/patch/18922/
>>>
>>> ported from the never merged HeapAllocator class to the current DmaHeap
>>> class.
>>
>> True statements, but not really specific to the content of the patch?
>>
>> Perhaps:
>>
>> """
>> The DMA heap allocator currently allocates from CMA and system heaps.
>>
>> These provide contiguous device memory but require dedicated and
>> reserved memory regions to be configured on the platform and may require
>> elevated privilidges to access.
> 
> The system heap doesn't require reserved memory regions, and also
> doesn't provide contiguous device memory. The CMA heap also doesn't
> required reserved memory regions actually.
> 
>> Extend the dma_heap allocator to support falling back to the memfd
>> allocator backed by the udmabuf framework to prepare buffers that are
>> compatible with linux drivers and the libcamera API.
> 
> When reading the patch, my very first thought is that udmabuf doesn't
> make use of DMA heaps, so the allocator should belong to a different
> class.
> 
> Thinking a bit more about it, I understand the appeal of hiding this
> fallback from the DmaHeap user. I'm however a bit concerned that the
> dmabuf instances allocated from the DMA system heap and through udmabuf
> will not operate in a fully identical way. Returning one or the other
> without the user being aware of what has been allocated may lead to
> issues.
> 
> I may worry needlessly though. I haven't compared the system heap and
> the udmabuf implementations, to see if the dmabuf operations differ in
> ways that are significant. I think this should at least be checked
> before we make a decision.
> 
>> The buffers allocated through memfd/udmabuf will not be contiguous and
>> are likely not suitable for directly rendering to a display or encoder
>> but may facilitate more usecases for allocating memory in pipelines that
> 
> Let's make a stronger statement : "are not suitable for zero-copy buffer
> sharing with other devices".

Sounds good I'll come up with a new commit message for v2 taking all the various
remarks into account.

>> make use of the CPU for processing including virtual pipeline handlers
>> or the SoftISP.
>> """
>>
>> Feel free to adapt the above of course if I've made any errors.
>>
>>> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
>>> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  include/libcamera/internal/dma_heaps.h |   3 +
>>>  src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
>>>  2 files changed, 109 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/libcamera/internal/dma_heaps.h b/include/libcamera/internal/dma_heaps.h
>>> index f0a8aa5d..7e1271ed 100644
>>> --- a/include/libcamera/internal/dma_heaps.h
>>> +++ b/include/libcamera/internal/dma_heaps.h
>>> @@ -30,7 +30,10 @@ public:
>>>         UniqueFD alloc(const char *name, std::size_t size);
>>>  
>>>  private:
>>> +       UniqueFD allocFromHeap(const char *name, std::size_t size);
>>> +       UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
>>>         UniqueFD dmaHeapHandle_;
>>> +       bool useUDmaBuf_;
>>>  };
>>>  
>>>  LIBCAMERA_FLAGS_ENABLE_OPERATORS(DmaHeap::DmaHeapFlag)
>>> diff --git a/src/libcamera/dma_heaps.cpp b/src/libcamera/dma_heaps.cpp
>>> index d4cb880b..bb707786 100644
>>> --- a/src/libcamera/dma_heaps.cpp
>>> +++ b/src/libcamera/dma_heaps.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>
>>>  
>>> @@ -36,13 +40,15 @@ namespace libcamera {
>>>  struct DmaHeapInfo {
>>>         DmaHeap::DmaHeapFlag type;
>>>         const char *deviceNodeName;
>>> +       bool useUDmaBuf;
>>>  };
>>>  #endif
>>>  
>>> -static constexpr std::array<DmaHeapInfo, 3> heapInfos = { {
>>> -       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma" },
>>> -       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved" },
>>> -       { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system" },
>>> +static constexpr std::array<DmaHeapInfo, 4> heapInfos = { {
>>> +       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma", false },
>>> +       { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved", false },
>>> +       { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system", false },
>>> +       { DmaHeap::DmaHeapFlag::System, "/dev/udmabuf", true },
>>
>> Should we distinguise UDMA with a different flag? or just simply keep it
>> as a fall back?
>>
>> If we do use a different DmaHeapFlag then I think we don't need the
>> boolean...
> 
> Exposing the difference to the user may alleviate some of my concerns.
> Should we then rename the DmaHeap class to DmaBufAllocator ?

That sounds like a good idea. I'll add a patch to rename the class
as first patch in the series for v2 of the series.

> 
>>>  } };
>>>  
>>>  LOG_DEFINE_CATEGORY(DmaHeap)
>>> @@ -105,6 +111,7 @@ DmaHeap::DmaHeap(DmaHeapFlags type)
>>>  
>>>                 LOG(DmaHeap, Debug) << "Using " << info.deviceNodeName;
>>>                 dmaHeapHandle_ = UniqueFD(ret);
>>> +               useUDmaBuf_ = info.useUDmaBuf;
>>>                 break;
>>>         }
>>>  
>>> @@ -123,25 +130,10 @@ DmaHeap::~DmaHeap() = default;
>>>   * \return True if the DmaHeap is valid, false otherwise
>>>   */
>>>  
>>> -/**
>>> - * \brief Allocate a dma-buf from the DmaHeap
>>> - * \param [in] name The name to set for the allocated buffer
>>> - * \param [in] size The size of the buffer to allocate
>>> - *
>>> - * Allocates a dma-buf with read/write access.
>>> - *
>>> - * If the allocation fails, return an invalid UniqueFD.
>>> - *
>>> - * \return The UniqueFD of the allocated buffer
>>> - */
>>> -UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>>> +UniqueFD DmaHeap::allocFromHeap(const char *name, std::size_t size)
>>>  {
>>> -       int ret;
>>> -
>>> -       if (!name)
>>> -               return {};
>>> -
>>>         struct dma_heap_allocation_data alloc = {};
>>> +       int ret;
>>>  
>>>         alloc.len = size;
>>>         alloc.fd_flags = O_CLOEXEC | O_RDWR;
>>> @@ -162,4 +154,97 @@ UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>>>         return allocFd;
>>>  }
>>>  
>>> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)
>>> +{
>>> +       /* size must be a multiple of the page-size round it up */
> 
> s/size/Size/
> s/ up/ up./

Ack.

>>> +       std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
>>> +       size = (size + pageMask) & ~pageMask;
>>> +
>>> +       int ret = memfd_create(name, MFD_ALLOW_SEALING);
> 
> Needs sys/mman.h.

Ack.

>>> +       if (ret < 0) {
>>> +               ret = errno;
>>> +               LOG(DmaHeap, Error)
>>> +                       << "UdmaHeap failed to allocate memfd storage: "
> 
> UdmaHeap sounds like a frankenstein monster :-) Did you mean UDmaBuf
> here ? I would just drop the prefix from this message and the ones below
> actually.

I'll drop the prefix from the log message for v2.

>>> +                       << strerror(ret);
>>
>> Completely optional/just an idea - I wonder if we should use 'name' in
>> the log messages here and below.
>>
>> Would probably apply to allocFromHeap() too so likely a patch on top -
>> not an update here.
>>
>>> +               return {};
>>> +       }
>>> +
>>> +       UniqueFD memfd(ret);
>>> +
>>> +       ret = ftruncate(memfd.get(), size);
>>> +       if (ret < 0) {
>>> +               ret = errno;
>>> +               LOG(DmaHeap, Error)
>>> +                       << "UdmaHeap failed to set memfd size: " << strerror(ret);
>>> +               return {};
>>> +       }
>>> +
>>> +       /* UdmaHeap Buffers *must* have the F_SEAL_SHRINK seal */
> 
> s/seal/seal./
> 
>>> +       ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK);
>>> +       if (ret < 0) {
>>> +               ret = errno;
>>> +               LOG(DmaHeap, 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(dmaHeapHandle_.get(), UDMABUF_CREATE, &create);
>>> +       if (ret < 0) {
>>> +               ret = errno;
>>> +               LOG(DmaHeap, Error)
>>> +                       << "UdmaHeap failed to allocate " << size << " bytes: "
>>> +                       << strerror(ret);
>>> +               return {};
>>> +       }
>>> +
>>> +       if (create.size < size) {
>>> +               LOG(DmaHeap, Error)
>>> +                       << "UdmaHeap allocated " << create.size << " bytes instead of "
>>> +                       << size << " bytes";
>>
>> Hrm, I think this path doesn't handle the now successfully created
>> UDMABuf handle. Do we need to free it?
>>
>> Maybe we should move the 'UniqueFD uDma(ret);' above this statement so
>> that the UniqueFD handles it correctly for both paths?
> 
> Sounds good.

Ack.

>>> +               return {};
>>> +       }
>>> +
>>> +       if (create.size != size)
>>> +               LOG(DmaHeap, Warning)
>>> +                       << "UdmaHeap allocated " << create.size << " bytes, "
>>> +                       << "which is greater than requested : " << size << " bytes";
> 
> Why/when does this happen ?

This was taken from the original patch which I based this on. Shall I drop
the check ?

> 
>>> +
>>> +       /* Fail if not suitable, the allocation will be free'd by UniqueFD */
> 
> s/UniqueFD/UniqueFD./
> 
> That comment doesn't seem correct though, there are no further checks.
> Is it a leftover ?

If we move the UniqueFD declaration + this comment up then it does seem like
a valid comment.


> 
>>> +       LOG(DmaHeap, Debug) << "UdmaHeap allocated " << create.size << " bytes";
>>> +
>>> +       /* The underlying memfd is kept as as a reference in the kernel */
>>> +       UniqueFD uDma(ret);
>>> +
>>> +       return uDma;
>>> +}
>>> +
>>> +/**
>>> + * \brief Allocate a dma-buf from the DmaHeap
>>
>> I think this would now change from 'the DmaHeap' to 'a DmaHeap' but
>> that's just a nit.
> 
> I don't think that's correct. The allocator still allocates the buffer
> from one specific backend, selected when the allocator is constructed.
> 
>> Otherwise / with the above considered I'd put my name to this ;-)
>> (err... I think I co-wrote it in the past so I'm biased ;D)
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>>> + * \param [in] name The name to set for the allocated buffer
>>> + * \param [in] size The size of the buffer to allocate
>>> + *
>>> + * Allocates a dma-buf with read/write access.
>>> + *
>>> + * If the allocation fails, return an invalid UniqueFD.
>>> + *
>>> + * \return The UniqueFD of the allocated buffer
>>> + */
>>> +UniqueFD DmaHeap::alloc(const char *name, std::size_t size)
>>> +{
>>> +       if (!name)
>>> +               return {};
>>> +
>>> +       if (useUDmaBuf_)
>>> +               return allocFromUDmaBuf(name, size);
>>> +       else
>>> +               return allocFromHeap(name, size);
>>> +}
>>> +
>>>  } /* namespace libcamera */
> 


Regards,

Hans

Quoting Hans de Goede (2024-05-29 17:26:50)
> Hi,
> 
> On 5/28/24 11:37 PM, Laurent Pinchart wrote:
> > Hello,
> > 
> > On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:
> >> Quoting Hans de Goede (2024-05-27 15:16:47)
> >>> Add support for using udmabuf to alloc DMA heaps, this is basically:
> >>> https://patchwork.libcamera.org/patch/18922/
> >>>
> >>> ported from the never merged HeapAllocator class to the current DmaHeap
> >>> class.
> >>
> >> True statements, but not really specific to the content of the patch?
> >>
> >> Perhaps:
> >>
> >> """
> >> The DMA heap allocator currently allocates from CMA and system heaps.
> >>
> >> These provide contiguous device memory but require dedicated and
> >> reserved memory regions to be configured on the platform and may require
> >> elevated privilidges to access.
> > 
> > The system heap doesn't require reserved memory regions, and also
> > doesn't provide contiguous device memory. The CMA heap also doesn't
> > required reserved memory regions actually.
> > 
> >> Extend the dma_heap allocator to support falling back to the memfd
> >> allocator backed by the udmabuf framework to prepare buffers that are
> >> compatible with linux drivers and the libcamera API.
> > 
> > When reading the patch, my very first thought is that udmabuf doesn't
> > make use of DMA heaps, so the allocator should belong to a different
> > class.
> > 
> > Thinking a bit more about it, I understand the appeal of hiding this
> > fallback from the DmaHeap user. I'm however a bit concerned that the
> > dmabuf instances allocated from the DMA system heap and through udmabuf
> > will not operate in a fully identical way. Returning one or the other
> > without the user being aware of what has been allocated may lead to
> > issues.
> > 
> > I may worry needlessly though. I haven't compared the system heap and
> > the udmabuf implementations, to see if the dmabuf operations differ in
> > ways that are significant. I think this should at least be checked
> > before we make a decision.
> > 
> >> The buffers allocated through memfd/udmabuf will not be contiguous and
> >> are likely not suitable for directly rendering to a display or encoder
> >> but may facilitate more usecases for allocating memory in pipelines that
> > 
> > Let's make a stronger statement : "are not suitable for zero-copy buffer
> > sharing with other devices".
> 
> Sounds good I'll come up with a new commit message for v2 taking all the various
> remarks into account.
> 
> >> make use of the CPU for processing including virtual pipeline handlers
> >> or the SoftISP.
> >> """
> >>
> >> Feel free to adapt the above of course if I've made any errors.
> >>
> >>> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> >>> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> ---
> >>>  include/libcamera/internal/dma_heaps.h |   3 +
> >>>  src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
> >>>  2 files changed, 109 insertions(+), 21 deletions(-)

... snip ...

> >>> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)
> >>> +{
> >>> +       /* size must be a multiple of the page-size round it up */
> > 
> > s/size/Size/
> > s/ up/ up./
> 
> Ack.
> 
> >>> +       std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> >>> +       size = (size + pageMask) & ~pageMask;
> >>> +
> >>> +       int ret = memfd_create(name, MFD_ALLOW_SEALING);
> > 
> > Needs sys/mman.h.
> 
> Ack.

... snip ...

> 
> >>> +               return {};
> >>> +       }
> >>> +
> >>> +       if (create.size != size)
> >>> +               LOG(DmaHeap, Warning)
> >>> +                       << "UdmaHeap allocated " << create.size << " bytes, "
> >>> +                       << "which is greater than requested : " << size << " bytes";
> > 
> > Why/when does this happen ?
> 
> This was taken from the original patch which I based this on. Shall I drop
> the check ?

I'm fine dropping it. I bet it was from my prototyping code where I was
likely investigating why I didn't get the size I requested, which
probably led to the comment "size must be a multiple of the page-size
round it up" at the top.

--
Kieran

Hi,

On 5/29/24 6:43 PM, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-05-29 17:26:50)
>> Hi,
>>
>> On 5/28/24 11:37 PM, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:
>>>> Quoting Hans de Goede (2024-05-27 15:16:47)
>>>>> Add support for using udmabuf to alloc DMA heaps, this is basically:
>>>>> https://patchwork.libcamera.org/patch/18922/
>>>>>
>>>>> ported from the never merged HeapAllocator class to the current DmaHeap
>>>>> class.
>>>>
>>>> True statements, but not really specific to the content of the patch?
>>>>
>>>> Perhaps:
>>>>
>>>> """
>>>> The DMA heap allocator currently allocates from CMA and system heaps.
>>>>
>>>> These provide contiguous device memory but require dedicated and
>>>> reserved memory regions to be configured on the platform and may require
>>>> elevated privilidges to access.
>>>
>>> The system heap doesn't require reserved memory regions, and also
>>> doesn't provide contiguous device memory. The CMA heap also doesn't
>>> required reserved memory regions actually.
>>>
>>>> Extend the dma_heap allocator to support falling back to the memfd
>>>> allocator backed by the udmabuf framework to prepare buffers that are
>>>> compatible with linux drivers and the libcamera API.
>>>
>>> When reading the patch, my very first thought is that udmabuf doesn't
>>> make use of DMA heaps, so the allocator should belong to a different
>>> class.
>>>
>>> Thinking a bit more about it, I understand the appeal of hiding this
>>> fallback from the DmaHeap user. I'm however a bit concerned that the
>>> dmabuf instances allocated from the DMA system heap and through udmabuf
>>> will not operate in a fully identical way. Returning one or the other
>>> without the user being aware of what has been allocated may lead to
>>> issues.
>>>
>>> I may worry needlessly though. I haven't compared the system heap and
>>> the udmabuf implementations, to see if the dmabuf operations differ in
>>> ways that are significant. I think this should at least be checked
>>> before we make a decision.
>>>
>>>> The buffers allocated through memfd/udmabuf will not be contiguous and
>>>> are likely not suitable for directly rendering to a display or encoder
>>>> but may facilitate more usecases for allocating memory in pipelines that
>>>
>>> Let's make a stronger statement : "are not suitable for zero-copy buffer
>>> sharing with other devices".
>>
>> Sounds good I'll come up with a new commit message for v2 taking all the various
>> remarks into account.
>>
>>>> make use of the CPU for processing including virtual pipeline handlers
>>>> or the SoftISP.
>>>> """
>>>>
>>>> Feel free to adapt the above of course if I've made any errors.
>>>>
>>>>> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
>>>>> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>>  include/libcamera/internal/dma_heaps.h |   3 +
>>>>>  src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
>>>>>  2 files changed, 109 insertions(+), 21 deletions(-)
> 
> ... snip ...
> 
>>>>> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)
>>>>> +{
>>>>> +       /* size must be a multiple of the page-size round it up */
>>>
>>> s/size/Size/
>>> s/ up/ up./
>>
>> Ack.
>>
>>>>> +       std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
>>>>> +       size = (size + pageMask) & ~pageMask;
>>>>> +
>>>>> +       int ret = memfd_create(name, MFD_ALLOW_SEALING);
>>>
>>> Needs sys/mman.h.
>>
>> Ack.
> 
> ... snip ...
> 
>>
>>>>> +               return {};
>>>>> +       }
>>>>> +
>>>>> +       if (create.size != size)
>>>>> +               LOG(DmaHeap, Warning)
>>>>> +                       << "UdmaHeap allocated " << create.size << " bytes, "
>>>>> +                       << "which is greater than requested : " << size << " bytes";
>>>
>>> Why/when does this happen ?
>>
>> This was taken from the original patch which I based this on. Shall I drop
>> the check ?
> 
> I'm fine dropping it. I bet it was from my prototyping code where I was
> likely investigating why I didn't get the size I requested,

Ok, I'll drop it.

> which
> probably led to the comment "size must be a multiple of the page-size
> round it up" at the top.

The page-rounding of size is actually an addition by me, otherwise the
UDMABUF_CREATE call fails with errno=EINVAL.

Regards,

Hans

Quoting Hans de Goede (2024-05-29 18:07:01)
> Hi,
> 
> On 5/29/24 6:43 PM, Kieran Bingham wrote:
> > Quoting Hans de Goede (2024-05-29 17:26:50)
> >> Hi,
> >>
> >> On 5/28/24 11:37 PM, Laurent Pinchart wrote:
> >>> Hello,
> >>>
> >>> On Tue, May 28, 2024 at 12:32:10PM +0100, Kieran Bingham wrote:
> >>>> Quoting Hans de Goede (2024-05-27 15:16:47)
> >>>>> Add support for using udmabuf to alloc DMA heaps, this is basically:
> >>>>> https://patchwork.libcamera.org/patch/18922/
> >>>>>
> >>>>> ported from the never merged HeapAllocator class to the current DmaHeap
> >>>>> class.
> >>>>
> >>>> True statements, but not really specific to the content of the patch?
> >>>>
> >>>> Perhaps:
> >>>>
> >>>> """
> >>>> The DMA heap allocator currently allocates from CMA and system heaps.
> >>>>
> >>>> These provide contiguous device memory but require dedicated and
> >>>> reserved memory regions to be configured on the platform and may require
> >>>> elevated privilidges to access.
> >>>
> >>> The system heap doesn't require reserved memory regions, and also
> >>> doesn't provide contiguous device memory. The CMA heap also doesn't
> >>> required reserved memory regions actually.
> >>>
> >>>> Extend the dma_heap allocator to support falling back to the memfd
> >>>> allocator backed by the udmabuf framework to prepare buffers that are
> >>>> compatible with linux drivers and the libcamera API.
> >>>
> >>> When reading the patch, my very first thought is that udmabuf doesn't
> >>> make use of DMA heaps, so the allocator should belong to a different
> >>> class.
> >>>
> >>> Thinking a bit more about it, I understand the appeal of hiding this
> >>> fallback from the DmaHeap user. I'm however a bit concerned that the
> >>> dmabuf instances allocated from the DMA system heap and through udmabuf
> >>> will not operate in a fully identical way. Returning one or the other
> >>> without the user being aware of what has been allocated may lead to
> >>> issues.
> >>>
> >>> I may worry needlessly though. I haven't compared the system heap and
> >>> the udmabuf implementations, to see if the dmabuf operations differ in
> >>> ways that are significant. I think this should at least be checked
> >>> before we make a decision.
> >>>
> >>>> The buffers allocated through memfd/udmabuf will not be contiguous and
> >>>> are likely not suitable for directly rendering to a display or encoder
> >>>> but may facilitate more usecases for allocating memory in pipelines that
> >>>
> >>> Let's make a stronger statement : "are not suitable for zero-copy buffer
> >>> sharing with other devices".
> >>
> >> Sounds good I'll come up with a new commit message for v2 taking all the various
> >> remarks into account.
> >>
> >>>> make use of the CPU for processing including virtual pipeline handlers
> >>>> or the SoftISP.
> >>>> """
> >>>>
> >>>> Feel free to adapt the above of course if I've made any errors.
> >>>>
> >>>>> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> >>>>> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>> ---
> >>>>>  include/libcamera/internal/dma_heaps.h |   3 +
> >>>>>  src/libcamera/dma_heaps.cpp            | 127 +++++++++++++++++++++----
> >>>>>  2 files changed, 109 insertions(+), 21 deletions(-)
> > 
> > ... snip ...
> > 
> >>>>> +UniqueFD DmaHeap::allocFromUDmaBuf(const char *name, std::size_t size)
> >>>>> +{
> >>>>> +       /* size must be a multiple of the page-size round it up */
> >>>
> >>> s/size/Size/
> >>> s/ up/ up./
> >>
> >> Ack.
> >>
> >>>>> +       std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1;
> >>>>> +       size = (size + pageMask) & ~pageMask;
> >>>>> +
> >>>>> +       int ret = memfd_create(name, MFD_ALLOW_SEALING);
> >>>
> >>> Needs sys/mman.h.
> >>
> >> Ack.
> > 
> > ... snip ...
> > 
> >>
> >>>>> +               return {};
> >>>>> +       }
> >>>>> +
> >>>>> +       if (create.size != size)
> >>>>> +               LOG(DmaHeap, Warning)
> >>>>> +                       << "UdmaHeap allocated " << create.size << " bytes, "
> >>>>> +                       << "which is greater than requested : " << size << " bytes";
> >>>
> >>> Why/when does this happen ?
> >>
> >> This was taken from the original patch which I based this on. Shall I drop
> >> the check ?
> > 
> > I'm fine dropping it. I bet it was from my prototyping code where I was
> > likely investigating why I didn't get the size I requested,
> 
> Ok, I'll drop it.

I dug up the original WIP:
 - https://github.com/kbingham/libcamera/commit/8d79af53eaf461bab246c2fdeb1d9fdcbb288743#diff-fb4c595461a4d74b20a4684fa27b9ea77b2571102ba45381666d5e6e40f0a143R102-R109

So it probably started there indeed. Anyway - I think this code has come
a long way (for the better) since then ;-)


> > which
> > probably led to the comment "size must be a multiple of the page-size
> > round it up" at the top.
> 
> The page-rounding of size is actually an addition by me, otherwise the
> UDMABUF_CREATE call fails with errno=EINVAL.

Aha, well then it's a great addition ;-)

Thanks!

--
Kieran

> 
> Regards,
> 
> Hans
> 
> 
>

Hi Brian,

On 5/29/24 2:29 AM, Bryan O'Donoghue wrote:
> On 27/05/2024 15:16, Hans de Goede wrote:
>> +static constexpr std::array<DmaHeapInfo, 4> heapInfos = { {
>> +    { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma", false },
>> +    { DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved", false },
>> +    { DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system", false },
>> +    { DmaHeap::DmaHeapFlag::System, "/dev/udmabuf", true },
>>   } };
> 
> I have
> 
> deckard@x13s-linux:~/Development/linux$ ls /dev/udmabuf
> /dev/udmabuf
> deckard@x13s-linux:~/Development/linux$ ls /dev/d
> disk/     dma_heap/ dri/
> deckard@x13s-linux:~/Development/linux$ ls /dev/dma_heap/
> linux,cma  system
> 
> LIBCAMERA_LOG_LEVELS=*:DEBUG ./build.master.dbg/src/apps/qcam/qcam
> 
> Gives
> 
> [0:15:09.128108059] [6874] DEBUG DmaHeap dma_heaps.cpp:112 Using /dev/dma_heap/linux,cma
> 
> I thought we said udmabuf should be default over cma_heap ?
> 
> At a risk of displaying a lack of knowledge, I tried forcing the flag to useUDmaBuf = true;
> 
> I get
> 
> Zero-copy enabled
> [0:18:13.834305484] [7388] ERROR DmaHeap dma_heaps.cpp:201 UdmaHeap failed to allocate 15073280 bytes: Invalid argument
> [0:18:13.834365480] [7388] ERROR SoftwareIsp software_isp.cpp:246 failed to allocate a dma_buf
> 
> Do I need to do something more than CONFIG_UDAMBUF=y ?

If you just set "useUDmaBuf = true;" then the DmaHeap code will still
open /dev/dma_heap/linux,cma  but you are now using that fd with
code-paths which expect an fd for /dev/udmabuf so it is unsurprising
that that does not work.

To test the udmabuf code you can either modify:

static constexpr std::array<DmaHeapInfo, 4> heapInfos = { {
	{ DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/linux,cma", false },
	{ DmaHeap::DmaHeapFlag::Cma, "/dev/dma_heap/reserved", false },
	{ DmaHeap::DmaHeapFlag::System, "/dev/dma_heap/system", false },
	{ DmaHeap::DmaHeapFlag::System, "/dev/udmabuf", true },
} };

To only list the /dev/udmabuf line; or just do:

sudo rm /dev/dma_heap/*

Note I'm posting a v2 of the udmabuf series in a couple of minutes,
if you are going to re-test please test that.

Regards,

Hans


 

> 
> ---
> bod
>