[{"id":27094,"web_url":"https://patchwork.libcamera.org/comment/27094/","msgid":"<20230513162915.n7qty4q6whi7bkyy@lati>","date":"2023-05-13T16:29:15","subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Tue, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via libcamera-devel wrote:\n> From: Cheng-Hao Yang <chenghaoyang@chromium.org>\n>\n> Add a helper function exportFrameBuffers in HeapAllocator to make it\n> easier to use.\n\nWill you use these with the Virtual pipeline handler ?\n\n>\n> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>\n> ---\n>  include/libcamera/heap_allocator.h | 11 ++++++\n>  src/libcamera/heap_allocator.cpp   | 56 ++++++++++++++++++++++++++++++\n>  2 files changed, 67 insertions(+)\n>\n> diff --git a/include/libcamera/heap_allocator.h b/include/libcamera/heap_allocator.h\n> index cd7ed1a3..076f0951 100644\n> --- a/include/libcamera/heap_allocator.h\n> +++ b/include/libcamera/heap_allocator.h\n> @@ -8,6 +8,7 @@\n>  #pragma once\n>\n>  #include <stddef.h>\n> +#include <vector>\n>\n>  #include <libcamera/base/unique_fd.h>\n>\n> @@ -15,6 +16,11 @@\n>\n>  namespace libcamera {\n>\n> +class Camera;\n> +class Stream;\n> +class FrameBuffer;\n> +class PixelFormatInfo;\n\nNot used\n\n> +\n>  class HeapAllocator\n>  {\n>  public:\n> @@ -23,7 +29,12 @@ public:\n>  \tbool isValid() const { return heap_->isValid(); }\n>  \tUniqueFD alloc(const char *name, std::size_t size);\n>\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t       std::vector<std::unique_ptr<FrameBuffer>> *buffers);\n> +\n>  private:\n> +\tstd::unique_ptr<FrameBuffer> createBuffer(Stream *stream);\n> +\n>  \tstd::unique_ptr<Heap> heap_;\n>  };\n>\n> diff --git a/src/libcamera/heap_allocator.cpp b/src/libcamera/heap_allocator.cpp\n> index 179c2c21..33e9eaca 100644\n> --- a/src/libcamera/heap_allocator.cpp\n> +++ b/src/libcamera/heap_allocator.cpp\n> @@ -17,7 +17,11 @@\n>\n>  #include <libcamera/base/log.h>\n>\n> +#include <libcamera/camera.h>\n>  #include <libcamera/dma_heap.h>\n> +#include <libcamera/framebuffer.h>\n> +#include <libcamera/internal/formats.h>\n> +#include <libcamera/stream.h>\n>  #include <libcamera/udma_heap.h>\n>\n>  namespace libcamera {\n> @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name, std::size_t size)\n>  \treturn heap_->alloc(name, size);\n>  }\n>\n> +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n> +\t\t\t\t      std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> +{\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\n> +\t/** \\todo: Support multiplanar allocations */\n\nno : after \\todo\n\n> +\n> +\tfor (unsigned i = 0; i < count; ++i) {\n> +\t\tstd::unique_ptr<FrameBuffer> buffer = createBuffer(stream);\n> +\t\tif (!buffer) {\n> +\t\t\tLOG(HeapAllocator, Error) << \"Unable to create buffer\";\n> +\n> +\t\t\tbuffers->clear();\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tbuffers->push_back(std::move(buffer));\n> +\t}\n> +\n> +\treturn count;\n> +}\n> +\n> +std::unique_ptr<FrameBuffer> HeapAllocator::createBuffer(Stream *stream)\n> +{\n> +\tstd::vector<FrameBuffer::Plane> planes;\n> +\n> +\tint num_page = (stream->configuration().frameSize + getpagesize() - 1) / getpagesize();\n\nCould you break this rather long line ?\nCan num_page be unsigned ?\nlibcamera uses camelCase and not snake_case for variables\n\n\tunsigned int numPage = (stream->configuration().frameSize + getpagesize() - 1)\n                             / getpagesize();\n\nWhat are the implications of overallocating ? The last plane will be\nslightly larger, is this an issue at all ?\n\n> +\n> +\tUniqueFD fd = alloc(\"Buffer\", num_page * getpagesize());\n\nWhat are the implications of using the same name ?\n\n> +\tif (!fd.isValid())\n> +\t\treturn nullptr;\n> +\n> +\tauto info = PixelFormatInfo::info(stream->configuration().pixelFormat);\n> +\tSharedFD shared_fd(std::move(fd));\n> +\tunsigned int offset = 0;\n> +\tfor (long unsigned int i = 0; i < info.planes.size(); ++i) {\n\nDoes this need to be long ?\n\n> +\t\tunsigned int planeSize = info.planeSize(stream->configuration().size, i);\n\nconfiguration().size is the size in pixel, is this intended ?\n\n> +\t\tif (planeSize == 0)\n> +\t\t\tcontinue;\n> +\n> +\t\tFrameBuffer::Plane plane;\n\nI would note down a \\todo to remind that we don't support allocating\nbuffers with a dedicated fd per plane\n\nSorry, lot of questions :)\n\nCheers\n  j\n\n\n> +\t\tplane.fd = shared_fd;\n> +\t\tplane.offset = offset;\n> +\t\tplane.length = planeSize;\n> +\t\tplanes.push_back(std::move(plane));\n> +\n> +\t\toffset += planeSize;\n> +\t}\n> +\n> +\treturn std::make_unique<FrameBuffer>(planes);\n> +}\n> +\n>  } /* namespace libcamera */\n> --\n> 2.40.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EB767BD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 13 May 2023 16:29:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65558627DE;\n\tSat, 13 May 2023 18:29:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 848AF60539\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 13 May 2023 18:29:18 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E36E16C8;\n\tSat, 13 May 2023 18:29:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683995360;\n\tbh=ghFuByqKNAQlyCYjIDxXh238vhMMFjLL7xueiy8st10=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Ra6WVJIh1ot7Rv8izJl+1koKPImp5FosHlIHGoghOhBTAHXjUJa9bzex5CYsT1Wp6\n\tkig7w0rDpeNQX5VMGFPlAfFo419SwPGc+XZtdpcrqzfaDBKnjcw8c1KoePuWZrJFRG\n\tsbH5/YUp5eJmqYGJ3xbjvJMMQBP9Q608wc7KP20YZK33XlAabp1cLKiTYftp0Fqcda\n\tzEMQ/84ZwWhGlIIIcOU77j43fqZ/tJWxQL95p11o3rrJN7xgZsy/+A70tHAkZGYPT3\n\tQoWU7b+MzM6l5mx26fB+Hp+CAY26pRipQFjHPCUTDHCwh7vP/Dnu1wqaPthU9OI5cn\n\t/2i0WXhn/sffw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1683995349;\n\tbh=ghFuByqKNAQlyCYjIDxXh238vhMMFjLL7xueiy8st10=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GCLTlPK04/YubVVleqlN69lFMXl9qXFUgj9XW0aKGBVZbmGxRWpIHBSCutzbEJFBS\n\tgSrQScTY//gb2CBLW7BioEPX8UXheN/+TAEiwoTGQoHtZ+EhdJit/rLbf1rrDAmgzf\n\tViFL36wbJ1KO77zd/QYYEti6mqpESF9Cd5syLkKk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GCLTlPK0\"; dkim-atps=neutral","Date":"Sat, 13 May 2023 18:29:15 +0200","To":"Harvey Yang <chenghaoyang@chromium.org>","Message-ID":"<20230513162915.n7qty4q6whi7bkyy@lati>","References":"<20230328095534.3584-1-harveyycyang@gmail.com>\n\t<20230328095534.3584-4-harveyycyang@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230328095534.3584-4-harveyycyang@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27105,"web_url":"https://patchwork.libcamera.org/comment/27105/","msgid":"<CAEB1ahv12W4X=Vr4M1XrDdszhGHGzt4BpP-yH0MmYPDamKgWNQ@mail.gmail.com>","date":"2023-05-16T08:12:08","subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Jacopo!\n\nOn Sun, May 14, 2023 at 12:29 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com>\nwrote:\n\n> Hi Harvey\n>\n> On Tue, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via libcamera-devel\n> wrote:\n> > From: Cheng-Hao Yang <chenghaoyang@chromium.org>\n> >\n> > Add a helper function exportFrameBuffers in HeapAllocator to make it\n> > easier to use.\n>\n> Will you use these with the Virtual pipeline handler ?\n>\n>\nYes, you can find it in |PipelindHandlerVirtual::exportFrameBuffer|.\nRef: https://patchwork.libcamera.org/patch/18533/\n\n\n\n> >\n> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>\n> > ---\n> >  include/libcamera/heap_allocator.h | 11 ++++++\n> >  src/libcamera/heap_allocator.cpp   | 56 ++++++++++++++++++++++++++++++\n> >  2 files changed, 67 insertions(+)\n> >\n> > diff --git a/include/libcamera/heap_allocator.h\n> b/include/libcamera/heap_allocator.h\n> > index cd7ed1a3..076f0951 100644\n> > --- a/include/libcamera/heap_allocator.h\n> > +++ b/include/libcamera/heap_allocator.h\n> > @@ -8,6 +8,7 @@\n> >  #pragma once\n> >\n> >  #include <stddef.h>\n> > +#include <vector>\n> >\n> >  #include <libcamera/base/unique_fd.h>\n> >\n> > @@ -15,6 +16,11 @@\n> >\n> >  namespace libcamera {\n> >\n> > +class Camera;\n> > +class Stream;\n> > +class FrameBuffer;\n> > +class PixelFormatInfo;\n>\n> Not used\n>\n>\nRemoved.\n\n\n> > +\n> >  class HeapAllocator\n> >  {\n> >  public:\n> > @@ -23,7 +29,12 @@ public:\n> >       bool isValid() const { return heap_->isValid(); }\n> >       UniqueFD alloc(const char *name, std::size_t size);\n> >\n> > +     int exportFrameBuffers(Camera *camera, Stream *stream,\n> > +                            std::vector<std::unique_ptr<FrameBuffer>>\n> *buffers);\n> > +\n> >  private:\n> > +     std::unique_ptr<FrameBuffer> createBuffer(Stream *stream);\n> > +\n> >       std::unique_ptr<Heap> heap_;\n> >  };\n> >\n> > diff --git a/src/libcamera/heap_allocator.cpp\n> b/src/libcamera/heap_allocator.cpp\n> > index 179c2c21..33e9eaca 100644\n> > --- a/src/libcamera/heap_allocator.cpp\n> > +++ b/src/libcamera/heap_allocator.cpp\n> > @@ -17,7 +17,11 @@\n> >\n> >  #include <libcamera/base/log.h>\n> >\n> > +#include <libcamera/camera.h>\n> >  #include <libcamera/dma_heap.h>\n> > +#include <libcamera/framebuffer.h>\n> > +#include <libcamera/internal/formats.h>\n> > +#include <libcamera/stream.h>\n> >  #include <libcamera/udma_heap.h>\n> >\n> >  namespace libcamera {\n> > @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name,\n> std::size_t size)\n> >       return heap_->alloc(name, size);\n> >  }\n> >\n> > +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera,\n> Stream *stream,\n> > +\n>  std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> > +{\n> > +     unsigned int count = stream->configuration().bufferCount;\n> > +\n> > +     /** \\todo: Support multiplanar allocations */\n>\n> no : after \\todo\n>\n>\nDone\n\n> +\n> > +     for (unsigned i = 0; i < count; ++i) {\n> > +             std::unique_ptr<FrameBuffer> buffer = createBuffer(stream);\n> > +             if (!buffer) {\n> > +                     LOG(HeapAllocator, Error) << \"Unable to create\n> buffer\";\n> > +\n> > +                     buffers->clear();\n> > +                     return -EINVAL;\n> > +             }\n> > +\n> > +             buffers->push_back(std::move(buffer));\n> > +     }\n> > +\n> > +     return count;\n> > +}\n> > +\n> > +std::unique_ptr<FrameBuffer> HeapAllocator::createBuffer(Stream *stream)\n> > +{\n> > +     std::vector<FrameBuffer::Plane> planes;\n> > +\n> > +     int num_page = (stream->configuration().frameSize + getpagesize()\n> - 1) / getpagesize();\n>\n> Could you break this rather long line ?\n>\n\nDefined \"frameSize\" and \"pageSize\" beforehand. Should be shorter now.\n\n\n> Can num_page be unsigned ?\n>\n\nSure.\n\n\n> libcamera uses camelCase and not snake_case for variables\n>\n>         unsigned int numPage = (stream->configuration().frameSize +\n> getpagesize() - 1)\n>                              / getpagesize();\n>\n> What are the implications of overallocating ? The last plane will be\n> slightly larger, is this an issue at all ?\n>\n>\nThe allocation only works if the requested size is a direct multiple of\n|getpagesize()|. Added a comment.\nRef:\nhttps://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72\n\n\n> > +\n> > +     UniqueFD fd = alloc(\"Buffer\", num_page * getpagesize());\n>\n> What are the implications of using the same name ?\n>\n>\nSince it's a private function that is only called by |exportFrameBuffer|,\nrenamed it to \"FrameBuffer\" instead.\n\n\n> > +     if (!fd.isValid())\n> > +             return nullptr;\n> > +\n> > +     auto info =\n> PixelFormatInfo::info(stream->configuration().pixelFormat);\n> > +     SharedFD shared_fd(std::move(fd));\n> > +     unsigned int offset = 0;\n> > +     for (long unsigned int i = 0; i < info.planes.size(); ++i) {\n>\n> Does this need to be long ?\n>\n>\nI should use size_t instead.\n\n\n> > +             unsigned int planeSize =\n> info.planeSize(stream->configuration().size, i);\n>\n> configuration().size is the size in pixel, is this intended ?\n>\n>\nYes, I guess so, according to the comment of PixelFormatInfo:\nhttps://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056\n\n\n> > +             if (planeSize == 0)\n> > +                     continue;\n> > +\n> > +             FrameBuffer::Plane plane;\n>\n> I would note down a \\todo to remind that we don't support allocating\n> buffers with a dedicated fd per plane\n>\n>\nIs it preferred to do so? I guess we can also support that by calling\n|alloc()| multiple times...?\n\n\n> Sorry, lot of questions :)\n>\n>\nThanks for doing so :)\n\n\n> Cheers\n>   j\n>\n>\n> > +             plane.fd = shared_fd;\n> > +             plane.offset = offset;\n> > +             plane.length = planeSize;\n> > +             planes.push_back(std::move(plane));\n> > +\n> > +             offset += planeSize;\n> > +     }\n> > +\n> > +     return std::make_unique<FrameBuffer>(planes);\n> > +}\n> > +\n> >  } /* namespace libcamera */\n> > --\n> > 2.40.0\n> >\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B6B97BDE6B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 16 May 2023 08:12:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 82DDC627DE;\n\tTue, 16 May 2023 10:12:22 +0200 (CEST)","from mail-vk1-xa29.google.com (mail-vk1-xa29.google.com\n\t[IPv6:2607:f8b0:4864:20::a29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50BBF627DE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 16 May 2023 10:12:20 +0200 (CEST)","by mail-vk1-xa29.google.com with SMTP id\n\t71dfb90a1353d-44fd6c24d5aso4916628e0c.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 16 May 2023 01:12:20 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1684224742;\n\tbh=RBBk8FRs5AxYVuFQ2swJ94+EIFabSqbSM1NxY5hIVhw=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3p25xu2rWCScUdpeouHk7VYQfufytx8n554j+9sYtzby8CVrow804Ks/PmdbKCkjy\n\tpVJ3meTHhiy9TP9+x0NqDfVVZ4EEgr24B62d81XjuGahmhevDw2fswz2AcekdRf4Xk\n\tDZnpMukQhVdhGGK21ZqDXztcfcgPLbgGQ4Mp71p+6F8pNZtHiKMepAg6sxdDBaZ4pl\n\tafoHrU4456+zZohqXS49slqTS/W904yTZS6mx4N4yjNvkNRzxuknFi4zo74quB/0jw\n\tCVj/7CRw7+GfcI8I839QkGKJIK67W1B7UGuBBh78ZstFDbydERnP6WyWoHt30PxcC8\n\tDWPIgtMSu7Ffg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1684224739; x=1686816739;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=QN4YgSsi0GoHYlnnQzqPDBi9hNejeAzpb2NJ4XfpClM=;\n\tb=EnYQlW/RGowc7XzF5fYWeZSzTymtycjUw8qiGFgI60tjW4n3/SEX1rh0J9OOZFjoLz\n\tH2lV9VWXNKDgs20R49kr8NeiqhmmVVAAr1TfopHWNDuc5+cGPaAUzzrhUBqNqGrH8i37\n\troZynOjiwYpJ1aVRnOjXxjgZjkxjXTKB7qabo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"EnYQlW/R\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1684224739; x=1686816739;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=QN4YgSsi0GoHYlnnQzqPDBi9hNejeAzpb2NJ4XfpClM=;\n\tb=GLaxyg0ikzUbiSorc893XUvPnjx0GsE24o5/8C6ncJrBEKxFQNH9H+faGtglNFlv9k\n\tAbd3LdoyENhJjDwPpEnFvYWf9YbHUuJ0tTnhIJlQSnApdGWVD26eC+3rTXywwJs2YYzZ\n\tR2lN+8dbJEBDZoCpK3wZo8W7vgV/gr/Rgj5eKHquM9GdjpmSlvOfFMScE0apykwoadok\n\tQrTG+GFRZrM57saFvAgchElNeO4Qy5cMxSRne9EWdl3JtZLaLl5KhMQzjLwm9YeyP3iA\n\tAr/lF8cQeuIK7muy4uhF5tsT7cauxdCnDKe7E3C6TplMxMgzSR6HZ3Xen4JWTu3FUYow\n\tczhg==","X-Gm-Message-State":"AC+VfDygmftp43ivZe3+1YXoNua5r7+MOtql3C2TYFgI8OBQcD2raFz/\n\tg1vNjfOtHqvAV3tZ5EXvKm+xP6p5q0pGXNIG2awAzGjdYbsF2Dpy+4o=","X-Google-Smtp-Source":"ACHHUZ6ZdZlknMJX1cUQfzhM9IXeMdb+KFmQZit2Sq4NbBfSj1OpSw+CsGvuVPoht7iGYlThUlp9q23ocv9i7zH/E7o=","X-Received":"by 2002:a1f:e306:0:b0:44f:d1f5:6bec with SMTP id\n\ta6-20020a1fe306000000b0044fd1f56becmr11795922vkh.4.1684224739149;\n\tTue, 16 May 2023 01:12:19 -0700 (PDT)","MIME-Version":"1.0","References":"<20230328095534.3584-1-harveyycyang@gmail.com>\n\t<20230328095534.3584-4-harveyycyang@gmail.com>\n\t<20230513162915.n7qty4q6whi7bkyy@lati>","In-Reply-To":"<20230513162915.n7qty4q6whi7bkyy@lati>","Date":"Tue, 16 May 2023 16:12:08 +0800","Message-ID":"<CAEB1ahv12W4X=Vr4M1XrDdszhGHGzt4BpP-yH0MmYPDamKgWNQ@mail.gmail.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"0000000000004ca48905fbcb21f0\"","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Cheng-Hao Yang via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27106,"web_url":"https://patchwork.libcamera.org/comment/27106/","msgid":"<20230516102101.lj5p4lsyyx6vskdg@lati>","date":"2023-05-16T10:21:01","subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Tue, May 16, 2023 at 04:12:08PM +0800, Cheng-Hao Yang via libcamera-devel wrote:\n> Thanks Jacopo!\n>\n> On Sun, May 14, 2023 at 12:29 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> wrote:\n>\n> > Hi Harvey\n> >\n> > On Tue, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via libcamera-devel\n> > wrote:\n> > > From: Cheng-Hao Yang <chenghaoyang@chromium.org>\n> > >\n> > > Add a helper function exportFrameBuffers in HeapAllocator to make it\n> > > easier to use.\n> >\n> > Will you use these with the Virtual pipeline handler ?\n> >\n> >\n> Yes, you can find it in |PipelindHandlerVirtual::exportFrameBuffer|.\n> Ref: https://patchwork.libcamera.org/patch/18533/\n>\n>\n>\n> > >\n> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>\n> > > ---\n> > >  include/libcamera/heap_allocator.h | 11 ++++++\n> > >  src/libcamera/heap_allocator.cpp   | 56 ++++++++++++++++++++++++++++++\n> > >  2 files changed, 67 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/heap_allocator.h\n> > b/include/libcamera/heap_allocator.h\n> > > index cd7ed1a3..076f0951 100644\n> > > --- a/include/libcamera/heap_allocator.h\n> > > +++ b/include/libcamera/heap_allocator.h\n> > > @@ -8,6 +8,7 @@\n> > >  #pragma once\n> > >\n> > >  #include <stddef.h>\n> > > +#include <vector>\n> > >\n> > >  #include <libcamera/base/unique_fd.h>\n> > >\n> > > @@ -15,6 +16,11 @@\n> > >\n> > >  namespace libcamera {\n> > >\n> > > +class Camera;\n> > > +class Stream;\n> > > +class FrameBuffer;\n> > > +class PixelFormatInfo;\n> >\n> > Not used\n> >\n> >\n> Removed.\n>\n>\n> > > +\n> > >  class HeapAllocator\n> > >  {\n> > >  public:\n> > > @@ -23,7 +29,12 @@ public:\n> > >       bool isValid() const { return heap_->isValid(); }\n> > >       UniqueFD alloc(const char *name, std::size_t size);\n> > >\n> > > +     int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > +                            std::vector<std::unique_ptr<FrameBuffer>>\n> > *buffers);\n> > > +\n> > >  private:\n> > > +     std::unique_ptr<FrameBuffer> createBuffer(Stream *stream);\n> > > +\n> > >       std::unique_ptr<Heap> heap_;\n> > >  };\n> > >\n> > > diff --git a/src/libcamera/heap_allocator.cpp\n> > b/src/libcamera/heap_allocator.cpp\n> > > index 179c2c21..33e9eaca 100644\n> > > --- a/src/libcamera/heap_allocator.cpp\n> > > +++ b/src/libcamera/heap_allocator.cpp\n> > > @@ -17,7 +17,11 @@\n> > >\n> > >  #include <libcamera/base/log.h>\n> > >\n> > > +#include <libcamera/camera.h>\n> > >  #include <libcamera/dma_heap.h>\n> > > +#include <libcamera/framebuffer.h>\n> > > +#include <libcamera/internal/formats.h>\n> > > +#include <libcamera/stream.h>\n> > >  #include <libcamera/udma_heap.h>\n> > >\n> > >  namespace libcamera {\n> > > @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name,\n> > std::size_t size)\n> > >       return heap_->alloc(name, size);\n> > >  }\n> > >\n> > > +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera *camera,\n> > Stream *stream,\n> > > +\n> >  std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> > > +{\n> > > +     unsigned int count = stream->configuration().bufferCount;\n> > > +\n> > > +     /** \\todo: Support multiplanar allocations */\n> >\n> > no : after \\todo\n> >\n> >\n> Done\n>\n> > +\n> > > +     for (unsigned i = 0; i < count; ++i) {\n> > > +             std::unique_ptr<FrameBuffer> buffer = createBuffer(stream);\n> > > +             if (!buffer) {\n> > > +                     LOG(HeapAllocator, Error) << \"Unable to create\n> > buffer\";\n> > > +\n> > > +                     buffers->clear();\n> > > +                     return -EINVAL;\n> > > +             }\n> > > +\n> > > +             buffers->push_back(std::move(buffer));\n> > > +     }\n> > > +\n> > > +     return count;\n> > > +}\n> > > +\n> > > +std::unique_ptr<FrameBuffer> HeapAllocator::createBuffer(Stream *stream)\n> > > +{\n> > > +     std::vector<FrameBuffer::Plane> planes;\n> > > +\n> > > +     int num_page = (stream->configuration().frameSize + getpagesize()\n> > - 1) / getpagesize();\n> >\n> > Could you break this rather long line ?\n> >\n>\n> Defined \"frameSize\" and \"pageSize\" beforehand. Should be shorter now.\n>\n>\n> > Can num_page be unsigned ?\n> >\n>\n> Sure.\n>\n>\n> > libcamera uses camelCase and not snake_case for variables\n> >\n> >         unsigned int numPage = (stream->configuration().frameSize +\n> > getpagesize() - 1)\n> >                              / getpagesize();\n> >\n> > What are the implications of overallocating ? The last plane will be\n> > slightly larger, is this an issue at all ?\n> >\n> >\n> The allocation only works if the requested size is a direct multiple of\n> |getpagesize()|. Added a comment.\n> Ref:\n> https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72\n>\n\nYes, I understand the requirement, I was wondering about the\nimplications of overallocation, but I presume this is safe ?\n\n>\n> > > +\n> > > +     UniqueFD fd = alloc(\"Buffer\", num_page * getpagesize());\n> >\n> > What are the implications of using the same name ?\n> >\n> >\n> Since it's a private function that is only called by |exportFrameBuffer|,\n> renamed it to \"FrameBuffer\" instead.\n>\n>\n> > > +     if (!fd.isValid())\n> > > +             return nullptr;\n> > > +\n> > > +     auto info =\n> > PixelFormatInfo::info(stream->configuration().pixelFormat);\n> > > +     SharedFD shared_fd(std::move(fd));\n> > > +     unsigned int offset = 0;\n> > > +     for (long unsigned int i = 0; i < info.planes.size(); ++i) {\n> >\n> > Does this need to be long ?\n> >\n> >\n> I should use size_t instead.\n>\n>\n> > > +             unsigned int planeSize =\n> > info.planeSize(stream->configuration().size, i);\n> >\n> > configuration().size is the size in pixel, is this intended ?\n> >\n> >\n> Yes, I guess so, according to the comment of PixelFormatInfo:\n> https://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056\n>\n\nAh you're right\n\n \\param[in] size The size of the frame, in pixels\n\n\n>\n> > > +             if (planeSize == 0)\n> > > +                     continue;\n> > > +\n> > > +             FrameBuffer::Plane plane;\n> >\n> > I would note down a \\todo to remind that we don't support allocating\n> > buffers with a dedicated fd per plane\n> >\n> >\n> Is it preferred to do so? I guess we can also support that by calling\n> |alloc()| multiple times...?\n>\n\nI don't think it's specifically preferred, but might be required for\nfully-planar formats where each plane lives in its own possibly\nnon-contiguous memory area. I would for now record a todo item\n\nWe have a similar issue here iirc\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/android/mm/cros_frame_buffer_allocator.cpp#n57\n\n\n>\n> > Sorry, lot of questions :)\n> >\n> >\n> Thanks for doing so :)\n>\n>\n> > Cheers\n> >   j\n> >\n> >\n> > > +             plane.fd = shared_fd;\n> > > +             plane.offset = offset;\n> > > +             plane.length = planeSize;\n> > > +             planes.push_back(std::move(plane));\n> > > +\n> > > +             offset += planeSize;\n> > > +     }\n> > > +\n> > > +     return std::make_unique<FrameBuffer>(planes);\n> > > +}\n> > > +\n> > >  } /* namespace libcamera */\n> > > --\n> > > 2.40.0\n> > >\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 34F06C3284\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 16 May 2023 10:21:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90E03627D1;\n\tTue, 16 May 2023 12:21:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1354660399\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 16 May 2023 12:21:05 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9003D4DB;\n\tTue, 16 May 2023 12:20:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1684232466;\n\tbh=AL9tPtZeV0pXrpRElXfgnkijLyEo6avtiEidNWvkCBg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=l9UGFyDynud6yGnudUX/4BVMK0LlG/jeN4ErP59fuBoXcPtVWVSovPoWXLUIkk1Gb\n\t9Hrq6dnuCIf9bMvr/u9nABf//XIi0c3hI6qcYm63jYvGsKh25EEsMqGQ5z/4FwXS2r\n\ttOceN4e3ZLgu3FrD5wHTp6tF1OTVZXwFFR9mz1J52TrBMYJTQ2N8LTgzauOgEyGMzF\n\tQMHYAYYQAQAxzRkaDFUEWKNgGL5XDyC9KytLsnCqngVpjOCdp3Nbi5XMv+wgOlbP3A\n\tQ6ynanpdjLxWgR22ml1IVb03pcP5OR/ArxgdTRLVGuzMetzV7ddpOTStLXJr75H79G\n\t7Cy4kGRgU2Qjg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1684232453;\n\tbh=AL9tPtZeV0pXrpRElXfgnkijLyEo6avtiEidNWvkCBg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m3hMRiyBYq+IuCg9dUte+WhwPPqhmYJ6cPEisUeUDUZu6vFTtzWI6wUYzpoYsdAOT\n\tb2Q/Yyp6JR3QzKsF0QR8VgeWyIjR3dxNAvY+no/hxHqyDzCYbBofvTvITZBijaEV9t\n\trL7lkDIEoJ7PQVQXfgqn+lOUhgfzSxiYslfdg81I="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"m3hMRiyB\"; dkim-atps=neutral","Date":"Tue, 16 May 2023 12:21:01 +0200","To":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Message-ID":"<20230516102101.lj5p4lsyyx6vskdg@lati>","References":"<20230328095534.3584-1-harveyycyang@gmail.com>\n\t<20230328095534.3584-4-harveyycyang@gmail.com>\n\t<20230513162915.n7qty4q6whi7bkyy@lati>\n\t<CAEB1ahv12W4X=Vr4M1XrDdszhGHGzt4BpP-yH0MmYPDamKgWNQ@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<CAEB1ahv12W4X=Vr4M1XrDdszhGHGzt4BpP-yH0MmYPDamKgWNQ@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27124,"web_url":"https://patchwork.libcamera.org/comment/27124/","msgid":"<CAEB1ahum2PQ6fW4a97_roF_qh0dxm9vx+-f7sjc9EtHAzA33_g@mail.gmail.com>","date":"2023-05-22T08:36:50","subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Jacopo.\n\nOn Tue, May 16, 2023 at 6:21 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>\nwrote:\n\n> Hi Harvey\n>\n> On Tue, May 16, 2023 at 04:12:08PM +0800, Cheng-Hao Yang via\n> libcamera-devel wrote:\n> > Thanks Jacopo!\n> >\n> > On Sun, May 14, 2023 at 12:29 AM Jacopo Mondi <\n> jacopo.mondi@ideasonboard.com>\n> > wrote:\n> >\n> > > Hi Harvey\n> > >\n> > > On Tue, Mar 28, 2023 at 05:55:34PM +0800, Harvey Yang via\n> libcamera-devel\n> > > wrote:\n> > > > From: Cheng-Hao Yang <chenghaoyang@chromium.org>\n> > > >\n> > > > Add a helper function exportFrameBuffers in HeapAllocator to make it\n> > > > easier to use.\n> > >\n> > > Will you use these with the Virtual pipeline handler ?\n> > >\n> > >\n> > Yes, you can find it in |PipelindHandlerVirtual::exportFrameBuffer|.\n> > Ref: https://patchwork.libcamera.org/patch/18533/\n> >\n> >\n> >\n> > > >\n> > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>\n> > > > ---\n> > > >  include/libcamera/heap_allocator.h | 11 ++++++\n> > > >  src/libcamera/heap_allocator.cpp   | 56\n> ++++++++++++++++++++++++++++++\n> > > >  2 files changed, 67 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/heap_allocator.h\n> > > b/include/libcamera/heap_allocator.h\n> > > > index cd7ed1a3..076f0951 100644\n> > > > --- a/include/libcamera/heap_allocator.h\n> > > > +++ b/include/libcamera/heap_allocator.h\n> > > > @@ -8,6 +8,7 @@\n> > > >  #pragma once\n> > > >\n> > > >  #include <stddef.h>\n> > > > +#include <vector>\n> > > >\n> > > >  #include <libcamera/base/unique_fd.h>\n> > > >\n> > > > @@ -15,6 +16,11 @@\n> > > >\n> > > >  namespace libcamera {\n> > > >\n> > > > +class Camera;\n> > > > +class Stream;\n> > > > +class FrameBuffer;\n> > > > +class PixelFormatInfo;\n> > >\n> > > Not used\n> > >\n> > >\n> > Removed.\n> >\n> >\n> > > > +\n> > > >  class HeapAllocator\n> > > >  {\n> > > >  public:\n> > > > @@ -23,7 +29,12 @@ public:\n> > > >       bool isValid() const { return heap_->isValid(); }\n> > > >       UniqueFD alloc(const char *name, std::size_t size);\n> > > >\n> > > > +     int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > > +\n> std::vector<std::unique_ptr<FrameBuffer>>\n> > > *buffers);\n> > > > +\n> > > >  private:\n> > > > +     std::unique_ptr<FrameBuffer> createBuffer(Stream *stream);\n> > > > +\n> > > >       std::unique_ptr<Heap> heap_;\n> > > >  };\n> > > >\n> > > > diff --git a/src/libcamera/heap_allocator.cpp\n> > > b/src/libcamera/heap_allocator.cpp\n> > > > index 179c2c21..33e9eaca 100644\n> > > > --- a/src/libcamera/heap_allocator.cpp\n> > > > +++ b/src/libcamera/heap_allocator.cpp\n> > > > @@ -17,7 +17,11 @@\n> > > >\n> > > >  #include <libcamera/base/log.h>\n> > > >\n> > > > +#include <libcamera/camera.h>\n> > > >  #include <libcamera/dma_heap.h>\n> > > > +#include <libcamera/framebuffer.h>\n> > > > +#include <libcamera/internal/formats.h>\n> > > > +#include <libcamera/stream.h>\n> > > >  #include <libcamera/udma_heap.h>\n> > > >\n> > > >  namespace libcamera {\n> > > > @@ -43,4 +47,56 @@ UniqueFD HeapAllocator::alloc(const char *name,\n> > > std::size_t size)\n> > > >       return heap_->alloc(name, size);\n> > > >  }\n> > > >\n> > > > +int HeapAllocator::exportFrameBuffers([[maybe_unused]] Camera\n> *camera,\n> > > Stream *stream,\n> > > > +\n> > >  std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n> > > > +{\n> > > > +     unsigned int count = stream->configuration().bufferCount;\n> > > > +\n> > > > +     /** \\todo: Support multiplanar allocations */\n> > >\n> > > no : after \\todo\n> > >\n> > >\n> > Done\n> >\n> > > +\n> > > > +     for (unsigned i = 0; i < count; ++i) {\n> > > > +             std::unique_ptr<FrameBuffer> buffer =\n> createBuffer(stream);\n> > > > +             if (!buffer) {\n> > > > +                     LOG(HeapAllocator, Error) << \"Unable to create\n> > > buffer\";\n> > > > +\n> > > > +                     buffers->clear();\n> > > > +                     return -EINVAL;\n> > > > +             }\n> > > > +\n> > > > +             buffers->push_back(std::move(buffer));\n> > > > +     }\n> > > > +\n> > > > +     return count;\n> > > > +}\n> > > > +\n> > > > +std::unique_ptr<FrameBuffer> HeapAllocator::createBuffer(Stream\n> *stream)\n> > > > +{\n> > > > +     std::vector<FrameBuffer::Plane> planes;\n> > > > +\n> > > > +     int num_page = (stream->configuration().frameSize +\n> getpagesize()\n> > > - 1) / getpagesize();\n> > >\n> > > Could you break this rather long line ?\n> > >\n> >\n> > Defined \"frameSize\" and \"pageSize\" beforehand. Should be shorter now.\n> >\n> >\n> > > Can num_page be unsigned ?\n> > >\n> >\n> > Sure.\n> >\n> >\n> > > libcamera uses camelCase and not snake_case for variables\n> > >\n> > >         unsigned int numPage = (stream->configuration().frameSize +\n> > > getpagesize() - 1)\n> > >                              / getpagesize();\n> > >\n> > > What are the implications of overallocating ? The last plane will be\n> > > slightly larger, is this an issue at all ?\n> > >\n> > >\n> > The allocation only works if the requested size is a direct multiple of\n> > |getpagesize()|. Added a comment.\n> > Ref:\n> >\n> https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/drivers/dma-buf/udmabuf.c#L72\n> >\n>\n> Yes, I understand the requirement, I was wondering about the\n> implications of overallocation, but I presume this is safe ?\n>\n>\nAh sorry to misunderstand your question. I assume the plane size is\ndetermined by |FrameBuffer::Plane::length|, not the remaining space\nof the allocated buffer. It's specified in line 282. Please correct me if\nI'm wrong :)\n\n\n> >\n> > > > +\n> > > > +     UniqueFD fd = alloc(\"Buffer\", num_page * getpagesize());\n> > >\n> > > What are the implications of using the same name ?\n> > >\n> > >\n> > Since it's a private function that is only called by |exportFrameBuffer|,\n> > renamed it to \"FrameBuffer\" instead.\n> >\n> >\n> > > > +     if (!fd.isValid())\n> > > > +             return nullptr;\n> > > > +\n> > > > +     auto info =\n> > > PixelFormatInfo::info(stream->configuration().pixelFormat);\n> > > > +     SharedFD shared_fd(std::move(fd));\n> > > > +     unsigned int offset = 0;\n> > > > +     for (long unsigned int i = 0; i < info.planes.size(); ++i) {\n> > >\n> > > Does this need to be long ?\n> > >\n> > >\n> > I should use size_t instead.\n> >\n> >\n> > > > +             unsigned int planeSize =\n> > > info.planeSize(stream->configuration().size, i);\n> > >\n> > > configuration().size is the size in pixel, is this intended ?\n> > >\n> > >\n> > Yes, I guess so, according to the comment of PixelFormatInfo:\n> >\n> https://github.com/kbingham/libcamera/blob/master/src/libcamera/formats.cpp#L1056\n> >\n>\n> Ah you're right\n>\n>  \\param[in] size The size of the frame, in pixels\n>\n>\n> >\n> > > > +             if (planeSize == 0)\n> > > > +                     continue;\n> > > > +\n> > > > +             FrameBuffer::Plane plane;\n> > >\n> > > I would note down a \\todo to remind that we don't support allocating\n> > > buffers with a dedicated fd per plane\n> > >\n> > >\n> > Is it preferred to do so? I guess we can also support that by calling\n> > |alloc()| multiple times...?\n> >\n>\n> I don't think it's specifically preferred, but might be required for\n> fully-planar formats where each plane lives in its own possibly\n> non-contiguous memory area. I would for now record a todo item\n>\n> We have a similar issue here iirc\n>\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/mm/cros_frame_buffer_allocator.cpp#n57\n>\n>\n>\nOkay. Added a comment. Feel free to modify it when landing.\nAlso let me know if a dedicated fd is needed here, as I think\nit's not that hard to do so :)\n\n\n> >\n> > > Sorry, lot of questions :)\n> > >\n> > >\n> > Thanks for doing so :)\n> >\n> >\n> > > Cheers\n> > >   j\n> > >\n> > >\n> > > > +             plane.fd = shared_fd;\n> > > > +             plane.offset = offset;\n> > > > +             plane.length = planeSize;\n> > > > +             planes.push_back(std::move(plane));\n> > > > +\n> > > > +             offset += planeSize;\n> > > > +     }\n> > > > +\n> > > > +     return std::make_unique<FrameBuffer>(planes);\n> > > > +}\n> > > > +\n> > > >  } /* namespace libcamera */\n> > > > --\n> > > > 2.40.0\n> > > >\n> > >\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 959EEC31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 22 May 2023 08:37:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5026960536;\n\tMon, 22 May 2023 10:37:05 +0200 (CEST)","from mail-ua1-x933.google.com (mail-ua1-x933.google.com\n\t[IPv6:2607:f8b0:4864:20::933])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDC2060536\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 22 May 2023 10:37:02 +0200 (CEST)","by mail-ua1-x933.google.com with SMTP id\n\ta1e0cc1a2514c-783e4666738so1882998241.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 22 May 2023 01:37:02 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1684744625;\n\tbh=BGq7z1Gvsr/1YKujzCMIUqKlalNga92MjzsTyNdHg3A=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=V3vHL5XDr8kL45vVZFXvX9EePKOAeWEPxr6Cjrvvb5OtgdKQfFpsIYaXfUFi1Lkkt\n\tChYm7B/k4GiuL2rLJRTATFSliyv77JtXohRiV1nbRlenL22LFtwRB96kajFlxVSsh9\n\tsq1iU5Fy3LN6Y1lDhKtMjb0hOapEMTAtBe4CR+yR+whIR0V0H8C8C6Wd/0zvL4frI+\n\tLrYlCMKLPjBRB8XfJrLav4Lsq6JlrNEASDgoOBRMgnLzeC2xaGam2ixBMvoKPp3Fh/\n\tufmR/ZPs3FjRcNI8y6dftoH4SAQqM0K/9M5qt1bVKXBG7e4GWH1UA7fpsnJmVCRqHT\n\ty6jT3y0WuCtNg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1684744622; x=1687336622;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=JUoJC9rEjBDb79GMuh6MsL6xNeAZ+WN7S3T/qiO7kcg=;\n\tb=Ne9KU3IGrcF9+5Qp2JG5/IEwkTHTHKL78p7Cvh0wnNbA3ryHOpii3UqThs0xIoXYML\n\t2u4GpUyIpY/ZXGvSd8IdfHfuJvr+aOU4PRjkc/KIcBuS+O07JCSsMgU9nkfFR1VWeiv3\n\tMuLexnQJHSgyBmDKrhIA1ulZrExDFOViQXsso="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"Ne9KU3IG\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1684744622; x=1687336622;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=JUoJC9rEjBDb79GMuh6MsL6xNeAZ+WN7S3T/qiO7kcg=;\n\tb=k1Ak71pi5GczV4R/QMIoBk/KAxV9mSO2wAI67l4JbuyJd/1R3xuLFwtAw4h7xYHTSZ\n\thue+Bt7KwJcL3T8ivaN7fpdqluuaLZKzGZxZ0yb6wdJ4GBuykgfYr6frvthBzcgBP2bU\n\tNVkFGHdFJBs5PYecmt53wRnJX2H/XdefpaZjbdQmfbVZkHnaP/IaZXteXgDwoJJ0fw86\n\tnf9DNXbHtwD1EsxV3U2CaZTpR67fwYcWgQ2uaHMsYmhaNGXyJma8KJNPg5xshn8HMZWD\n\tJy3t0I4oOf40ko8k0O/WJ5ALTsCdTEdepbrYxOwFG6UWh0O/62WdZqCFDJ147swFuRaS\n\ti8ww==","X-Gm-Message-State":"AC+VfDz08NLrQ2sOdH+kVQeeeR1z8V4w84GJyBvUHfHI9mY2DNBbawmx\n\tmO4E0WnxAaZXE9/YMF7ufuaT9i65XicppqZtmUWs9IwK6zEuUAlaBFA=","X-Google-Smtp-Source":"ACHHUZ5lUuxELsyZlEDI+OOeKib2GkHIxUL5dgk7/nZTQxeG1V/YEKbPZE9+5GlT1sWA12KTqd5TG3FrO7OvLD5TqkA=","X-Received":"by 2002:a67:af07:0:b0:439:33bf:ac86 with SMTP id\n\tv7-20020a67af07000000b0043933bfac86mr2584199vsl.11.1684744621712;\n\tMon, 22 May 2023 01:37:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20230328095534.3584-1-harveyycyang@gmail.com>\n\t<20230328095534.3584-4-harveyycyang@gmail.com>\n\t<20230513162915.n7qty4q6whi7bkyy@lati>\n\t<CAEB1ahv12W4X=Vr4M1XrDdszhGHGzt4BpP-yH0MmYPDamKgWNQ@mail.gmail.com>\n\t<20230516102101.lj5p4lsyyx6vskdg@lati>","In-Reply-To":"<20230516102101.lj5p4lsyyx6vskdg@lati>","Date":"Mon, 22 May 2023 16:36:50 +0800","Message-ID":"<CAEB1ahum2PQ6fW4a97_roF_qh0dxm9vx+-f7sjc9EtHAzA33_g@mail.gmail.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"000000000000b6fbfa05fc442c5d\"","Subject":"Re: [libcamera-devel] [PATCH v4 3/3] libcamera: Add\n\texportFrameBuffers in HeapAllocator","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Cheng-Hao Yang via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Cheng-Hao Yang <chenghaoyang@chromium.org>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]