<div dir="ltr"><div dir="ltr">Thanks Jacopo.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 16, 2023 at 6:21 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, May 16, 2023 at 04:12:08PM +0800, Cheng-Hao Yang via libcamera-devel wrote:<br>
> Thanks Jacopo!<br>
><br>
> On Sun, May 14, 2023 at 12:29 AM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> wrote:<br>
><br>
> > Hi Harvey<br>
> ><br>
> > On Tue, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via libcamera-devel<br>
> > 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>
> ><br>
> Yes, you can find it in |PipelindHandlerVirtual::exportFrameBuffer|.<br>
> Ref: <a href="https://patchwork.libcamera.org/patch/18533/" rel="noreferrer" target="_blank">https://patchwork.libcamera.org/patch/18533/</a><br>
><br>
><br>
><br>
> > ><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<br>
> > 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>
> ><br>
> Removed.<br>
><br>
><br>
> > > +<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>><br>
> > *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<br>
> > 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,<br>
> > std::size_t size)<br>
> > > return heap_->alloc(name, size);<br>
> > > }<br>
> > ><br>
> > > +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera,<br>
> > Stream *stream,<br>
> > > +<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>
> ><br>
> Done<br>
><br>
> > +<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<br>
> > 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()<br>
> > - 1) / getpagesize();<br>
> ><br>
> > Could you break this rather long line ?<br>
> ><br>
><br>
> Defined "frameSize" and "pageSize" beforehand. Should be shorter now.<br>
><br>
><br>
> > Can num_page be unsigned ?<br>
> ><br>
><br>
> Sure.<br>
><br>
><br>
> > libcamera uses camelCase and not snake_case for variables<br>
> ><br>
> > unsigned int numPage = (stream->configuration().frameSize +<br>
> > 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>
> ><br>
> The allocation only works if the requested size is a direct multiple of<br>
> |getpagesize()|. Added a comment.<br>
> Ref:<br>
> <a href="https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72" rel="noreferrer" target="_blank">https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72</a><br>
><br>
<br>
Yes, I understand the requirement, I was wondering about the<br>
implications of overallocation, but I presume this is safe ?<br>
<br></blockquote><div><br></div><div>Ah sorry to misunderstand your question. I assume the plane size is</div><div>determined by |FrameBuffer::Plane::length|, not the remaining space</div><div>of the allocated buffer. It's specified in line 282. Please correct me if</div><div>I'm wrong :)</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>
> > > + UniqueFD fd = alloc("Buffer", num_page * getpagesize());<br>
> ><br>
> > What are the implications of using the same name ?<br>
> ><br>
> ><br>
> Since it's a private function that is only called by |exportFrameBuffer|,<br>
> renamed it to "FrameBuffer" instead.<br>
><br>
><br>
> > > + if (!fd.isValid())<br>
> > > + return nullptr;<br>
> > > +<br>
> > > + auto info =<br>
> > 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>
> ><br>
> I should use size_t instead.<br>
><br>
><br>
> > > + unsigned int planeSize =<br>
> > info.planeSize(stream->configuration().size, i);<br>
> ><br>
> > configuration().size is the size in pixel, is this intended ?<br>
> ><br>
> ><br>
> Yes, I guess so, according to the comment of PixelFormatInfo:<br>
> <a href="https://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056" rel="noreferrer" target="_blank">https://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056</a><br>
><br>
<br>
Ah you're right<br>
<br>
\param[in] size The size of the frame, in pixels<br>
<br>
<br>
><br>
> > > + 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>
> ><br>
> Is it preferred to do so? I guess we can also support that by calling<br>
> |alloc()| multiple times...?<br>
><br>
<br>
I don't think it's specifically preferred, but might be required for<br>
fully-planar formats where each plane lives in its own possibly<br>
non-contiguous memory area. I would for now record a todo item<br>
<br>
We have a similar issue here iirc<br>
<a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/mm/cros_frame_buffer_allocator.cpp#n57" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/mm/cros_frame_buffer_allocator.cpp#n57</a><br>
<br>
<br></blockquote><div><br></div><div>Okay. Added a comment. Feel free to modify it when landing.</div><div>Also let me know if a dedicated fd is needed here, as I think</div><div>it's not that hard to do 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">
><br>
> > Sorry, lot of questions :)<br>
> ><br>
> ><br>
> Thanks for doing so :)<br>
><br>
><br>
> > 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>
> ><br>
</blockquote></div></div>