<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:32 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 08:03:19AM +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>
> 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            | 61 +++++++++++++++++++++<br>
>  2 files changed, 70 insertions(+)<br>
><br>
> diff --git a/include/libcamera/internal/heap_allocator.h b/include/libcamera/internal/heap_allocator.h<br>
> index d061fdce..b6488fbc 100644<br>
> --- a/include/libcamera/internal/heap_allocator.h<br>
> +++ b/include/libcamera/internal/heap_allocator.h<br>
> @@ -8,12 +8,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>
> @@ -23,7 +27,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 682a7e01..ce3a815d 100644<br>
> --- a/src/libcamera/heap_allocator.cpp<br>
> +++ b/src/libcamera/heap_allocator.cpp<br>
> @@ -21,6 +21,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>
> @@ -231,4 +237,59 @@ 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>
> +     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 c++ style comments, please<br>
<br></blockquote><div><br></div><div>You mean to use "/**" and "*/", right?</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 numPage = (frameSize + pageSize - 1) / pageSize;<br>
> +<br>
> +     UniqueFD fd = alloc("FrameBuffer", numPage * pageSize);<br>
> +     if (!fd.isValid())<br>
> +             return nullptr;<br>
> +<br>
<br>
And I would add here a comment as reported in the review of v4<br>
<br>
Can be fixed when applying<br></blockquote><div><br></div><div>You mean the todo of the dedicated fd for each plane, right?</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">
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
<br>
Thanks<br>
  j<br>
<br>
> +     auto info = PixelFormatInfo::info(stream->configuration().pixelFormat);<br>
> +     SharedFD shared_fd(std::move(fd));<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>
> +             if (planeSize == 0)<br>
> +                     continue;<br>
> +<br>
> +             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>
>  } /* namespace libcamera */<br>
> --<br>
> 2.40.1.606.ga4b1b128d6-goog<br>
><br>
</blockquote></div></div>