<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thanks for the review. Updated in v8.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Aug 3, 2024 at 10:21 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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 Chenghao,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Aug 01, 2024 at 07:30:57AM +0000, Harvey Yang wrote:<br>
> Add a helper function exportFrameBuffers in DmaBufAllocator to make it<br>
> easier to use.<br>
> <br>
> It'll be used in Virtual Pipeline Handler specifically.<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 | 10 +++<br>
> src/libcamera/dma_buf_allocator.cpp | 68 ++++++++++++++++++-<br>
> 2 files changed, 76 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 36ec1696..dd2cc237 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>
> class DmaBufAllocator<br>
> {<br>
> public:<br>
> @@ -30,7 +34,13 @@ public:<br>
> bool isValid() const { return providerHandle_.isValid(); }<br>
> UniqueFD alloc(const char *name, std::size_t size);<br>
> <br>
> + int exportFrameBuffers(<br>
> + const StreamConfiguration &config,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers);<br>
<br>
I wonder if the DmaBufAllocator class is really the best place to<br>
implement this. It would help reviewing how generic the implementation<br>
is if we had two users as part of this series. Maybe converting the soft<br>
ISP to the new function could be such a second user.<br>
<br></blockquote><div><br></div><div>Right, it's a good idea that we add the helper functions in a way that works</div><div>for soft ISP as 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">
The part that bothers me the most is, I think, usage of<br>
StreamConfiguration in this function. Maybe the createBuffer() function<br>
could be kept, and be passed a pixel format and a size, whie the<br>
exportFrameBuffers() could be moved to the pipeline handler.<br>
<br></blockquote><div><br></div><div>Hmm, as |SoftwareIsp::exportFrames| only uses `debayer_->frameSize()`, </div><div>I updated the DmaBufAllocator's helper function with `count` and a list of</div><div>`frameSize`s. Please check if it's better.</div><div><br></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(const StreamConfiguration &config);<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 c06eca7d..6b406880 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>
> +#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 change.<br>
<br></blockquote><div><br></div><div>Sorry, my linter auto-corrected it. 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">
> #endif<br>
> #endif<br>
> <br>
> @@ -243,4 +248,63 @@ 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>
> + * \param[in] config The config of the stream to allocate buffers for<br>
> + * \param[out] buffers Array of buffers successfully allocated<br>
> + *<br>
> + * Allocates buffers for a stream from the DmaBufAllocator. It's a helper<br>
> + * function that'll be used in PipelineHandler::exportFrameBuffers().<br>
> + *<br>
> + * \return The number of allocated buffers on success or a negative error code<br>
> + * otherwise<br>
> + */<br>
> +int DmaBufAllocator::exportFrameBuffers(<br>
> + const StreamConfiguration &config,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
> +{<br>
> + unsigned int count = config.bufferCount;<br>
> +<br>
> + for (unsigned i = 0; i < count; ++i) {<br>
> + std::unique_ptr<FrameBuffer> buffer = createBuffer(config);<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> DmaBufAllocator::createBuffer(<br>
> + const StreamConfiguration &config)<br>
> +{<br>
> + std::vector<FrameBuffer::Plane> planes;<br>
> +<br>
> + auto info = PixelFormatInfo::info(config.pixelFormat);<br>
> + for (size_t i = 0; i < info.planes.size(); ++i) {<br>
> + unsigned int planeSize = info.planeSize(config.size, i);<br>
> + if (planeSize == 0)<br>
> + continue;<br>
> +<br>
> + UniqueFD fd = alloc("FrameBuffer", planeSize);<br>
> + if (!fd.isValid())<br>
> + return nullptr;<br>
> +<br>
> + SharedFD sharedFd(std::move(fd));<br>
> +<br>
> + FrameBuffer::Plane plane;<br>
> + plane.fd = sharedFd;<br>
<br>
plane.fd = SharedFD(std::move(fd));<br>
<br>
and drop the sharedFd variable.<br>
<br></blockquote><div><br></div><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">
> + plane.offset = 0;<br>
> + plane.length = planeSize;<br>
> + planes.push_back(std::move(plane));<br>
> + }<br>
> +<br>
> + return std::make_unique<FrameBuffer>(planes);<br>
> +}<br>
> +<br>
> } /* namespace libcamera */<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br></blockquote><div><br>Thanks!<br>Harvey </div></div></div>