<div dir="ltr"><div dir="ltr">Thanks Jacopo!</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, May 14, 2023 at 12:29 AM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">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, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via libcamera-devel wrote:<br>
> From: Cheng-Hao Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
><br>
> Add a helper function exportFrameBuffers in HeapAllocator to make it<br>
> easier to use.<br>
<br>
Will you use these with the Virtual pipeline handler ?<br>
<br></blockquote><div><br></div><div>Yes, you can find it in |PipelindHandlerVirtual::exportFrameBuffer|.</div><div>Ref: <a href="https://patchwork.libcamera.org/patch/18533/">https://patchwork.libcamera.org/patch/18533/</a></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>
> Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> include/libcamera/heap_allocator.h | 11 ++++++<br>
> src/libcamera/heap_allocator.cpp | 56 ++++++++++++++++++++++++++++++<br>
> 2 files changed, 67 insertions(+)<br>
><br>
> diff --git a/include/libcamera/heap_allocator.h b/include/libcamera/heap_allocator.h<br>
> index cd7ed1a3..076f0951 100644<br>
> --- a/include/libcamera/heap_allocator.h<br>
> +++ b/include/libcamera/heap_allocator.h<br>
> @@ -8,6 +8,7 @@<br>
> #pragma once<br>
><br>
> #include <stddef.h><br>
> +#include <vector><br>
><br>
> #include <libcamera/base/unique_fd.h><br>
><br>
> @@ -15,6 +16,11 @@<br>
><br>
> namespace libcamera {<br>
><br>
> +class Camera;<br>
> +class Stream;<br>
> +class FrameBuffer;<br>
> +class PixelFormatInfo;<br>
<br>
Not used<br>
<br></blockquote><div><br></div><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>
> class HeapAllocator<br>
> {<br>
> public:<br>
> @@ -23,7 +29,12 @@ public:<br>
> bool isValid() const { return heap_->isValid(); }<br>
> UniqueFD alloc(const char *name, std::size_t size);<br>
><br>
> + int exportFrameBuffers(Camera *camera, Stream *stream,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers);<br>
> +<br>
> private:<br>
> + std::unique_ptr<FrameBuffer> createBuffer(Stream *stream);<br>
> +<br>
> std::unique_ptr<Heap> heap_;<br>
> };<br>
><br>
> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp<br>
> index 179c2c21..33e9eaca 100644<br>
> --- a/src/libcamera/heap_allocator.cpp<br>
> +++ b/src/libcamera/heap_allocator.cpp<br>
> @@ -17,7 +17,11 @@<br>
><br>
> #include <libcamera/base/log.h><br>
><br>
> +#include <libcamera/camera.h><br>
> #include <libcamera/dma_heap.h><br>
> +#include <libcamera/framebuffer.h><br>
> +#include <libcamera/internal/formats.h><br>
> +#include <libcamera/stream.h><br>
> #include <libcamera/udma_heap.h><br>
><br>
> namespace libcamera {<br>
> @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name, std::size_t size)<br>
> return heap_->alloc(name, size);<br>
> }<br>
><br>
> +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,<br>
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)<br>
> +{<br>
> + unsigned int count = stream->configuration().bufferCount;<br>
> +<br>
> + /** \todo: Support multiplanar allocations */<br>
<br>
no : after \todo<br>
<br></blockquote><div><br></div><div>Done</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + for (unsigned i = 0; i < count; ++i) {<br>
> + std::unique_ptr<FrameBuffer> buffer = createBuffer(stream);<br>
> + if (!buffer) {<br>
> + LOG(HeapAllocator, 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> HeapAllocator::createBuffer(Stream *stream)<br>
> +{<br>
> + std::vector<FrameBuffer::Plane> planes;<br>
> +<br>
> + int num_page = (stream->configuration().frameSize + getpagesize() - 1) / getpagesize();<br>
<br>
Could you break this rather long line ?<br></blockquote><div><br></div><div>Defined "frameSize" and "pageSize" beforehand. Should be shorter now.</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">
Can num_page be unsigned ?<br></blockquote><div><br></div><div>Sure.</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">
libcamera uses camelCase and not snake_case for variables<br>
<br>
unsigned int numPage = (stream->configuration().frameSize + getpagesize() - 1)<br>
/ getpagesize();<br>
<br>
What are the implications of overallocating ? The last plane will be<br>
slightly larger, is this an issue at all ?<br>
<br></blockquote><div><br></div><div>The allocation only works if the requested size is a direct multiple of |getpagesize()|. Added a comment.</div><div>Ref: <a href="https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72">https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72</a></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 fd = alloc("Buffer", num_page * getpagesize());<br>
<br>
What are the implications of using the same name ?<br>
<br></blockquote><div><br></div><div>Since it's a private function that is only called by |exportFrameBuffer|, renamed it to "FrameBuffer" instead.</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">
> + if (!fd.isValid())<br>
> + return nullptr;<br>
> +<br>
> + auto info = PixelFormatInfo::info(stream->configuration().pixelFormat);<br>
> + SharedFD shared_fd(std::move(fd));<br>
> + unsigned int offset = 0;<br>
> + for (long unsigned int i = 0; i < info.planes.size(); ++i) {<br>
<br>
Does this need to be long ?<br>
<br></blockquote><div><br></div><div>I should use size_t instead.</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">
> + unsigned int planeSize = info.planeSize(stream->configuration().size, i);<br>
<br>
configuration().size is the size in pixel, is this intended ?<br>
<br></blockquote><div><br></div><div>Yes, I guess so, according to the comment of PixelFormatInfo: <a href="https://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056">https://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056</a></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">
> + if (planeSize == 0)<br>
> + continue;<br>
> +<br>
> + FrameBuffer::Plane plane;<br>
<br>
I would note down a \todo to remind that we don't support allocating<br>
buffers with a dedicated fd per plane<br>
<br></blockquote><div><br></div><div>Is it preferred to do so? I guess we can also support that by calling |alloc()| multiple times...?</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">
Sorry, lot of questions :)<br>
<br></blockquote><div><br></div><div>Thanks for doing so :)</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">
Cheers<br>
j<br>
<br>
<br>
> + plane.fd = shared_fd;<br>
> + plane.offset = offset;<br>
> + plane.length = planeSize;<br>
> + planes.push_back(std::move(plane));<br>
> +<br>
> + offset += planeSize;<br>
> + }<br>
> +<br>
> + return std::make_unique<FrameBuffer>(planes);<br>
> +}<br>
> +<br>
> } /* namespace libcamera */<br>
> --<br>
> 2.40.0<br>
><br>
</blockquote></div></div>