[PATCH v9 1/8] libcamera: add DmaBufAllocator::exportBuffers()

[Jacopo Mondi]

Hi Harvey

On Tue, Aug 20, 2024 at 04:23:32PM GMT, Harvey Yang wrote:
> Add a helper function exportBuffers in DmaBufAllocator to make it easier
> to use.
>
> It'll be used in Virtual Pipeline Handler and SoftwareIsp.
>
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  .../libcamera/internal/dma_buf_allocator.h    | 12 ++++
>  src/libcamera/dma_buf_allocator.cpp           | 64 ++++++++++++++++++-
>  2 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h
> index 36ec1696b..3a9b56b1c 100644
> --- a/include/libcamera/internal/dma_buf_allocator.h
> +++ b/include/libcamera/internal/dma_buf_allocator.h
> @@ -8,12 +8,16 @@
>  #pragma once
>
>  #include <stddef.h>
> +#include <vector>
>
>  #include <libcamera/base/flags.h>
>  #include <libcamera/base/unique_fd.h>
>
>  namespace libcamera {
>
> +class FrameBuffer;
> +struct StreamConfiguration;

Where do you use this ?

> +
>  class DmaBufAllocator
>  {
>  public:
> @@ -30,7 +34,15 @@ public:
>  	bool isValid() const { return providerHandle_.isValid(); }
>  	UniqueFD alloc(const char *name, std::size_t size);
>
> +	int exportBuffers(

weird indent

	int exportBuffers(std::size_t count,
			  const std::vector<std::size_t> &frameSize,
			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);

> +		std::size_t count,

#include <cstddef>

> +		std::vector<std::size_t> frameSize,
> +		std::vector<std::unique_ptr<FrameBuffer>> *buffers);

#include <memory>

> +
>  private:
> +	std::unique_ptr<FrameBuffer> createBuffer(
> +		std::string name, std::vector<std::size_t> frameSizes);

#include <string>
> +
>  	UniqueFD allocFromHeap(const char *name, std::size_t size);
>  	UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);
>  	UniqueFD providerHandle_;
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp
> index c06eca7d0..2d88b8b2b 100644
> --- a/src/libcamera/dma_buf_allocator.cpp
> +++ b/src/libcamera/dma_buf_allocator.cpp
> @@ -23,6 +23,11 @@
>
>  #include <libcamera/base/log.h>
>
> +#include <libcamera/framebuffer.h>
> +#include <libcamera/stream.h>

Not used

> +
> +#include "libcamera/internal/formats.h"
> +
>  /**
>   * \file dma_buf_allocator.cpp
>   * \brief dma-buf allocator
> @@ -130,8 +135,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;
>  /* uClibc doesn't provide the file sealing API. */
>  #ifndef __DOXYGEN__
>  #if not HAVE_FILE_SEALS
> -#define F_ADD_SEALS		1033
> -#define F_SEAL_SHRINK		0x0002
> +#define F_ADD_SEALS 1033
> +#define F_SEAL_SHRINK 0x0002

Unrelated

>  #endif
>  #endif
>
> @@ -243,4 +248,59 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size)
>  		return allocFromHeap(name, size);
>  }
>
> +/**
> + * \brief Allocate and export buffers for \a stream from the DmaBufAllocator

There's no \a stream

> + * \param[in] count The number of FrameBuffers required

"of requested FrameBuffers" ?

> + * \param[in] frameSizes The sizes of planes in the FrameBuffer
> + * \param[out] buffers Array of buffers successfully allocated
> + *
> + * \return The number of allocated buffers on success or a negative error code
> + * otherwise
> + */
> +int DmaBufAllocator::exportBuffers(
> +	std::size_t count,

nit: This can be an unsigned int maybe

> +	std::vector<std::size_t> frameSizes,

can this be passed as a const reference ?

> +	std::vector<std::unique_ptr<FrameBuffer>> *buffers)

weird indentation, I think

int DmaBufAllocator::exportBuffers(std::size_t count,
				   const std::vector<std::size_t> &frameSizes,
				   std::vector<std::unique_ptr<FrameBuffer>> *buffers)

would do (some heretics consider 120 cols to be ok for libcamera, and
nowadays it is accepted for linux as well !! )

> +{
> +	for (unsigned i = 0; i < count; ++i) {

I wonder if we should be stricter when defining this interface: is
there a maximum number of buffers that can be allocated ? How many
planes can a buffer have ? (I don't see this addressed in our
FrameBuffer class, so it can be left out from here too ?)

> +		std::unique_ptr<FrameBuffer> buffer =
> +			createBuffer("frame-" + std::to_string(i), frameSizes);
> +		if (!buffer) {
> +			LOG(DmaBufAllocator, Error) << "Unable to create buffer";
> +
> +			buffers->clear();
> +			return -EINVAL;
> +		}
> +
> +		buffers->push_back(std::move(buffer));
> +	}
> +
> +	return count;

if you want to have all the requested buffers to be allocated, and do
not allow less than that, there's no point in returning count here, you
can return 0

> +}
> +
> +std::unique_ptr<FrameBuffer> DmaBufAllocator::createBuffer(
> +	std::string name, std::vector<std::size_t> frameSizes)

Or

std::unique_ptr<FrameBuffer>
DmaBufAllocator::createBuffer(std::string name,
			      const std::vector<std::size_t> &frameSizes)

can frameSize be passed as a const reference ?


> +{
> +	std::vector<FrameBuffer::Plane> planes;
> +
> +	std::size_t bufferSize = 0, offset = 0;
> +	for (auto frameSize : frameSizes)
> +		bufferSize += frameSize;

This API allows unbounded size allocation, just sayin', maybe it's
fine

> +
> +	SharedFD fd(alloc(name.c_str(), bufferSize));
> +	if (!fd.isValid())
> +		return nullptr;
> +
> +	for (auto frameSize : frameSizes) {
> +		FrameBuffer::Plane plane;
> +		plane.fd = fd;
> +		plane.offset = offset;
> +		plane.length = frameSize;
> +		planes.push_back(std::move(plane));
> +		offset += plane.length;
> +	}

With a little implicit type casting, you can

	unsigned int offset = 0;
	for (unsigned int frameSize : frameSizes) {
		planes.emplace_back(FrameBuffer::Plane{fd, offset, frameSize});
		offset += frameSize;
	}

But it won't save you going a temporary object I'm afraid. I'm not
sure it's actually any better, up to you.

> +
> +	return std::make_unique<FrameBuffer>(planes);
> +}
> +
>  } /* namespace libcamera */
> --
> 2.46.0.184.g6999bdac58-goog
>