[libcamera-devel] [PATCH v4 3/3] libcamera: Add exportFrameBuffers in HeapAllocator

[Jacopo Mondi]

Hi Harvey

On Tue, May 16, 2023 at 04:12:08PM +0800, Cheng-Hao Yang via libcamera-devel wrote:
> Thanks Jacopo!
>
> On Sun, May 14, 2023 at 12:29 AM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> wrote:
>
> > Hi Harvey
> >
> > On Tue, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via libcamera-devel
> > wrote:
> > > From: Cheng-Hao Yang <chenghaoyang at chromium.org>
> > >
> > > Add a helper function exportFrameBuffers in HeapAllocator to make it
> > > easier to use.
> >
> > Will you use these with the Virtual pipeline handler ?
> >
> >
> Yes, you can find it in |PipelindHandlerVirtual::exportFrameBuffer|.
> Ref: https://patchwork.libcamera.org/patch/18533/
>
>
>
> > >
> > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > ---
> > >  include/libcamera/heap_allocator.h | 11 ++++++
> > >  src/libcamera/heap_allocator.cpp   | 56 ++++++++++++++++++++++++++++++
> > >  2 files changed, 67 insertions(+)
> > >
> > > diff --git a/include/libcamera/heap_allocator.h
> > b/include/libcamera/heap_allocator.h
> > > index cd7ed1a3..076f0951 100644
> > > --- a/include/libcamera/heap_allocator.h
> > > +++ b/include/libcamera/heap_allocator.h
> > > @@ -8,6 +8,7 @@
> > >  #pragma once
> > >
> > >  #include <stddef.h>
> > > +#include <vector>
> > >
> > >  #include <libcamera/base/unique_fd.h>
> > >
> > > @@ -15,6 +16,11 @@
> > >
> > >  namespace libcamera {
> > >
> > > +class Camera;
> > > +class Stream;
> > > +class FrameBuffer;
> > > +class PixelFormatInfo;
> >
> > Not used
> >
> >
> Removed.
>
>
> > > +
> > >  class HeapAllocator
> > >  {
> > >  public:
> > > @@ -23,7 +29,12 @@ public:
> > >       bool isValid() const { return heap_->isValid(); }
> > >       UniqueFD alloc(const char *name, std::size_t size);
> > >
> > > +     int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +                            std::vector<std::unique_ptr<FrameBuffer>>
> > *buffers);
> > > +
> > >  private:
> > > +     std::unique_ptr<FrameBuffer> createBuffer(Stream *stream);
> > > +
> > >       std::unique_ptr<Heap> heap_;
> > >  };
> > >
> > > diff --git a/src/libcamera/heap_allocator.cpp
> > b/src/libcamera/heap_allocator.cpp
> > > index 179c2c21..33e9eaca 100644
> > > --- a/src/libcamera/heap_allocator.cpp
> > > +++ b/src/libcamera/heap_allocator.cpp
> > > @@ -17,7 +17,11 @@
> > >
> > >  #include <libcamera/base/log.h>
> > >
> > > +#include <libcamera/camera.h>
> > >  #include <libcamera/dma_heap.h>
> > > +#include <libcamera/framebuffer.h>
> > > +#include <libcamera/internal/formats.h>
> > > +#include <libcamera/stream.h>
> > >  #include <libcamera/udma_heap.h>
> > >
> > >  namespace libcamera {
> > > @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name,
> > std::size_t size)
> > >       return heap_->alloc(name, size);
> > >  }
> > >
> > > +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera,
> > Stream *stream,
> > > +
> >  std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > > +{
> > > +     unsigned int count = stream->configuration().bufferCount;
> > > +
> > > +     /** \todo: Support multiplanar allocations */
> >
> > no : after \todo
> >
> >
> Done
>
> > +
> > > +     for (unsigned i = 0; i < count; ++i) {
> > > +             std::unique_ptr<FrameBuffer> buffer = createBuffer(stream);
> > > +             if (!buffer) {
> > > +                     LOG(HeapAllocator, Error) << "Unable to create
> > buffer";
> > > +
> > > +                     buffers->clear();
> > > +                     return -EINVAL;
> > > +             }
> > > +
> > > +             buffers->push_back(std::move(buffer));
> > > +     }
> > > +
> > > +     return count;
> > > +}
> > > +
> > > +std::unique_ptr<FrameBuffer> HeapAllocator::createBuffer(Stream *stream)
> > > +{
> > > +     std::vector<FrameBuffer::Plane> planes;
> > > +
> > > +     int num_page = (stream->configuration().frameSize + getpagesize()
> > - 1) / getpagesize();
> >
> > Could you break this rather long line ?
> >
>
> Defined "frameSize" and "pageSize" beforehand. Should be shorter now.
>
>
> > Can num_page be unsigned ?
> >
>
> Sure.
>
>
> > libcamera uses camelCase and not snake_case for variables
> >
> >         unsigned int numPage = (stream->configuration().frameSize +
> > getpagesize() - 1)
> >                              / getpagesize();
> >
> > What are the implications of overallocating ? The last plane will be
> > slightly larger, is this an issue at all ?
> >
> >
> The allocation only works if the requested size is a direct multiple of
> |getpagesize()|. Added a comment.
> Ref:
> https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72
>

Yes, I understand the requirement, I was wondering about the
implications of overallocation, but I presume this is safe ?

>
> > > +
> > > +     UniqueFD fd = alloc("Buffer", num_page * getpagesize());
> >
> > What are the implications of using the same name ?
> >
> >
> Since it's a private function that is only called by |exportFrameBuffer|,
> renamed it to "FrameBuffer" instead.
>
>
> > > +     if (!fd.isValid())
> > > +             return nullptr;
> > > +
> > > +     auto info =
> > PixelFormatInfo::info(stream->configuration().pixelFormat);
> > > +     SharedFD shared_fd(std::move(fd));
> > > +     unsigned int offset = 0;
> > > +     for (long unsigned int i = 0; i < info.planes.size(); ++i) {
> >
> > Does this need to be long ?
> >
> >
> I should use size_t instead.
>
>
> > > +             unsigned int planeSize =
> > info.planeSize(stream->configuration().size, i);
> >
> > configuration().size is the size in pixel, is this intended ?
> >
> >
> Yes, I guess so, according to the comment of PixelFormatInfo:
> https://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056
>

Ah you're right

 \param[in] size The size of the frame, in pixels


>
> > > +             if (planeSize == 0)
> > > +                     continue;
> > > +
> > > +             FrameBuffer::Plane plane;
> >
> > I would note down a \todo to remind that we don't support allocating
> > buffers with a dedicated fd per plane
> >
> >
> Is it preferred to do so? I guess we can also support that by calling
> |alloc()| multiple times...?
>

I don't think it's specifically preferred, but might be required for
fully-planar formats where each plane lives in its own possibly
non-contiguous memory area. I would for now record a todo item

We have a similar issue here iirc
https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/mm/cros_frame_buffer_allocator.cpp#n57


>
> > Sorry, lot of questions :)
> >
> >
> Thanks for doing so :)
>
>
> > Cheers
> >   j
> >
> >
> > > +             plane.fd = shared_fd;
> > > +             plane.offset = offset;
> > > +             plane.length = planeSize;
> > > +             planes.push_back(std::move(plane));
> > > +
> > > +             offset += planeSize;
> > > +     }
> > > +
> > > +     return std::make_unique<FrameBuffer>(planes);
> > > +}
> > > +
> > >  } /* namespace libcamera */
> > > --
> > > 2.40.0
> > >
> >