[libcamera-devel] [PATCH v6 3/3] libcamera: Add exportFrameBuffers in HeapAllocator
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 2 22:07:07 CEST 2023
On Wed, Aug 02, 2023 at 03:08:05PM +0800, Cheng-Hao Yang wrote:
> On Tue, May 30, 2023 at 2:56 PM Laurent Pinchart wrote:
> > On Mon, May 29, 2023 at 10:02:18AM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > On Mon, May 22, 2023 at 08:35:08AM +0000, Harvey Yang via libcamera-devel wrote:
> > > > From: Cheng-Hao Yang <chenghaoyang at chromium.org>
> > >
> > > This is the same email address with 2 different names. Unless this is
> > > intentional could you change the authorship of the patch and use a
> > > single identity (git commit --amend --autor="...")
> > >
> > > > Add a helper function exportFrameBuffers in HeapAllocator to make it
> > > > easier to use.
> > > >
> > > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > ---
> > > > include/libcamera/internal/heap_allocator.h | 9 +++
> > > > src/libcamera/heap_allocator.cpp | 62 +++++++++++++++++++++
> > > > 2 files changed, 71 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/internal/heap_allocator.h b/include/libcamera/internal/heap_allocator.h
> > > > index 92d4488a..92beaaa4 100644
> > > > --- a/include/libcamera/internal/heap_allocator.h
> > > > +++ b/include/libcamera/internal/heap_allocator.h
> > > > @@ -9,12 +9,16 @@
> > > > #pragma once
> > > >
> > > > #include <stddef.h>
> > > > +#include <vector>
> > > >
> > > > #include <libcamera/base/unique_fd.h>
> > > >
> > > > namespace libcamera {
> > > >
> > > > +class Camera;
> > > > +class FrameBuffer;
> > > > class Heap;
> > > > +class Stream;
> > > >
> > > > class HeapAllocator
> > > > {
> > > > @@ -24,7 +28,12 @@ public:
> > > > bool isValid() const;
> > > > 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 8f99f590..e99cdbe5 100644
> > > > --- a/src/libcamera/heap_allocator.cpp
> > > > +++ b/src/libcamera/heap_allocator.cpp
> > > > @@ -22,6 +22,12 @@
> > > >
> > > > #include <libcamera/base/log.h>
> > > >
> > > > +#include <libcamera/camera.h>
> > > > +#include <libcamera/framebuffer.h>
> > > > +#include <libcamera/stream.h>
> > > > +
> > > > +#include "libcamera/internal/formats.h"
> > > > +
> > > > namespace libcamera {
> > > >
> > > > /*
> > > > @@ -227,4 +233,60 @@ 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)
> >
> > Why do you pass a camera pointer to this function if it's unused ?
>
> Removed.
>
> > > > +{
> > > > + unsigned int count = stream->configuration().bufferCount;
> >
> > You're creating a tight dependency between the allocator and the stream
> > here, as you require the stream to have already been configured, and
> > rely on the current configuration. Passing a stream configuration
> > explicitly would be better to make the allocator more generic.
>
> Good point. Using `const StreamConfiguration&` directly.
>
> > > > +
> > > > + /** \todo Support multiplanar allocations */
> >
> > I'd like to see this being addressed already, as it's a key requirement
> > that will show whether the API design is good or not.
>
> IIUC, this means the same thing as `a dedicated fd per plane`, right?
> Let me know if I misunderstood.
That's right, yes.
> > > > +
> > > > + 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;
> > > > +
> > > > + unsigned int frameSize = stream->configuration().frameSize;
> > > > + int pageSize = getpagesize();
> > > > + /** Allocation size need to be a direct multiple of |pageSize|. */
> > >
> > > No need for /**, just use /*
> > >
> > > > + unsigned int numPage = (frameSize + pageSize - 1) / pageSize;
> >
> > What are the pros and cons for rounding it up to the page size here
> > compared to the alloc() function ? Does the kernel require a rounded
> > size, of will it round internally ?
>
> Moved to `HeapAllocator::alloc` to prevent misuse in other components.
> Yeah, the kernel requires a rounded size or it fails (in my environment with
> udmabuf).
>
> > > +
> > > > + UniqueFD fd = alloc("FrameBuffer", numPage * pageSize);
> > > > + if (!fd.isValid())
> > > > + return nullptr;
> > > > +
> > > > + auto info = PixelFormatInfo::info(stream->configuration().pixelFormat);
> > > > + SharedFD shared_fd(std::move(fd));
> > >
> > > s/shared_fd/sharedFd/
> > >
> > > > + unsigned int offset = 0;
> > > > + for (size_t i = 0; i < info.planes.size(); ++i) {
> > > > + unsigned int planeSize = info.planeSize(stream->configuration().size, i);
> >
> > This doesn't take line stride into account.
>
> IIUC, `PixelFormatInfo::planeSize` considers stride in the
> implementation...?
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.cpp#n1022
What I meant is that it doesn't take StreamConfiguration::stride into
account.
> > > > + if (planeSize == 0)
> > > > + continue;
> > > > +
> > > > + /** \todo We don't support allocating buffers with a dedicated fd per plane */
> >
> > This then makes the buffer incompatible with V4L2, that seems to be a
> > bad issue for a generic allocator in libcamera.
>
> Updated. Please check if it makes sense.
>
> > > > + FrameBuffer::Plane plane;
> > > > + plane.fd = shared_fd;
> > > > + plane.offset = offset;
> > > > + plane.length = planeSize;
> > > > + planes.push_back(std::move(plane));
> > > > +
> > > > + offset += planeSize;
> > > > + }
> > > > +
> > > > + return std::make_unique<FrameBuffer>(planes);
> > > > +}
> > > > +
> > >
> > > Only minors, the patch looks good. The series looks good as well,
> > > please add documentation and fix all the minors and I think we can
> > > collect it :)
> >
> > I'm afraid I disagree. See my comment in patch 2/3, in addition to the
> > above.
> >
> > > > } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list