[libcamera-devel] [PATCH v4 3/3] libcamera: Add exportFrameBuffers in HeapAllocator
Cheng-Hao Yang
chenghaoyang at chromium.org
Mon May 22 10:36:50 CEST 2023
Thanks Jacopo.
On Tue, May 16, 2023 at 6:21 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> 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 ?
>
>
Ah sorry to misunderstand your question. I assume the plane size is
determined by |FrameBuffer::Plane::length|, not the remaining space
of the allocated buffer. It's specified in line 282. Please correct me if
I'm wrong :)
> >
> > > > +
> > > > + 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
>
>
>
Okay. Added a comment. Feel free to modify it when landing.
Also let me know if a dedicated fd is needed here, as I think
it's not that hard to do so :)
> >
> > > 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
> > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230522/440dc718/attachment.htm>
More information about the libcamera-devel
mailing list