[libcamera-devel,v6,3/3] libcamera: Add exportFrameBuffers in HeapAllocator

18645 diff mbox series

From: Cheng-Hao Yang <chenghaoyang@chromium.org>

Add a helper function exportFrameBuffers in HeapAllocator to make it
easier to use.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/internal/heap_allocator.h |  9 +++
 src/libcamera/heap_allocator.cpp            | 62 +++++++++++++++++++++
 2 files changed, 71 insertions(+)

Hi Harvey

On Mon, May 22, 2023 at 08:35:08AM +0000, Harvey Yang via libcamera-devel wrote:
> From: Cheng-Hao Yang <chenghaoyang@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@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)
> +{
> +	unsigned int count = stream->configuration().bufferCount;
> +
> +	/** \todo Support multiplanar allocations */
> +
> +	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;
> +
> +	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);
> +		if (planeSize == 0)
> +			continue;
> +
> +		/** \todo We don't support allocating buffers with a dedicated fd per plane */
> +		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 :)

Thanks
  j

>  } /* namespace libcamera */
> --
> 2.40.1.698.g37aff9b760-goog
>

Hi Harvey,

Thank you for the patch.

On 5/22/23 2:05 PM, Harvey Yang via libcamera-devel wrote:
> From: Cheng-Hao Yang <chenghaoyang@chromium.org>
>
> Add a helper function exportFrameBuffers in HeapAllocator to make it
> easier to use.
>
> Signed-off-by: Harvey Yang <chenghaoyang@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);
> +

Can this be made to accept references instead of pointers? I believe 
every argument is non-nullable here...
>   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)
> +{
> +	unsigned int count = stream->configuration().bufferCount;
> +
> +	/** \todo Support multiplanar allocations */
> +
> +	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|. */

	/* Allocation size need to be a direct multiple of pageSize. */

> +	unsigned int numPage = (frameSize + pageSize - 1) / pageSize;
> +
> +	UniqueFD fd = alloc("FrameBuffer", numPage * pageSize);
> +	if (!fd.isValid())
> +		return nullptr;
> +
> +	auto info = PixelFormatInfo::info(stream->configuration().pixelFormat);
> +	SharedFD shared_fd(std::move(fd));
> +	unsigned int offset = 0;
> +	for (size_t i = 0; i < info.planes.size(); ++i) {
> +		unsigned int planeSize = info.planeSize(stream->configuration().size, i);
> +		if (planeSize == 0)
> +			continue;
> +
> +		/** \todo We don't support allocating buffers with a dedicated fd per plane */

		/* \todo We don't support allocating buffers with a dedicated fd per plane */

> +		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);
> +}
> +
>   } /* namespace libcamera */

On Mon, May 29, 2023 at 10:02:18AM +0200, Jacopo Mondi via libcamera-devel wrote:
> Hi Harvey
> 
> On Mon, May 22, 2023 at 08:35:08AM +0000, Harvey Yang via libcamera-devel wrote:
> > From: Cheng-Hao Yang <chenghaoyang@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@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 ?

> > +{
> > +	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.

> > +
> > +	/** \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.

> > +
> > +	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 ?

> > +
> > +	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.

> > +		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.

> > +		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 */

Thanks Laurent,

On Tue, May 30, 2023 at 2:56 PM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> On Mon, May 29, 2023 at 10:02:18AM +0200, Jacopo Mondi via libcamera-devel
> wrote:
> > Hi Harvey
> >
> > On Mon, May 22, 2023 at 08:35:08AM +0000, Harvey Yang via
> libcamera-devel wrote:
> > > From: Cheng-Hao Yang <chenghaoyang@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@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.


> > > +
> > > +   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



> > > +           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
>

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@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@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 */