[PATCH v7 1/7] libcamera: add DmaBufAllocation::exportFrameBuffers()

[Laurent Pinchart]

Hi Chenghao,

Thank you for the patch.

On Thu, Aug 01, 2024 at 07:30:57AM +0000, Harvey Yang wrote:
> Add a helper function exportFrameBuffers in DmaBufAllocator to make it
> easier to use.
> 
> It'll be used in Virtual Pipeline Handler specifically.
> 
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  .../libcamera/internal/dma_buf_allocator.h    | 10 +++
>  src/libcamera/dma_buf_allocator.cpp           | 68 ++++++++++++++++++-
>  2 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h
> index 36ec1696..dd2cc237 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;
> +
>  class DmaBufAllocator
>  {
>  public:
> @@ -30,7 +34,13 @@ public:
>  	bool isValid() const { return providerHandle_.isValid(); }
>  	UniqueFD alloc(const char *name, std::size_t size);
>  
> +	int exportFrameBuffers(
> +		const StreamConfiguration &config,
> +		std::vector<std::unique_ptr<FrameBuffer>> *buffers);

I wonder if the DmaBufAllocator class is really the best place to
implement this. It would help reviewing how generic the implementation
is if we had two users as part of this series. Maybe converting the soft
ISP to the new function could be such a second user.

The part that bothers me the most is, I think, usage of
StreamConfiguration in this function. Maybe the createBuffer() function
could be kept, and be passed a pixel format and a size, whie the
exportFrameBuffers() could be moved to the pipeline handler.

> +
>  private:
> +	std::unique_ptr<FrameBuffer> createBuffer(const StreamConfiguration &config);
> +
>  	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 c06eca7d..6b406880 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>
> +
> +#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 change.

>  #endif
>  #endif
>  
> @@ -243,4 +248,63 @@ 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
> + * \param[in] config The config of the stream to allocate buffers for
> + * \param[out] buffers Array of buffers successfully allocated
> + *
> + * Allocates buffers for a stream from the DmaBufAllocator. It's a helper
> + * function that'll be used in PipelineHandler::exportFrameBuffers().
> + *
> + * \return The number of allocated buffers on success or a negative error code
> + * otherwise
> + */
> +int DmaBufAllocator::exportFrameBuffers(
> +	const StreamConfiguration &config,
> +	std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> +	unsigned int count = config.bufferCount;
> +
> +	for (unsigned i = 0; i < count; ++i) {
> +		std::unique_ptr<FrameBuffer> buffer = createBuffer(config);
> +		if (!buffer) {
> +			LOG(DmaBufAllocator, Error) << "Unable to create buffer";
> +
> +			buffers->clear();
> +			return -EINVAL;
> +		}
> +
> +		buffers->push_back(std::move(buffer));
> +	}
> +
> +	return count;
> +}
> +
> +std::unique_ptr<FrameBuffer> DmaBufAllocator::createBuffer(
> +	const StreamConfiguration &config)
> +{
> +	std::vector<FrameBuffer::Plane> planes;
> +
> +	auto info = PixelFormatInfo::info(config.pixelFormat);
> +	for (size_t i = 0; i < info.planes.size(); ++i) {
> +		unsigned int planeSize = info.planeSize(config.size, i);
> +		if (planeSize == 0)
> +			continue;
> +
> +		UniqueFD fd = alloc("FrameBuffer", planeSize);
> +		if (!fd.isValid())
> +			return nullptr;
> +
> +		SharedFD sharedFd(std::move(fd));
> +
> +		FrameBuffer::Plane plane;
> +		plane.fd = sharedFd;

		plane.fd = SharedFD(std::move(fd));
	
and drop the sharedFd variable.

> +		plane.offset = 0;
> +		plane.length = planeSize;
> +		planes.push_back(std::move(plane));
> +	}
> +
> +	return std::make_unique<FrameBuffer>(planes);
> +}
> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart