[PATCH v7 1/7] libcamera: add DmaBufAllocation::exportFrameBuffers()

[Cheng-Hao Yang]

Hi Laurent,

Thanks for the review. Updated in v8.

On Sat, Aug 3, 2024 at 10:21 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Chenghao,
>
> Thank you for the patch.
>
> On Thu, Aug 01, 2024 at 07:30:57AM +0000, Harvey Yang wrote:
> > Add a helper function exportFrameBuffers in DmaBufAllocator to make it
> > easier to use.
> >
> > It'll be used in Virtual Pipeline Handler specifically.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> >  .../libcamera/internal/dma_buf_allocator.h    | 10 +++
> >  src/libcamera/dma_buf_allocator.cpp           | 68 ++++++++++++++++++-
> >  2 files changed, 76 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/internal/dma_buf_allocator.h
> b/include/libcamera/internal/dma_buf_allocator.h
> > index 36ec1696..dd2cc237 100644
> > --- a/include/libcamera/internal/dma_buf_allocator.h
> > +++ b/include/libcamera/internal/dma_buf_allocator.h
> > @@ -8,12 +8,16 @@
> >  #pragma once
> >
> >  #include <stddef.h>
> > +#include <vector>
> >
> >  #include <libcamera/base/flags.h>
> >  #include <libcamera/base/unique_fd.h>
> >
> >  namespace libcamera {
> >
> > +class FrameBuffer;
> > +struct StreamConfiguration;
> > +
> >  class DmaBufAllocator
> >  {
> >  public:
> > @@ -30,7 +34,13 @@ public:
> >       bool isValid() const { return providerHandle_.isValid(); }
> >       UniqueFD alloc(const char *name, std::size_t size);
> >
> > +     int exportFrameBuffers(
> > +             const StreamConfiguration &config,
> > +             std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> I wonder if the DmaBufAllocator class is really the best place to
> implement this. It would help reviewing how generic the implementation
> is if we had two users as part of this series. Maybe converting the soft
> ISP to the new function could be such a second user.
>
>
Right, it's a good idea that we add the helper functions in a way that works
for soft ISP as well.


> The part that bothers me the most is, I think, usage of
> StreamConfiguration in this function. Maybe the createBuffer() function
> could be kept, and be passed a pixel format and a size, whie the
> exportFrameBuffers() could be moved to the pipeline handler.
>
>
Hmm, as |SoftwareIsp::exportFrames| only uses `debayer_->frameSize()`,
I updated the DmaBufAllocator's helper function with `count` and a list of
`frameSize`s. Please check if it's better.



> > +
> >  private:
> > +     std::unique_ptr<FrameBuffer> createBuffer(const
> StreamConfiguration &config);
> > +
> >       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 c06eca7d..6b406880 100644
> > --- a/src/libcamera/dma_buf_allocator.cpp
> > +++ b/src/libcamera/dma_buf_allocator.cpp
> > @@ -23,6 +23,11 @@
> >
> >  #include <libcamera/base/log.h>
> >
> > +#include <libcamera/framebuffer.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "libcamera/internal/formats.h"
> > +
> >  /**
> >   * \file dma_buf_allocator.cpp
> >   * \brief dma-buf allocator
> > @@ -130,8 +135,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;
> >  /* uClibc doesn't provide the file sealing API. */
> >  #ifndef __DOXYGEN__
> >  #if not HAVE_FILE_SEALS
> > -#define F_ADD_SEALS          1033
> > -#define F_SEAL_SHRINK                0x0002
> > +#define F_ADD_SEALS 1033
> > +#define F_SEAL_SHRINK 0x0002
>
> Unrelated change.
>
>
Sorry, my linter auto-corrected it. Removed.


> >  #endif
> >  #endif
> >
> > @@ -243,4 +248,63 @@ UniqueFD DmaBufAllocator::alloc(const char *name,
> std::size_t size)
> >               return allocFromHeap(name, size);
> >  }
> >
> > +/**
> > + * \brief Allocate and export buffers for \a stream from the
> DmaBufAllocator
> > + * \param[in] config The config of the stream to allocate buffers for
> > + * \param[out] buffers Array of buffers successfully allocated
> > + *
> > + * Allocates buffers for a stream from the DmaBufAllocator. It's a
> helper
> > + * function that'll be used in PipelineHandler::exportFrameBuffers().
> > + *
> > + * \return The number of allocated buffers on success or a negative
> error code
> > + * otherwise
> > + */
> > +int DmaBufAllocator::exportFrameBuffers(
> > +     const StreamConfiguration &config,
> > +     std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > +{
> > +     unsigned int count = config.bufferCount;
> > +
> > +     for (unsigned i = 0; i < count; ++i) {
> > +             std::unique_ptr<FrameBuffer> buffer = createBuffer(config);
> > +             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(
> > +     const StreamConfiguration &config)
> > +{
> > +     std::vector<FrameBuffer::Plane> planes;
> > +
> > +     auto info = PixelFormatInfo::info(config.pixelFormat);
> > +     for (size_t i = 0; i < info.planes.size(); ++i) {
> > +             unsigned int planeSize = info.planeSize(config.size, i);
> > +             if (planeSize == 0)
> > +                     continue;
> > +
> > +             UniqueFD fd = alloc("FrameBuffer", planeSize);
> > +             if (!fd.isValid())
> > +                     return nullptr;
> > +
> > +             SharedFD sharedFd(std::move(fd));
> > +
> > +             FrameBuffer::Plane plane;
> > +             plane.fd = sharedFd;
>
>                 plane.fd = SharedFD(std::move(fd));
>
> and drop the sharedFd variable.
>
>
Done.


> > +             plane.offset = 0;
> > +             plane.length = planeSize;
> > +             planes.push_back(std::move(plane));
> > +     }
> > +
> > +     return std::make_unique<FrameBuffer>(planes);
> > +}
> > +
> >  } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
>

Thanks!
Harvey
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240805/d0e22b55/attachment.htm>