[v11,1/7] libcamera: add DmaBufAllocator::exportBuffers()

21193 diff mbox series

Add a helper function exportBuffers in DmaBufAllocator to make it easier
to use.

It'll be used in Virtual Pipeline Handler and SoftwareIsp.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 .../libcamera/internal/dma_buf_allocator.h    | 13 +++++
 src/libcamera/dma_buf_allocator.cpp           | 55 +++++++++++++++++++
 2 files changed, 68 insertions(+)

Quoting Harvey Yang (2024-09-07 15:28:26)
> Add a helper function exportBuffers in DmaBufAllocator to make it easier
> to use.
> 
> It'll be used in Virtual Pipeline Handler and SoftwareIsp.
> 
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  .../libcamera/internal/dma_buf_allocator.h    | 13 +++++
>  src/libcamera/dma_buf_allocator.cpp           | 55 +++++++++++++++++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h
> index d2a0a0d1..66d3b419 100644
> --- a/include/libcamera/internal/dma_buf_allocator.h
> +++ b/include/libcamera/internal/dma_buf_allocator.h
> @@ -7,11 +7,17 @@
>  
>  #pragma once
>  
> +#include <memory>
> +#include <string>
> +#include <vector>
> +
>  #include <libcamera/base/flags.h>
>  #include <libcamera/base/unique_fd.h>
>  
>  namespace libcamera {
>  
> +class FrameBuffer;
> +
>  class DmaBufAllocator
>  {
>  public:
> @@ -28,7 +34,14 @@ public:
>         bool isValid() const { return providerHandle_.isValid(); }
>         UniqueFD alloc(const char *name, std::size_t size);
>  
> +       int exportBuffers(unsigned int count,
> +                         const std::vector<unsigned int> &frameSize,
> +                         std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> +
>  private:
> +       std::unique_ptr<FrameBuffer> createBuffer(
> +               std::string name, const std::vector<unsigned int> &frameSizes);
> +
>         UniqueFD allocFromHeap(const char *name, std::size_t size);
>         UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
>         UniqueFD providerHandle_;
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index be6efb89..d2ac175f 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -23,6 +23,10 @@
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/memfd.h>
>  
> +#include <libcamera/framebuffer.h>
> +
> +#include "libcamera/internal/formats.h"
> +
>  /**
>   * \file dma_buf_allocator.cpp
>   * \brief dma-buf allocator
> @@ -205,4 +209,55 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size)
>                 return allocFromHeap(name, size);
>  }
>  
> +/**
> + * \brief Allocate and export buffers from the DmaBufAllocator
> + * \param[in] count The number of requested FrameBuffers
> + * \param[in] frameSizes The sizes of planes in each FrameBuffer
> + * \param[out] buffers Array of buffers successfully allocated
> + *
> + * \return The number of allocated buffers on success or a negative error code
> + * otherwise
> + */
> +int DmaBufAllocator::exportBuffers(unsigned int count,
> +                                  const std::vector<unsigned int> &frameSizes,
> +                                  std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> +       for (unsigned int i = 0; i < count; ++i) {
> +               std::unique_ptr<FrameBuffer> buffer =
> +                       createBuffer("frame-" + std::to_string(i), frameSizes);

I wonder if we should find a better name for the buffer names ... but
this is what already happens in softisp anyway so I'm fine with this.



> +               if (!buffer) {
> +                       LOG(DmaBufAllocator, Error) << "Unable to create buffer";
> +
> +                       buffers->clear();
> +                       return -EINVAL;
> +               }
> +
> +               buffers->push_back(std::move(buffer));
> +       }
> +
> +       return count;
> +}
> +
> +std::unique_ptr<FrameBuffer>
> +DmaBufAllocator::createBuffer(std::string name,
> +                             const std::vector<unsigned int> &frameSizes)
> +{
> +       std::vector<FrameBuffer::Plane> planes;
> +
> +       unsigned int bufferSize = 0, offset = 0;
> +       for (auto frameSize : frameSizes)
> +               bufferSize += frameSize;
> +
> +       SharedFD fd(alloc(name.c_str(), bufferSize));
> +       if (!fd.isValid())
> +               return nullptr;

So this creates the allocation with a single dma buf ... I'm weary if
this will work generically - but I also think we don't need to worry
about this now.

Some allocations might want each of the planes to be separated or have
specific alignments...

But without a user requiring those - I think this is fine for a first
step. I wonder if this should be documented somewhere, probably in the
exportBuffers description.

With that


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

> +
> +       for (auto frameSize : frameSizes) {
> +               planes.emplace_back(FrameBuffer::Plane{ fd, offset, frameSize });
> +               offset += frameSize;
> +       }
> +
> +       return std::make_unique<FrameBuffer>(planes);
> +}
> +
>  } /* namespace libcamera */
> -- 
> 2.46.0.469.g59c65b2a67-goog
>

Hi Kieran,

On Mon, Sep 9, 2024 at 9:23 PM Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Harvey Yang (2024-09-07 15:28:26)
> > Add a helper function exportBuffers in DmaBufAllocator to make it easier
> > to use.
> >
> > It'll be used in Virtual Pipeline Handler and SoftwareIsp.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  .../libcamera/internal/dma_buf_allocator.h    | 13 +++++
> >  src/libcamera/dma_buf_allocator.cpp           | 55 +++++++++++++++++++
> >  2 files changed, 68 insertions(+)
> >
> > diff --git a/include/libcamera/internal/dma_buf_allocator.h
> b/include/libcamera/internal/dma_buf_allocator.h
> > index d2a0a0d1..66d3b419 100644
> > --- a/include/libcamera/internal/dma_buf_allocator.h
> > +++ b/include/libcamera/internal/dma_buf_allocator.h
> > @@ -7,11 +7,17 @@
> >
> >  #pragma once
> >
> > +#include <memory>
> > +#include <string>
> > +#include <vector>
> > +
> >  #include <libcamera/base/flags.h>
> >  #include <libcamera/base/unique_fd.h>
> >
> >  namespace libcamera {
> >
> > +class FrameBuffer;
> > +
> >  class DmaBufAllocator
> >  {
> >  public:
> > @@ -28,7 +34,14 @@ public:
> >         bool isValid() const { return providerHandle_.isValid(); }
> >         UniqueFD alloc(const char *name, std::size_t size);
> >
> > +       int exportBuffers(unsigned int count,
> > +                         const std::vector<unsigned int> &frameSize,
> > +                         std::vector<std::unique_ptr<FrameBuffer>>
> *buffers);
> > +
> >  private:
> > +       std::unique_ptr<FrameBuffer> createBuffer(
> > +               std::string name, const std::vector<unsigned int>
> &frameSizes);
> > +
> >         UniqueFD allocFromHeap(const char *name, std::size_t size);
> >         UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
> >         UniqueFD providerHandle_;
> > diff --git a/src/libcamera/dma_buf_allocator.cpp
> b/src/libcamera/dma_buf_allocator.cpp
> > index be6efb89..d2ac175f 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -23,6 +23,10 @@
> >  #include <libcamera/base/log.h>
> >  #include <libcamera/base/memfd.h>
> >
> > +#include <libcamera/framebuffer.h>
> > +
> > +#include "libcamera/internal/formats.h"
> > +
> >  /**
> >   * \file dma_buf_allocator.cpp
> >   * \brief dma-buf allocator
> > @@ -205,4 +209,55 @@ UniqueFD DmaBufAllocator::alloc(const char *name,
> std::size_t size)
> >                 return allocFromHeap(name, size);
> >  }
> >
> > +/**
> > + * \brief Allocate and export buffers from the DmaBufAllocator
> > + * \param[in] count The number of requested FrameBuffers
> > + * \param[in] frameSizes The sizes of planes in each FrameBuffer
> > + * \param[out] buffers Array of buffers successfully allocated
> > + *
> > + * \return The number of allocated buffers on success or a negative
> error code
> > + * otherwise
> > + */
> > +int DmaBufAllocator::exportBuffers(unsigned int count,
> > +                                  const std::vector<unsigned int>
> &frameSizes,
> > +
> std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > +{
> > +       for (unsigned int i = 0; i < count; ++i) {
> > +               std::unique_ptr<FrameBuffer> buffer =
> > +                       createBuffer("frame-" + std::to_string(i),
> frameSizes);
>
> I wonder if we should find a better name for the buffer names ... but
> this is what already happens in softisp anyway so I'm fine with this.
>
>
I'd be glad to accept suggestions :)


>
>
> > +               if (!buffer) {
> > +                       LOG(DmaBufAllocator, Error) << "Unable to create
> buffer";
> > +
> > +                       buffers->clear();
> > +                       return -EINVAL;
> > +               }
> > +
> > +               buffers->push_back(std::move(buffer));
> > +       }
> > +
> > +       return count;
> > +}
> > +
> > +std::unique_ptr<FrameBuffer>
> > +DmaBufAllocator::createBuffer(std::string name,
> > +                             const std::vector<unsigned int>
> &frameSizes)
> > +{
> > +       std::vector<FrameBuffer::Plane> planes;
> > +
> > +       unsigned int bufferSize = 0, offset = 0;
> > +       for (auto frameSize : frameSizes)
> > +               bufferSize += frameSize;
> > +
> > +       SharedFD fd(alloc(name.c_str(), bufferSize));
> > +       if (!fd.isValid())
> > +               return nullptr;
>
> So this creates the allocation with a single dma buf ... I'm weary if
> this will work generically - but I also think we don't need to worry
> about this now.
>
> Some allocations might want each of the planes to be separated or have
> specific alignments...
>
> But without a user requiring those - I think this is fine for a first
> step. I wonder if this should be documented somewhere, probably in the
> exportBuffers description.
>

True, some of the previous versions are actually allocating buffers with
their own dma buf allocation respectively, while I thought that using a
single
dma buf is generally more preferred (Please let me know if I'm wrong :p).

Added a description and a todo in `exportBuffers`.


>
> With that
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > +
> > +       for (auto frameSize : frameSizes) {
> > +               planes.emplace_back(FrameBuffer::Plane{ fd, offset,
> frameSize });
> > +               offset += frameSize;
> > +       }
> > +
> > +       return std::make_unique<FrameBuffer>(planes);
> > +}
> > +
> >  } /* namespace libcamera */
> > --
> > 2.46.0.469.g59c65b2a67-goog
> >
>