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

[Jacopo Mondi]

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 ?

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

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

> +
> +	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 ?
Can num_page be unsigned ?
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 ?

> +
> +	UniqueFD fd = alloc("Buffer", num_page * getpagesize());

What are the implications of using the same name ?

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

> +		unsigned int planeSize = info.planeSize(stream->configuration().size, i);

configuration().size is the size in pixel, is this intended ?

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

Sorry, lot of questions :)

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
>