<div dir="ltr"><div dir="ltr">Thanks Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 30, 2023 at 2:56 PM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">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">On Mon, May 29, 2023 at 10:02:18AM +0200, Jacopo Mondi via libcamera-devel wrote:<br>
> Hi Harvey<br>
> <br>
> On Mon, May 22, 2023 at 08:35:08AM +0000, 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>
> This is the same email address with 2 different names. Unless this is<br>
> intentional could you change the authorship of the patch and use a<br>
> single identity (git commit --amend --autor="...")<br>
> <br>
> > Add a helper function exportFrameBuffers in HeapAllocator to make it<br>
> > easier to use.<br>
> ><br>
> > Signed-off-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> > ---<br>
> > include/libcamera/internal/heap_allocator.h | 9 +++<br>
> > src/libcamera/heap_allocator.cpp | 62 +++++++++++++++++++++<br>
> > 2 files changed, 71 insertions(+)<br>
> ><br>
> > diff --git a/include/libcamera/internal/heap_allocator.h b/include/libcamera/internal/heap_allocator.h<br>
> > index 92d4488a..92beaaa4 100644<br>
> > --- a/include/libcamera/internal/heap_allocator.h<br>
> > +++ b/include/libcamera/internal/heap_allocator.h<br>
> > @@ -9,12 +9,16 @@<br>
> > #pragma once<br>
> ><br>
> > #include <stddef.h><br>
> > +#include <vector><br>
> ><br>
> > #include <libcamera/base/unique_fd.h><br>
> ><br>
> > namespace libcamera {<br>
> ><br>
> > +class Camera;<br>
> > +class FrameBuffer;<br>
> > class Heap;<br>
> > +class Stream;<br>
> ><br>
> > class HeapAllocator<br>
> > {<br>
> > @@ -24,7 +28,12 @@ public:<br>
> > bool isValid() const;<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 8f99f590..e99cdbe5 100644<br>
> > --- a/src/libcamera/heap_allocator.cpp<br>
> > +++ b/src/libcamera/heap_allocator.cpp<br>
> > @@ -22,6 +22,12 @@<br>
> ><br>
> > #include <libcamera/base/log.h><br>
> ><br>
> > +#include <libcamera/camera.h><br>
> > +#include <libcamera/framebuffer.h><br>
> > +#include <libcamera/stream.h><br>
> > +<br>
> > +#include "libcamera/internal/formats.h"<br>
> > +<br>
> > namespace libcamera {<br>
> ><br>
> > /*<br>
> > @@ -227,4 +233,60 @@ 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>
Why do you pass a camera pointer to this function if it's unused ?<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>
> > + unsigned int count = stream->configuration().bufferCount;<br>
<br>
You're creating a tight dependency between the allocator and the stream<br>
here, as you require the stream to have already been configured, and<br>
rely on the current configuration. Passing a stream configuration<br>
explicitly would be better to make the allocator more generic.<br>
<br></blockquote><div><br></div><div>Good point. Using `const StreamConfiguration&` directly.</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>
> > + /** \todo Support multiplanar allocations */<br>
<br>
I'd like to see this being addressed already, as it's a key requirement<br>
that will show whether the API design is good or not.<br>
<br></blockquote><div><br></div><div>IIUC, this means the same thing as `a dedicated fd per plane`, right?</div><div>Let me know if I misunderstood.</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>
> > + 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>
> > + unsigned int frameSize = stream->configuration().frameSize;<br>
> > + int pageSize = getpagesize();<br>
> > + /** Allocation size need to be a direct multiple of |pageSize|. */<br>
> <br>
> No need for /**, just use /*<br>
> <br>
> > + unsigned int numPage = (frameSize + pageSize - 1) / pageSize;<br>
<br>
What are the pros and cons for rounding it up to the page size here<br>
compared to the alloc() function ? Does the kernel require a rounded<br>
size, of will it round internally ?<br>
<br></blockquote><div><br></div><div>Moved to `HeapAllocator::alloc` to prevent misuse in other components.</div><div>Yeah, the kernel requires a rounded size or it fails (in my environment with</div><div>udmabuf).</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>
> > + UniqueFD fd = alloc("FrameBuffer", numPage * pageSize);<br>
> > + if (!fd.isValid())<br>
> > + return nullptr;<br>
> > +<br>
> > + auto info = PixelFormatInfo::info(stream->configuration().pixelFormat);<br>
> > + SharedFD shared_fd(std::move(fd));<br>
> <br>
> s/shared_fd/sharedFd/<br>
> <br>
> > + unsigned int offset = 0;<br>
> > + for (size_t i = 0; i < info.planes.size(); ++i) {<br>
> > + unsigned int planeSize = info.planeSize(stream->configuration().size, i);<br>
<br>
This doesn't take line stride into account.<br>
<br></blockquote><div><br></div><div>IIUC, `PixelFormatInfo::planeSize` considers stride in the implementation...?<br><a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.cpp#n1022" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/formats.cpp#n1022</a><br></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">
> > + if (planeSize == 0)<br>
> > + continue;<br>
> > +<br>
> > + /** \todo We don't support allocating buffers with a dedicated fd per plane */<br>
<br>
This then makes the buffer incompatible with V4L2, that seems to be a<br>
bad issue for a generic allocator in libcamera.<br>
<br></blockquote><div><br></div><div>Updated. Please check if it makes sense.</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">
> > + FrameBuffer::Plane plane;<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>
> <br>
> Only minors, the patch looks good. The series looks good as well,<br>
> please add documentation and fix all the minors and I think we can<br>
> collect it :)<br>
<br>
I'm afraid I disagree. See my comment in patch 2/3, in addition to the<br>
above.<br>
<br>
> > } /* namespace libcamera */<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>