<div dir="ltr"><div dir="ltr">Thanks Jacopo for the review.</div><div dir="ltr"><br></div><div>The corresponding updates are included in v9.1. Please check.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Aug 27, 2024 at 4:13 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@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">Hi Harvey<br>
<br>
On Tue, Aug 20, 2024 at 04:23:32PM GMT, Harvey Yang wrote:<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 | 12 ++++<br>
> src/libcamera/dma_buf_allocator.cpp | 64 ++++++++++++++++++-<br>
> 2 files changed, 74 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/include/libcamera/internal/dma_buf_allocator.h b/include/libcamera/internal/dma_buf_allocator.h<br>
> index 36ec1696b..3a9b56b1c 100644<br>
> --- a/include/libcamera/internal/dma_buf_allocator.h<br>
> +++ b/include/libcamera/internal/dma_buf_allocator.h<br>
> @@ -8,12 +8,16 @@<br>
> #pragma once<br>
><br>
> #include <stddef.h><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>
> +struct StreamConfiguration;<br>
<br>
Where do you use this ?<br>
<br></blockquote><div><br></div><div>Right, it's only used in the previous versions. Removed.</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>
> class DmaBufAllocator<br>
> {<br>
> public:<br>
> @@ -30,7 +34,15 @@ public:<br>
> bool isValid() const { return providerHandle_.isValid(); }<br>
> UniqueFD alloc(const char *name, std::size_t size);<br>
><br>
> + int exportBuffers(<br>
<br>
weird indent<br>
<br>
int exportBuffers(std::size_t count,<br>
const std::vector<std::size_t> &frameSize,<br>
std::vector<std::unique_ptr<FrameBuffer>> *buffers);<br>
<br></blockquote><div>Done</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">
> + std::size_t count,<br>
<br>
#include <cstddef><br></blockquote><div>Removed std::size_t.</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>
> + std::vector<std::size_t> frameSize,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers);<br>
<br>
#include <memory><br>
<br></blockquote><div>Done</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>
> private:<br>
> + std::unique_ptr<FrameBuffer> createBuffer(<br>
> + std::string name, std::vector<std::size_t> frameSizes);<br>
<br>
#include <string><br></blockquote><div>Done</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>
> 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 c06eca7d0..2d88b8b2b 100644<br>
> --- a/src/libcamera/dma_buf_allocator.cpp<br>
> +++ b/src/libcamera/dma_buf_allocator.cpp<br>
> @@ -23,6 +23,11 @@<br>
><br>
> #include <libcamera/base/log.h><br>
><br>
> +#include <libcamera/framebuffer.h><br>
> +#include <libcamera/stream.h><br>
<br>
Not used<br></blockquote><div>Removed</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>
> +#include "libcamera/internal/formats.h"<br>
> +<br>
> /**<br>
> * \file dma_buf_allocator.cpp<br>
> * \brief dma-buf allocator<br>
> @@ -130,8 +135,8 @@ DmaBufAllocator::~DmaBufAllocator() = default;<br>
> /* uClibc doesn't provide the file sealing API. */<br>
> #ifndef __DOXYGEN__<br>
> #if not HAVE_FILE_SEALS<br>
> -#define F_ADD_SEALS 1033<br>
> -#define F_SEAL_SHRINK 0x0002<br>
> +#define F_ADD_SEALS 1033<br>
> +#define F_SEAL_SHRINK 0x0002<br>
<br>
Unrelated<br>
<br></blockquote><div>Removed the changes</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">
> #endif<br>
> #endif<br>
><br>
> @@ -243,4 +248,59 @@ UniqueFD DmaBufAllocator::alloc(const char *name, std::size_t size)<br>
> return allocFromHeap(name, size);<br>
> }<br>
><br>
> +/**<br>
> + * \brief Allocate and export buffers for \a stream from the DmaBufAllocator<br>
<br>
There's no \a stream<br></blockquote><div>Removed.</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>
> + * \param[in] count The number of FrameBuffers required<br>
<br>
"of requested FrameBuffers" ?<br></blockquote><div>Thanks! Adopted.</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>
> + * \param[in] frameSizes The sizes of planes in the 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(<br>
> + std::size_t count,<br>
<br>
nit: This can be an unsigned int maybe<br></blockquote><div>Done</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>
> + std::vector<std::size_t> frameSizes,<br>
<br>
can this be passed as a const reference ?<br></blockquote><div>Done</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>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
<br>
weird indentation, I think<br>
<br>
int DmaBufAllocator::exportBuffers(std::size_t count,<br>
const std::vector<std::size_t> &frameSizes,<br>
std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
<br>
would do (some heretics consider 120 cols to be ok for libcamera, and<br>
nowadays it is accepted for linux as well !! )<br></blockquote><div>Thanks! Adopted.</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>
> + for (unsigned i = 0; i < count; ++i) {<br>
<br>
I wonder if we should be stricter when defining this interface: is<br>
there a maximum number of buffers that can be allocated ? How many<br>
planes can a buffer have ? (I don't see this addressed in our<br>
FrameBuffer class, so it can be left out from here too ?)<br></blockquote><div><br></div><div>Hmm, as it's just a helper function, I think the users should know what</div><div>they're doing, unless we have more specific rules, like the maximum</div><div>number of planes in FrameBuffer.</div><div><br></div><div>We can't stop users allocating too many buffers with or without this</div><div>helper function, right :p ?</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>
> + std::unique_ptr<FrameBuffer> buffer =<br>
> + createBuffer("frame-" + std::to_string(i), frameSizes);<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>
if you want to have all the requested buffers to be allocated, and do<br>
not allow less than that, there's no point in returning count here, you<br>
can return 0<br></blockquote><div><br></div><div>Hmm, you're right, while I wonder if it's the same case for</div><div>V4L2VideoDevice::exportBuffers/createBuffers? I want to align with</div><div>other exportBuffers API.</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>
> +<br>
> +std::unique_ptr<FrameBuffer> DmaBufAllocator::createBuffer(<br>
> + std::string name, std::vector<std::size_t> frameSizes)<br>
<br>
Or<br>
<br>
std::unique_ptr<FrameBuffer><br>
DmaBufAllocator::createBuffer(std::string name,<br>
const std::vector<std::size_t> &frameSizes)<br>
<br>
can frameSize be passed as a const reference ?<br></blockquote><div>Thanks! Adopted.</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>
> +{<br>
> + std::vector<FrameBuffer::Plane> planes;<br>
> +<br>
> + std::size_t bufferSize = 0, offset = 0;<br>
> + for (auto frameSize : frameSizes)<br>
> + bufferSize += frameSize;<br>
<br>
This API allows unbounded size allocation, just sayin', maybe it's<br>
fine<br></blockquote><div><br></div><div>Yeah... as it's just a helper function, I still think that the users should</div><div>know what they're doing.</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>
> + SharedFD fd(alloc(name.c_str(), bufferSize));<br>
> + if (!fd.isValid())<br>
> + return nullptr;<br>
> +<br>
> + for (auto frameSize : frameSizes) {<br>
> + FrameBuffer::Plane plane;<br>
> + plane.fd = fd;<br>
> + plane.offset = offset;<br>
> + plane.length = frameSize;<br>
> + planes.push_back(std::move(plane));<br>
> + offset += plane.length;<br>
> + }<br>
<br>
With a little implicit type casting, you can<br>
<br>
unsigned int offset = 0;<br>
for (unsigned int frameSize : frameSizes) {<br>
planes.emplace_back(FrameBuffer::Plane{fd, offset, frameSize});<br>
offset += frameSize;<br>
}<br>
<br>
But it won't save you going a temporary object I'm afraid. I'm not<br>
sure it's actually any better, up to you.<br>
<br></blockquote><div>Thanks, adopted. Also, as the offset's and length's types are both</div><div>unsigned int, I changed `frameSize` to be an array of unsigned as</div><div>well.</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>
> + return std::make_unique<FrameBuffer>(planes);<br>
> +}<br>
> +<br>
> } /* namespace libcamera */<br>
> --<br>
> 2.46.0.184.g6999bdac58-goog<br>
><br>
</blockquote></div></div>