<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 9, 2024 at 9:23 PM Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Harvey Yang (2024-09-07 15:28:26)<br>
> Add a helper function exportBuffers in DmaBufAllocator to make it easier<br>
> to use.<br>
> <br>
> It'll be used in Virtual Pipeline Handler and SoftwareIsp.<br>
> <br>
> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> .../libcamera/internal/dma_buf_allocator.h | 13 +++++<br>
> src/libcamera/dma_buf_allocator.cpp | 55 +++++++++++++++++++<br>
> 2 files changed, 68 insertions(+)<br>
> <br>
> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h<br>
> index d2a0a0d1..66d3b419 100644<br>
> --- a/include/libcamera/internal/dma_buf_allocator.h<br>
> +++ b/include/libcamera/internal/dma_buf_allocator.h<br>
> @@ -7,11 +7,17 @@<br>
> <br>
> #pragma once<br>
> <br>
> +#include <memory><br>
> +#include <string><br>
> +#include <vector><br>
> +<br>
> #include <libcamera/base/flags.h><br>
> #include <libcamera/base/unique_fd.h><br>
> <br>
> namespace libcamera {<br>
> <br>
> +class FrameBuffer;<br>
> +<br>
> class DmaBufAllocator<br>
> {<br>
> public:<br>
> @@ -28,7 +34,14 @@ public:<br>
> bool isValid() const { return providerHandle_.isValid(); }<br>
> UniqueFD alloc(const char *name, std::size_t size);<br>
> <br>
> + int exportBuffers(unsigned int count,<br>
> + const std::vector<unsigned int> &frameSize,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers);<br>
> +<br>
> private:<br>
> + std::unique_ptr<FrameBuffer> createBuffer(<br>
> + std::string name, const std::vector<unsigned int> &frameSizes);<br>
> +<br>
> UniqueFD allocFromHeap(const char *name, std::size_t size);<br>
> UniqueFD allocFromUDmaBuf(const char *name, std::size_t size);<br>
> UniqueFD providerHandle_;<br>
> diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp<br>
> index be6efb89..d2ac175f 100644<br>
> --- a/src/libcamera/dma_buf_allocator.cpp<br>
> +++ b/src/libcamera/dma_buf_allocator.cpp<br>
> @@ -23,6 +23,10 @@<br>
> #include <libcamera/base/log.h><br>
> #include <libcamera/base/memfd.h><br>
> <br>
> +#include <libcamera/framebuffer.h><br>
> +<br>
> +#include "libcamera/internal/formats.h"<br>
> +<br>
> /**<br>
> * \file dma_buf_allocator.cpp<br>
> * \brief dma-buf allocator<br>
> @@ -205,4 +209,55 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size)<br>
> return allocFromHeap(name, size);<br>
> }<br>
> <br>
> +/**<br>
> + * \brief Allocate and export buffers from the DmaBufAllocator<br>
> + * \param[in] count The number of requested FrameBuffers<br>
> + * \param[in] frameSizes The sizes of planes in each FrameBuffer<br>
> + * \param[out] buffers Array of buffers successfully allocated<br>
> + *<br>
> + * \return The number of allocated buffers on success or a negative error code<br>
> + * otherwise<br>
> + */<br>
> +int DmaBufAllocator::exportBuffers(unsigned int count,<br>
> + const std::vector<unsigned int> &frameSizes,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
> +{<br>
> + for (unsigned int i = 0; i < count; ++i) {<br>
> + std::unique_ptr<FrameBuffer> buffer =<br>
> + createBuffer("frame-" + std::to_string(i), frameSizes);<br>
<br>
I wonder if we should find a better name for the buffer names ... but<br>
this is what already happens in softisp anyway so I'm fine with this.<br>
<br></blockquote><div><br></div><div>I'd be glad to accept suggestions :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> + if (!buffer) {<br>
> + LOG(DmaBufAllocator, Error) << "Unable to create buffer";<br>
> +<br>
> + buffers->clear();<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + buffers->push_back(std::move(buffer));<br>
> + }<br>
> +<br>
> + return count;<br>
> +}<br>
> +<br>
> +std::unique_ptr<FrameBuffer><br>
> +DmaBufAllocator::createBuffer(std::string name,<br>
> + const std::vector<unsigned int> &frameSizes)<br>
> +{<br>
> + std::vector<FrameBuffer::Plane> planes;<br>
> +<br>
> + unsigned int bufferSize = 0, offset = 0;<br>
> + for (auto frameSize : frameSizes)<br>
> + bufferSize += frameSize;<br>
> +<br>
> + SharedFD fd(alloc(name.c_str(), bufferSize));<br>
> + if (!fd.isValid())<br>
> + return nullptr;<br>
<br>
So this creates the allocation with a single dma buf ... I'm weary if<br>
this will work generically - but I also think we don't need to worry<br>
about this now.<br>
<br>
Some allocations might want each of the planes to be separated or have<br>
specific alignments...<br>
<br>
But without a user requiring those - I think this is fine for a first<br>
step. I wonder if this should be documented somewhere, probably in the<br>
exportBuffers description.<br></blockquote><div><br></div><div>True, some of the previous versions are actually allocating buffers with</div><div>their own dma buf allocation respectively, while I thought that using a single</div><div>dma buf is generally more preferred (Please let me know if I'm wrong :p).</div><div><br></div><div>Added a description and a todo in `exportBuffers`.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
With that<br>
<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
> +<br>
> + for (auto frameSize : frameSizes) {<br>
> + planes.emplace_back(FrameBuffer::Plane{ fd, offset, frameSize });<br>
> + offset += frameSize;<br>
> + }<br>
> +<br>
> + return std::make_unique<FrameBuffer>(planes);<br>
> +}<br>
> +<br>
> } /* namespace libcamera */<br>
> -- <br>
> 2.46.0.469.g59c65b2a67-goog<br>
><br>
</blockquote></div></div>