[PATCH v9 1/8] libcamera: add DmaBufAllocator::exportBuffers()

[Jacopo Mondi]

Hi Harvey

On Tue, Aug 27, 2024 at 05:34:43PM GMT, Cheng-Hao Yang wrote:
> Thanks Jacopo for the review.
>
> The corresponding updates are included in v9.1. Please check.
>
> On Tue, Aug 27, 2024 at 4:13 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> wrote:
>
> > Hi Harvey
> >
> > On Tue, Aug 20, 2024 at 04:23:32PM GMT, Harvey Yang wrote:
> > > 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 at chromium.org>
> > > ---
> > >  .../libcamera/internal/dma_buf_allocator.h    | 12 ++++
> > >  src/libcamera/dma_buf_allocator.cpp           | 64 ++++++++++++++++++-
> > >  2 files changed, 74 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/dma_buf_allocator.h
> > b/include/libcamera/internal/dma_buf_allocator.h
> > > index 36ec1696b..3a9b56b1c 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;
> >
> > Where do you use this ?
> >
> >
> Right, it's only used in the previous versions. Removed.
>
>
> > > +
> > >  class DmaBufAllocator
> > >  {
> > >  public:
> > > @@ -30,7 +34,15 @@ public:
> > >       bool isValid() const { return providerHandle_.isValid(); }
> > >       UniqueFD alloc(const char *name, std::size_t size);
> > >
> > > +     int exportBuffers(
> >
> > weird indent
> >
> >         int exportBuffers(std::size_t count,
> >                           const std::vector<std::size_t> &frameSize,
> >                           std::vector<std::unique_ptr<FrameBuffer>>
> > *buffers);
> >
> > Done

Sometimes you replies are indendented at the same level as the
previous email you replied to, making it really hard to distinguish
the two. Could you check why it happens ?

>
>
> > > +             std::size_t count,
> >
> > #include <cstddef>
> >
> Removed std::size_t.
>
>
> >
> > > +             std::vector<std::size_t> frameSize,
> > > +             std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >
> > #include <memory>
> >
> > Done
>
>
> > > +
> > >  private:
> > > +     std::unique_ptr<FrameBuffer> createBuffer(
> > > +             std::string name, std::vector<std::size_t> frameSizes);
> >
> > #include <string>
> >
> Done
>
>
> > > +
> > >       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 c06eca7d0..2d88b8b2b 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>
> >
> > Not used
> >
> Removed
>
>
> >
> > > +
> > > +#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
> >
> > Removed the changes
>
>
> > >  #endif
> > >  #endif
> > >
> > > @@ -243,4 +248,59 @@ 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
> >
> > There's no \a stream
> >
> Removed.
>
>
> >
> > > + * \param[in] count The number of FrameBuffers required
> >
> > "of requested FrameBuffers" ?
> >
> Thanks! Adopted.
>
>
> >
> > > + * \param[in] frameSizes The sizes of planes in the 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(
> > > +     std::size_t count,
> >
> > nit: This can be an unsigned int maybe
> >
> Done
>
>
> >
> > > +     std::vector<std::size_t> frameSizes,
> >
> > can this be passed as a const reference ?
> >
> Done
>
>
> >
> > > +     std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >
> > weird indentation, I think
> >
> > int DmaBufAllocator::exportBuffers(std::size_t count,
> >                                    const std::vector<std::size_t>
> > &frameSizes,
> >
> >  std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >
> > would do (some heretics consider 120 cols to be ok for libcamera, and
> > nowadays it is accepted for linux as well !! )
> >
> Thanks! Adopted.
>
>
> >
> > > +{
> > > +     for (unsigned i = 0; i < count; ++i) {
> >
> > I wonder if we should be stricter when defining this interface: is
> > there a maximum number of buffers that can be allocated ? How many
> > planes can a buffer have ? (I don't see this addressed in our
> > FrameBuffer class, so it can be left out from here too ?)
> >
>
> Hmm, as it's just a helper function, I think the users should know what
> they're doing, unless we have more specific rules, like the maximum
> number of planes in FrameBuffer.

Never assume how an API could be wrongly used :)

>
> We can't stop users allocating too many buffers with or without this
> helper function, right :p ?
>

True indeed, but a good API should minimize the surface for making it
used wrongly, even more when it gives access to system resource.

See below.

>
> >
> > > +             std::unique_ptr<FrameBuffer> buffer =
> > > +                     createBuffer("frame-" + std::to_string(i),
> > frameSizes);
> > > +             if (!buffer) {
> > > +                     LOG(DmaBufAllocator, Error) << "Unable to create
> > buffer";
> > > +
> > > +                     buffers->clear();
> > > +                     return -EINVAL;
> > > +             }
> > > +
> > > +             buffers->push_back(std::move(buffer));
> > > +     }
> > > +
> > > +     return count;
> >
> > if you want to have all the requested buffers to be allocated, and do
> > not allow less than that, there's no point in returning count here, you
> > can return 0
> >
>
> Hmm, you're right, while I wonder if it's the same case for
> V4L2VideoDevice::exportBuffers/createBuffers? I want to align with
> other exportBuffers API.
>

I think it's fine

>
> >
> > > +}
> > > +
> > > +std::unique_ptr<FrameBuffer> DmaBufAllocator::createBuffer(
> > > +     std::string name, std::vector<std::size_t> frameSizes)
> >
> > Or
> >
> > std::unique_ptr<FrameBuffer>
> > DmaBufAllocator::createBuffer(std::string name,
> >                               const std::vector<std::size_t> &frameSizes)
> >
> > can frameSize be passed as a const reference ?
> >
> Thanks! Adopted.
>
>
> >
> >
> > > +{
> > > +     std::vector<FrameBuffer::Plane> planes;
> > > +
> > > +     std::size_t bufferSize = 0, offset = 0;
> > > +     for (auto frameSize : frameSizes)
> > > +             bufferSize += frameSize;
> >
> > This API allows unbounded size allocation, just sayin', maybe it's
> > fine
> >
>
> Yeah... as it's just a helper function, I still think that the users should
> know what they're doing.
>

As long as they don't :)

I'm particularly concerned about udmabuf, as it uses memfd as backing
storage, and we allow to allocate a lof of memory (we did already to
be completely honest, as a user can call alloc with any size).

Also, I've now noticed the dma_buf_allocator API are not public, so
this makes me way less concerned.

>
> >
> > > +
> > > +     SharedFD fd(alloc(name.c_str(), bufferSize));
> > > +     if (!fd.isValid())
> > > +             return nullptr;
> > > +
> > > +     for (auto frameSize : frameSizes) {
> > > +             FrameBuffer::Plane plane;
> > > +             plane.fd = fd;
> > > +             plane.offset = offset;
> > > +             plane.length = frameSize;
> > > +             planes.push_back(std::move(plane));
> > > +             offset += plane.length;
> > > +     }
> >
> > With a little implicit type casting, you can
> >
> >         unsigned int offset = 0;
> >         for (unsigned int frameSize : frameSizes) {
> >                 planes.emplace_back(FrameBuffer::Plane{fd, offset,
> > frameSize});
> >                 offset += frameSize;
> >         }
> >
> > But it won't save you going a temporary object I'm afraid. I'm not
> > sure it's actually any better, up to you.
> >
> > Thanks, adopted. Also, as the offset's and length's types are both
> unsigned int, I changed `frameSize` to be an array of unsigned as
> well.
>
>
> > > +
> > > +     return std::make_unique<FrameBuffer>(planes);
> > > +}
> > > +
> > >  } /* namespace libcamera */
> > > --
> > > 2.46.0.184.g6999bdac58-goog
> > >
> >